Remove portions of solution crawler #72147
Conversation
|
@mavasani ptal. |
| using Roslyn.Utilities; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.LegacySolutionEvents; | ||
|
|
||
| /// <summary> | ||
| /// Event listener that hears about workspaces and exists solely to let unit testing continue to work using their own |
There was a problem hiding this comment.
exists solely to let unit testing continue to work
Is it possible to move this type to test project?
There was a problem hiding this comment.
Sorry. By "unit testing" I don't mean "our unit tests". I mean "the unit testing team". They have a crawler to discover tests. But it's extremely trimmed down, and owned by them.
src/EditorFeatures/Test/Diagnostics/DiagnosticAnalyzerServiceTests.cs
Outdated
Show resolved
Hide resolved
| { | ||
| using var workspace = CreateWorkspace(); | ||
| Assert.True(workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions( | ||
| workspace.CurrentSolution.Options.WithChangedOption(new OptionKey(DiagnosticOptionsStorage.PullDiagnosticsFeatureFlag), false)))); | ||
|
|
||
| var globalOptions = GetGlobalOptions(workspace); | ||
| globalOptions.SetGlobalOption(SolutionCrawlerOptionsStorage.BackgroundAnalysisScopeOption, LanguageNames.CSharp, analysisScope); |
There was a problem hiding this comment.
What is the equivalent of BackgroundAnalysisScopeOption in lsp-pull diags world? Is this option now respected by the lsp clients? If not, this will cause a functionality regression.
It also seems concerning that we are using test coverage for this option, which we still need to support in lsp-pull diagnostics world. Are we tracking porting these tests over to the lsp client that supports this option?
There was a problem hiding this comment.
Answering offline.
| @@ -61,17 +59,6 @@ public async Task RunAsync(CancellationToken cancellationToken) | |||
| throw new InvalidOperationException("Benchmark is not configured to use persistent storage."); | |||
| } | |||
| } | |||
|
|
|||
| var incrementalAnalyzerProviders = exportProvider.GetExports<IIncrementalAnalyzerProvider, IncrementalAnalyzerProviderMetadata>(); | |||
| foreach (var incrementalAnalyzerName in _options.IncrementalAnalyzerNames) | |||
There was a problem hiding this comment.
I believe we will also need to make changes to the supported command line arguments for AnalyzerRunner to no longer support IncrementalAnalyzerNames option. FYI @sharwell
| @@ -106,7 +106,7 @@ public static async Task Main(string[] args) | |||
| { | |||
| if (!string.IsNullOrEmpty(options.ProfileRoot)) | |||
| { | |||
| ProfileOptimization.StartProfile(nameof(Microsoft.CodeAnalysis.SolutionCrawler.IIncrementalAnalyzer)); | |||
| ProfileOptimization.StartProfile("IncrementalAnalyzer"); | |||
There was a problem hiding this comment.
Is this still required? Can't we delete the incremental analyzer runner completely? Can be done in a follow-up PR though..
There was a problem hiding this comment.
yeah. can delete later.
| @@ -10,6 +10,7 @@ | |||
| using System.Threading; | |||
| using System.Threading.Tasks; | |||
| using Microsoft.CodeAnalysis; | |||
| using Microsoft.CodeAnalysis.Diagnostics; | |||
There was a problem hiding this comment.
I think we can delete this entire file and even the IncrementalAnalyzerNames option
| // user might have built solution before workspace fires its first event yet (which is when solution crawler is initialized) | ||
| // here we give initializeLazily: false so that solution crawler is fully initialized when we do de-dup live and build errors, | ||
| // otherwise, we will think none of error we have here belong to live errors since diagnostic service is not initialized yet. | ||
| if (_diagnosticService.GlobalOptions.GetOption(SolutionCrawlerRegistrationService.EnableSolutionCrawler)) |
There was a problem hiding this comment.
Wonder what purpose is served by ExternalErrorDiagnosticUpdateSource in lsp-pull only world. I thought it had two purposes - build/live de-duping and reporting build-only diagnostics, neither of which are owned by us in lsp-pull diagnostics world.
There was a problem hiding this comment.
i wanted to check on that with yuo. i'm totally good with removing this :) i just wanted certain if there was something i was missing here.
There was a problem hiding this comment.
This is amazing! So happy to see the solution crawler finally go away :-D
The product changes seem all reasonable, with further scope of cleanup in the follow-up PRs. My primary concern would be the test holes being opened up for background analysis scope option, I am assuming that the lsp clients now own this option and testing part. Can we file a tracking issue for porting over some of these tests to the lsp client?
Now that we've moved to only having pull diagnostics, we no longer need solution crawler. This PR aims to remove a large swath of that component that is definitely not needed. Unfortunately, the actual diagnostic-analyzer-component is intertwined with the solution-crawler system, so it's not as simple as just deleting everything.
Intuitively, this PR removes the portions of solution crawler related to 'WorkCoordinator' and all the things it calls. WorkCoordinator is the piece that actually tracks all users actions and all workspace changes, and then comes up with a plan for how to process all of those and use them to drive the "crawl" over the solution. This is completely unneeded with pull-diagnostics as the LSP client drives thsi by askign us exactly what they want.