From cae31a55f90a91d7d98cab2f8ee0abfd38f21761 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Thu, 11 Jul 2019 15:37:03 -0700 Subject: [PATCH] Add analyzer to warn for invalid Component parameter usage. - The newly added analyzer warns users if they try to assign another components parameter.It does sanity checks to ensure that 1. The property reference is indeed a component parameter 2. The property reference is from a component 3. The assignment is outside of the parameters type hierarchy. Aka, we don't warn users for setting a components parameter if it's in the same class. - Updated existing `ComponentsFacts` to add additional utility methods to properly interact with components. - Added tests to ensure we're analyzing all the goods properly. #8825 --- .../Analyzers/src/ComponentFacts.cs | 22 ++ .../src/ComponentParameterUsageAnalyzer.cs | 104 ++++++ .../Analyzers/src/ComponentSymbols.cs | 20 +- src/Components/Analyzers/src/ComponentsApi.cs | 6 + .../Analyzers/src/DiagnosticDescriptors.cs | 9 + .../Analyzers/src/Resources.Designer.cs | 27 ++ src/Components/Analyzers/src/Resources.resx | 9 + .../ComponentParameterUsageAnalyzerTest.cs | 298 ++++++++++++++++++ .../test/ComponentsTestDeclarations.cs | 4 + 9 files changed, 496 insertions(+), 3 deletions(-) create mode 100644 src/Components/Analyzers/src/ComponentParameterUsageAnalyzer.cs create mode 100644 src/Components/Analyzers/test/ComponentParameterUsageAnalyzerTest.cs diff --git a/src/Components/Analyzers/src/ComponentFacts.cs b/src/Components/Analyzers/src/ComponentFacts.cs index 031df5586f..75403ce282 100644 --- a/src/Components/Analyzers/src/ComponentFacts.cs +++ b/src/Components/Analyzers/src/ComponentFacts.cs @@ -4,6 +4,7 @@ using System; using System.Linq; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; namespace Microsoft.AspNetCore.Components.Analyzers { @@ -85,5 +86,26 @@ namespace Microsoft.AspNetCore.Components.Analyzers return property.GetAttributes().Any(a => a.AttributeClass == symbols.CascadingParameterAttribute); } + + public static bool IsComponent(ComponentSymbols symbols, Compilation compilation, INamedTypeSymbol type) + { + if (symbols is null) + { + throw new ArgumentNullException(nameof(symbols)); + } + + if (type is null) + { + throw new ArgumentNullException(nameof(type)); + } + + var conversion = compilation.ClassifyConversion(symbols.IComponentType, type); + if (!conversion.Exists || !conversion.IsExplicit) + { + return false; + } + + return true; + } } } diff --git a/src/Components/Analyzers/src/ComponentParameterUsageAnalyzer.cs b/src/Components/Analyzers/src/ComponentParameterUsageAnalyzer.cs new file mode 100644 index 0000000000..0d45edcabc --- /dev/null +++ b/src/Components/Analyzers/src/ComponentParameterUsageAnalyzer.cs @@ -0,0 +1,104 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Immutable; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace Microsoft.AspNetCore.Components.Analyzers +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class ComponentParameterUsageAnalyzer : DiagnosticAnalyzer + { + public ComponentParameterUsageAnalyzer() + { + SupportedDiagnostics = ImmutableArray.Create(new[] + { + DiagnosticDescriptors.ComponentParametersShouldNotBeSetOutsideOfTheirDeclaredComponent, + }); + } + + public override ImmutableArray SupportedDiagnostics { get; } + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); + context.RegisterCompilationStartAction(context => + { + if (!ComponentSymbols.TryCreate(context.Compilation, out var symbols)) + { + // Types we need are not defined. + return; + } + + context.RegisterOperationBlockStartAction(startBlockContext => + { + startBlockContext.RegisterOperationAction(context => + { + var assignmentOperation = (IAssignmentOperation)context.Operation; + var leftHandSide = assignmentOperation.Target; + if (leftHandSide == null) + { + // Malformed assignment, no left hand side. + return; + } + + if (leftHandSide.Kind != OperationKind.PropertyReference) + { + // We don't want to capture situations where a user does + // MyOtherProperty = aComponent.SomeParameter + return; + } + + var propertyReference = (IPropertyReferenceOperation)leftHandSide; + var componentProperty = (IPropertySymbol)propertyReference.Member; + + if (!ComponentFacts.IsParameter(symbols, componentProperty)) + { + // This is not a property reference that we care about, it is not decorated with [Parameter]. + return; + } + + var propertyContainingType = componentProperty.ContainingType; + if (!ComponentFacts.IsComponent(symbols, context.Compilation, propertyContainingType)) + { + // Someone referenced a property as [Parameter] inside something that is not a component. + return; + } + + var assignmentContainingType = startBlockContext.OwningSymbol?.ContainingType; + if (assignmentContainingType == null) + { + // Assignment location has no containing type. Most likely we're operating on malformed code, don't try and validate. + return; + } + + var conversion = context.Compilation.ClassifyConversion(propertyContainingType, assignmentContainingType); + if (conversion.Exists && conversion.IsIdentity) + { + // The assignment is taking place inside of the declaring component. + return; + } + + if (conversion.Exists && conversion.IsExplicit) + { + // The assignment is taking place within the components type hierarchy. This means the user is setting this in a supported + // scenario. + return; + } + + // At this point the user is referencing a component parameter outside of its declaring class. + + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.ComponentParametersShouldNotBeSetOutsideOfTheirDeclaredComponent, + propertyReference.Syntax.GetLocation(), + propertyReference.Member.Name)); + }, OperationKind.SimpleAssignment, OperationKind.CompoundAssignment, OperationKind.CoalesceAssignment); + }); + }); + } + } +} diff --git a/src/Components/Analyzers/src/ComponentSymbols.cs b/src/Components/Analyzers/src/ComponentSymbols.cs index ba1bfc4869..d115e27f53 100644 --- a/src/Components/Analyzers/src/ComponentSymbols.cs +++ b/src/Components/Analyzers/src/ComponentSymbols.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; using Microsoft.CodeAnalysis; namespace Microsoft.AspNetCore.Components.Analyzers @@ -30,6 +29,13 @@ namespace Microsoft.AspNetCore.Components.Analyzers return false; } + var icomponentType = compilation.GetTypeByMetadataName(ComponentsApi.IComponent.MetadataName); + if (icomponentType == null) + { + symbols = null; + return false; + } + var dictionary = compilation.GetTypeByMetadataName("System.Collections.Generic.Dictionary`2"); var @string = compilation.GetSpecialType(SpecialType.System_String); var @object = compilation.GetSpecialType(SpecialType.System_Object); @@ -41,18 +47,24 @@ namespace Microsoft.AspNetCore.Components.Analyzers var parameterCaptureUnmatchedValuesRuntimeType = dictionary.Construct(@string, @object); - symbols = new ComponentSymbols(parameterAttribute, cascadingParameterAttribute, parameterCaptureUnmatchedValuesRuntimeType); + symbols = new ComponentSymbols( + parameterAttribute, + cascadingParameterAttribute, + parameterCaptureUnmatchedValuesRuntimeType, + icomponentType); return true; } private ComponentSymbols( INamedTypeSymbol parameterAttribute, INamedTypeSymbol cascadingParameterAttribute, - INamedTypeSymbol parameterCaptureUnmatchedValuesRuntimeType) + INamedTypeSymbol parameterCaptureUnmatchedValuesRuntimeType, + INamedTypeSymbol icomponentType) { ParameterAttribute = parameterAttribute; CascadingParameterAttribute = cascadingParameterAttribute; ParameterCaptureUnmatchedValuesRuntimeType = parameterCaptureUnmatchedValuesRuntimeType; + IComponentType = icomponentType; } public INamedTypeSymbol ParameterAttribute { get; } @@ -61,5 +73,7 @@ namespace Microsoft.AspNetCore.Components.Analyzers public INamedTypeSymbol ParameterCaptureUnmatchedValuesRuntimeType { get; } public INamedTypeSymbol CascadingParameterAttribute { get; } + + public INamedTypeSymbol IComponentType { get; } } } diff --git a/src/Components/Analyzers/src/ComponentsApi.cs b/src/Components/Analyzers/src/ComponentsApi.cs index 83eb39cc2c..73ceff39fd 100644 --- a/src/Components/Analyzers/src/ComponentsApi.cs +++ b/src/Components/Analyzers/src/ComponentsApi.cs @@ -22,5 +22,11 @@ namespace Microsoft.AspNetCore.Components.Analyzers public static readonly string FullTypeName = "Microsoft.AspNetCore.Components.CascadingParameterAttribute"; public static readonly string MetadataName = FullTypeName; } + + public static class IComponent + { + public static readonly string FullTypeName = "Microsoft.AspNetCore.Components.IComponent"; + public static readonly string MetadataName = FullTypeName; + } } } diff --git a/src/Components/Analyzers/src/DiagnosticDescriptors.cs b/src/Components/Analyzers/src/DiagnosticDescriptors.cs index 74632adc6f..a34102f39b 100644 --- a/src/Components/Analyzers/src/DiagnosticDescriptors.cs +++ b/src/Components/Analyzers/src/DiagnosticDescriptors.cs @@ -46,5 +46,14 @@ namespace Microsoft.AspNetCore.Components.Analyzers DiagnosticSeverity.Warning, isEnabledByDefault: true, description: new LocalizableResourceString(nameof(Resources.ComponentParametersShouldBePublic_Description), Resources.ResourceManager, typeof(Resources))); + + public static readonly DiagnosticDescriptor ComponentParametersShouldNotBeSetOutsideOfTheirDeclaredComponent = new DiagnosticDescriptor( + "BL0005", + new LocalizableResourceString(nameof(Resources.ComponentParameterShouldNotBeSetOutsideOfTheirDeclaredComponent_Title), Resources.ResourceManager, typeof(Resources)), + new LocalizableResourceString(nameof(Resources.ComponentParameterShouldNotBeSetOutsideOfTheirDeclaredComponent_Format), Resources.ResourceManager, typeof(Resources)), + "Usage", + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: new LocalizableResourceString(nameof(Resources.ComponentParameterShouldNotBeSetOutsideOfTheirDeclaredComponent_Description), Resources.ResourceManager, typeof(Resources))); } } diff --git a/src/Components/Analyzers/src/Resources.Designer.cs b/src/Components/Analyzers/src/Resources.Designer.cs index 13c55946a3..a6e48e8c63 100644 --- a/src/Components/Analyzers/src/Resources.Designer.cs +++ b/src/Components/Analyzers/src/Resources.Designer.cs @@ -159,6 +159,33 @@ namespace Microsoft.AspNetCore.Components.Analyzers { } } + /// + /// Looks up a localized string similar to Component parameters should not be set outside of their declared component. Doing so may result in unexpected behavior at runtime.. + /// + internal static string ComponentParameterShouldNotBeSetOutsideOfTheirDeclaredComponent_Description { + get { + return ResourceManager.GetString("ComponentParameterShouldNotBeSetOutsideOfTheirDeclaredComponent_Description", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Component parameter '{0}' should not be set outside of its component.. + /// + internal static string ComponentParameterShouldNotBeSetOutsideOfTheirDeclaredComponent_Format { + get { + return ResourceManager.GetString("ComponentParameterShouldNotBeSetOutsideOfTheirDeclaredComponent_Format", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Component parameter should not be set outside of its component.. + /// + internal static string ComponentParameterShouldNotBeSetOutsideOfTheirDeclaredComponent_Title { + get { + return ResourceManager.GetString("ComponentParameterShouldNotBeSetOutsideOfTheirDeclaredComponent_Title", resourceCulture); + } + } + /// /// Looks up a localized string similar to Component parameters should be public.. /// diff --git a/src/Components/Analyzers/src/Resources.resx b/src/Components/Analyzers/src/Resources.resx index 1c4319cb5b..648adb2c3b 100644 --- a/src/Components/Analyzers/src/Resources.resx +++ b/src/Components/Analyzers/src/Resources.resx @@ -156,4 +156,13 @@ Make component parameters public. + + Component parameters should not be set outside of their declared component. Doing so may result in unexpected behavior at runtime. + + + Component parameter '{0}' should not be set outside of its component. + + + Component parameter should not be set outside of its component. + \ No newline at end of file diff --git a/src/Components/Analyzers/test/ComponentParameterUsageAnalyzerTest.cs b/src/Components/Analyzers/test/ComponentParameterUsageAnalyzerTest.cs new file mode 100644 index 0000000000..db664a2e76 --- /dev/null +++ b/src/Components/Analyzers/test/ComponentParameterUsageAnalyzerTest.cs @@ -0,0 +1,298 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using TestHelper; +using Xunit; + +namespace Microsoft.AspNetCore.Components.Analyzers +{ + public class ComponentParameterUsageAnalyzerTest : DiagnosticVerifier + { + public ComponentParameterUsageAnalyzerTest() + { + ComponentTestSource = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class TestComponent : IComponent + {{ + [Parameter] public string TestProperty {{ get; set; }} + public string NonParameter {{ get; set; }} + }} + }}" + ComponentsTestDeclarations.Source; + } + + private string ComponentTestSource { get; } + + [Fact] + public void ComponentPropertySimpleAssignment_Warns() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class OtherComponent : IComponent + {{ + private TestComponent _testComponent; + void Render() + {{ + _testComponent = new TestComponent(); + _testComponent.TestProperty = ""Hello World""; + }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test, + new DiagnosticResult + { + Id = DiagnosticDescriptors.ComponentParametersShouldNotBeSetOutsideOfTheirDeclaredComponent.Id, + Message = "Component parameter 'TestProperty' should not be set outside of its component.", + Severity = DiagnosticSeverity.Warning, + Locations = new[] + { + new DiagnosticResultLocation("Test0.cs", 11, 17) + } + }); + } + + [Fact] + public void ComponentPropertyCoalesceAssignment__Warns() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class OtherComponent : IComponent + {{ + private TestComponent _testComponent; + void Render() + {{ + _testComponent = new TestComponent(); + _testComponent.TestProperty ??= ""Hello World""; + }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test, + new DiagnosticResult + { + Id = DiagnosticDescriptors.ComponentParametersShouldNotBeSetOutsideOfTheirDeclaredComponent.Id, + Message = "Component parameter 'TestProperty' should not be set outside of its component.", + Severity = DiagnosticSeverity.Warning, + Locations = new[] + { + new DiagnosticResultLocation("Test0.cs", 11, 17) + } + }); + } + + [Fact] + public void ComponentPropertyCompoundAssignment__Warns() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class OtherComponent : IComponent + {{ + private TestComponent _testComponent; + void Render() + {{ + _testComponent = new TestComponent(); + _testComponent.TestProperty += ""Hello World""; + }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test, + new DiagnosticResult + { + Id = DiagnosticDescriptors.ComponentParametersShouldNotBeSetOutsideOfTheirDeclaredComponent.Id, + Message = "Component parameter 'TestProperty' should not be set outside of its component.", + Severity = DiagnosticSeverity.Warning, + Locations = new[] + { + new DiagnosticResultLocation("Test0.cs", 11, 17) + } + }); + } + + [Fact] + public void ComponentPropertyExpression_Ignores() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class TypeName + {{ + void Method() + {{ + System.IO.Console.WriteLine(new TestComponent().TestProperty); + }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test); + } + + [Fact] + public void ComponentPropertyExpressionInStatement_Ignores() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class TypeName + {{ + void Method() + {{ + var testComponent = new TestComponent(); + for (var i = 0; i < testComponent.TestProperty.Length; i++) + {{ + }} + }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test); + } + + [Fact] + public void RetrievalOfComponentPropertyValueInAssignment_Ignores() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class TypeName + {{ + void Method() + {{ + var testComponent = new TestComponent(); + AnotherProperty = testComponent.TestProperty; + }} + + public string AnotherProperty {{ get; set; }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test); + } + + [Fact] + public void ShadowedComponentPropertyAssignment_Ignores() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class TypeName + {{ + void Method() + {{ + var testComponent = new InheritedComponent(); + testComponent.TestProperty = ""Hello World""; + }} + }} + + class InheritedComponent : TestComponent + {{ + public new string TestProperty {{ get; set; }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test); + } + + [Fact] + public void InheritedImplicitComponentPropertyAssignment_Ignores() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class TypeName : TestComponent + {{ + void Method() + {{ + this.TestProperty = ""Hello World""; + }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test); + } + + [Fact] + public void ImplicitComponentPropertyAssignment_Ignores() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class TypeName : IComponent + {{ + void Method() + {{ + TestProperty = ""Hello World""; + }} + + [Parameter] public string TestProperty {{ get; set; }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test); + } + + [Fact] + public void ComponentPropertyAssignment_NonParameter_Ignores() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class OtherComponent : IComponent + {{ + private TestComponent _testComponent; + void Render() + {{ + _testComponent = new TestComponent(); + _testComponent.NonParameter = ""Hello World""; + }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test); + } + + [Fact] + public void NonComponentPropertyAssignment_Ignores() + { + var test = $@" + namespace ConsoleApplication1 + {{ + using {typeof(ParameterAttribute).Namespace}; + class OtherComponent : IComponent + {{ + private SomethingElse _testNonComponent; + void Render() + {{ + _testNonComponent = new NotAComponent(); + _testNonComponent.TestProperty = ""Hello World""; + }} + }} + class NotAComponent + {{ + [Parameter] public string TestProperty {{ get; set; }} + }} + }}" + ComponentTestSource; + + VerifyCSharpDiagnostic(test); + } + + protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() => new ComponentParameterUsageAnalyzer(); + } +} diff --git a/src/Components/Analyzers/test/ComponentsTestDeclarations.cs b/src/Components/Analyzers/test/ComponentsTestDeclarations.cs index e09f156ff4..3c71a82566 100644 --- a/src/Components/Analyzers/test/ComponentsTestDeclarations.cs +++ b/src/Components/Analyzers/test/ComponentsTestDeclarations.cs @@ -16,6 +16,10 @@ namespace Microsoft.AspNetCore.Components.Analyzers public class {typeof(CascadingParameterAttribute).Name} : System.Attribute {{ }} + + public interface {typeof(IComponent).Name} + {{ + }} }} "; }