Fix document diagnostics document open check #79298
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dibarbet
force-pushed
the
diagnostics_casing
branch
from
c59fb07 to
f6d4d97
Compare
dibarbet
changed the title
dibarbet
force-pushed
the
diagnostics_casing
branch
2 times, most recently
from
b79547c to
5f1e719
Compare
dibarbet
force-pushed
the
diagnostics_casing
branch
from
5f1e719 to
805447c
Compare
dibarbet
commented
Jul 9, 2025
| { | ||
| context.TraceDebug("Ignoring diagnostics request because no text document was provided"); | ||
| return new([]); | ||
| } | ||
|
|
||
| if (!context.IsTracking(textDocument.GetURI())) |
There was a problem hiding this comment.
this was the issue - for non-unc paths it does a case sensitive comparison, so the URI created from the file path on disk did not match the opened URI (vscode normalizes it).
instead we use the request uri which per the LSP spec must be consistent with the open URI
dibarbet
commented
Jul 9, 2025
| } | ||
|
|
||
| return new([]); | ||
| return new([new DocumentDiagnosticSource(kind, context.GetRequiredDocument())]); |
There was a problem hiding this comment.
the AbstractDocumentPullDiagnosticHandler.cs code above already verifies that the document is tracked. No need to do it again.
| var textDocument = context.TextDocument; | ||
| if (textDocument is null) | ||
| var identifier = GetTextDocumentIdentifier(diagnosticsParams); | ||
| if (identifier is null || context.Document is null) | ||
| { | ||
| context.TraceDebug("Ignoring diagnostics request because no text document was provided"); | ||
| return new([]); | ||
| } |
There was a problem hiding this comment.
consider breaking this into two checks for better tra ing.
CyrusNajmabadi
approved these changes
Jul 9, 2025
JoeRobich
approved these changes
Jul 9, 2025
dibarbet
added a commit
that referenced
this pull request
Jul 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves dotnet/vscode-csharp#8400
VSCode normalizes URIs and always uses the URI based on when the file was first opened (even if it is renamed to change casing). See microsoft/vscode-languageserver-node#1186. So VSCode sends the server a URI that may not match the casing of file on disk.
While we already ignore casing when looking up file paths in the workspace (and therefore find the correct document in the workspace for requests), we had an issue with diagnostics when checking if the file was open. We were comparing a URI created from the file with the URI VSCode sends us. With UNC URIs (windows file paths), case sensitivity is ignored when comparing paths, and so it doesn't matter. However with unix file paths, case sensitivity is not ignored and so the file path URI and VSCode open URI did not match.
The fix here is to instead use the request URI we were given to check if the document is open, instead of attempting to derive it from the file path.