Add support for skipping analyzing banned API analysis in generated files#82713
Add support for skipping analyzing banned API analysis in generated files#82713333fred wants to merge 4 commits intodotnet:mainfrom
Conversation
…iles Adds a new `.editorconfig`/`.globalconfig` options for the banned API analyzer to skip analyzing generated code. Closes dotnet#82114.
333fred
force-pushed
the
banned-api-option
branch
from
209c313 to
807db56
Compare
...RoslynAnalyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers/Core/SymbolIsBannedAnalyzerBase.cs
Outdated
Show resolved
Hide resolved
...RoslynAnalyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers/Core/SymbolIsBannedAnalyzerBase.cs
Outdated
Show resolved
Hide resolved
...RoslynAnalyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers/Core/SymbolIsBannedAnalyzerBase.cs
Outdated
Show resolved
Hide resolved
src/RoslynAnalyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers/BannedApiAnalyzers.Help.md
Show resolved
Hide resolved
...Analyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers/UnitTests/SymbolIsBannedAnalyzerTests.cs
Show resolved
Hide resolved
...ynAnalyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers/CSharp/CSharpSymbolIsBannedAnalyzer.cs
Show resolved
Hide resolved
...RoslynAnalyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers/Core/SymbolIsBannedAnalyzerBase.cs
Outdated
Show resolved
Hide resolved
...Analyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers/UnitTests/SymbolIsBannedAnalyzerTests.cs
Show resolved
Hide resolved
|
@jjonescz for review |
...RoslynAnalyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers/Core/SymbolIsBannedAnalyzerBase.cs
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| return !generatedCodeOptions.ExcludesGeneratedCode || | ||
| !(isGeneratedCode ?? |
There was a problem hiding this comment.
It feels like we could return fast if isGeneratedCode is false (and perhaps the other checks we do in this ?? chain) and don't go into the complex GetOrAdd code path above in that case.
| protected override IEnumerable<SyntaxNode> GetTypeSyntaxNodesFromBaseType(SyntaxNode syntaxNode) => ((BaseListSyntax)syntaxNode).Types.Select(t => (SyntaxNode)t.Type); | ||
|
|
||
| protected override bool IsRegularCommentOrDocumentationComment(SyntaxTrivia trivia) | ||
| => trivia.Kind() is SyntaxKind.SingleLineCommentTrivia or SyntaxKind.MultiLineCommentTrivia or SyntaxKind.ShebangDirectiveTrivia or SyntaxKind.SingleLineDocumentationCommentTrivia or SyntaxKind.MultiLineDocumentationCommentTrivia; |
There was a problem hiding this comment.
Other callers like CSharpCompilation.CreateAnalyzerDriver only consider the first two kinds. Should we unify this logic?
| context => VerifyAttributes(context.ReportDiagnostic, context.Symbol.GetAttributes(), context.CancellationToken), | ||
| context => | ||
| { | ||
| VerifyAttributes(context.ReportDiagnostic, context.Symbol.GetAttributes(), context.IsGeneratedCode, context.CancellationToken); |
There was a problem hiding this comment.
What is context.IsGeneratedCode going to be if context.Symbol is partial and one part is in generated code and other is not? It looks like currently this symbol-level flag has precedence but shouldn't we prefer the tree-level flags instead?
There was a problem hiding this comment.
It will return false. My thinking was that this is fine, but as I try to type out a justification for that I'm failing convince myself now. I'll update it.
| } | ||
| """), | ||
| ("Test0.cs", """ | ||
| [{|#0:Banned|}] |
There was a problem hiding this comment.
Do we have a test when the attribute is in the generated file?
| if (applicationSyntaxReference == null) | ||
| continue; | ||
|
|
||
| if (applicationSyntaxReference.SyntaxTree == null) |
There was a problem hiding this comment.
It doesn't look like SyntaxTree property is nullable. Is this null check necessary?
Adds a new
.editorconfig/.globalconfigoptions for the banned API analyzer to skip analyzing generated code. Closes #82114.Microsoft Reviewers: Open in CodeFlow