Skip to content

Commit 4796e92

Browse files
authored
Do not parse URIs during LSP serialization/deserialization (#76691)
Resolves dotnet/vscode-csharp#7638 Resolves microsoft/vscode-dotnettools#1932 Related to dotnet/runtime#64707 ## Problem `System.Uri` will throw attempting to parse URIs that have `sub-delims` in the host name. This is because it does additional host name validation beyond what is defined in the URI RFC spec. See above linked issues for specific examples. When we get passed these URIs from VSCode, the server crashes because we cannot successfully deserialize the LSP URI string into a `System.Uri`. We are unable to easily recover from these as it happens before we get to the queue processing. ## Solution The approach I took in this PR is to stop parsing URIs during LSP message deserialization and remove usages of `System.Uri` from LSP directly. Instead, I created a wrapper type `DocumentUri` which initially stores just the string representation of the URI (exactly how [LSP defines URIs](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#uri)). The `System.Uri` can be optionally retrieved from this (which will parse it). Only places which need to extract information from the URI should retrieve the `System.Uri` and must handle failures.   This allows URI parsing to be delayed until much later (for example looking at the scheme or file path to find a matching document in the workspace). While we still cannot parse the URI, we can handle the error and provide a misc document instead of crashing the server. These aren't normal files anyway (they don't conform to the `file:///<path>` format) and would go to misc regardless of if we succeed in parsing it or not. If at some point the runtime adds a new parsing mode, or we switch to our own URI parser (similar to O#), this still provides value. Deserialization of the URI string and parsing the URI string are two separate logical operations and should not be tied together. Additionally, any runtime improvements here would only be available in .NET 10. ## Key areas to look at 1. `DocumentUri` 2. `DocumentUriConverter` 3. `LspWorkspaceManager` 4. `ProtocolConversions` Razor PR - https://github.com/dotnet/razor/pull/11390/files
2 parents c2b6f46 + 4fc9a6c commit 4796e92

File tree

138 files changed

+1362
-1073
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

138 files changed

+1362
-1073
lines changed

src/EditorFeatures/Core/ExternalAccess/VSTypeScript/Api/AbstractVSTypeScriptRequestHandler.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ internal abstract class AbstractVSTypeScriptRequestHandler<TRequestType, TRespon
3131

3232
var textDocumentIdentifier = new VSTextDocumentIdentifier
3333
{
34-
Uri = typeScriptIdentifier.Value.Uri,
34+
DocumentUri = new(typeScriptIdentifier.Value.Uri),
3535
};
3636

3737
if (typeScriptIdentifier.Value.ProjectId != null)

src/EditorFeatures/Test/LanguageServer/VSTypeScriptHandlerTests.cs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
using StreamJsonRpc;
2323
using Xunit;
2424
using Xunit.Abstractions;
25+
using System.Text.Json.Serialization;
2526

2627
namespace Microsoft.CodeAnalysis.Editor.UnitTests.LanguageServer;
2728

@@ -138,7 +139,7 @@ public static async Task<VSTypeScriptTestLspServer> CreateAsync(LspTestWorkspace
138139
}
139140
}
140141

141-
internal sealed record TSRequest(Uri Document, string Project);
142+
internal sealed record TSRequest([property: JsonConverter(typeof(DocumentUriConverter))] DocumentUri Document, string Project);
142143

143144
[ExportTypeScriptLspServiceFactory(typeof(TypeScriptHandler)), PartNotDiscoverable, Shared]
144145
internal sealed class TypeScriptHandlerFactory : AbstractVSTypeScriptRequestHandlerFactory
@@ -168,7 +169,7 @@ internal sealed class TypeScriptHandler : AbstractVSTypeScriptRequestHandler<TSR
168169

169170
protected override TypeScriptTextDocumentIdentifier? GetTypeSciptTextDocumentIdentifier(TSRequest request)
170171
{
171-
return new TypeScriptTextDocumentIdentifier(request.Document, request.Project);
172+
return new TypeScriptTextDocumentIdentifier(request.Document.GetRequiredParsedUri(), request.Project);
172173
}
173174

174175
protected override Task<int> HandleRequestAsync(TSRequest request, TypeScriptRequestContext context, CancellationToken cancellationToken)

src/Features/Lsif/Generator/Generator.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ public async Task GenerateForProjectAsync(
212212

213213
var contentBase64Encoded = await GetBase64EncodedContentAsync(document, cancellationToken);
214214

215-
var documentVertex = new Graph.LsifDocument(document.GetURI(), GetLanguageKind(semanticModel.Language), contentBase64Encoded, idFactory);
215+
var documentVertex = new Graph.LsifDocument(document.GetURI().GetRequiredParsedUri(), GetLanguageKind(semanticModel.Language), contentBase64Encoded, idFactory);
216216
lsifJsonWriter.Write(documentVertex);
217217
lsifJsonWriter.Write(new Event(Event.EventKind.Begin, documentVertex.GetId(), idFactory));
218218

src/Features/Lsif/Generator/Graph/LsifDocument.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// See the LICENSE file in the project root for more information.
44

55
using System;
6+
using Roslyn.LanguageServer.Protocol;
67

78
namespace Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator.Graph;
89

src/Features/Lsif/Generator/Graph/LsifProject.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// See the LICENSE file in the project root for more information.
44

55
using System;
6+
using Roslyn.LanguageServer.Protocol;
67

78
namespace Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator.Graph;
89

src/Features/Lsif/GeneratorTest/ProjectStructureTests.vb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ Namespace Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator.UnitTests
7676
Await TestLsifOutput.GenerateForWorkspaceAsync(workspace, New LineModeLsifJsonWriter(stringWriter))
7777

7878
Dim generatedDocument = Assert.Single(Await workspace.CurrentSolution.Projects.Single().GetSourceGeneratedDocumentsAsync())
79-
Dim uri = SourceGeneratedDocumentUri.Create(generatedDocument.Identity)
79+
Dim uri = SourceGeneratedDocumentUri.Create(generatedDocument.Identity).GetRequiredParsedUri()
8080
Dim outputText = stringWriter.ToString()
8181
Assert.Contains(uri.AbsoluteUri, outputText)
8282
End Function

src/LanguageServer/ExternalAccess/CompilerDeveloperSDK/Handler/AbstractCompilerDeveloperSdkLspServiceDocumentRequestHandler.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ internal abstract class AbstractCompilerDeveloperSdkLspServiceDocumentRequestHan
2222
bool ISolutionRequiredHandler.RequiresLSPSolution => RequiresLSPSolution;
2323

2424
TextDocumentIdentifier ITextDocumentIdentifierHandler<TRequest, TextDocumentIdentifier>.GetTextDocumentIdentifier(TRequest request)
25-
=> new() { Uri = GetTextDocumentIdentifier(request) };
25+
=> new() { DocumentUri = new(GetTextDocumentIdentifier(request)) };
2626
Task<TResponse> IRequestHandler<TRequest, TResponse, LspRequestContext>.HandleRequestAsync(TRequest request, LspRequestContext context, CancellationToken cancellationToken)
2727
=> HandleRequestAsync(request, new RequestContext(context), cancellationToken);
2828
}

src/LanguageServer/ExternalAccess/Copilot/Handler/AbstractCopilotLspServiceDocumentRequestHandler.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ internal abstract class AbstractCopilotLspServiceDocumentRequestHandler<TRequest
2121
bool ISolutionRequiredHandler.RequiresLSPSolution => true;
2222

2323
TextDocumentIdentifier ITextDocumentIdentifierHandler<TRequest, TextDocumentIdentifier>.GetTextDocumentIdentifier(TRequest request)
24-
=> new() { Uri = GetTextDocumentUri(request) };
24+
=> new() { DocumentUri = new(GetTextDocumentUri(request)) };
2525

2626
Task<TResponse> IRequestHandler<TRequest, TResponse, RequestContext>.HandleRequestAsync(TRequest request, RequestContext context, CancellationToken cancellationToken)
2727
=> HandleRequestAsync(request, new CopilotRequestContext(context), cancellationToken);

src/LanguageServer/ExternalAccess/VisualDiagnostics/Internal/HotReloadDiagnosticSource.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ public async Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(RequestCon
2424
return result;
2525
}
2626

27-
public TextDocumentIdentifier? GetDocumentIdentifier() => new() { Uri = textDocument.GetURI() };
27+
public TextDocumentIdentifier? GetDocumentIdentifier() => new() { DocumentUri = textDocument.GetURI() };
2828
public ProjectOrDocumentId GetId() => new(textDocument.Id);
2929
public Project GetProject() => textDocument.Project;
3030
public bool IsLiveSource() => true;

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/LspFileChangeWatcherTests.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public async Task CreatingDirectoryWatchRequestsDirectoryWatch()
6262

6363
var watcher = GetSingleFileWatcher(dynamicCapabilitiesRpcTarget);
6464

65-
Assert.Equal(tempDirectory.Path, watcher.GlobPattern.Second.BaseUri.Second.LocalPath);
65+
Assert.Equal(tempDirectory.Path, watcher.GlobPattern.Second.BaseUri.Second.GetRequiredParsedUri().LocalPath);
6666
Assert.Equal("**/*", watcher.GlobPattern.Second.Pattern);
6767

6868
// Get rid of the registration and it should be gone again
@@ -93,7 +93,7 @@ public async Task CreatingFileWatchRequestsFileWatch()
9393

9494
var watcher = GetSingleFileWatcher(dynamicCapabilitiesRpcTarget);
9595

96-
Assert.Equal("Z:\\", watcher.GlobPattern.Second.BaseUri.Second.LocalPath);
96+
Assert.Equal("Z:\\", watcher.GlobPattern.Second.BaseUri.Second.GetRequiredParsedUri().LocalPath);
9797
Assert.Equal("SingleFile.txt", watcher.GlobPattern.Second.Pattern);
9898

9999
// Get rid of the registration and it should be gone again

0 commit comments

Comments
 (0)