Ensure we always get up to date diagnostics when getting pull diagnostics.#49256
Ensure we always get up to date diagnostics when getting pull diagnostics.#4925613 commits merged intodotnet:masterfrom
Conversation
| // prioritize the files from currently active projects, but then also include all other docs in all projects | ||
| // (depending on current FSA settings). | ||
|
|
||
| // Oen docs will not be included as those are handled by DocumentPullDiagnosticHandler. |
There was a problem hiding this comment.
| // Oen docs will not be included as those are handled by DocumentPullDiagnosticHandler. | |
| // Open docs will not be included as those are handled by DocumentPullDiagnosticHandler. |
| // For closed files, go to the IDiagnosticService for results. These won't necessarily be totally up to | ||
| // date. However, that's fine as these are closed files and won't be in the process of being edited. So | ||
| // any deviations in the spans of diagnostics shouldn't be impactful for the user. | ||
| var diagnostics = this.DiagnosticService.GetPullDiagnostics(document, includeSuppressedDiagnostics: false, diagnosticMode, cancellationToken); |
There was a problem hiding this comment.
this split actually makes a fair amount of sense to me then. Workspace diagnostics use whatever is in the workspace (which the server owns as these are closed files) whereas files with content managed via LSP (open) force a calculation based on the client's known content.
The only issue I can see is if the content in the open file changes the workspace diagnostics (e.g. delete a method in an open file that's used elsewhere). I guess in that case the experience would that the error list could show stale errors until the next workspace/diagnostic message comes in that acts on the new workspace content.
But squiggles are always fine as they act on open documents (and therefore LSP content)
| { | ||
| [DisallowNull] | ||
| private Solution? _currentSolution; | ||
| private TestWorkspace? _workspace; |
| Document document, Option2<DiagnosticMode> diagnosticMode, CancellationToken cancellationToken) | ||
| { | ||
| // We only support doc diagnostics for open files. | ||
| if (!document.IsOpen()) |
There was a problem hiding this comment.
It looks like this is using the workspace notion of a document being opened, which is currently tied to the textbuffer being opened via VS. It is currently true that document textbuffers are opened on the server via VS, but may not be forever (and may not be exactly sync'd with LSP open events similarly to didChange).
Perhaps instead for the LSP handlers, we should check if we've received a didOpen event for the document (basically if it's a member of this - https://github.com/dotnet/roslyn/blob/master/src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.DocumentChangeTracker.cs#L24 ). Same in the workspace handler
There was a problem hiding this comment.
yup. i've changed this now.
| // we're passing in. If information is already cached for that snapshot, it will be returned. Otherwise, | ||
| // it will be computed on demand. Because it is always accurate as per this snapshot, all spans are correct | ||
| // and do not need to be adjusted. | ||
| return _analyzerService.GetDiagnosticsAsync(document.Project.Solution, documentId: document.Id, cancellationToken: cancellationToken); |
There was a problem hiding this comment.
this will re-calculate every time until we fix the fork on every request behavior right?
|
|
||
| #nullable disable | ||
|
|
||
| using System; |
There was a problem hiding this comment.
is this needed? Looks like there aren't other changes in this file
davidwengier
left a comment
There was a problem hiding this comment.
This makes sense to me. Am I right in guessing that the don't-fork-on-every-request doesn't require any changes here, it just makes it faster/more efficient?
| Document document, Option2<DiagnosticMode> diagnosticMode, CancellationToken cancellationToken) | ||
| { | ||
| // We only support doc diagnostics for open files. | ||
| if (!document.IsOpen()) |
There was a problem hiding this comment.
Is this going to work for Razor? Does the LSP didOpen handler need to do something to tell the workspace the document is open (as part of the server per workspace changes, not this PR)?
There was a problem hiding this comment.
i've switched this to be based on LSP semantics now.
| if (diagnosticParams.TextDocument != null) | ||
| { | ||
| var document = _solutionProvider.GetDocument(diagnosticParams.TextDocument); | ||
| var document = context.Solution.GetDocument(diagnosticParams.TextDocument); |
Yup. |
|
Hello @CyrusNajmabadi! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
This splits our LSP diagnostics into two effective 'worlds'. A client can get diagnostics for open documents (specifically, documents they sent 'didOpen' for, and for closed diagnostics (aka. Workspace-Diagnostics). For open diagnostics we always grab the diagnostics for the current contents of hte file, ensuring that we never have to worry about syncing mismatches, albeit with the cost of computing the diagnostics right htere, and not with the rest of the diagnostics subsystem.
For Workspce-Diagnostics, we still defer to the normal diagnostics subsystem, meaning we can get stale data. This is ok though as that's for closed files and in general we don't need the diagnostics to strictly be up to date, as long as they eventually get up to date.