Use operations in our analyzers that need symbols (#17001)

Fixes: #16922

This change updates our analyzers that need access to the symbols to use
`IOperation` where possible. Using syntax analysis for this kind of
analyzer has worse performance. These analyzers run on generated code,
which can include EF migrations, the design of which amplifies these
effects.

On the path to this, I also added support for a few more cases that
operations make easy. Since we're doing this anyway, I want to have
confidence that we're checking everything (within reason). In some cases
the diagnostic experience changed a bit (including more of the
declaration/code) - I felt like all of these were OK changes. Given the
kinds of error message reported by the analyzers "don't use that type"
it seems like it's still a good enough experience without
micro-optimizing all of the locations.

src/Components/Analyzers/src/InternalUsageAnalyzer.cs

@@ -2,10 +2,10 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Linq;
 using Microsoft.CodeAnalysis;
-using Microsoft.CodeAnalysis.CSharp;
-using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Operations;
 
 namespace Microsoft.Extensions.Internal
 {
@@ -35,87 +35,148 @@ public InternalUsageAnalyzer(Func<ISymbol, bool> isInInternalNamespace, Func<ISy
         public void Register(AnalysisContext context)
         {
             context.EnableConcurrentExecution();
-            context.RegisterSyntaxNodeAction(AnalyzeNode,
-                SyntaxKind.SimpleMemberAccessExpression,
-                SyntaxKind.ObjectCreationExpression,
-                SyntaxKind.ClassDeclaration,
-                SyntaxKind.Parameter);
+
+            // Analyze usage of our internal types in method bodies.
+            context.RegisterOperationAction(
+                AnalyzeOperation,
+                OperationKind.ObjectCreation,
+                OperationKind.FieldReference,
+                OperationKind.MethodReference,
+                OperationKind.PropertyReference,
+                OperationKind.EventReference);
+
+            // Analyze declarations that use our internal types in API surface.
+            context.RegisterSymbolAction(
+                AnalyzeSymbol,
+                SymbolKind.NamedType,
+                SymbolKind.Field,
+                SymbolKind.Method,
+                SymbolKind.Property,
+                SymbolKind.Event);
+        }
+
+        private void AnalyzeOperation(OperationAnalysisContext context)
+        {
+            var symbol = context.Operation switch
+            {
+                IObjectCreationOperation creation => creation.Constructor,
+                IInvocationOperation invocation => invocation.TargetMethod,
+                IFieldReferenceOperation field => field.Member,
+                IMethodReferenceOperation method => method.Member,
+                IPropertyReferenceOperation property => property.Member,
+                IEventReferenceOperation @event => @event.Member,
+                _ => throw new InvalidOperationException("Unexpected operation kind: " + context.Operation.Kind),
+            };
+
+            VisitOperationSymbol(context, symbol);
         }
 
-        private void AnalyzeNode(SyntaxNodeAnalysisContext context)
+        private void AnalyzeSymbol(SymbolAnalysisContext context)
         {
-            switch (context.Node)
+            // Note: we don't currently try to detect second-order usage of these types
+            // like public Task<InternalFoo> GetFooAsync() { }.
+            //
+            // This probably accomplishes our goals OK for now, which are focused on use of these
+            // types in method bodies.
+            switch (context.Symbol)
             {
-                case MemberAccessExpressionSyntax memberAccessSyntax:
+                case INamedTypeSymbol type:
+                    VisitDeclarationSymbol(context, type.BaseType, type);
+                    foreach (var @interface in type.Interfaces)
                     {
-                        if (context.SemanticModel.GetSymbolInfo(context.Node, context.CancellationToken).Symbol is ISymbol symbol &&
-                            symbol.ContainingAssembly != context.Compilation.Assembly)
-                        {
-                            var containingType = symbol.ContainingType;
-
-                            if (HasInternalAttribute(symbol))
-                            {
-                                context.ReportDiagnostic(Diagnostic.Create(_descriptor, memberAccessSyntax.Name.GetLocation(), $"{containingType}.{symbol.Name}"));
-                                return;
-                            }
-
-                            if (IsInInternalNamespace(containingType) || HasInternalAttribute(containingType))
-                            {
-                                context.ReportDiagnostic(Diagnostic.Create(_descriptor, memberAccessSyntax.Name.GetLocation(), containingType));
-                                return;
-                            }
-                        }
-                        return;
+                        VisitDeclarationSymbol(context, @interface, type);
                     }
+                    break;
 
-                case ObjectCreationExpressionSyntax creationSyntax:
-                    {
-                        if (context.SemanticModel.GetSymbolInfo(context.Node, context.CancellationToken).Symbol is ISymbol symbol &&
-                            symbol.ContainingAssembly != context.Compilation.Assembly)
-                        {
-                            var containingType = symbol.ContainingType;
-
-                            if (HasInternalAttribute(symbol))
-                            {
-                                context.ReportDiagnostic(Diagnostic.Create(_descriptor, creationSyntax.GetLocation(), containingType));
-                                return;
-                            }
-
-                            if (IsInInternalNamespace(containingType) || HasInternalAttribute(containingType))
-                            {
-                                context.ReportDiagnostic(Diagnostic.Create(_descriptor, creationSyntax.Type.GetLocation(), containingType));
-                                return;
-                            }
-                        }
+                case IFieldSymbol field:
+                    VisitDeclarationSymbol(context, field.Type, field);
+                    break;
 
-                        return;
+                case IMethodSymbol method:
+
+                    // Ignore return types on property-getters. Those will be reported through
+                    // the property analysis.
+                    if (method.MethodKind != MethodKind.PropertyGet)
+                    {
+                        VisitDeclarationSymbol(context, method.ReturnType, method);
                     }
 
-                case ClassDeclarationSyntax declarationSyntax:
+                    // Ignore parameters on property-setters. Those will be reported through
+                    // the property analysis.
+                    if (method.MethodKind != MethodKind.PropertySet)
                     {
-                        if (context.SemanticModel.GetDeclaredSymbol(declarationSyntax)?.BaseType is ISymbol symbol &&
-                            symbol.ContainingAssembly != context.Compilation.Assembly &&
-                            (IsInInternalNamespace(symbol) || HasInternalAttribute(symbol)) &&
-                            declarationSyntax.BaseList?.Types.Count > 0)
+                        foreach (var parameter in method.Parameters)
                         {
-                            context.ReportDiagnostic(Diagnostic.Create(_descriptor, declarationSyntax.BaseList.Types[0].GetLocation(), symbol));
+                            VisitDeclarationSymbol(context, parameter.Type, method);
                         }
-
-                        return;
                     }
+                    break;
 
-                case ParameterSyntax parameterSyntax:
-                    {
-                        if (context.SemanticModel.GetDeclaredSymbol(parameterSyntax)?.Type is ISymbol symbol &&
-                            symbol.ContainingAssembly != context.Compilation.Assembly &&
-                            (IsInInternalNamespace(symbol) || HasInternalAttribute(symbol)))
-                        {
+                case IPropertySymbol property:
+                    VisitDeclarationSymbol(context, property.Type, property);
+                    break;
 
-                            context.ReportDiagnostic(Diagnostic.Create(_descriptor, parameterSyntax.GetLocation(), symbol));
-                        }
+                case IEventSymbol @event:
+                    VisitDeclarationSymbol(context, @event.Type, @event);
+                    break;
+            }
+        }
 
-                        return;
-                    }
+        // Similar logic here to VisitDeclarationSymbol, keep these in sync.
+        private void VisitOperationSymbol(OperationAnalysisContext context, ISymbol symbol)
+        {
+            if (symbol.ContainingAssembly == context.Compilation.Assembly)
+            {
+                // The type is being referenced within the same assembly. This is valid use of an "internal" type
+                return;
+            }
+
+            if (HasInternalAttribute(symbol))
+            {
+                context.ReportDiagnostic(Diagnostic.Create(
+                    _descriptor,
+                    context.Operation.Syntax.GetLocation(),
+                    symbol.ToDisplayString(SymbolDisplayFormat.CSharpShortErrorMessageFormat)));
+                return;
+            }
+
+            var containingType = symbol.ContainingType;
+            if (IsInInternalNamespace(containingType) || HasInternalAttribute(containingType))
+            {
+                context.ReportDiagnostic(Diagnostic.Create(
+                    _descriptor,
+                    context.Operation.Syntax.GetLocation(),
+                    containingType.ToDisplayString(SymbolDisplayFormat.CSharpShortErrorMessageFormat)));
+                return;
+            }
+        }
+
+        // Similar logic here to VisitOperationSymbol, keep these in sync.
+        private void VisitDeclarationSymbol(SymbolAnalysisContext context, ISymbol symbol, ISymbol symbolForDiagnostic)
+        {
+            if (symbol.ContainingAssembly == context.Compilation.Assembly)
+            {
+                // This is part of the compilation, avoid this analyzer when building from source.
+                return;
+            }
+
+            if (HasInternalAttribute(symbol))
+            {
+                context.ReportDiagnostic(Diagnostic.Create(
+                    _descriptor,
+                    symbolForDiagnostic.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax().GetLocation() ?? Location.None,
+                    symbol.ToDisplayString(SymbolDisplayFormat.CSharpShortErrorMessageFormat)));
+                return;
+            }
+
+            var containingType = symbol as INamedTypeSymbol ?? symbol.ContainingType;
+            if (IsInInternalNamespace(containingType) || HasInternalAttribute(containingType))
+            {
+                context.ReportDiagnostic(Diagnostic.Create(
+                    _descriptor,
+                    symbolForDiagnostic.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax().GetLocation() ?? Location.None,
+                    containingType.ToDisplayString(SymbolDisplayFormat.CSharpShortErrorMessageFormat)));
+                return;
             }
         }
 

src/Components/Analyzers/test/ComponentInternalUsageDiagnosticsAnalyzerTest.cs

@@ -20,10 +20,10 @@ public ComponentInternalUsageDiagnosticsAnalyzerTest()
         private ComponentAnalyzerDiagnosticAnalyzerRunner Runner { get; }
 
         [Fact]
-        public async Task InternalUsage_FindsUseOfRenderTreeFrameAsParameter()
+        public async Task InternalUsage_FindsUseOfInternalTypesInDeclarations()
         {
             // Arrange
-            var source = Read("UsesRenderTreeFrameAsParameter");
+            var source = Read("UsesRendererTypesInDeclarations");
 
             // Act
             var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);
@@ -34,15 +34,35 @@ public async Task InternalUsage_FindsUseOfRenderTreeFrameAsParameter()
                 diagnostic =>
                 {
                     Assert.Same(DiagnosticDescriptors.DoNotUseRenderTreeTypes, diagnostic.Descriptor);
-                    AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
+                    AnalyzerAssert.DiagnosticLocation(source.MarkerLocations["MMBaseClass"], diagnostic.Location);
+                },
+                diagnostic =>
+                {
+                    Assert.Same(DiagnosticDescriptors.DoNotUseRenderTreeTypes, diagnostic.Descriptor);
+                    AnalyzerAssert.DiagnosticLocation(source.MarkerLocations["MMField"], diagnostic.Location);
+                },
+                diagnostic =>
+                {
+                    Assert.Same(DiagnosticDescriptors.DoNotUseRenderTreeTypes, diagnostic.Descriptor);
+                    AnalyzerAssert.DiagnosticLocation(source.MarkerLocations["MMProperty"], diagnostic.Location);
+                },
+                diagnostic =>
+                {
+                    Assert.Same(DiagnosticDescriptors.DoNotUseRenderTreeTypes, diagnostic.Descriptor);
+                    AnalyzerAssert.DiagnosticLocation(source.MarkerLocations["MMParameter"], diagnostic.Location);
+                },
+                diagnostic =>
+                {
+                    Assert.Same(DiagnosticDescriptors.DoNotUseRenderTreeTypes, diagnostic.Descriptor);
+                    AnalyzerAssert.DiagnosticLocation(source.MarkerLocations["MMReturnType"], diagnostic.Location);
                 });
         }
 
         [Fact]
-        public async Task InternalUsage_FindsUseOfRenderTreeType()
+        public async Task InternalUsage_FindsUseOfInternalTypesInMethodBody()
         {
             // Arrange
-            var source = Read("UsesRenderTreeFrameTypeAsLocal");
+            var source = Read("UsersRendererTypesInMethodBody");
 
             // Act
             var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);
@@ -53,7 +73,17 @@ public async Task InternalUsage_FindsUseOfRenderTreeType()
                 diagnostic =>
                 {
                     Assert.Same(DiagnosticDescriptors.DoNotUseRenderTreeTypes, diagnostic.Descriptor);
-                    AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
+                    AnalyzerAssert.DiagnosticLocation(source.MarkerLocations["MMField"], diagnostic.Location);
+                },
+                diagnostic =>
+                {
+                    Assert.Same(DiagnosticDescriptors.DoNotUseRenderTreeTypes, diagnostic.Descriptor);
+                    AnalyzerAssert.DiagnosticLocation(source.MarkerLocations["MMNewObject"], diagnostic.Location);
+                },
+                diagnostic =>
+                {
+                    Assert.Same(DiagnosticDescriptors.DoNotUseRenderTreeTypes, diagnostic.Descriptor);
+                    AnalyzerAssert.DiagnosticLocation(source.MarkerLocations["MMProperty"], diagnostic.Location);
                 });
         }
     }

src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRenderTreeFrameTypeAsLocal.cs → src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsersRendererTypesInMethodBody.cs

@@ -3,12 +3,15 @@
 
 namespace Microsoft.AspNetCore.Components.Analyzers.Tests.TestFiles.ComponentInternalUsageDiagnosticsAnalyzerTest
 {
-    class UsesRenderTreeFrameTypeAsLocal
+    class UsersRendererTypesInMethodBody
     {
         private void Test()
         {
-            var test = RenderTreeFrameType./*MM*/Attribute;
+            var test = /*MMField*/RenderTreeFrameType.Attribute;
             GC.KeepAlive(test);
+
+            var frame = /*MMNewObject*/new RenderTreeFrame();
+            GC.KeepAlive(/*MMProperty*/frame.Component);
         }
 
     }

src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRendererAsBaseClass.cs

@@ -0,0 +1,26 @@
+using System;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Components.RenderTree;
+
+namespace Microsoft.AspNetCore.Components.Analyzers.Tests.TestFiles.ComponentInternalUsageDiagnosticsAnalyzerTest
+{
+    /*MM*/class UsesRendererAsBaseClass : Renderer
+    {
+        public UsesRendererAsBaseClass()
+            : base(null, null)
+        {
+        }
+
+        public override Dispatcher Dispatcher => throw new NotImplementedException();
+
+        protected override void HandleException(Exception exception)
+        {
+            throw new NotImplementedException();
+        }
+
+        protected override Task UpdateDisplayAsync(/*M1*/in RenderBatch renderBatch)
+        {
+            throw new NotImplementedException();
+        }
+    }
+}

src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRendererTypesInDeclarations.cs

@@ -0,0 +1,32 @@
+using System;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Components.RenderTree;
+
+namespace Microsoft.AspNetCore.Components.Analyzers.Tests.TestFiles.ComponentInternalUsageDiagnosticsAnalyzerTest
+{
+    /*MMBaseClass*/class UsesRendererTypesInDeclarations : Renderer
+    {
+        private Renderer /*MMField*/_field = null;
+
+        public UsesRendererTypesInDeclarations()
+            : base(null, null)
+        {
+        }
+
+        public override Dispatcher Dispatcher => throw new NotImplementedException();
+
+        /*MMProperty*/public Renderer Property { get; set; }
+
+        protected override void HandleException(Exception exception)
+        {
+            throw new NotImplementedException();
+        }
+
+        /*MMParameter*/protected override Task UpdateDisplayAsync(in RenderBatch renderBatch) 
+        {
+            throw new NotImplementedException();
+        }
+
+        /*MMReturnType*/private Renderer GetRenderer() => _field;
+    }
+}