From 0bcc8e6a6ecd69be36387475ebbfcc76940eaf2e Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Mon, 10 Mar 2025 14:16:11 -0700 Subject: [PATCH 01/12] Add a suppressor for CS8618 --- .../src/Analyzers/AnalyzerResources.resx | 123 ++++++++++++++++++ ...onentParameterNullableWarningSuppressor.cs | 36 +++++ ...crosoft.CodeAnalysis.Razor.Compiler.csproj | 3 +- 3 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerResources.resx create mode 100644 src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerResources.resx b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerResources.resx new file mode 100644 index 00000000000..950386a6330 --- /dev/null +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerResources.resx @@ -0,0 +1,123 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + text/microsoft-resx + + + 2.0 + + + System.Resources.ResXResourceReader, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + Suppress CS8618 when a component parameter is marked with Microsoft.AspNetCore.Components.EditorRequired + + \ No newline at end of file diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs new file mode 100644 index 00000000000..d4f9afd3631 --- /dev/null +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs @@ -0,0 +1,36 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using System.Resources; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Microsoft.CodeAnalysis.Razor.Compiler.Analyzers; + +#pragma warning disable RS1041 // Compiler extensions should be implemented in assemblies targeting netstandard2.0 + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class ComponentParameterNullableWarningSuppressor : DiagnosticSuppressor +{ + private static readonly LocalizableString Description = new LocalizableResourceString(nameof(AnalyzerResources.ComponentParameterNullableWarningSuppressorDescription), AnalyzerResources.ResourceManager, typeof(AnalyzerResources)); + + public override ImmutableArray SupportedSuppressions => [ + new SuppressionDescriptor("RZS1001", "CS8618", Description) + ]; + + public override void ReportSuppressions(SuppressionAnalysisContext context) + { + foreach (var diagnostic in context.ReportedDiagnostics) + { + var node = diagnostic.Location.SourceTree?.GetRoot().FindNode(diagnostic.Location.SourceSpan); + if (node is PropertyDeclarationSyntax property && property.AttributeLists.Any(a => a.Attributes.Any(a => a.Name.ToString() == "EditorRequired"))) + { + context.ReportSuppression(Suppression.Create(SupportedSuppressions[0], diagnostic)); + } + } + } +} + diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Microsoft.CodeAnalysis.Razor.Compiler.csproj b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Microsoft.CodeAnalysis.Razor.Compiler.csproj index 6428e0f55ed..2b3d5eb80e3 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Microsoft.CodeAnalysis.Razor.Compiler.csproj +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Microsoft.CodeAnalysis.Razor.Compiler.csproj @@ -1,4 +1,4 @@ - + Razor is a markup syntax for adding server-side logic to web pages. This package contains the Razor compiler. @@ -24,6 +24,7 @@ + From 31485a7d9aebeb7100bee116b4da35d874a7a1b0 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Tue, 11 Mar 2025 10:47:59 -0700 Subject: [PATCH 02/12] PR Feedback: - Use symbols rather than syntax - Only report if both parameter and editor required are on the property - Thread through the cancellation token --- .../src/Analyzers/AnalyzerResources.resx | 2 +- ...onentParameterNullableWarningSuppressor.cs | 21 ++++++++++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerResources.resx b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerResources.resx index 950386a6330..83c248dcd6b 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerResources.resx +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerResources.resx @@ -118,6 +118,6 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 - Suppress CS8618 when a component parameter is marked with Microsoft.AspNetCore.Components.EditorRequired + Suppress CS8618 when a component parameter is marked with Microsoft.AspNetCore.Components.EditorRequiredAttribute \ No newline at end of file diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs index d4f9afd3631..f6896d0e8f6 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs @@ -4,7 +4,6 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; -using System.Resources; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; @@ -13,7 +12,7 @@ namespace Microsoft.CodeAnalysis.Razor.Compiler.Analyzers; #pragma warning disable RS1041 // Compiler extensions should be implemented in assemblies targeting netstandard2.0 [DiagnosticAnalyzer(LanguageNames.CSharp)] -public class ComponentParameterNullableWarningSuppressor : DiagnosticSuppressor +public sealed class ComponentParameterNullableWarningSuppressor : DiagnosticSuppressor { private static readonly LocalizableString Description = new LocalizableResourceString(nameof(AnalyzerResources.ComponentParameterNullableWarningSuppressorDescription), AnalyzerResources.ResourceManager, typeof(AnalyzerResources)); @@ -23,12 +22,24 @@ public class ComponentParameterNullableWarningSuppressor : DiagnosticSuppressor public override void ReportSuppressions(SuppressionAnalysisContext context) { + var editorRequiredSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Components.EditorRequiredAttribute"); + var parameterSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Components.ParameterAttribute"); + foreach (var diagnostic in context.ReportedDiagnostics) { - var node = diagnostic.Location.SourceTree?.GetRoot().FindNode(diagnostic.Location.SourceSpan); - if (node is PropertyDeclarationSyntax property && property.AttributeLists.Any(a => a.Attributes.Any(a => a.Name.ToString() == "EditorRequired"))) + var node = diagnostic.Location.SourceTree?.GetRoot(context.CancellationToken).FindNode(diagnostic.Location.SourceSpan); + if (node is PropertyDeclarationSyntax propertySyntax && propertySyntax.AttributeLists.Any()) { - context.ReportSuppression(Suppression.Create(SupportedSuppressions[0], diagnostic)); + var symbol = context.GetSemanticModel(propertySyntax.SyntaxTree).GetDeclaredSymbol(propertySyntax, context.CancellationToken); + if (symbol is IPropertySymbol property) + { + var attributes = property.GetAttributes(); + if (attributes.Any(ad => SymbolEqualityComparer.Default.Equals(ad.AttributeClass, parameterSymbol)) + && attributes.Any(ad => SymbolEqualityComparer.Default.Equals(ad.AttributeClass, editorRequiredSymbol))) + { + context.ReportSuppression(Suppression.Create(SupportedSuppressions[0], diagnostic)); + } + } } } } From 8b1f80acbe6985f28686b652eccd0e887d27a268 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Tue, 11 Mar 2025 15:05:01 -0700 Subject: [PATCH 03/12] Update to latest ms.cs.analyzer.testing: - remove XunitVerifiers and use default verifier instead (see https://github.com/dotnet/roslyn-sdk/blob/main/src/Microsoft.CodeAnalysis.Testing/README.md#obsolete-packages) - Remove uneeded .xunit package references --- Directory.Packages.props | 7 +++---- .../Razor.Diagnostics.Analyzers.Test.csproj | 3 +-- .../Verifiers/CSharpAnalyzerVerifier`1+Test.cs | 4 ++-- .../Verifiers/CSharpAnalyzerVerifier`1.cs | 7 +++---- 4 files changed, 9 insertions(+), 12 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 972622cab02..6c578e1ff7b 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -32,13 +32,13 @@ - + - + @@ -49,8 +49,7 @@ - - + diff --git a/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Razor.Diagnostics.Analyzers.Test.csproj b/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Razor.Diagnostics.Analyzers.Test.csproj index d8977ed8c0f..ab94ba1cf89 100644 --- a/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Razor.Diagnostics.Analyzers.Test.csproj +++ b/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Razor.Diagnostics.Analyzers.Test.csproj @@ -8,8 +8,7 @@ - - + diff --git a/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Verifiers/CSharpAnalyzerVerifier`1+Test.cs b/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Verifiers/CSharpAnalyzerVerifier`1+Test.cs index d59cd6bfe2e..f01ea1f7665 100644 --- a/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Verifiers/CSharpAnalyzerVerifier`1+Test.cs +++ b/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Verifiers/CSharpAnalyzerVerifier`1+Test.cs @@ -3,14 +3,14 @@ using Microsoft.CodeAnalysis.CSharp.Testing; using Microsoft.CodeAnalysis.Diagnostics; -using Microsoft.CodeAnalysis.Testing.Verifiers; +using Microsoft.CodeAnalysis.Testing; namespace Razor.Diagnostics.Analyzers.Test; public static partial class CSharpAnalyzerVerifier where TAnalyzer : DiagnosticAnalyzer, new() { - public class Test : CSharpAnalyzerTest + public class Test : CSharpAnalyzerTest { public Test() { diff --git a/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Verifiers/CSharpAnalyzerVerifier`1.cs b/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Verifiers/CSharpAnalyzerVerifier`1.cs index a24421ec5b5..56194eec436 100644 --- a/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Verifiers/CSharpAnalyzerVerifier`1.cs +++ b/src/Analyzers/Razor.Diagnostics.Analyzers.Test/Verifiers/CSharpAnalyzerVerifier`1.cs @@ -7,7 +7,6 @@ using Microsoft.CodeAnalysis.CSharp.Testing; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Testing; -using Microsoft.CodeAnalysis.Testing.Verifiers; namespace Razor.Diagnostics.Analyzers.Test; @@ -16,15 +15,15 @@ public static partial class CSharpAnalyzerVerifier { /// public static DiagnosticResult Diagnostic() - => CSharpAnalyzerVerifier.Diagnostic(); + => CSharpAnalyzerVerifier.Diagnostic(); /// public static DiagnosticResult Diagnostic(string diagnosticId) - => CSharpAnalyzerVerifier.Diagnostic(diagnosticId); + => CSharpAnalyzerVerifier.Diagnostic(diagnosticId); /// public static DiagnosticResult Diagnostic(DiagnosticDescriptor descriptor) - => CSharpAnalyzerVerifier.Diagnostic(descriptor); + => CSharpAnalyzerVerifier.Diagnostic(descriptor); /// public static async Task VerifyAnalyzerAsync(string source, params DiagnosticResult[] expected) From 827a89bac1f1008245686613eedb1e53611cabd7 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Tue, 11 Mar 2025 15:05:23 -0700 Subject: [PATCH 04/12] Add suppressor tests --- ...ParameterNullableWarningSuppressorTests.cs | 178 ++++++++++++++++++ ...NET.Sdk.Razor.SourceGenerators.Test.csproj | 1 + 2 files changed, 179 insertions(+) create mode 100644 src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs diff --git a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs new file mode 100644 index 00000000000..a8a07b18221 --- /dev/null +++ b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs @@ -0,0 +1,178 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Threading.Tasks; +using Microsoft.AspNetCore.Components; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Razor.Compiler.Analyzers; +using Microsoft.CodeAnalysis.Testing; +using Xunit; + +namespace Microsoft.CodeAnalysis.Razor.Analyzers.Tests +{ + public class ComponentParameterNullableWarningSuppressorTests + { + [Fact] + public async Task ParameterEditorRequiredNoWarning() + { + var testCode = """ + #nullable enable + using System; + using Microsoft.AspNetCore.Components; + + public class MyComponent : ComponentBase + { + [Parameter, EditorRequired] + public string MyParameter { get; set; } + } + """; + + await VerifyAnalyzerAsync(testCode, + // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. + DiagnosticResult.CompilerWarning("CS8618").WithSpan(8, 19, 8, 30).WithSpan(8, 19, 8, 30).WithArguments("property", "MyParameter").WithIsSuppressed(true) + ); + } + + [Fact] + public async Task NoEditorRequiredStillReports() + { + var testCode = """ + #nullable enable + using System; + using Microsoft.AspNetCore.Components; + + public class MyComponent : ComponentBase + { + [Parameter] + public string MyParameter { get; set; } + } + """; + + await VerifyAnalyzerAsync(testCode, + // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. + DiagnosticResult.CompilerWarning("CS8618").WithSpan(8, 19, 8, 30).WithSpan(8, 19, 8, 30).WithArguments("property", "MyParameter") + ); + } + + [Fact] + public async Task NoParameterRequiredStillReports() + { + var testCode = """ + #nullable enable + using System; + using Microsoft.AspNetCore.Components; + + public class MyComponent : ComponentBase + { + [EditorRequired] + public string MyParameter { get; set; } + } + """; + + await VerifyAnalyzerAsync(testCode, + // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. + DiagnosticResult.CompilerWarning("CS8618").WithSpan(8, 19, 8, 30).WithSpan(8, 19, 8, 30).WithArguments("property", "MyParameter") + ); + } + + [Fact] + public async Task AliasedAttributes() + { + var testCode = """ + #nullable enable + using System; + using Microsoft.AspNetCore.Components; + using MyParameter = Microsoft.AspNetCore.Components.ParameterAttribute; + using MyRequired = Microsoft.AspNetCore.Components.EditorRequiredAttribute; + + public class MyComponent : ComponentBase + { + [MyParameter, MyRequired] + public string MyParameter { get; set; } + } + """; + + await VerifyAnalyzerAsync(testCode, + // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. + DiagnosticResult.CompilerWarning("CS8618").WithSpan(10, 19, 10, 30).WithSpan(10, 19, 10, 30).WithArguments("property", "MyParameter").WithIsSuppressed(true) + ); + } + + [Fact] + public async Task LocallyDefinedAttributes() + { + var testCode = """ + + #nullable enable + using System; + using Microsoft.AspNetCore.Components; + + public class MyComponent + { + [Parameter, EditorRequired] + public string MyParameter { get; set; } + } + + namespace Microsoft.AspNetCore.Components + { + public class ParameterAttribute : Attribute { } + public class EditorRequiredAttribute : Attribute { } + } + + """; + + await VerifyAnalyzerAsync(testCode, ReferenceAssemblies.Net.Net80, + // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. + DiagnosticResult.CompilerWarning("CS8618").WithSpan(9, 19, 9, 30).WithSpan(9, 19, 9, 30).WithArguments("property", "MyParameter").WithIsSuppressed(true) + ); + } + + [Fact] + public async Task LocallyDefinedAttributesDifferentNamespace() + { + var testCode = """ + + #nullable enable + using System; + using MyNamespace; + + public class MyComponent + { + [Parameter, EditorRequired] + public string MyParameter { get; set; } + } + + namespace MyNamespace + { + public class ParameterAttribute : Attribute { } + public class EditorRequiredAttribute : Attribute { } + } + + """; + + await VerifyAnalyzerAsync(testCode, ReferenceAssemblies.Net.Net80, + // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. + DiagnosticResult.CompilerWarning("CS8618").WithSpan(9, 19, 9, 30).WithSpan(9, 19, 9, 30).WithArguments("property", "MyParameter") + ); + } + + private static Task VerifyAnalyzerAsync(string source, params DiagnosticResult[] expected) + => VerifyAnalyzerAsync(source, + ReferenceAssemblies.Net.Net80.WithAssemblies([typeof(EditorRequiredAttribute).Assembly.Location[0..^4]]), + expected); + + private static async Task VerifyAnalyzerAsync(string source, ReferenceAssemblies referenceAssemblies, params DiagnosticResult[] expected) + { + var test = new CSharpAnalyzerTest + { + TestCode = source, + ReferenceAssemblies = referenceAssemblies, + CompilerDiagnostics = CompilerDiagnostics.Warnings, + }; + + test.DisabledDiagnostics.Add("CS1591"); + test.ExpectedDiagnostics.AddRange(expected); + await test.RunAsync(); + } + } +} diff --git a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Test.csproj b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Test.csproj index 581f9841034..c0829710ad6 100644 --- a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Test.csproj +++ b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Test.csproj @@ -31,6 +31,7 @@ + From bfa08d7ec78ca789870fdf59764e7adfba01b480 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Tue, 11 Mar 2025 15:07:54 -0700 Subject: [PATCH 05/12] Extract id into a shared class --- .../src/Analyzers/AnalyzerIDs.cs | 9 +++++++++ .../ComponentParameterNullableWarningSuppressor.cs | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerIDs.cs diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerIDs.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerIDs.cs new file mode 100644 index 00000000000..1c4fdb47bda --- /dev/null +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerIDs.cs @@ -0,0 +1,9 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.CodeAnalysis.Razor.Compiler.Analyzers; + +internal static class AnalyzerIDs +{ + internal const string ComponentParameterNullableWarningSuppressionId = "RZS1001"; +} diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs index f6896d0e8f6..2de2d123081 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs @@ -17,7 +17,7 @@ public sealed class ComponentParameterNullableWarningSuppressor : DiagnosticSupp private static readonly LocalizableString Description = new LocalizableResourceString(nameof(AnalyzerResources.ComponentParameterNullableWarningSuppressorDescription), AnalyzerResources.ResourceManager, typeof(AnalyzerResources)); public override ImmutableArray SupportedSuppressions => [ - new SuppressionDescriptor("RZS1001", "CS8618", Description) + new SuppressionDescriptor(AnalyzerIDs.ComponentParameterNullableWarningSuppressionId, "CS8618", Description) ]; public override void ReportSuppressions(SuppressionAnalysisContext context) From f7dded101ee2c59168ccd468d03cc8495666b3fd Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Tue, 11 Mar 2025 15:21:43 -0700 Subject: [PATCH 06/12] Don't iterate the attributes twice --- ...onentParameterNullableWarningSuppressor.cs | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs index 2de2d123081..a46000af1df 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs @@ -33,15 +33,41 @@ public override void ReportSuppressions(SuppressionAnalysisContext context) var symbol = context.GetSemanticModel(propertySyntax.SyntaxTree).GetDeclaredSymbol(propertySyntax, context.CancellationToken); if (symbol is IPropertySymbol property) { - var attributes = property.GetAttributes(); - if (attributes.Any(ad => SymbolEqualityComparer.Default.Equals(ad.AttributeClass, parameterSymbol)) - && attributes.Any(ad => SymbolEqualityComparer.Default.Equals(ad.AttributeClass, editorRequiredSymbol))) + if (IsEditorRequiredParam(property.GetAttributes())) { context.ReportSuppression(Suppression.Create(SupportedSuppressions[0], diagnostic)); } } } } + + bool IsEditorRequiredParam(ImmutableArray attributes) + { + bool hasParameter = false, hasRequired = false; + foreach (var attribute in attributes) + { + if (!hasParameter && SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, parameterSymbol)) + { + hasParameter = true; + if (hasRequired) + { + break; + } + continue; + } + + if (!hasRequired && SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, editorRequiredSymbol)) + { + hasRequired = true; + if (hasParameter) + { + break; + } + continue; + } + } + return hasParameter && hasRequired; + } } } From fbbafbb4cba096e28bff5469663999590cf6c50f Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Tue, 11 Mar 2025 15:28:39 -0700 Subject: [PATCH 07/12] Add comment about what we're suppressing --- .../src/Analyzers/ComponentParameterNullableWarningSuppressor.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs index a46000af1df..f3f242cac9d 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs @@ -16,6 +16,7 @@ public sealed class ComponentParameterNullableWarningSuppressor : DiagnosticSupp { private static readonly LocalizableString Description = new LocalizableResourceString(nameof(AnalyzerResources.ComponentParameterNullableWarningSuppressorDescription), AnalyzerResources.ResourceManager, typeof(AnalyzerResources)); + //Suppress CS8618: "Non-nullable {0} '{1}' must contain a non-null value when exiting constructor. Consider declaring the {0} as nullable." public override ImmutableArray SupportedSuppressions => [ new SuppressionDescriptor(AnalyzerIDs.ComponentParameterNullableWarningSuppressionId, "CS8618", Description) ]; From 0ec81ad66ebc7dee9078074bcee684c99a865c77 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Wed, 12 Mar 2025 09:58:17 -0700 Subject: [PATCH 08/12] Remove NoWarn --- Directory.Packages.props | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 6c578e1ff7b..45c868a31fb 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -32,9 +32,9 @@ - + - + From d2c4580e0c2ab2aebc2ead94feaf589682624679 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Wed, 12 Mar 2025 10:48:51 -0700 Subject: [PATCH 09/12] Use basic.ref.assemblies for aspnetcore assemblies --- ...nentParameterNullableWarningSuppressorTests.cs | 15 ++++++++------- ...oft.NET.Sdk.Razor.SourceGenerators.Test.csproj | 3 ++- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs index a8a07b18221..b2eba18aa57 100644 --- a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs +++ b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs @@ -1,8 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Immutable; using System.Threading.Tasks; -using Microsoft.AspNetCore.Components; using Microsoft.CodeAnalysis.CSharp.Testing; using Microsoft.CodeAnalysis.Razor.Compiler.Analyzers; using Microsoft.CodeAnalysis.Testing; @@ -121,7 +121,7 @@ public class EditorRequiredAttribute : Attribute { } """; - await VerifyAnalyzerAsync(testCode, ReferenceAssemblies.Net.Net80, + await VerifyAnalyzerAsync(testCode, extraReferences: [], // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. DiagnosticResult.CompilerWarning("CS8618").WithSpan(9, 19, 9, 30).WithSpan(9, 19, 9, 30).WithArguments("property", "MyParameter").WithIsSuppressed(true) ); @@ -150,7 +150,7 @@ public class EditorRequiredAttribute : Attribute { } """; - await VerifyAnalyzerAsync(testCode, ReferenceAssemblies.Net.Net80, + await VerifyAnalyzerAsync(testCode, extraReferences: [], // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. DiagnosticResult.CompilerWarning("CS8618").WithSpan(9, 19, 9, 30).WithSpan(9, 19, 9, 30).WithArguments("property", "MyParameter") ); @@ -158,19 +158,20 @@ await VerifyAnalyzerAsync(testCode, ReferenceAssemblies.Net.Net80, private static Task VerifyAnalyzerAsync(string source, params DiagnosticResult[] expected) => VerifyAnalyzerAsync(source, - ReferenceAssemblies.Net.Net80.WithAssemblies([typeof(EditorRequiredAttribute).Assembly.Location[0..^4]]), + Basic.Reference.Assemblies.AspNet80.References.All, expected); - private static async Task VerifyAnalyzerAsync(string source, ReferenceAssemblies referenceAssemblies, params DiagnosticResult[] expected) + private static async Task VerifyAnalyzerAsync(string source, ImmutableArray extraReferences, params DiagnosticResult[] expected) { var test = new CSharpAnalyzerTest { TestCode = source, - ReferenceAssemblies = referenceAssemblies, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, CompilerDiagnostics = CompilerDiagnostics.Warnings, + DisabledDiagnostics = { "CS1591" } }; - test.DisabledDiagnostics.Add("CS1591"); + test.TestState.AdditionalReferences.AddRange(extraReferences); test.ExpectedDiagnostics.AddRange(expected); await test.RunAsync(); } diff --git a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Test.csproj b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Test.csproj index c0829710ad6..8c47d77c2d9 100644 --- a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Test.csproj +++ b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Test.csproj @@ -23,15 +23,16 @@ + + - From c3869e396408d87bc80b1e991dc47d9294096848 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Wed, 12 Mar 2025 13:43:36 -0700 Subject: [PATCH 10/12] Return early if we cant find symbols Ensure the parameter is well defined --- ...onentParameterNullableWarningSuppressor.cs | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs index f3f242cac9d..a9eefc78490 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs @@ -25,6 +25,10 @@ public override void ReportSuppressions(SuppressionAnalysisContext context) { var editorRequiredSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Components.EditorRequiredAttribute"); var parameterSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Components.ParameterAttribute"); + if (parameterSymbol is null || editorRequiredSymbol is null) + { + return; + } foreach (var diagnostic in context.ReportedDiagnostics) { @@ -32,20 +36,24 @@ public override void ReportSuppressions(SuppressionAnalysisContext context) if (node is PropertyDeclarationSyntax propertySyntax && propertySyntax.AttributeLists.Any()) { var symbol = context.GetSemanticModel(propertySyntax.SyntaxTree).GetDeclaredSymbol(propertySyntax, context.CancellationToken); - if (symbol is IPropertySymbol property) + if (IsValidEditorRequiredParameter(symbol)) { - if (IsEditorRequiredParam(property.GetAttributes())) - { - context.ReportSuppression(Suppression.Create(SupportedSuppressions[0], diagnostic)); - } + context.ReportSuppression(Suppression.Create(SupportedSuppressions[0], diagnostic)); } } } - bool IsEditorRequiredParam(ImmutableArray attributes) + bool IsValidEditorRequiredParameter(ISymbol? symbol) { + // public instance property, with a public setter + if (symbol is not IPropertySymbol { DeclaredAccessibility: Accessibility.Public, IsStatic: false, SetMethod: not null and { DeclaredAccessibility: Accessibility.Public } }) + { + return false; + } + + // has both [Parameter] and [EditorRequired] attributes bool hasParameter = false, hasRequired = false; - foreach (var attribute in attributes) + foreach (var attribute in symbol.GetAttributes()) { if (!hasParameter && SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, parameterSymbol)) { From 864cbbdf42512f68a2cb9fe98d0db6a085f42609 Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Wed, 12 Mar 2025 13:59:19 -0700 Subject: [PATCH 11/12] Add more tests --- ...onentParameterNullableWarningSuppressor.cs | 2 - ...ParameterNullableWarningSuppressorTests.cs | 109 +++++++++++++++++- 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs index a9eefc78490..f9c28d43fb9 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs @@ -1,9 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Collections.Generic; using System.Collections.Immutable; -using System.Linq; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; diff --git a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs index b2eba18aa57..f4e8c2e3483 100644 --- a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs +++ b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs @@ -102,7 +102,6 @@ await VerifyAnalyzerAsync(testCode, public async Task LocallyDefinedAttributes() { var testCode = """ - #nullable enable using System; using Microsoft.AspNetCore.Components; @@ -123,7 +122,7 @@ public class EditorRequiredAttribute : Attribute { } await VerifyAnalyzerAsync(testCode, extraReferences: [], // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. - DiagnosticResult.CompilerWarning("CS8618").WithSpan(9, 19, 9, 30).WithSpan(9, 19, 9, 30).WithArguments("property", "MyParameter").WithIsSuppressed(true) + DiagnosticResult.CompilerWarning("CS8618").WithSpan(8, 19, 8, 30).WithSpan(8, 19, 8, 30).WithArguments("property", "MyParameter").WithIsSuppressed(true) ); } @@ -131,7 +130,6 @@ await VerifyAnalyzerAsync(testCode, extraReferences: [], public async Task LocallyDefinedAttributesDifferentNamespace() { var testCode = """ - #nullable enable using System; using MyNamespace; @@ -152,10 +150,111 @@ public class EditorRequiredAttribute : Attribute { } await VerifyAnalyzerAsync(testCode, extraReferences: [], // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. - DiagnosticResult.CompilerWarning("CS8618").WithSpan(9, 19, 9, 30).WithSpan(9, 19, 9, 30).WithArguments("property", "MyParameter") + DiagnosticResult.CompilerWarning("CS8618").WithSpan(8, 19, 8, 30).WithSpan(8, 19, 8, 30).WithArguments("property", "MyParameter") + ); + } + + [Fact] + public async Task LocallyDefinedAttributesAndSdkAttributes() + { + var testCode = """ + #nullable enable + using System; + using Microsoft.AspNetCore.Components; + + public class MyComponent + { + [Parameter, EditorRequired] + public string MyParameter { get; set; } + } + + namespace Microsoft.AspNetCore.Components + { + public class ParameterAttribute : Attribute { } + public class EditorRequiredAttribute : Attribute { } + } + + """; + + await VerifyAnalyzerAsync(testCode, + // /0/Test0.cs(8,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. + DiagnosticResult.CompilerWarning("CS8618").WithSpan(8, 19, 8, 30).WithSpan(8, 19, 8, 30).WithArguments("property", "MyParameter").WithIsSuppressed(true), + // /0/Test0.cs(7,6): warning CS0436: The type 'ParameterAttribute' in '/0/Test0.cs' conflicts with the imported type 'ParameterAttribute' in 'Microsoft.AspNetCore.Components, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'. Using the type defined in '/0/Test0.cs'. + DiagnosticResult.CompilerWarning("CS0436").WithSpan(7, 6, 7, 15).WithArguments("/0/Test0.cs", "Microsoft.AspNetCore.Components.ParameterAttribute", "Microsoft.AspNetCore.Components, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60", "Microsoft.AspNetCore.Components.ParameterAttribute"), + // /0/Test0.cs(7,17): warning CS0436: The type 'EditorRequiredAttribute' in '/0/Test0.cs' conflicts with the imported type 'EditorRequiredAttribute' in 'Microsoft.AspNetCore.Components, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'. Using the type defined in '/0/Test0.cs'. + DiagnosticResult.CompilerWarning("CS0436").WithSpan(7, 17, 7, 31).WithArguments("/0/Test0.cs", "Microsoft.AspNetCore.Components.EditorRequiredAttribute", "Microsoft.AspNetCore.Components, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60", "Microsoft.AspNetCore.Components.EditorRequiredAttribute") + ); + } + + [Theory] + [InlineData("internal")] + [InlineData("private")] + [InlineData("protected internal")] + [InlineData("protected")] + [InlineData("public static")] + public async Task IncorrectModifiersStillReport(string modifiers) + { + var testCode = $$""" + #nullable enable + using System; + using Microsoft.AspNetCore.Components; + + public class MyComponent : ComponentBase + { + [Parameter] + {{modifiers}} + string MyParameter { get; set; } + } + """; + + await VerifyAnalyzerAsync(testCode, + // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. + DiagnosticResult.CompilerWarning("CS8618").WithSpan(9, 12, 9, 23).WithSpan(9, 12, 9, 23).WithArguments("property", "MyParameter") + ); + } + + [Theory] + [InlineData("init;")] + [InlineData("private set;")] + [InlineData("private init;")] + public async Task IncorrectSetterStillReport(string setter) + { + var testCode = $$""" + #nullable enable + using System; + using Microsoft.AspNetCore.Components; + + public class MyComponent : ComponentBase + { + [Parameter] + public string MyParameter { get; {{setter}} } + } + """; + + await VerifyAnalyzerAsync(testCode, + // /0/Test0.cs(9,19): warning CS8618: Non-nullable property 'MyParameter' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. + DiagnosticResult.CompilerWarning("CS8618").WithSpan(8, 19, 8, 30).WithSpan(8, 19, 8, 30).WithArguments("property", "MyParameter") ); } + [Fact] + public async Task RequiredPropertyDoesNotReport() + { + var testCode = $$""" + #nullable enable + using System; + using Microsoft.AspNetCore.Components; + + public class MyComponent : ComponentBase + { + [Parameter] + public required string MyParameter { get; set; } + } + """; + + await VerifyAnalyzerAsync(testCode); + } + private static Task VerifyAnalyzerAsync(string source, params DiagnosticResult[] expected) => VerifyAnalyzerAsync(source, Basic.Reference.Assemblies.AspNet80.References.All, @@ -168,7 +267,7 @@ private static async Task VerifyAnalyzerAsync(string source, ImmutableArray Date: Thu, 13 Mar 2025 13:39:42 -0700 Subject: [PATCH 12/12] Apply suggestions from code review Co-authored-by: Jan Jones --- .../ComponentParameterNullableWarningSuppressorTests.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs index f4e8c2e3483..1d373e52485 100644 --- a/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs +++ b/src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs @@ -201,7 +201,7 @@ public async Task IncorrectModifiersStillReport(string modifiers) public class MyComponent : ComponentBase { - [Parameter] + [Parameter, EditorRequired] {{modifiers}} string MyParameter { get; set; } } @@ -214,6 +214,7 @@ await VerifyAnalyzerAsync(testCode, } [Theory] + [InlineData("")] [InlineData("init;")] [InlineData("private set;")] [InlineData("private init;")] @@ -226,7 +227,7 @@ public async Task IncorrectSetterStillReport(string setter) public class MyComponent : ComponentBase { - [Parameter] + [Parameter, EditorRequired] public string MyParameter { get; {{setter}} } } """; @@ -247,7 +248,7 @@ public async Task RequiredPropertyDoesNotReport() public class MyComponent : ComponentBase { - [Parameter] + [Parameter, EditorRequired] public required string MyParameter { get; set; } } """; @@ -267,7 +268,7 @@ private static async Task VerifyAnalyzerAsync(string source, ImmutableArray