Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Feb 23, 2023

SolutionCrawler here serves no purpose. The preview window is already only in one of two modes:

  1. Roslyn-Native-Pull-Diagnostic-Tagging. This has been how tagging has worked in roslyn since Move diagnostic tagging to 'pull diagnostics' #63858. In this world, diagnostics taggers just pull directly from teh diagnostic subsystem, and do not need the crawler to push results into them.
  2. LSP-pull-diagnostics. The future (and enabled for a subset of people). In this world the preview window doesn't get any diagnostics anyways as there's no channel to feed that information back to the client. We'll likely have to work through an LSP-native way to get this to work. But, regardless, that won't be Solution-Crawler-Based anyways.

So yaay, we just get to ditch this.

There is still work to be done to get the pull-tagging in the preview window to do the computation in OOP, not in proc. But that's upcoming.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 23, 2023 23:24

[WorkItem(923196, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/923196")]
[WpfFact]
public async Task TestPreviewDiagnostic()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pull tagger test is below. this one was testing the push/solution-crawler version.

workspace.TryApplyChanges(workspace.CurrentSolution.WithAnalyzerReferences(new[] { DiagnosticExtensions.GetCompilerDiagnosticAnalyzerReference(LanguageNames.CSharp) }));

// set up listener to wait until diagnostic finish running
_ = workspace.ExportProvider.GetExportedValue<IDiagnosticService>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was previously using push. updated to pull below.

// We rely on LSP to query us for diagnostics when things have changed and poll us for changes that might
// have happened to the project or closed files outside of VS. However, we still need to create the analyzer
// so that the map contains the analyzer to run when pull diagnostics asks.
return GlobalOptions.IsLspPullDiagnostics() ? NoOpIncrementalAnalyzer.Instance : analyzer;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same logic a before. i just rejiggered. in both paths below we were adding hte item to the map. i pulled that above, and then just changed the logic if the NoOp analyzer is returned or the actual instance.

@mavasani
Copy link
Contributor

Merging to facilitate lightbulb performance measurements with this change.

@mavasani mavasani merged commit dd8fb67 into dotnet:main Feb 24, 2023
@ghost ghost added this to the Next milestone Feb 24, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the previewCrawler branch February 27, 2023 23:44
@RikkiGibson RikkiGibson modified the milestones: Next, 17.6 P2 Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants