From a159473c5729d4e334954d185c69ac144641685f Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 12 Nov 2019 17:48:12 -0800 Subject: [PATCH] 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. --- .../Analyzers/src/InternalUsageAnalyzer.cs | 191 ++++++++++++------ ...entInternalUsageDiagnosticsAnalyzerTest.cs | 42 +++- ...l.cs => UsersRendererTypesInMethodBody.cs} | 7 +- .../UsesRenderTreeFrameAsParameter.cs | 11 - .../UsesRendererAsBaseClass.cs | 26 +++ .../UsesRendererTypesInDeclarations.cs | 32 +++ .../src/AvoidHtmlPartialAnalyzer.cs | 18 +- .../src/ViewFeaturesAnalyzerContext.cs | 5 - 8 files changed, 231 insertions(+), 101 deletions(-) rename src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/{UsesRenderTreeFrameTypeAsLocal.cs => UsersRendererTypesInMethodBody.cs} (54%) delete mode 100644 src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRenderTreeFrameAsParameter.cs create mode 100644 src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRendererAsBaseClass.cs create mode 100644 src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRendererTypesInDeclarations.cs diff --git a/src/Components/Analyzers/src/InternalUsageAnalyzer.cs b/src/Components/Analyzers/src/InternalUsageAnalyzer.cs index 92b07a7ab2..2d76122377 100644 --- a/src/Components/Analyzers/src/InternalUsageAnalyzer.cs +++ b/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 @@ namespace Microsoft.Extensions.Internal 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 AnalyzeNode(SyntaxNodeAnalysisContext context) + private void AnalyzeOperation(OperationAnalysisContext context) { - switch (context.Node) + var symbol = context.Operation switch { - case MemberAccessExpressionSyntax memberAccessSyntax: + 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 AnalyzeSymbol(SymbolAnalysisContext context) + { + // Note: we don't currently try to detect second-order usage of these types + // like public Task GetFooAsync() { }. + // + // This probably accomplishes our goals OK for now, which are focused on use of these + // types in method bodies. + switch (context.Symbol) + { + 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; + VisitDeclarationSymbol(context, @interface, type); + } + break; - if (HasInternalAttribute(symbol)) - { - context.ReportDiagnostic(Diagnostic.Create(_descriptor, memberAccessSyntax.Name.GetLocation(), $"{containingType}.{symbol.Name}")); - return; - } + case IFieldSymbol field: + VisitDeclarationSymbol(context, field.Type, field); + break; - if (IsInInternalNamespace(containingType) || HasInternalAttribute(containingType)) - { - context.ReportDiagnostic(Diagnostic.Create(_descriptor, memberAccessSyntax.Name.GetLocation(), containingType)); - return; - } - } - 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 ObjectCreationExpressionSyntax creationSyntax: + // Ignore parameters on property-setters. Those will be reported through + // the property analysis. + if (method.MethodKind != MethodKind.PropertySet) { - if (context.SemanticModel.GetSymbolInfo(context.Node, context.CancellationToken).Symbol is ISymbol symbol && - symbol.ContainingAssembly != context.Compilation.Assembly) + foreach (var parameter in method.Parameters) { - 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; - } + VisitDeclarationSymbol(context, parameter.Type, method); } - - return; } + break; - case ClassDeclarationSyntax declarationSyntax: - { - if (context.SemanticModel.GetDeclaredSymbol(declarationSyntax)?.BaseType is ISymbol symbol && - symbol.ContainingAssembly != context.Compilation.Assembly && - (IsInInternalNamespace(symbol) || HasInternalAttribute(symbol)) && - declarationSyntax.BaseList?.Types.Count > 0) - { - context.ReportDiagnostic(Diagnostic.Create(_descriptor, declarationSyntax.BaseList.Types[0].GetLocation(), symbol)); - } + case IPropertySymbol property: + VisitDeclarationSymbol(context, property.Type, property); + break; - return; - } + case IEventSymbol @event: + VisitDeclarationSymbol(context, @event.Type, @event); + break; + } + } - case ParameterSyntax parameterSyntax: - { - if (context.SemanticModel.GetDeclaredSymbol(parameterSyntax)?.Type is ISymbol symbol && - symbol.ContainingAssembly != context.Compilation.Assembly && - (IsInInternalNamespace(symbol) || HasInternalAttribute(symbol))) - { + // 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; + } - context.ReportDiagnostic(Diagnostic.Create(_descriptor, parameterSyntax.GetLocation(), symbol)); - } + if (HasInternalAttribute(symbol)) + { + context.ReportDiagnostic(Diagnostic.Create( + _descriptor, + context.Operation.Syntax.GetLocation(), + symbol.ToDisplayString(SymbolDisplayFormat.CSharpShortErrorMessageFormat))); + return; + } - 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; } } diff --git a/src/Components/Analyzers/test/ComponentInternalUsageDiagnosticsAnalyzerTest.cs b/src/Components/Analyzers/test/ComponentInternalUsageDiagnosticsAnalyzerTest.cs index e478f1ef45..3c5b11b577 100644 --- a/src/Components/Analyzers/test/ComponentInternalUsageDiagnosticsAnalyzerTest.cs +++ b/src/Components/Analyzers/test/ComponentInternalUsageDiagnosticsAnalyzerTest.cs @@ -20,10 +20,10 @@ namespace Microsoft.AspNetCore.Components.Analyzers 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 @@ namespace Microsoft.AspNetCore.Components.Analyzers 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 @@ namespace Microsoft.AspNetCore.Components.Analyzers 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); }); } } diff --git a/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRenderTreeFrameTypeAsLocal.cs b/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsersRendererTypesInMethodBody.cs similarity index 54% rename from src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRenderTreeFrameTypeAsLocal.cs rename to src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsersRendererTypesInMethodBody.cs index daf857f996..c6495e084e 100644 --- a/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRenderTreeFrameTypeAsLocal.cs +++ b/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsersRendererTypesInMethodBody.cs @@ -3,12 +3,15 @@ using Microsoft.AspNetCore.Components.RenderTree; 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); } } diff --git a/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRenderTreeFrameAsParameter.cs b/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRenderTreeFrameAsParameter.cs deleted file mode 100644 index 453cd69d74..0000000000 --- a/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRenderTreeFrameAsParameter.cs +++ /dev/null @@ -1,11 +0,0 @@ -using Microsoft.AspNetCore.Components.RenderTree; - -namespace Microsoft.AspNetCore.Components.Analyzers.Tests.TestFiles.ComponentInternalUsageDiagnosticsAnalyzerTest -{ - class UsesRenderTreeFrameAsParameter - { - private void Test(/*MM*/RenderTreeFrame frame) - { - } - } -} diff --git a/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRendererAsBaseClass.cs b/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRendererAsBaseClass.cs new file mode 100644 index 0000000000..7ca9dfccf5 --- /dev/null +++ b/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(); + } + } +} diff --git a/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRendererTypesInDeclarations.cs b/src/Components/Analyzers/test/TestFiles/ComponentInternalUsageDiagnosticsAnalyzerTest/UsesRendererTypesInDeclarations.cs new file mode 100644 index 0000000000..ad0e10bbc5 --- /dev/null +++ b/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; + } +} diff --git a/src/Mvc/Mvc.Analyzers/src/AvoidHtmlPartialAnalyzer.cs b/src/Mvc/Mvc.Analyzers/src/AvoidHtmlPartialAnalyzer.cs index 3563fa525b..c4ba61910d 100644 --- a/src/Mvc/Mvc.Analyzers/src/AvoidHtmlPartialAnalyzer.cs +++ b/src/Mvc/Mvc.Analyzers/src/AvoidHtmlPartialAnalyzer.cs @@ -6,6 +6,7 @@ using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; namespace Microsoft.AspNetCore.Mvc.Analyzers { @@ -19,16 +20,9 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers protected override void InitializeWorker(ViewFeaturesAnalyzerContext analyzerContext) { - analyzerContext.Context.RegisterSyntaxNodeAction(context => + analyzerContext.Context.RegisterOperationAction(context => { - var invocationExpression = (InvocationExpressionSyntax)context.Node; - var symbol = context.SemanticModel.GetSymbolInfo(invocationExpression, context.CancellationToken).Symbol; - if (symbol == null || symbol.Kind != SymbolKind.Method) - { - return; - } - - var method = (IMethodSymbol)symbol; + var method = ((IInvocationOperation)context.Operation).TargetMethod; if (!analyzerContext.IsHtmlHelperExtensionMethod(method)) { return; @@ -38,17 +32,17 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers { context.ReportDiagnostic(Diagnostic.Create( SupportedDiagnostic, - invocationExpression.GetLocation(), + context.Operation.Syntax.GetLocation(), new[] { SymbolNames.PartialMethod })); } else if (string.Equals(SymbolNames.RenderPartialMethod, method.Name, StringComparison.Ordinal)) { context.ReportDiagnostic(Diagnostic.Create( SupportedDiagnostic, - invocationExpression.GetLocation(), + context.Operation.Syntax.GetLocation(), new[] { SymbolNames.RenderPartialMethod })); } - }, SyntaxKind.InvocationExpression); + }, OperationKind.Invocation); } } } diff --git a/src/Mvc/Mvc.Analyzers/src/ViewFeaturesAnalyzerContext.cs b/src/Mvc/Mvc.Analyzers/src/ViewFeaturesAnalyzerContext.cs index 096132d67f..6fe12d0b3d 100644 --- a/src/Mvc/Mvc.Analyzers/src/ViewFeaturesAnalyzerContext.cs +++ b/src/Mvc/Mvc.Analyzers/src/ViewFeaturesAnalyzerContext.cs @@ -32,11 +32,6 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return false; } - if (method.ReceiverType != HtmlHelperType) - { - return false; - } - if (method.ContainingType != HtmlHelperPartialExtensionsType) { return false;