Fix Union to match the semantics of GetApplicableAnalyzers by davidwengier · Pull Request #64484 · dotnet/roslyn

davidwengier · 2022-10-04T21:12:24Z

No description provided.

davidwengier · 2022-10-05T02:04:45Z

src/EditorFeatures/Test/SolutionCrawler/WorkCoordinatorTests.cs

+        }
+
+        [Fact]
+        public async Task SpecificAnalyzers_SpecificThenAll()


This test fails without the changeso the Union method

davidwengier · 2022-10-05T02:05:16Z

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

                        asyncToken);
                }

-                public WorkItem With(ImmutableHashSet<IIncrementalAnalyzer> specificAnalyzers, IAsyncToken asyncToken)


This method was unused, and didn't call Union at all, so I thought it best to remove before someone calls it and does who-knows-what

davidwengier · 2022-10-05T06:25:09Z

Sadly, this assert is failing,
https://github.com/dotnet/roslyn/blob/main/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.NormalPriorityProcessor.cs#L446

Not sure if thats a problem though, or just validating an old assumption. @mavasani any context you could help with?

I've validated on my machine that SymbolTreeInfoIncrementalAnalyzer.AnalyzeProjectAsync is never called if I turn off the "Load projects faster" Preview Feature in Tools > Options, but with this fix, it is called. Thats essentially the same thing as I'm seeing on VS Mac, which doesn't have operation progress (which taht preview feature controls)

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

mavasani · 2022-10-05T09:48:53Z

Sadly, this assert is failing, https://github.com/dotnet/roslyn/blob/main/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.NormalPriorityProcessor.cs#L446

Not sure if thats a problem though, or just validating an old assumption. @mavasani any context you could help with?

I've validated on my machine that SymbolTreeInfoIncrementalAnalyzer.AnalyzeProjectAsync is never called if I turn off the "Load projects faster" Preview Feature in Tools > Options, but with this fix, it is called. Thats essentially the same thing as I'm seeing on VS Mac, which doesn't have operation progress (which taht preview feature controls)

So, the assert seems to be still valid, and the subsequent code in this method is actually relying on it. If you look at the below line:

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

Line 456 in d04c23b

var reanalyzers = workItem.SpecificAnalyzers.ToImmutableArray();

For your case, this will be empty set of analyzers, so the entire method ProcessReanalyzeDocumentAsync essentially becomes a no-op. I think you should remove the assert you mentioned but also change the above line to:

var reanalyzers = workItem.GetApplicableAnalyzers(analyzers).ToImmutableArray();

This would require you to thread in the analyzers parameter into ProcessReanalyzeDocumentAsync from its callsite.

davidwengier · 2022-10-05T10:27:01Z

Looks like the first thing that RunAnalyzersAsync does with the reanalyzers is to call GetApplicableAnalyzers anyway, so I think just threading through the parameter should be enough.

davidwengier · 2022-10-05T10:38:33Z

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

                            }

                            // First reset the document state in analyzers.
-                            var reanalyzers = workItem.SpecificAnalyzers.ToImmutableArray();


Just from looking at the code, and having no other context, I'm wondering if perhaps this method was intended to run only when there are specific analyzers on a work item.. in which case perhaps rather than removing the assert, it would make more sense to simply add || workItem.SpecificAnalyzers.Count > 0 to the early-return guard above?

But at the same time, what I have here also makes sense. All of the tests pass too, so I really don't know what exactly to go on, to try to work out what this method is intended to do.

Yeah, the core issue seems to be "Reanalyze" was designed only for a single/subset of specific analyzers. I think your change is fine to unblock us, and lets pray that @CyrusNajmabadi kills the solution crawler before this change uncovers more bugs :-)

So I shouldn't point out the bit of code I found, where if you call item.With(..., ..., item.AsyncToken) the first thing the With method does is dispose the async token that you're trying to re-use? :D

Doesn't it dispose the old one and re-use the new one?

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

Lines 133 to 143 in d04c23b

// dispose old one

AsyncToken.Dispose();

// create new work item

return new WorkItem(

DocumentId, ProjectId, Language,

InvocationReasons.With(invocationReasons),

IsLowPriority,

ActiveMember == currentMember ? currentMember : null,

Union(analyzers), IsRetry || retry,

asyncToken);

Right, bit there are spots where the old one is passed in, to be the new one, presumably assuming it will just be reused.

Probably add a guard that AsyncToken != asyncToken around the above Dispose call?

I don't know if I want to touch it.. this is going into 17.4 under QB and I don't understand the implications

davidwengier · 2022-10-05T22:53:02Z

Bug for QB: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1639039

@arkalyanms for approval

Fix Union to match the semantics of GetApplicableAnalyzers

0dcb1f1

ghost added the Area-IDE label Oct 4, 2022

davidwengier changed the base branch from main to release/dev17.4 October 4, 2022 21:22

Tests

e759773

davidwengier commented Oct 5, 2022

View reviewed changes

davidwengier requested review from CyrusNajmabadi and mavasani October 5, 2022 02:06

davidwengier marked this pull request as ready for review October 5, 2022 02:06

davidwengier requested a review from a team as a code owner October 5, 2022 02:06

mavasani reviewed Oct 5, 2022

View reviewed changes

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.WorkItem.cs Outdated Show resolved Hide resolved

davidwengier added 2 commits October 5, 2022 21:29

Move to local function and rename some things

37aea7c

Pass analyzer list through so that GetApplicableAnalyzers works

662f6cb

davidwengier commented Oct 5, 2022

View reviewed changes

mavasani approved these changes Oct 5, 2022

View reviewed changes

arunchndr approved these changes Oct 5, 2022

View reviewed changes

build-analysis bot mentioned this pull request Oct 6, 2022

503's dotnet/arcade#10943

Closed

2 tasks

davidwengier merged commit 53c4590 into dotnet:release/dev17.4 Oct 6, 2022

davidwengier deleted the SolutionCrawler branch October 6, 2022 02:11

allisonchou mentioned this pull request Oct 11, 2022

[Automated] PRs inserted in VS build main-33011.173 #64652

Closed

ryzngard mentioned this pull request Oct 19, 2022

Fix Color Contrast For STE Close Button #64834

Merged

	// dispose old one
	AsyncToken.Dispose();

	// create new work item
	return new WorkItem(
	DocumentId, ProjectId, Language,
	InvocationReasons.With(invocationReasons),
	IsLowPriority,
	ActiveMember == currentMember ? currentMember : null,
	Union(analyzers), IsRetry \|\| retry,
	asyncToken);

Conversation

davidwengier commented Oct 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier commented Oct 5, 2022

Uh oh!

Uh oh!

mavasani commented Oct 5, 2022

Uh oh!

davidwengier commented Oct 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier commented Oct 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidwengier Oct 5, 2022 •

edited

Loading