Flip 'pull diagnostics' to be on by default. #70040

CyrusNajmabadi · 2023-09-20T20:03:58Z

We've had this on internally for a while, and since all recent fixes have gone through, have not heard any complaints about the diagnostics experience.

We'd like to enable this early for everyone to get this out there for everyone to use. Note: this comes with a feature-flag we can use to switch back to the non-pull (solutioncrawler model) in case we discover issues. Once we've had enough time with people using the default (like a full point release), we can start removing the older systems.

PR: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=8713552&view=results
Build: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/512411

PR: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=8730199&view=results

sharwell · 2023-09-20T20:55:32Z

The text "experimental" needs to be removed from the option name at the same time.

CyrusNajmabadi · 2023-09-20T20:56:39Z

The text "experimental" needs to be removed from the option name at the same time.

Indeed!

sharwell · 2023-09-20T21:03:04Z

Filed #70042, #70044 based on initial impression when I tried turning this on

sharwell

Requesting this be blocked on #65967. In its current form, LSP pull diagnostics does not work at all.

Fixes dotnet#65967 Prior to this change, Run Code Analysis command force executed all the analyzers on the selected project/solution, but the reported analyzer diagnostics were not getting reported from LSP pull handlers. This PR makes the following changes to ensure these diagnostics get reported as workspace pull diagnostics: 1. Modify WorkspacePullDiagnosticHandler so that it reports diagnostics when either of the following conditions are met: 1. Full solution analysis is enabled (it was already doing so prior to this PR) 2. Project was already force analyzed and has cached diagnostics (it could be via Run Code Analysis command or any other client in future). 2. Update the Workspace and Project diagnostic sources added by the Workspace pull handler to do the following: 1. If full solution analysis is enabled, compute and report the closed file diagnostics for the latest solution snapshot 2. Otherwise, return the already cached closed file diagnostics, which may be from a prior snapshot (this matches the non-LSP behavior when Run Code Analysis command is executed) 3. Update the `DiagnosticAnalyzerService.ForceAnalyzeAsync` command that is invoked during Run Code Analysis to keep track of projects that were force analyzed. Additionally, also send a workspace diagnostic refresh request to the LSP client after each project has finished analysis. With the above changes, Run Code Analysis command executes and reports diagnostics as expected in LSP pull diagnostics mode. There is still a considerable delay (10 sec) in error list getting populated with diagnostics from this command as the workspace diagnostic refresh is triggered at a delay. We will be addressing this in a separate follow-up change, where in we will trigger a new kind of diagnostic refresh request specifically catered to code analysis execution, such that the client does the workspace pull without any delay. We are still unsure if we need to pass in the project context around in this refresh request OR let it be a full refresh, given the server is only going to attempt to return workspace diagnostics from the cache, so a full refresh request may not incur any significant cost. TBD: Add unit tests for this scenario. I am going to do this in a follow-up PR so that dotnet#70040 does not stay blocked on the product changes here.

Changes have gone in on the platform and in CPS.

sharwell · 2023-10-17T13:23:01Z

I have been really trying to see a case where Pull Diagnostics works, but to date I haven't been able to use it for more than a few minutes before I reach a time box and have to turn it off to get work done. I'll give it another try this week, but I haven't seen any evidence to suggest this feature is ready for delivery.

dibarbet

speedometer passed, we still have the feature flag we can use to turn this off.
OK with this going in, just need to make sure the feature flag is sync'd with this default

src/VisualStudio/IntegrationTest/New.IntegrationTests/CSharp/CSharpCodeActions.cs

sharwell · 2023-12-21T21:05:55Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/ErrorListInProcess.cs

@@ -122,7 +121,7 @@ private static string GetMessage(ITableEntryHandle item)
                var unknown => unknown.ToString(),
            };

-            var message = $"({source}) {document}({line + 1}, {column + 1}): {severity} {errorCode}: {text}";


Why was this changed?

'source' is not a thing anymore. we previously placed it in this info just so we could distinguish if we made the error item based on live items vs build items.

sharwell · 2023-12-21T21:08:05Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

+            var tags = aggregator
+                .GetTags(new SnapshotSpan(view.TextSnapshot, 0, view.TextSnapshot.Length));
+
+            return tags.SelectAsArray(tag => new TagSpan<IErrorTag>(tag.Span.GetSpans(view.TextBuffer).Single(), tag.Tag));


How is this different from what it did before?

the other codepath converts the tags to IMappingTagSpans, which these new editor tags do not impl.

sharwell · 2023-12-21T21:08:29Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorVerifierInProcess.cs

-            Assert.Equal(expectedTags.Length, actualTags.Length);
-            for (var i = 0; i < expectedTags.Length; i++)
+            while (true)
            {
-                var expectedTag = expectedTags[i];
-                var actualTaggedSpan = actualTags[i];
-                Assert.Equal(expectedTag.errorType, actualTaggedSpan.Tag.ErrorType);
-                Assert.Equal(expectedTag.textSpan.Start, actualTaggedSpan.Span.Start.Position);
-                Assert.Equal(expectedTag.textSpan.Length, actualTaggedSpan.Span.Length);
-
-                var actualTaggedText = actualTaggedSpan.Span.GetText();
-                Assert.Equal(expectedTag.taggedText, actualTaggedText);
-
-                AssertEx.NotNull(actualTaggedSpan.Tag.ToolTipContent);
-                var containerElement = (ContainerElement)actualTaggedSpan.Tag.ToolTipContent;
-                var actualTooltipText = CollectTextInRun(containerElement);
-                Assert.Equal(expectedTag.tooltipText, actualTooltipText);
+                cancellationToken.ThrowIfCancellationRequested();
+                await Task.Delay(TimeSpan.FromSeconds(1));


Need to wait directly for pull diagnostics

CyrusNajmabadi · 2024-01-23T00:58:02Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

+            // return GetTagsAsync<IErrorTag>(cancellationToken);
+


Suggested change

// return GetTagsAsync<IErrorTag>(cancellationToken);

CyrusNajmabadi · 2024-01-25T21:53:52Z

@dibarbet this has been merged in. We'll track the insertion to deal with targeted notifications.

dibarbet · 2024-01-29T19:20:06Z

submitted the targeted notification change to get CPS onto the same place
(internal only link - https://targetednotifications.vsdata.io/rule/e7b1535a-03d7-4d4e-8254-418517d57cfc)

Flip 'pull diagnostics' to be on by default.

4cfed74

CyrusNajmabadi requested a review from a team as a code owner September 20, 2023 20:03

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 20, 2023

CyrusNajmabadi requested a review from dibarbet September 20, 2023 20:05

CyrusNajmabadi added 14 commits September 20, 2023 14:22

Update UI

92f82b7

Update UI

2a3d19d

Fixup tests

3e2dc5f

Fixup tests

e4bf028

Fixup tests

1223332

Fixup tests

a4e1212

Disable

a5b2c5b

Update tests

f83763f

remove

1b79cd5

Add

ce1cde9

simpler

0d29887

simpler

1e6a382

Merge remote-tracking branch 'upstream/main' into enablePullDiagnostics

4ec003a

Update tests

9deb1b6

build-analysis bot mentioned this pull request Sep 21, 2023

Microsoft.DotNet.Build.Tasks.Workloads.Tests.MsiTests.ItCanBuildAManifestMsi test failure in Arcade dotnet/arcade#14046

Closed

Workaround

ed50062

sharwell previously requested changes Sep 22, 2023

View reviewed changes

mavasani mentioned this pull request Sep 29, 2023

Enable LSP pull diagnostics from Run Code Analysis command #70186

Merged

Merge remote-tracking branch 'upstream/main' into enablePullDiagnostics

9d8b34a

Merge branch 'sourceTextChecksum' into enablePullDiagnostics

cfe048b

CyrusNajmabadi requested review from a team as code owners November 20, 2023 16:32

CyrusNajmabadi added 5 commits November 20, 2023 16:56

Merge remote-tracking branch 'upstream/main' into enablePullDiagnostics

8294eb1

Merge branch 'main' into enablePullDiagnostics

eec1cc1

Merge remote-tracking branch 'upstream/main' into enablePullDiagnostics

d23900c

Fixup test

14eece1

inline

03a4d3d

CyrusNajmabadi requested a review from dibarbet November 27, 2023 21:23

dibarbet approved these changes Nov 27, 2023

View reviewed changes

CyrusNajmabadi and others added 3 commits December 11, 2023 12:56

Merge remote-tracking branch 'upstream/main' into enablePullDiagnostics

97734b8

Merge branch 'main' into enablePullDiagnostics

907ac99

Merge branch 'main' into enablePullDiagnostics

283811b

sharwell reviewed Dec 21, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into enablePullDiagnostics

664b59f

CyrusNajmabadi commented Jan 23, 2024

View reviewed changes

CyrusNajmabadi added 4 commits January 22, 2024 16:58

Cleanup

a8c97d7

Merge branch 'simplifyTagTests' into enablePullDiagnostics

68eb904

Merge branch 'main' into enablePullDiagnostics

d123879

Make tests explicit that they are in push-diagnostics mode

8371a9f

CyrusNajmabadi mentioned this pull request Jan 23, 2024

Make tests explicit that they are in push-diagnostics mode #71775

Merged

Merge branch 'explicitTests' into enablePullDiagnostics

7343bf9

CyrusNajmabadi merged commit 13d160d into dotnet:main Jan 25, 2024
28 checks passed

CyrusNajmabadi deleted the enablePullDiagnostics branch January 25, 2024 21:53

ghost added this to the Next milestone Jan 25, 2024

Cosifne modified the milestones: Next, 17.10 P1 Jan 30, 2024

Cosifne mentioned this pull request Feb 2, 2024

[Automated] PRs inserted in VS build main-34526.79 #71943

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flip 'pull diagnostics' to be on by default. #70040

Flip 'pull diagnostics' to be on by default. #70040

CyrusNajmabadi commented Sep 20, 2023 •

edited

Loading

sharwell commented Sep 20, 2023

CyrusNajmabadi commented Sep 20, 2023

sharwell commented Sep 20, 2023 •

edited

Loading

sharwell left a comment

sharwell commented Oct 17, 2023

dibarbet left a comment

sharwell Dec 21, 2023

CyrusNajmabadi Jan 23, 2024

sharwell Dec 21, 2023

CyrusNajmabadi Jan 23, 2024

sharwell Dec 21, 2023

CyrusNajmabadi Jan 23, 2024

CyrusNajmabadi commented Jan 25, 2024

dibarbet commented Jan 29, 2024

Flip 'pull diagnostics' to be on by default. #70040

Flip 'pull diagnostics' to be on by default. #70040

Conversation

CyrusNajmabadi commented Sep 20, 2023 • edited Loading

sharwell commented Sep 20, 2023

CyrusNajmabadi commented Sep 20, 2023

sharwell commented Sep 20, 2023 • edited Loading

sharwell left a comment

Choose a reason for hiding this comment

sharwell commented Oct 17, 2023

dibarbet left a comment

Choose a reason for hiding this comment

sharwell Dec 21, 2023

Choose a reason for hiding this comment

CyrusNajmabadi Jan 23, 2024

Choose a reason for hiding this comment

sharwell Dec 21, 2023

Choose a reason for hiding this comment

CyrusNajmabadi Jan 23, 2024

Choose a reason for hiding this comment

sharwell Dec 21, 2023

Choose a reason for hiding this comment

CyrusNajmabadi Jan 23, 2024

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 25, 2024

dibarbet commented Jan 29, 2024

CyrusNajmabadi commented Sep 20, 2023 •

edited

Loading

sharwell commented Sep 20, 2023 •

edited

Loading