Add the UseConreteType analyzer by geeknoid · Pull Request #6370 · dotnet/roslyn-analyzers

geeknoid · 2022-12-19T23:59:48Z

As per dotnet/runtime#51193. This recommends using concrete types for fields, local, parameters, and method returns instead of interface/abstract types when possible and when it doesn't affect the API surface of a class. The idea is to help eliminate virtual/interface dispatch, while also making available potentially richer APIs exposed by implementations relative to their interface.

codecov · 2022-12-20T01:15:44Z

Codecov Report

Merging #6370 (5fe2c65) into main (c4909ec) will increase coverage by 0.02%.
The diff coverage is 95.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6370      +/-   ##
==========================================
+ Coverage   96.12%   96.14%   +0.02%     
==========================================
  Files        1361     1365       +4     
  Lines      316084   317413    +1329     
  Branches    10183    10263      +80     
==========================================
+ Hits       303841   305187    +1346     
+ Misses       9814     9790      -24     
- Partials     2429     2436       +7

buyaa-n · 2023-01-03T23:03:32Z

Thank you @geeknoid, I run the analyzer in runtime repo and it is found 544 diagnostics for one build (over 3000 for all platforms build). With that many diagnostics it would be very useful to have a fixer. Do you plan to add a fixer in this PR or later?

I take a look some of the diagnostics (the ones found in System.Private.CoreLib) and suspect some of the are false positives:

Why might false positive	Location	Diagnostic reported
Warns on parameter of a public API	runtime\src\libraries\System.Private.CoreLib\ src\System\MemoryExtensions.cs(2155,124)	Change type of parameter 'comparer' from 'System.Collections.Generic.IEqualityComparer?' to 'System.Collections.Generic.EqualityComparer' for improved performance
Warns on parameter of a public API	runtime\src\libraries\System.Private.CoreLib\ src\System\MemoryExtensions.cs(3197,121)	Change type of parameter 'comparer' from 'System.Collections.Generic.IEqualityComparer?' to 'System.Collections.Generic.EqualityComparer' for improved performance
Method not always return the suggested type	runtime\src\coreclr\System.Private.CoreLib\ src\System\Reflection\Emit\TypeBuilder.cs(1711,27)	Change return type of method 'CreateTypeNoLock' from 'System.Reflection.TypeInfo?' to 'System.RuntimeType?' for improved performance
Method returns the interface method	runtime\src\libraries\System.Private.CoreLib\ src\System\Resources\ResourceSet.cs(126,26)	Change type of variable 'copyOfTableAsIDictionary' from 'System.Collections.IDictionary?' to 'System.Collections.Generic.Dictionary<object, object?>?' for improved performance
Private/internal method called in public API that returns the interface	runtime\src\libraries\System.Private.CoreLib\ src\System\Environment.Win32.cs(64,36)	Change return type of method 'GetEnvironmentVariablesFromRegistry' from 'System.Collections.IDictionary' to 'System.Collections.Hashtable' for improved performance
Private/internal method called in public API that returns the interface	src\coreclr\System.Private.CoreLib\ src\System\Reflection\Assembly.CoreCLR.cs(86,34)	Change return type of method 'GetEntryAssemblyInternal' from 'System.Reflection.Assembly?' to 'System.Reflection.RuntimeAssembly?' for improved performance
Private/internal method called in public API that returns the interface	runtime\src\coreclr\System.Private.CoreLib\ src\System\Runtime\Loader\ AssemblyLoadContext.CoreCLR.cs(57,26)	Change return type of method 'InternalLoadFromPath' from 'System.Reflection.Assembly' to 'System.Reflection.RuntimeAssembly' for improved performance
Private/internal method called in public API that returns the interface	runtime\src\coreclr\System.Private.CoreLib\ src\System\Reflection\ RuntimeCustomAttributeData.cs(95,51)	Change return type of method 'GetCombinedList' from 'System.Collections.Generic.IList<System.Reflection.CustomAttributeData>' to 'System.Collections.ObjectModel.ReadOnlyCollection<System.Reflection.CustomAttributeData>' for improved performance
Private/internal method called in public API that returns the interface	runtime\src\coreclr\System.Private.CoreLib\ src\System\Reflection\Emit\TypeBuilder.cs(1711,27)	Change return type of method 'CreateTypeNoLock' from 'System.Reflection.TypeInfo?' to 'System.RuntimeType?' for improved performance

for Private/internal method called in public API that returns the interface - not sure it is actually a false positive and changing the return type of private/internal method would make a difference

I have attached the findings for reference: CA1859-log.txt

buyaa-n · 2023-01-03T23:08:22Z

+
+                    public class Test
+                    {
+                        private void Method(IFoo {|#0:foo|})


It would be better to squiggle below the symbol that needs an action, i.e. it would make more sense to squiggle under IFoo not foo ( {|#0:IFoo|} foo )

buyaa-n · 2023-01-03T23:09:38Z

+
+                public class Test
+                {
+                    private IFoo? {|#0:Method1|}(FooProvider? provider)


Same here and others places, private {|#0:IFoo|}? Method1(FooProvider? provider) not private IFoo? {|#0:Method1|}(FooProvider? provider)

As per dotnet/runtime#51193. This recommends using concrete types for fields, local, parameters, and method returns instead of interface/abstract types when possible and when it doesn't affect the API surface of a class. The idea is to help eliminate virtual/interface dispatch, while also making available potentially richer APIs exposed by implementations relative to their interface.

geeknoid · 2023-01-04T05:25:34Z

@buyaa-n Thanks for the detailed test and report. I tried each case you highlighted. Here's my input in order:

// STATUS
//   1..2 : this is related to some confusion in handling of generics
//   3 ; pushed a fix, the logic generally wasn't recognizing uses of 'this'
//   4..8 : all as designed, upgrading these types are all part of the point of the analyzer
//   9 : same case as #3, so that's fixed.

I've updated the PR to fix case 3.

I could use some help for cases 1 and 2 however. I think I don't understand how Roslyn deals with generic types, and this is leading to both false positives and false negatives.

If you look my test cases called ShouldTrigger1 and ShouldTrigger2, these should lead to a diagnostic about upgrading the parameter of the Do method to be of type Foo or Foo. But it's not firing. I have a list of candidate parameters which could be upgraded, which includes the parameter "foo" of the Do method. Now I also have a list of the different types assigned to each parameter by all the call sites. The problem is that when I use SymbolComparer.Default.Equals to compare those two (identical!) parameters, I am told they are different.

If I compare the two parameter instances by hand, comparing each field, they seem to be identical. Yet Roslyn is claiming otherwise.

If I take the generics out of the picture, this kind of code triggers just fine.

I've just spent a couple hours on this and made no progress. I could use some help.

Thanks

geeknoid · 2023-01-04T05:26:56Z

@buyaa-n To answer your question, I'm not currently planning a fixer for this. I unfortunately don't have the bandwidth for that at this point.

mavasani · 2023-01-04T05:43:27Z

I could use some help for cases 1 and 2 however. I think I don't understand how Roslyn deals with generic types, and this is leading to both false positives and false negatives.

@geeknoid This normally indicates that you are trying to compare a symbol reference with a symbol definition. The solution in most cases should be to use ISymbol.OriginalDefinition property on the symbol reference to get the definition and then compare the definition symbols.

geeknoid · 2023-01-04T15:26:49Z

Thanks @mavasani , that did the trick!

@buyaa-n I've updated the PR which will hopefully address the false positives. That just leaves reporting the squiggles on the type name, rather than on the symbol name. I'm going to have to think about that, I think it might require a fair bit of change to how the code works now. But maybe I can find some clever way to do it.

Youssef1313 · 2023-01-04T15:34:15Z

+            public INamedTypeSymbol? Void { get; private set; }
+


Suggested change

public INamedTypeSymbol? Void { get; private set; }

Youssef1313 · 2023-01-04T15:34:24Z

+                Void = null;
+


Suggested change

Void = null;

Youssef1313 · 2023-01-04T15:34:43Z

+                var c = _pool.Allocate();
+                c.Void = compilation.GetSpecialType(SpecialType.System_Void);
+                return c;


Suggested change

var c = _pool.Allocate();

c.Void = compilation.GetSpecialType(SpecialType.System_Void);

return c;

return _pool.Allocate();

Youssef1313 · 2023-01-04T15:36:47Z

+            context.RegisterSymbolStartAction(context =>
+            {
+                var coll = Collector.GetInstance(context.Compilation);
+
+                context.RegisterOperationAction(context => coll.HandleInvocation((IInvocationOperation)context.Operation), OperationKind.Invocation);
+                context.RegisterOperationAction(context => coll.HandleSimpleAssignment((ISimpleAssignmentOperation)context.Operation), OperationKind.SimpleAssignment);
+                context.RegisterOperationAction(context => coll.HandleCoalesceAssignment((ICoalesceAssignmentOperation)context.Operation), OperationKind.CoalesceAssignment);
+                context.RegisterOperationAction(context => coll.HandleDeconstructionAssignment((IDeconstructionAssignmentOperation)context.Operation), OperationKind.DeconstructionAssignment);
+                context.RegisterOperationAction(context => coll.HandleFieldInitializer((IFieldInitializerOperation)context.Operation), OperationKind.FieldInitializer);
+                context.RegisterOperationAction(context => coll.HandleVariableDeclarator((IVariableDeclaratorOperation)context.Operation), OperationKind.VariableDeclarator);
+                context.RegisterOperationAction(context => coll.HandleDeclarationExpression((IDeclarationExpressionOperation)context.Operation), OperationKind.DeclarationExpression);
+                context.RegisterOperationAction(context => coll.HandleReturn((IReturnOperation)context.Operation), OperationKind.Return);
+
+                context.RegisterSymbolEndAction(context =>
+                {
+                    Report(context, coll);
+                    Collector.ReturnInstance(coll, context.CancellationToken);
+                });
+            }, SymbolKind.NamedType);


Suggested change

context.RegisterSymbolStartAction(context =>

{

var coll = Collector.GetInstance(context.Compilation);

context.RegisterOperationAction(context => coll.HandleInvocation((IInvocationOperation)context.Operation), OperationKind.Invocation);

context.RegisterOperationAction(context => coll.HandleSimpleAssignment((ISimpleAssignmentOperation)context.Operation), OperationKind.SimpleAssignment);

context.RegisterOperationAction(context => coll.HandleCoalesceAssignment((ICoalesceAssignmentOperation)context.Operation), OperationKind.CoalesceAssignment);

context.RegisterOperationAction(context => coll.HandleDeconstructionAssignment((IDeconstructionAssignmentOperation)context.Operation), OperationKind.DeconstructionAssignment);

context.RegisterOperationAction(context => coll.HandleFieldInitializer((IFieldInitializerOperation)context.Operation), OperationKind.FieldInitializer);

context.RegisterOperationAction(context => coll.HandleVariableDeclarator((IVariableDeclaratorOperation)context.Operation), OperationKind.VariableDeclarator);

context.RegisterOperationAction(context => coll.HandleDeclarationExpression((IDeclarationExpressionOperation)context.Operation), OperationKind.DeclarationExpression);

context.RegisterOperationAction(context => coll.HandleReturn((IReturnOperation)context.Operation), OperationKind.Return);

context.RegisterSymbolEndAction(context =>

{

Report(context, coll);

Collector.ReturnInstance(coll, context.CancellationToken);

});

}, SymbolKind.NamedType);

context.RegisterCompilationStartAction(context =>

{

var voidType = context.Compilation.GetSpecialType(SpecialType.System_Void);

context.RegisterSymbolStartAction(context =>

{

var coll = Collector.GetInstance(context.Compilation);

context.RegisterOperationAction(context => coll.HandleInvocation((IInvocationOperation)context.Operation), OperationKind.Invocation);

context.RegisterOperationAction(context => coll.HandleSimpleAssignment((ISimpleAssignmentOperation)context.Operation), OperationKind.SimpleAssignment);

context.RegisterOperationAction(context => coll.HandleCoalesceAssignment((ICoalesceAssignmentOperation)context.Operation), OperationKind.CoalesceAssignment);

context.RegisterOperationAction(context => coll.HandleDeconstructionAssignment((IDeconstructionAssignmentOperation)context.Operation), OperationKind.DeconstructionAssignment);

context.RegisterOperationAction(context => coll.HandleFieldInitializer((IFieldInitializerOperation)context.Operation), OperationKind.FieldInitializer);

context.RegisterOperationAction(context => coll.HandleVariableDeclarator((IVariableDeclaratorOperation)context.Operation), OperationKind.VariableDeclarator);

context.RegisterOperationAction(context => coll.HandleDeclarationExpression((IDeclarationExpressionOperation)context.Operation), OperationKind.DeclarationExpression);

context.RegisterOperationAction(context => coll.HandleReturn((IReturnOperation)context.Operation), OperationKind.Return);

context.RegisterSymbolEndAction(context =>

{

Report(context, coll, voidType);

Collector.ReturnInstance(coll, context.CancellationToken);

});

}, SymbolKind.NamedType);

}

Youssef1313 · 2023-01-04T15:37:05Z

+        /// <summary>
+        /// Given all the accumulated analysis state, generate the diagnostics.
+        /// </summary>
+        private static void Report(SymbolAnalysisContext context, Collector coll)


Suggested change

private static void Report(SymbolAnalysisContext context, Collector coll)

private static void Report(SymbolAnalysisContext context, Collector coll, INamedTypeSymbol voidType)

Youssef1313 · 2023-01-04T15:37:18Z

+            {
+                using var types = PooledHashSet<ITypeSymbol>.GetInstance(assignments, SymbolEqualityComparer.Default);
+
+                var assignedNull = types.Remove(coll.Void!);


Suggested change

var assignedNull = types.Remove(coll.Void!);

var assignedNull = types.Remove(voidType);

buyaa-n · 2023-01-04T19:29:37Z

To answer your question, I'm not currently planning a fixer for this. I unfortunately don't have the bandwidth for that at this point.

OK, thank you for letting us know. We can add that later, I will create an issue for fixer implementation and put it up for grabs.

That just leaves reporting the squiggles on the type name, rather than on the symbol name. I'm going to have to think about that, I think it might require a fair bit of change to how the code works now. But maybe I can find some clever way to do it.

Thank you, there is one analyzer that I know did that:

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureAnalyzer.cs

Lines 440 to 449 in 60f592c

    
           private SyntaxNode? GetPreviewSyntaxNodeFromSymbols(ISymbol symbol, 
        
                                                               ISymbol previewType) 
        
           { 
        
               switch (symbol) 
        
               { 
        
                   case IFieldSymbol: 
        
                   case IEventSymbol: 
        
                       return GetPreviewSyntaxNodeForFieldsOrEvents(symbol, previewType); 
        
                   case IMethodSymbol methodSymbol: 
        
                       return GetPreviewParameterSyntaxNodeForMethod(methodSymbol, previewType);

geeknoid · 2023-01-04T19:30:58Z

@Youssef1313 Unless I'm missing something, I think your suggestions of removing the Void property from the collector type doesn't work. Line 333 of the Collector.cs file needs this value, that's why it was present in the object in the first place.

Youssef1313 · 2023-01-04T19:34:36Z

@Youssef1313 Unless I'm missing something, I think your suggestions of removing the Void property from the collector type doesn't work. Line 333 of the Collector.cs file needs this value, that's why it was present in the object in the first place.

I'm getting the void type once in compilation start and passing it down with these suggestions. Yes I missed this usage, but the idea is the same.

buyaa-n

Overall LGTM, thank you!

buyaa-n · 2023-01-05T02:59:08Z

Created an issue for fixer and diagnostic location. Merging the PR

Merging the PR, @Youssef1313 your suggestions makes sense to me, but as @geeknoid mentioned above applying your suggestions on the PR directly might fail the build, feel free to raise a PR with your suggestions after merge, thank you!

geeknoid · 2023-01-05T14:40:35Z

@buyaa-n Thanks!

stephentoub · 2023-01-05T22:57:12Z

I'm trying this out on dotnet/runtime, and I'm getting these warnings:

D:\repos\runtime\src\libraries\System.Private.CoreLib\src\System\MemoryExtensions.cs(3192,121): warning CA1859: Change type of parameter 'comparer' from 'System.Collections.Generic.IEqualityComparer<T>?' to 'System.Collections.Generic.EqualityComparer<T>' for improved
performance [D:\repos\runtime\src\coreclr\System.Private.CoreLib\System.Private.CoreLib.csproj]
D:\repos\runtime\src\libraries\System.Private.CoreLib\src\System\MemoryExtensions.cs(2150,124): warning CA1859: Change type of parameter 'comparer' from 'System.Collections.Generic.IEqualityComparer<T>?' to 'System.Collections.Generic.EqualityComparer<T>' for improved
performance [D:\repos\runtime\src\coreclr\System.Private.CoreLib\System.Private.CoreLib.csproj]

That's for these public methods on a public type:
https://github.com/dotnet/runtime/blob/db28821852151f62fb81d83d0fd1c8de9e0e5640/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs#L3192
https://github.com/dotnet/runtime/blob/db28821852151f62fb81d83d0fd1c8de9e0e5640/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs#L2150

Is it a bug that these are being reported? I thought the intent was to only fire for internal/private? I tried adding:

dotnet_diagnostic.CA1859.api_surface = private, internal

and it didn't help.

geeknoid · 2023-01-05T23:12:44Z

These are bugs. Are these the only ones showing up for you? It's not supposed to fire on public signatures.

I specifically tried to repro these two and it's just not happening from within the test environment. I'm wondering if there's something funny going on because its corelib. This rule has been running for a year on hundreds of projects and this hasn't surfaced before, so there's something peculiar about those two methods.

buyaa-n · 2023-01-05T23:49:33Z

I'm trying this out on dotnet/runtime, and I'm getting these warnings:

Oops I thought those were fixed, @stephentoub would you suggest reverting this PR or can we fix this with different PR?

stephentoub · 2023-01-06T00:07:14Z

Are these the only ones showing up for you?

The only public ones for corelib. I've not yet tried beyond that.

would you suggest reverting this PR or can we fix this with different PR?

No need to revert. Separate PR once the issue is diagnosed is fine.

geeknoid · 2023-01-06T01:00:08Z

What magic do I need to do to run the analyzer against core lib in such a way that I can continuously run the analyzer while deleting code from MemoryExtensions.cs? I need to create a reproducible example. I've got a top-level condition to not produce diagnostics against public symbols, and somehow this is slipping through.

geeknoid · 2023-01-06T01:16:53Z

Aha, never mind. I have a repro. The problem emerges when functions assign values to their own incoming parameters. The analyzer is getting confused about what this means.

geeknoid · 2023-01-06T01:44:24Z

#6407

geeknoid · 2023-04-28T14:48:53Z

As a follow-up to this, I realized I never created docs for this. So here's a PR: dotnet/docs#35182

geeknoid requested a review from a team as a code owner December 19, 2022 23:59

github-actions Bot added the Community label Dec 20, 2022

mavasani reviewed Dec 20, 2022

View reviewed changes