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;