Don't expose MS.VS.LanguageServer.Protocol types from EA.Razor #68713
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| public static SemanticTokensSchema GetSchema(ClientCapabilities capabilities) | ||
| => capabilities.HasVisualStudioLspCapability() | ||
| => GetSchema(capabilities.HasVisualStudioLspCapability()); |
There was a problem hiding this comment.
Let me know if you want me to remove this overload completely. Every caller could just pass capabilities.HasVisualStudioLspCapability() and it would all work the same.
There was a problem hiding this comment.
I think its reasonable to remove the client capabilities overload entirely
| public static SemanticTokensSchema GetSchema(ClientCapabilities capabilities) | ||
| => capabilities.HasVisualStudioLspCapability() | ||
| => GetSchema(capabilities.HasVisualStudioLspCapability()); |
There was a problem hiding this comment.
I think its reasonable to remove the client capabilities overload entirely
| internal interface IRazorCapabilitiesProvider | ||
| { | ||
| ServerCapabilities GetCapabilities(ClientCapabilities clientCapabilities); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// A capabilities provider that should only be used from Razor tests |
There was a problem hiding this comment.
I wonder if there's anything we can use to ban the usage of any LSP protocol types in this project. Maybe ban the entire LSP protocol namespace?
https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.BannedApiAnalyzers/BannedApiAnalyzers.Help.md
There was a problem hiding this comment.
I thought about this too, but the issue is that we don't want to ban the usage, just the exposure. In fact we require the usage, because thats what the Roslyn LSP server we're interacting with needs.
| { | ||
| // To avoid exposing types from MS.VS.LanguageServer.Protocol types we serialize and deserialize the capabilities | ||
| // so we can just pass string around. This is obviously not great for perf, but it is only used in Razor tests. | ||
| var clientCapabilitiesJson = JsonConvert.SerializeObject(clientCapabilities); |
There was a problem hiding this comment.
Might need this for proper serialization / deserialization
https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient?path=/src/product/Protocol/LanguageServer.Protocol.Internal/Converters/VSInternalExtensionUtilities.cs&version=GBdevelop&line=24&lineEnd=25&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents
We use it here for example - https://sourceroslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/LanguageServer/AbstractInProcLanguageClient.cs,203
There was a problem hiding this comment.
For completeness, probably, but we don't actually need that, because I'm explicitly telling NewtonSoft that its a VSInternalServerCapabilities, which is the highest level type. The type converters are needed, AFAIK, for when you ask it to [de]serialize a lower level type, but don't want to lose the info from the higher types.
Also, this is only used by tests, our scenario is super simple. We only set SupportsDiagnosticsRequests, and always serialize a VSInternalServerCapabilities: https://github.com/dotnet/razor/blob/main/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/SingleServerDelegatingEndpointTestBase.cs#L75
|
Removed the extra overload. Also the Razor response is here in case you're curious, but its super unexciting: dotnet/razor#8855 |
ghost
added this to the 
We had a bit of a situation today, due to our EA having types from MS.VS.LanguageServer.Protocol exposed in its API. It means as soon as Roslyn inserts to VS referencing a new version, Razor functionality and local dev stops working.
To avoid a dual insertion I've just made the old API obsolete, and will follow up with removal once everything is inserted appropriately. Using strings and serializing capabilities is not ideal, but I couldn't think of another way, and the scenario is only used from our tests. In fact, I'd love to rename all of the language server wrapper classes to include "Test" in the name, but that seemed like an annoying amount of code to obsolete and duplicate.