Use vscode-uri semantics for parsing URIs - #83593
Conversation
9ca5e2b to
761fca8
Compare
9eb64f0 to
8d6c2d0
Compare
9a8a978 to
d9118a9
Compare
df6903b to
ea4a4b5
Compare
c9ba792 to
18cd21a
Compare
cdef7e6 to
a20777c
Compare
|
/pr-val |
|
View PR Validation Run triggered by @dibarbet Parameters
|
4e61e5f to
b2c8d49
Compare
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
|
View PR Validation Run triggered by @dibarbet Parameters
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 17dcbb0d-711d-4cca-91c3-574ec31aeea8
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 17dcbb0d-711d-4cca-91c3-574ec31aeea8
|
/pr-val |
|
View PR Validation Run triggered by @dibarbet Parameters
|
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Protocol/DocumentUri.cs — The XML doc references Protocol.ParsedUri, but there is no Protocol alias/type/namespace here;… |
|
src/LanguageServer/Protocol/Extensions/Extensions.cs — Project file path comparisons should be OS-sensitive. Using StringComparison.OrdinalIgnoreCase… |
Pre-existing issues (3)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Protocol/ParsedUri.cs — s_singleSlashStart and s_doubleSlashStart are declared but never used in this port. If they’re… View comment |
|
src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/UriExtensions.cs — These #pragma warning disable comments include "TODO" text. This repo’s guidance is to avoid… View comment |
|
src/LanguageServer/Protocol/Protocol/DocumentUri.cs — DocumentUri(Uri) no longer seeds the cached parsed representation, so the first access to… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
src/LanguageServer/BannedSymbols.txt — This PR introduces a new core URI abstraction (ParsedUri) and also adds a new repo-level rule… View resolved comment |
Suppressed comments (2)
src/LanguageServer/Protocol/Protocol/DocumentUri.cs:49
Protocol.ParsedUri.Parse(...)looks like an accidental qualification; there is noProtocolalias/type here. This should callParsedUri.Parse(...)directly or it won’t compile.
src/LanguageServer/Protocol/Protocol/DocumentUri.cs:67- Obsolete message says "Use ParsedDocUri instead" but the property is named
ParsedDocumentUri. The message should match the API name to avoid confusion during migrations.
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Extensions/Extensions.cs — GetRequiredLegacySystemUri calls the obsolete DocumentUri.ParsedUri without suppressing CS0618… |
Pre-existing issues (3)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Protocol/ParsedUri.cs — s_singleSlashStart and s_doubleSlashStart are declared but never used in this port. If they’re… View comment |
|
src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/UriExtensions.cs — These #pragma warning disable comments include "TODO" text. This repo’s guidance is to avoid… View comment |
|
src/LanguageServer/Protocol/Protocol/DocumentUri.cs — DocumentUri(Uri) no longer seeds the cached parsed representation, so the first access to… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Extensions/Extensions.cs — Project file path comparisons should be OS-sensitive. Using StringComparison.OrdinalIgnoreCase… View resolved comment |
|
src/LanguageServer/Protocol/Protocol/DocumentUri.cs — The XML doc references Protocol.ParsedUri, but there is no Protocol alias/type/namespace here;… View resolved comment |
Suppressed comments (3)
src/LanguageServer/Protocol/Protocol/DocumentUri.cs:68
- The Obsolete message references "ParsedDocUri", but the property is now named ParsedDocumentUri. This reads like a typo and makes guidance to callers unclear.
src/LanguageServer/Protocol/Extensions/Extensions.cs:228 - GetProject now compares project.FilePath to the request URI path using OrdinalIgnoreCase unconditionally. That can produce incorrect matches on case-sensitive file systems (Linux/macOS) and differs from Roslyn’s established path comparison behavior. Use PathUtilities.Comparer (OS-appropriate) instead.
public static Project? GetProject(this Solution solution, TextDocumentIdentifier projectIdentifier)
{
// We need to parse the URI (scheme, file path) to be able to lookup the URI in the solution.
var parsedDocumentUri = projectIdentifier.DocumentUri.ParsedDocumentUri;
if (parsedDocumentUri is null)
{
return null;
}
var projects = solution.Projects.WhereAsArray(project => string.Equals(project.FilePath, parsedDocumentUri.FsPath, StringComparison.OrdinalIgnoreCase));
return !projects.Any()
src/LanguageServer/Protocol/Extensions/ProtocolConversions.cs:635
- HelpLinkToCodeDescription now wraps any non-null string into a DocumentUri without validating that it’s an absolute URI. This is a behavior change from the prior Uri.TryCreate-based validation (via DiagnosticData.GetValidHelpLinkUri) and can accidentally emit invalid/empty href values (e.g., empty string becomes a parsed file URI under non-strict parsing). Consider validating with ParsedUri (strict for URLs, and ParsedUri.File for absolute file paths) and returning null on parse failure.
public static LSP.CodeDescription? HelpLinkToCodeDescription(string? helpLinkUri)
{
return (helpLinkUri != null) ? new LSP.CodeDescription { Href = new DocumentUri(helpLinkUri) } : null;
}
| #pragma warning disable RS0030 // Intentional for backwards compatibility. | ||
| var parsedUri = documentUri.ParsedUri; | ||
| Contract.ThrowIfNull(parsedUri, $"URI {documentUri} could not be parsed"); | ||
| #pragma warning restore RS0030 // Do not use banned APIs | ||
| return parsedUri; |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
It introduces at least one correctness issue in path comparison (OrdinalIgnoreCase instead of PathUtilities.Comparer) and contains misleading obsolete guidance text that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Extensions/Extensions.cs — GetProject compares project.FilePath to parsedDocumentUri.FsPath using… |
|
src/LanguageServer/Protocol/Protocol/DocumentUri.cs — The [Obsolete] message for ParsedUri says "Use ParsedDocUri instead", but the property is named… |
Pre-existing issues (4)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Extensions/Extensions.cs — GetRequiredLegacySystemUri calls the obsolete DocumentUri.ParsedUri without suppressing CS0618… View comment |
|
src/LanguageServer/Protocol/Protocol/ParsedUri.cs — s_singleSlashStart and s_doubleSlashStart are declared but never used in this port. If they’re… View comment |
|
src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/UriExtensions.cs — These #pragma warning disable comments include "TODO" text. This repo’s guidance is to avoid… View comment |
|
src/LanguageServer/Protocol/Protocol/DocumentUri.cs — DocumentUri(Uri) no longer seeds the cached parsed representation, so the first access to… View comment |
| public static Project? GetProject(this Solution solution, TextDocumentIdentifier projectIdentifier) | ||
| { | ||
| // We need to parse the URI (scheme, file path) to be able to lookup the URI in the solution. | ||
| if (projectIdentifier.DocumentUri.ParsedUri is null) | ||
| var parsedDocumentUri = projectIdentifier.DocumentUri.ParsedDocumentUri; | ||
| if (parsedDocumentUri is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var projects = solution.Projects.WhereAsArray(project => project.FilePath == projectIdentifier.DocumentUri.ParsedUri.LocalPath); | ||
| var projects = solution.Projects.WhereAsArray(project => string.Equals(project.FilePath, parsedDocumentUri.FsPath, StringComparison.OrdinalIgnoreCase)); | ||
| return !projects.Any() | ||
| ? null | ||
| : FindItemInProjectContext(projects, projectIdentifier, projectIdGetter: (item) => item.Id, defaultGetter: () => projects[0]); |
| [Obsolete("Use ParsedDocUri instead. Tracking: https://github.com/dotnet/roslyn/issues/84785")] | ||
| public Uri? ParsedUri |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It introduces an OS-inappropriate case-insensitive project path comparison and contains a misleading Obsolete message that should be corrected.
Review tier: Lite
Findings: 1
Pre-existing issues (6)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Extensions/Extensions.cs — GetRequiredLegacySystemUri calls the obsolete DocumentUri.ParsedUri without suppressing CS0618… View comment |
|
src/LanguageServer/Protocol/Extensions/Extensions.cs — GetProject compares project.FilePath to parsedDocumentUri.FsPath using… View comment |
|
src/LanguageServer/Protocol/Protocol/DocumentUri.cs — The [Obsolete] message for ParsedUri says "Use ParsedDocUri instead", but the property is named… View comment |
|
src/LanguageServer/Protocol/Protocol/ParsedUri.cs — s_singleSlashStart and s_doubleSlashStart are declared but never used in this port. If they’re… View comment |
|
src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/UriExtensions.cs — These #pragma warning disable comments include "TODO" text. This repo’s guidance is to avoid… View comment |
|
src/LanguageServer/Protocol/Protocol/DocumentUri.cs — DocumentUri(Uri) no longer seeds the cached parsed representation, so the first access to… View comment |
Suppressed comments (2)
src/LanguageServer/Protocol/Protocol/DocumentUri.cs:68
- The Obsolete message refers to "ParsedDocUri", but the property is named ParsedDocumentUri. This makes the guidance misleading for callers.
src/LanguageServer/Protocol/Extensions/Extensions.cs:228 - Project file paths are compared using OrdinalIgnoreCase unconditionally. On Unix-like platforms this can incorrectly match the wrong project when paths differ only by casing; Roslyn generally uses PathUtilities.Comparer for path equality to respect platform semantics.
return null;
}
var projects = solution.Projects.WhereAsArray(project => string.Equals(project.FilePath, parsedDocumentUri.FsPath, StringComparison.OrdinalIgnoreCase));
return !projects.Any()



We've had a number of issues with using
System.Uriover the years, mostly caused by VSCode's URI handling being more lenient thanSystem.Uri(andSystem.Uribeing more strict than even the URI RFC requires). Most recently is dotnet/vscode-csharp#9087 which this should resolve.There've been a number of fixes, including implementing our own equality checks, delaying URI parsing until we absolutely require it, and others - but none have been perfect - we continually see
System.Uriparsing failures.The goal of this PR is to switch to VSCode's URI parsing behavior (from vscode-uri) and avoid using
System.Urialtogether due to the number of issues we've had. This should ensure that any URI we get from VSCode we can parse.Mostly - this is a server-side behavior change. However this is visible to clients as this does change how we convert a URI into a string for serialization. These now serialize using vscode-uri semantics, so things like drive letters will get encoded (
c:/->file:///c%3A/).System.Urileft the colon in the drive letter unencoded in the URI string.We also cannot entirely delete
System.Urihandling from the LSP path yet - XAML EA APIs and various parts of Razor still rely onSystem.Uri- however we can limit usage to just those scenarios.The implementation of ParsedUri is intended to match
vscode-uri. For examples and details on how URIs are parsed, how they are equal, and how they round trip, see ParsedUriTests.[ x ] - vsc
[ x ] - vs
[ ] - copilot
Microsoft Reviewers: Open in CodeFlow