Ensure we always get up to date diagnostics when getting pull diagnostics. #49256

CyrusNajmabadi · 2020-11-10T02:25:42Z

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.

dibarbet · 2020-11-11T23:08:08Z

src/Features/LanguageServer/Protocol/Handler/Diagnostics/WorkspacePullDiagnosticHandler.cs

+            // 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.


Suggested change

// Oen docs will not be included as those are handled by DocumentPullDiagnosticHandler.

// Open docs will not be included as those are handled by DocumentPullDiagnosticHandler.

dibarbet · 2020-11-11T23:15:56Z

src/Features/LanguageServer/Protocol/Handler/Diagnostics/WorkspacePullDiagnosticHandler.cs

+            // 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);


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)

dibarbet · 2020-11-11T23:16:18Z

src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs

@@ -47,29 +47,28 @@ public abstract class AbstractLanguageServerProtocolTests
        internal class TestLspSolutionProvider : ILspSolutionProvider
        {
            [DisallowNull]
-            private Solution? _currentSolution;
+            private TestWorkspace? _workspace;


thanks for fixing this!

dibarbet · 2020-11-11T23:20:14Z

src/Features/LanguageServer/Protocol/Handler/Diagnostics/DocumentPullDiagnosticHandler.cs

+            Document document, Option2<DiagnosticMode> diagnosticMode, CancellationToken cancellationToken)
+        {
+            // We only support doc diagnostics for open files.
+            if (!document.IsOpen())


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

yup. i've changed this now.

dibarbet · 2020-11-11T23:21:22Z

src/Features/LanguageServer/Protocol/Handler/Diagnostics/DocumentPullDiagnosticHandler.cs

+            // 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);


this will re-calculate every time until we fix the fork on every request behavior right?

dibarbet · 2020-11-11T23:21:45Z

src/Workspaces/CSharpTest/OrganizeImports/OrganizeUsingsTests.cs

@@ -4,6 +4,7 @@

 #nullable disable

+using System;


is this needed? Looks like there aren't other changes in this file

davidwengier

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?

davidwengier · 2020-11-11T23:22:13Z

src/Features/LanguageServer/Protocol/Handler/Diagnostics/DocumentPullDiagnosticHandler.cs

+            Document document, Option2<DiagnosticMode> diagnosticMode, CancellationToken cancellationToken)
+        {
+            // We only support doc diagnostics for open files.
+            if (!document.IsOpen())


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)?

i've switched this to be based on LSP semantics now.

davidwengier · 2020-11-11T23:22:36Z

src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs

        {
            var result = new Dictionary<Document, DiagnosticParams>();
            foreach (var diagnosticParams in previousResults)
            {
                if (diagnosticParams.TextDocument != null)
                {
-                    var document = _solutionProvider.GetDocument(diagnosticParams.TextDocument);
+                    var document = context.Solution.GetDocument(diagnosticParams.TextDocument);


Best change ever <3

CyrusNajmabadi · 2020-11-11T23:24:40Z

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?

Yup.

ghost · 2020-11-12T02:47:51Z

Hello @CyrusNajmabadi!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost

Auto-approval

Experiment with getting live diagnostics

3c5a74d

Dotnet-GitSync-Bot added the Area-IDE label Nov 10, 2020

CyrusNajmabadi added 2 commits November 10, 2020 07:27

Merge remote-tracking branch 'upstream/master' into liveDiag

c112ab5

Sync text changes

ef55030

CyrusNajmabadi changed the title ~~Experiment with getting live diagnostics~~ Ensure we always get up to date diagnostics when getting pull diagnostics. Nov 11, 2020

CyrusNajmabadi added 5 commits November 11, 2020 10:13

Update tests

45bae63

Update tests

6f4b1b5

Updates.

7445c7e

Remove

c2de03a

Merge remote-tracking branch 'upstream/master' into liveDiag

a2516a6

CyrusNajmabadi requested review from dibarbet and davidwengier November 11, 2020 23:01

CyrusNajmabadi marked this pull request as ready for review November 11, 2020 23:01

CyrusNajmabadi requested a review from a team as a code owner November 11, 2020 23:01

dibarbet reviewed Nov 11, 2020

View reviewed changes

trust lsp for open/closed info.

9291fd3

davidwengier approved these changes Nov 11, 2020

View reviewed changes

dibarbet approved these changes Nov 11, 2020

View reviewed changes

CyrusNajmabadi added 4 commits November 11, 2020 16:26

Fix warning

da7b1ec

Formatting

6caa4cb

Add file path.

51be8aa

Fix mapped spans

b81ab58

CyrusNajmabadi added the auto-merge label Nov 12, 2020

ghost approved these changes Nov 12, 2020

View reviewed changes

davidwengier mentioned this pull request Nov 12, 2020

Reuse LSP solutions if they don't need re-forking #49305

Merged

ghost merged commit a351a14 into dotnet:master Nov 12, 2020

ghost added this to the Next milestone Nov 12, 2020

CyrusNajmabadi deleted the liveDiag branch November 12, 2020 16:53

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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. #49256

CyrusNajmabadi commented Nov 10, 2020 •

edited

Loading

dibarbet Nov 11, 2020

dibarbet Nov 11, 2020

dibarbet Nov 11, 2020

dibarbet Nov 11, 2020

CyrusNajmabadi Nov 11, 2020

dibarbet Nov 11, 2020

CyrusNajmabadi Nov 11, 2020

dibarbet Nov 11, 2020

davidwengier left a comment

davidwengier Nov 11, 2020

CyrusNajmabadi Nov 11, 2020

davidwengier Nov 11, 2020

CyrusNajmabadi commented Nov 11, 2020

ghost commented Nov 12, 2020

ghost left a 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.

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. #49256

Conversation

CyrusNajmabadi commented Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 11, 2020

ghost commented Nov 12, 2020

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 (@msftbot) and give me an instruction to get started! Learn more here.

ghost left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 10, 2020 •

edited

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.