Update CA1812 to not report on vbnet static-like holder type #3882
Conversation
| @@ -73,13 +73,18 @@ public override void Initialize(AnalysisContext context) | |||
| private static void AnalyzeSymbol(SymbolAnalysisContext context) | |||
| { | |||
| var symbol = (INamedTypeSymbol)context.Symbol; | |||
|
|
|||
| if (!symbol.IsStatic && | |||
There was a problem hiding this comment.
I have reordered the checks to postpone more expensive checks to later in the code. I am not sure if IsStaticHolderType is more expensive than MatchesConfiguredVisibility or not so feel free to tell me if I shall swap them.
| if (!symbol.IsStatic && | ||
| symbol.MatchesConfiguredVisibility(context.Options, Rule, context.Compilation, context.CancellationToken) && | ||
| !symbol.IsAbstract && | ||
| !IsSealedAndVisualBasic(symbol) && |
There was a problem hiding this comment.
In vb.net the rule recommends to mark the class as "sealed" (NotInheritable) because static doesn't exist, so if the class is already marked as such, bail-out before doing more complex process. Note that the change is required because before the IsStaticHolderType was returning false for a vbnet static-like type holder causing CA1812 to report FPs.
| Public Shared Event ThresholdReached As EventHandler | ||
| End Class | ||
|
|
||
| Friend Class C4 |
There was a problem hiding this comment.
The static-like type holder considers this type as correct (although it will report CA1052 to suggest adding NotInheritable) so we don't report CA1812.
|
|
||
| Friend NotInheritable Class C5 | ||
|
|
||
| Public Sub New() |
There was a problem hiding this comment.
The helper is not considering the default constructor visibility and there was no note on if there was a technical motivation to do so, hence I have decided not to change the behavior. We could revisit if we decide we want to have CA1812 on this type.
There was a problem hiding this comment.
@sharwell You wrote the helper, was there any technical motivation?
|
Ping @mavasani |
| // Sealed objects are presumed to be non-static holder types for C#. | ||
| // In VB.NET the type cannot be static and guidelines favor having a sealed (NotInheritable) type | ||
| // to act as static holder type. | ||
| if (symbol.IsSealed && symbol.Language == LanguageNames.CSharp) |
There was a problem hiding this comment.
Do you need to do changes to both this extension method and the analyzer? Can we not just do one of them?
There was a problem hiding this comment.
I need both of them because the helper is a little misleading, it's actually IsStaticHolderLikeType (or something like this). The helper checks that the member looks like a static holder but disregard the fact it is static to allow for the rule to report.
So in the end, the change here is just to prevent judging the type as not static-holder like when it's sealed. And the change in StaticHolderTypes.cs is made to ensure that if the class is sealed and it's vbnet we don't suggest to mark the class as static.
Codecov Report
@@ Coverage Diff @@
## master #3882 +/- ##
==========================================
- Coverage 95.63% 95.63% -0.01%
==========================================
Files 1155 1155
Lines 254876 254953 +77
Branches 15269 15272 +3
==========================================
+ Hits 243744 243813 +69
Misses 9189 9189
- Partials 1943 1951 +8 |
Fix #1878