Add a suppressor for CS8618 by chsienki · Pull Request #11601 · dotnet/razor

chsienki · 2025-03-10T21:18:23Z

Adds a suppressor for CS8618 when a component property has EditorRequired attached to it.

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs

src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyzers/AnalyzerResources.resx

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs

- Use symbols rather than syntax - Only report if both parameter and editor required are on the property - Thread through the cancellation token

chsienki · 2025-03-11T17:48:55Z

Ok, updated to use symbols. Will work on adding some tests if we're happy with this approach.

333fred

Think this is in a good enough place to go for the tests.

333fred · 2025-03-11T18:50:51Z

...piler/Microsoft.CodeAnalysis.Razor.Compiler/src/Microsoft.CodeAnalysis.Razor.Compiler.csproj

@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


Looks like it was missing the BOM and this adds it back.

333fred · 2025-03-11T18:51:44Z

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs

+                    if (attributes.Any(ad => SymbolEqualityComparer.Default.Equals(ad.AttributeClass, parameterSymbol))
+                        && attributes.Any(ad => SymbolEqualityComparer.Default.Equals(ad.AttributeClass, editorRequiredSymbol)))


Nit: cute syntax, but potentially loops through the list twice. Maybe a foreach would be better 🙂.

Updated. But also lol:

- 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

chsienki · 2025-03-11T22:30:03Z

...oft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs

+                using System;
+                using Microsoft.AspNetCore.Components;
+
+                public class MyComponent


This test makes me wonder. Should we be checking that these attributes are being used in something that implements IComponent?

I don't think that's required, consider this example:

abstract class B { [Parameter, EditorRequired] string Param { get; set; } } class C : B, IComponent;

B doesn't implement IComponent but can still be instantiated as a component through C.

IMO, for this type of edge case, I'd prefer to have false positives, rather than false negatives (iow, I think we should check for implementing IComponent). If we get feedback that this case is still being hit and causing pain, then we can take another look at it.

Yeah, lets keep it tightly scoped for now, and we can expand it later if we get feedback on it

Directory.Packages.props

...oft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs

jjonescz · 2025-03-12T09:36:44Z

...oft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs

+                using System;
+                using Microsoft.AspNetCore.Components;
+
+                public class MyComponent


I don't think that's required, consider this example:

abstract class B { [Parameter, EditorRequired] string Param { get; set; } } class C : B, IComponent;

B doesn't implement IComponent but can still be instantiated as a component through C.

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs

jaredpar · 2025-03-12T17:25:48Z

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs

+
+    public override void ReportSuppressions(SuppressionAnalysisContext context)
+    {
+        var editorRequiredSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Components.EditorRequiredAttribute");


Couple Qs:

Should we bail out of the analyzer here if the value is null?

This will fail in the face of multiple definitions of this type. Not against that but want to make sure we clearly decided this.

Added an early return if we can't find the types, and a test to show behavior in the presence of duplicate attributes.

jaredpar · 2025-03-12T17:26:03Z

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs

+    public override void ReportSuppressions(SuppressionAnalysisContext context)
+    {
+        var editorRequiredSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Components.EditorRequiredAttribute");
+        var parameterSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Components.ParameterAttribute");


Should we bail out if this is null?

Ensure the parameter is well defined

...oft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs

jjonescz · 2025-03-13T08:51:06Z

...oft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs

+                public class MyComponent : ComponentBase
+                {
+                    [Parameter, EditorRequired]
+                    public string MyParameter { get; set; }


Consider also testing string?.

jjonescz · 2025-03-13T08:53:13Z

...oft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs

+        }
+
+        [Theory]
+        [InlineData("init;")]


It looks like the runtime supports init parameters just fine, so perhaps we should suppress the warning for them as well?

So it turns out this already supported init properties, this test just wasn't explicitly checking IsSuppressed(false) so it was incorrectly passing. I've added an explicit suppress check for all the instances where it should be false and split init out into its own passing test.

...oft.NET.Sdk.Razor.SourceGenerators.Tests/ComponentParameterNullableWarningSuppressorTests.cs

Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

Add extra tests

Add init test

jjonescz · 2025-03-26T08:48:28Z

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs

            }

+            // containing type implements IComponent
+            if (!symbol.ContainingType.AllInterfaces.Any(@interface => @interface.Equals(componentSymbol, SymbolEqualityComparer.Default)))


nit: not sure if we have an overload like this available

Suggested change

if (!symbol.ContainingType.AllInterfaces.Any(@interface => @interface.Equals(componentSymbol, SymbolEqualityComparer.Default)))

if (!symbol.ContainingType.AllInterfaces.Any(static (@interface, componentSymbol) => @interface.Equals(componentSymbol, SymbolEqualityComparer.Default), componentSymbol))

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs

…rs/ComponentParameterNullableWarningSuppressor.cs Co-authored-by: Fred Silberberg <fred@silberberg.xyz>

This reverts commit c598cfa.

omccully · 2025-08-28T21:04:45Z

@chsienki If the component has a constructor, CS8618 puts the warning on the constructor rather than on the property. It seems this suppressor does not work as it should when components have a constructor defined.

Also, I would still love to see this suppressor for [Inject] as well. Is that still planned?

Add a suppressor for CS8618

0bcc8e6

chsienki added the area-compiler Umbrella for all compiler issues label Mar 10, 2025

chsienki requested a review from a team as a code owner March 10, 2025 21:18

333fred reviewed Mar 10, 2025

View reviewed changes

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs Outdated Show resolved Hide resolved

jjonescz reviewed Mar 11, 2025

View reviewed changes

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs Outdated Show resolved Hide resolved

PR Feedback:

31485a7

- Use symbols rather than syntax - Only report if both parameter and editor required are on the property - Thread through the cancellation token

333fred reviewed Mar 11, 2025

View reviewed changes

chsienki added 5 commits March 11, 2025 15:22

Update to latest ms.cs.analyzer.testing:

8b1f80a

- 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

Add suppressor tests

827a89b

Extract id into a shared class

bfa08d7

Don't iterate the attributes twice

f7dded1

Add comment about what we're suppressing

fbbafbb

chsienki commented Mar 11, 2025

View reviewed changes

jjonescz reviewed Mar 12, 2025

View reviewed changes

Remove NoWarn

0ec81ad

jaredpar reviewed Mar 12, 2025

View reviewed changes

chsienki added 3 commits March 12, 2025 10:48

Use basic.ref.assemblies for aspnetcore assemblies

d2c4580

Return early if we cant find symbols

c3869e3

Ensure the parameter is well defined

Add more tests

864cbbd

jjonescz reviewed Mar 13, 2025

View reviewed changes

chsienki and others added 3 commits March 13, 2025 13:39

Apply suggestions from code review

c2d51aa

Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

Ensure the property is on a class that implements IComponent

c08f5ab

Add extra tests

Ensure we always test IsSuppressed(false) too

79f1898

Add init test

jjonescz approved these changes Mar 26, 2025

View reviewed changes

333fred approved these changes Mar 26, 2025

View reviewed changes

...oft.CodeAnalysis.Razor.Compiler/src/Analyzers/ComponentParameterNullableWarningSuppressor.cs Outdated Show resolved Hide resolved

chsienki and others added 2 commits March 26, 2025 12:13

Update src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Analyze…

4d1368a

…rs/ComponentParameterNullableWarningSuppressor.cs Co-authored-by: Fred Silberberg <fred@silberberg.xyz>

Use argument version of any

36bcaf5

chsienki enabled auto-merge (squash) March 26, 2025 19:19

chsienki merged commit c598cfa into dotnet:main Mar 26, 2025
12 checks passed

dotnet-policy-service bot added this to the Next milestone Mar 26, 2025

jjonescz added a commit that referenced this pull request Mar 27, 2025

Revert "Add a suppressor for CS8618 (#11601)"

de91ec0

This reverts commit c598cfa.

jjonescz mentioned this pull request Mar 27, 2025

Revert "Add a suppressor for CS8618" #11666

Closed

dotnet-bot mentioned this pull request Mar 29, 2025

[Automated] PRs inserted in VS build main-35928.14 #11674

Closed

jjonescz modified the milestones: Next, 17.14 P3 Apr 1, 2025

This was referenced Apr 2, 2025

[Automated] PRs inserted in VS build feature.debugger.main-36001.245 #11693

Closed

[Automated] PRs inserted in VS build feature.d18initial-10502.05 #11698

Closed

chsienki mentioned this pull request Apr 11, 2025

Fix razor analyzer loading dotnet/roslyn#78116

Merged

This was referenced Apr 11, 2025

Update to 2.72.27 #11732

Merged

Cherry Pick 11727 into release/vscode #11733

Merged

dotnet-bot mentioned this pull request Apr 17, 2025

[Automated] PRs inserted in VS build feature.telemetry.d18OpenTelemetry-10516.102 #11747

Closed

This was referenced May 29, 2025

Blazor Analyzer/code fix: Add EditorRequiredAttribute to Blazor parameter when using null forgiving operator #11895

Open

Blazor Analyzer/code fix: Inject properties should be declared non-nullable and with null forgiving operator #11894

Open

		@@ -1,4 +1,4 @@
		<Project Sdk="Microsoft.NET.Sdk">
		<Project Sdk="Microsoft.NET.Sdk">

		if (attributes.Any(ad => SymbolEqualityComparer.Default.Equals(ad.AttributeClass, parameterSymbol))
		&& attributes.Any(ad => SymbolEqualityComparer.Default.Equals(ad.AttributeClass, editorRequiredSymbol)))

	if (!symbol.ContainingType.AllInterfaces.Any(@interface => @interface.Equals(componentSymbol, SymbolEqualityComparer.Default)))
	if (!symbol.ContainingType.AllInterfaces.Any(static (@interface, componentSymbol) => @interface.Equals(componentSymbol, SymbolEqualityComparer.Default), componentSymbol))

Comments

Conversation

chsienki commented Mar 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chsienki commented Mar 11, 2025

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

omccully commented Aug 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants