-
Notifications
You must be signed in to change notification settings - Fork 466
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
Add new analyzer for deprecating implementing an interface #7419
Conversation
We're officially marking `ISourceGenerator` as deprecated, but we can't actually deprecate the interface, because it's used in several APIs like AnalyzerFileReference. So instead, we just add an analyzer to indicate that the interface should not be implemented. I've made this more general so we can do this with more interfaces in the future if necessary, based the implementation on InternalImplementationOnlyAnalyzer.
Analyzer that will flag implementation is dotnet/roslyn-analyzers#7419. We don't just mark `ISourceGenerator` as obsolete directly because we do use it in public APIs like `AnalyzerFileReference` and `GeneratorDriver`, and we can't remove it from there.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7419 +/- ##
========================================
Coverage 96.50% 96.50%
========================================
Files 1443 1445 +2
Lines 346376 346598 +222
Branches 11387 11390 +3
========================================
+ Hits 334263 334488 +225
+ Misses 9232 9229 -3
Partials 2881 2881 |
// CodeAnalysis.dll itself has this attribute and if the user assembly also had it, symbol equality will fail | ||
// but we should still issue the error. | ||
if (attributes.FirstOrDefault(a => a is { AttributeClass.Name: ImplementationIsObsoleteAttributeName, ConstructorArguments: [{ Value: string }] } | ||
&& a.AttributeClass.ToDisplayString().Equals(ImplementationIsObsoleteAttributeFullName, StringComparison.Ordinal)) is { } attr && |
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.
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 copy/paste from our existing DoNotImplementInternalInterfaces analyzer (the one we use to flag IOperation/ISymbol). If this was a perf problem, I would expect that to have been updated a while ago.
src/Microsoft.CodeAnalysis.Analyzers/Core/ImplementationIsObsoleteAnalyzer.cs
Show resolved
Hide resolved
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 Thanks (iteration 1) with minor comments to consider
</data> | ||
<data name="ImplementationIsObsoleteMessage" xml:space="preserve"> | ||
<value>Type {0} cannot implement interface {1} because {1} is obsolete for implementation. See {2} for more details.</value> |
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.
📝 Placeholders should be surrounded by '
(single quotes)
src/Microsoft.CodeAnalysis.Analyzers/Core/ImplementationIsObsoleteAnalyzer.cs
Show resolved
Hide resolved
* Mark ISourceGenerator as obsolete for implementation Analyzer that will flag implementation is dotnet/roslyn-analyzers#7419. We don't just mark `ISourceGenerator` as obsolete directly because we do use it in public APIs like `AnalyzerFileReference` and `GeneratorDriver`, and we can't remove it from there. * Suppress warnings * Fix attribute name * Update Microsoft.CodeAnalysis.Analyzers * Address warnings
--- There was a breaking change between Microsoft.CodeAnalysis.Analyzers 3.3.4 and 3.11.0 that necessitated the usage of IIncrementalGenerator according to https://github.com/dotnet/roslyn/blob/main/docs/features/source-generators.cookbook.md, https://github.com/dotnet/roslyn/blob/main/docs/features/incremental-generators.md, and dotnet/roslyn-analyzers#7419. As a result, we've made necessary changes to achieve the same result, cleaning up clutter in the process. --- Type: imp Breaking: False Doc Required: False Backport Required: False Part: 1/1
We're officially marking
ISourceGenerator
as deprecated, but we can't actually deprecate the interface, because it's used in several APIs like AnalyzerFileReference. So instead, we just add an analyzer to indicate that the interface should not be implemented. I've made this more general so we can do this with more interfaces in the future if necessary, based the implementation on InternalImplementationOnlyAnalyzer.