Add "Active file" analysis scope for background analysis in the IDE #39699

mavasani · 2019-11-05T22:54:19Z

Replaces the existing "Full Solution Analysis" option, with a background analysis scope to allow users to control the analysis scope for all solution crawler based background analysis (except for Test explorer's unit testing incremental analyzer, which will always execute for full solution).

Fixes #38429. Active file scope minimizes all the background analysis to the active files, saving on CPU consumption from background analysis on remaining files in the solution.

I have also updated up the low virtual memory listener which detects low VM and used to turn off just the full solution analysis to now set the background analysis scope to "Active file" for the current session.

TODOs:

~~Add unit tests and integration tests for new analysis scope~~
~~Update DesignerAttributeIncrementalAnalyzer to work at single file level for active file analysis.~~

Replaces the existing "Full Solution Analysis" option, with a background analysis scope to allow users to control the analysis scope for all solution crawler based background analysis. Fixes dotnet#38429. Active file scope minimizes all the background analysis to the active files, saving on CPU consumption from background analysis on remaining files in the solution. I have also updated up the low virtual memory listener which detects low VM and used to turn off just the full solution analysis to now set the background analysis scope to "Active file" for the current session.

shyamnamboodiripad · 2019-11-05T23:09:22Z

src/Workspaces/Core/Portable/SolutionCrawler/IIncrementalAnalyzerExtensions.cs

+            if (incrementalAnalyzer.GetType().FullName == "Microsoft.CodeAnalysis.UnitTesting.SourceBasedTestDiscovery.SourceBasedTestDiscoveryIncrementalAnalyzer")
+            {
+                return BackgroundAnalysisScope.FullSolution;
+            }


@Cosifne After we merge the change to consume the unit testing IVTs, we should remove the above block of code. Perhaps this can be done as part of the second cleanup PR you were planning for the wrappers...

CyrusNajmabadi · 2019-11-06T23:18:06Z

Overall, i like your OP! A few questions:

I have also updated up the low virtual memory listener which detects low VM and used to turn off just the full solution analysis to now set the background analysis scope to "Active file" for the current session.

Is there any notification to the user that this has happened? perhaps a yellow bar stating that low-resource conditions were encountered and some services have been throttled to help out?

Update DesignerAttributeIncrementalAnalyzer to work at single file level for active file analysis.

What doe that mean overall? Say i'm opening a project for the first time and tehre are designable items in it... how does VS learn about this? What's the flow here? (Feel free to provide gif if that will be clearer). Thanks!

(except for Test explorer's unit testing incremental analyzer, which will always execute for full solution).

Out of curiosity, does this at least run OOP?

src/EditorFeatures/Core/Implementation/TodoComment/TodoCommentIncrementalAnalyzer.cs

src/EditorFeatures/Test2/Diagnostics/DiagnosticProviderTests.vb

src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.Executor.cs

src/Features/Core/Portable/Shared/Options/ServiceFeatureOnOffOptions.cs

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.AsyncProjectWorkItemQueue.cs

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.HighPriorityProcessor.cs

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.NormalPriorityProcessor.cs

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.NormalPriorityProcessor.cs

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.cs

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.WorkItem.cs

genlu · 2019-11-14T22:09:02Z

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs

@@ -170,12 +247,12 @@ private void ReportPendingWorkItemCount()
                    var reasons = workItem.InvocationReasons;
                    if (workItem.MustRefresh || reasons.Contains(PredefinedInvocationReasons.SyntaxChanged))
                    {
-                        await RunAnalyzersAsync(analyzers, document, (a, d, c) => a.AnalyzeSyntaxAsync(d, reasons, c), cancellationToken).ConfigureAwait(false);
+                        await RunAnalyzersAsync(analyzers, document, workItem, (a, d, c) => a.AnalyzeSyntaxAsync(d, reasons, c), cancellationToken).ConfigureAwait(false);


nit: we can calculate applicable analyzers up front and reuse it for calls to RunAnalyzersAsync

Yes, I thought about it, but then felt it is much cleaner and reliable to keep the computation of applicable analyzers at one place just prior to the core execution. If we see any perf issues, I will follow your suggestion.

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs

genlu · 2019-11-14T22:31:42Z

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.HighPriorityProcessor.cs

+                    {
+                        get
+                        {
+                            lock (_gate)


Is the lock necessary for the read? i.e. are we protecting read from write, or just concurrent reads (which can trigger initialization)?

I copied most of this from existing code in AbstractPriorityProcessor. I will keep it the same way and be defensive and consistent.

genlu · 2019-11-14T23:04:10Z

src/EditorFeatures/Core/Implementation/TodoComment/TodoCommentIncrementalAnalyzer.cs

+            // Now update the task list if there are any changes from the previously computed TODO comments, if any.
+            // NOTE: We don't check for cancellation here to ensure the task list view and our latest
+            // computed state are identical.
+            if (existingData == null || existingData.Items.Length != newData.Items.Length)


Can existingData and newData has different items but same length?

Good point. However, this hasn't change as part of this PR, I am just refactoring the code in this PR. I will play around a bit with this and see if I can get a repro to drive this change.

genlu

The change to the core logic looks good to me, other than a few minor comments. But I skipped the UI part and didn't pay very close attention to the parts involves old/new options.

mavasani · 2019-11-15T00:08:34Z

Marking as blocked on testing signoff from the unit testing team.

…tch the error list terms in its scope filter combo box `Active file` -> `Current Document` `Open files and projects -> `Open Documents and Projects` `Full Solution` -> `Entire Solution`

heejaechang · 2019-11-15T21:25:27Z

I didn't read whole PR so it might be already handled. just curious since it looks like it reuse options for diagnostics to general background work..

so when user move to different tab (editor), do you enqueue reanalysis to old doc to remove errors? or you just leave them as they are?

asking since for "open files", when user close file, we explicitly remove errors for that file. (except errors from build)

it was done that way since, at the beginning, option said "open files only" and some people wanted things to work literally as option said. so we actively removed errors that don't belong to open files when that option is on.

now, we re-worded the option to describe scope in general sense rather than what it does literally.

so I am +1 on not touching errors already reported (even for open files case), but want to let you know the implication of that.

if user fixes a semantic data, (unknown name "A"), you open new file, create new type "A", the error - unknown name "A" in other file, will not go away until user switch back to that file.

my opinion is that is fine since that is the behavior of "build error" and with "active file or open file" scope, one is basically mixing "live and build" error behavior and get "build error" behavior for out of scope errors?

heejaechang · 2019-11-15T21:30:02Z

another approach is putting high priority on removing errors but not on adding errors.

basically, always consider any file that has errors as [files to analyze], and always analyze those when snapshot changed. that will make errors go away without users opening a file, but it won't make errors show up unless it is opened or active.

doesn't matter for full solution analysis. it will show up at some point.

mavasani · 2019-11-15T22:13:14Z

asking since for "open files", when user close file, we explicitly remove errors for that file. (except errors from build)

I tried to repro this on latest 16.5 builds (prior to my change) and this behavior seems to have changed. Closing the files does not remove its diagnostics from the error list. Lets see if anyone complains about it, and if so we can first try to ask them to change their error list scope combo box to open files. If they still disagree, we can fix it to get the old behavior back.

so when user move to different tab (editor), do you enqueue reanalysis to old doc to remove errors? or you just leave them as they are?

Yes, I do enqueue a work item with "OnDocumentClosed" invocation reasons for previous active document, but that does not seem to be removing the diagnostics for that document. I presume it will also start working if we fix the open files case above.

if user fixes a semantic data, (unknown name "A"), you open new file, create new type "A", the error - unknown name "A" in other file, will not go away until user switch back to that file.

Yes, that is true for active file scope, and confirms the reduced background analysis scope. I think that is acceptable behavior for this scope.

basically, always consider any file that has errors as [files to analyze], and always analyze those when snapshot changed. that will make errors go away without users opening a file, but it won't make errors show up unless it is opened or active.

That is a good suggestion. I will wait for some more customer feedback on whether stale diagnostics in non-active file bother folks. I am also thinking on programmatically syncing the error list scope combo box when user switches their background scope in Tools Option. That might avoid us to add any additional removal logic.

…kground analysis scope in Tools Options.

…cted background analysis scope in Tools Options." This reverts commit be5bd27.

… test team has moved to IUnitTestingIncrementalAnalyzer

mavasani · 2019-11-20T01:09:25Z

Removing the "Blocked" label as @shyamnamboodiripad verified the test explorer functionality is unchanged on this branch. Thanks Shyam!

shyamnamboodiripad

I've verified that unit test discovery is not impacted by this change. Thanks for addressing all the concerns around unit test discovery!

shyamnamboodiripad · 2019-11-20T19:39:46Z

src/Workspaces/Core/Portable/SolutionCrawler/IIncrementalAnalyzerExtensions.cs

+        public static BackgroundAnalysisScope GetOverriddenBackgroundAnalysisScope(this IIncrementalAnalyzer incrementalAnalyzer, OptionSet options, BackgroundAnalysisScope defaultBackgroundAnalysisScope)
+        {
+            // Unit testing analyzer has special semantics for analysis scope.
+            if (incrementalAnalyzer is UnitTestingIncrementalAnalyzer unitTestingAnalyzer)


@mavasani As I'd mentioned offline, I think it would be better if this were controlled in a way that does not require special casing the unit testing analyzer.

One thought is that perhaps a setting could be added via IncrementalAnalyzerProviderMetadata (similar to HighPriorityForActiveFile). The metadata can also be provided via MEF if required (ExportIncrementalAnalyzerProviderAttribute). However, the unit testing analyzer is not exposed via MEF - so if you wanted to limit this to just the unit testing analyzer, you could make the opt out setting only available to analyzers that register via ISolutionCrawlerRegistrationService and supply IncrementalAnalyzerProviderMetadata...

Not urgent - but hopefully we can look into this at some point in the future...

Thanks @shyamnamboodiripad. I will try to get to this in a follow-up PR.

…ak for TS and F# Recent PR dotnet#39699 introduced a breaking API and functionality change for TypeScript and F# as they are using our internal option ClosedFileDiagnostics for controlling their full solution analysis experience. This PR restores the ClosedFileDiagnostics option and functionality for non C#/VB languages. Additionally, the newly added option SolutionCrawlerOptions.BackgroundAnalysisScope has been made a per-language option so that all languages can have different UI/setting for this option.

shyamnamboodiripad reviewed Nov 5, 2019

View reviewed changes

mavasani added 3 commits November 5, 2019 15:35

Fix build for unit tests

edb2504

Couple more test fixes

4c66dd8

Fix integration tests

3f20333