-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not run 'remove unnecessary imports' on generated code #74762
Do not run 'remove unnecessary imports' on generated code #74762
Conversation
@@ -71,8 +71,6 @@ private static ImmutableArray<DiagnosticDescriptor> GetDescriptors(LocalizableSt | |||
protected abstract bool IsRegularCommentOrDocComment(SyntaxTrivia trivia); | |||
protected abstract IUnnecessaryImportsProvider<TSyntaxNode> UnnecessaryImportsProvider { get; } | |||
|
|||
protected override GeneratedCodeAnalysisFlags GeneratedCodeAnalysisFlags => GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That way I think -generatedCodeClassificationIdDescriptor
can be removed along with the check that selects the descriptor based on whether this is generated code or not
@@ -186,22 +186,9 @@ public async Task TestGeneratedCode() | |||
var source = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a [WorkItem]
to the test to clarify, why no diagnostics there are expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but one thing to note. Do we know for sure that this is actually a performance issue? In some cases, the analyzer can be showing as expensive because it's calculating something that will happen anyways during the compilation (i.e, the cost is just moving around between the rest of the compilation process and the analyzer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a red herring. Remove unnecessary imports has no actual performance impact. It incorrectly captures unavoidable performance overhead from a different part of the system, suggesting it can be removed by disabling this analyzer when in fact it cannot.
@sharwell what value does this analyzer have though? The user cannot change the usings added. we don't run other style analyzers on generated code, why do we run this one.
Then we shouldn't report that it does. It also makes the other analyzers look good by comparison. I'd still like to take this though, if there's no use value provided. |
I have found value in this feature while writing source generators. |
The best code is no code. I don't see why we are special casing this style analyzer for generated code, and nothing else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, I agree that the test should have some indication that the behavior has changed
Fixes #74761.