Fix generators not being added or removed correctly#65038
Merged
arunchndr merged 2 commits intodotnet:release/dev17.4from Nov 9, 2022
Merged
Conversation
When the list of analyzer references change for a project, we produce a new list of AnalyzerFileReferences and pass that into Project.WithAnalyzerReferences. The implementation of that method figures out which references are added or removed, and also uses those changes to update the list of generators being held by the generator. This seems innocent enough, except that the AnalyzerFileReferences here implement value equality: two references are equal if they have the same file path. These references however will not return the same generator instances, because each reference does it's own loading and caching of that list. Thus, when we computed the list of analyzers that are added or removed, we won't see analyzer references that are equal, and won't update the generator driver. Later code that expects those to be in sync will then start throwing exceptions. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1655835
ghost
added
the
jasonmalinowski
commented
Oct 27, 2022
Comment on lines
+1039
to
+1040
| // An alternative approach would be to call oldProject.WithAnalyzerReferences keeping all the references in there that are value equal the same, | ||
| // but this avoids any surprises where other components calling WithAnalyzerReferences might not expect that. |
Member
Author
There was a problem hiding this comment.
Now that I'm aware of this gotcha, I'm questioning the generally goodness of having AnalyzerFileReferences be equal comparable in the first place, but since that'd be an API breaking change I'm doing this for 17.4 and 17.5 Preview 1; we can take as a follow up item questioning the wisdom of all of this.
Contributor
There was a problem hiding this comment.
oh yes. we're on the same page. i thin kwe should break this.
jasonmalinowski
commented
Oct 27, 2022
|
|
||
| private class TestGeneratorReferenceWithFilePathEquality : TestGeneratorReference, IEquatable<AnalyzerReference> | ||
| { | ||
| public TestGeneratorReferenceWithFilePathEquality(ISourceGenerator generator, string analyzerFilePath) : base(generator) |
Member
Author
There was a problem hiding this comment.
In 17.5 TestGeneratorReference also has a FullPath property, which this isn't passing to; I'll mop that up in main -- right now this PR cleanly applies to both branches this way.
src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs
Outdated
Show resolved
Hide resolved
| // | ||
| // When we're comparing AnalyzerReferences, we'll compare with reference equality; AnalyzerReferences like AnalyzerFileReference | ||
| // may implement their own equality, but that can result in things getting out of sync: two references that are value equal can still | ||
| // have their own generator instances; it's important that as we're adding and removing references that are value equal that we |
Contributor
There was a problem hiding this comment.
ick ick ick. perhaps they should not implement equality?
CyrusNajmabadi
approved these changes
Oct 27, 2022
Contributor
CyrusNajmabadi
left a comment
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
lgtm. but please invert the if.
If the last generator was removed, and it was generating trees, we would have potentially left that tree in the Compilation that was returned. I believe this would have been a transient issue -- any later change to the project would have created a new CompilationTracker with an InProgress state; the code that processes an InProgress state into the final state would have correctly seen we no longer had a generator and would have dropped the old compilation at that point.
jasonmalinowski
force-pushed
the
fix-generators-not-being-added-or-removed-correctly
branch
from
b22df25 to
68391a3
Compare
genlu
approved these changes
Oct 27, 2022
arunchndr
approved these changes
Oct 27, 2022
This was referenced Oct 27, 2022
Closed
jasonmalinowski
deleted the
fix-generators-not-being-added-or-removed-correctly
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the list of analyzer references change for a project, we produce a new list of AnalyzerFileReferences and pass that into Project.WithAnalyzerReferences. The implementation of that method figures out which references are added or removed, and also uses those changes to update the list of generators being held by the generator.
This seems innocent enough, except that the AnalyzerFileReferences here implement value equality: two references are equal if they have the same file path. These references however will not return the same generator instances, because each reference does it's own loading and caching of that list. Thus, when we computed the list of analyzers that are added or removed, we won't see analyzer references that are equal, and won't update the generator driver. Later code that expects those to be in sync will then start throwing exceptions.
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1655835
While then writing tests for that, I found a second issue:
If the last generator was removed, and it was generating trees, we would have potentially left that tree in the Compilation that was returned. I believe this would have been a transient issue -- any later change to the project would have created a new CompilationTracker with an InProgress state; the code that processes an InProgress state into the final state would have correctly seen we no longer had a generator and would have dropped the old compilation at that point.