-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add analyzer to warn when SupplyParameterFromForm properties have non-default initializers #63110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Be sure to read through #51420 (comment) which contains the plan here. |
Co-authored-by: javiercn <[email protected]>
Co-authored-by: javiercn <[email protected]>
@@ -180,4 +180,13 @@ | |||
<data name="ComponentParametersShouldBeAutoProperties_Title" xml:space="preserve"> | |||
<value>Component parameters should be auto properties</value> | |||
</data> | |||
<data name="SupplyParameterFromFormShouldNotHavePropertyInitializer_Description" xml:space="preserve"> | |||
<value>Initializing a property decorated with [SupplyParameterFromForm] can be overwritten with null during a form post. To ensure the property is never null, move the initialization to a component lifecycle method.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tweak the description.
The value of a property decorated with [SupplyParameterFromForm] and initialized with a property initializer can be overwritten with null when the component receives parameters. To ensure the initialized value is not overwritten, move the initialization to a component lifecycle method like OnInitialized or OnInitializedAsync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot check this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the description in Resources.resx as requested (commit 93a68fa). The new description reads: "The value of a property decorated with [SupplyParameterFromForm] and initialized with a property initializer can be overwritten with null when the component receives parameters."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the description to include the complete suggested text including the lifecycle method recommendation (commit 0305c3f). The description now reads: "The value of a property decorated with [SupplyParameterFromForm] and initialized with a property initializer can be overwritten with null when the component receives parameters. To ensure the initialized value is not overwritten, move the initialization to a component lifecycle method like OnInitialized or OnInitializedAsync"
Co-authored-by: javiercn <[email protected]>
Thanks for the reference! I've reviewed the detailed plan in comment 3151926128. The current implementation aligns with the plan - it's an analyzer (BL0008) that detects properties with both |
public void ReportsWarningWithFormNameSpecified() | ||
{ | ||
var test = $@" | ||
namespace ConsoleApplication1 | ||
{{ | ||
using {typeof(ParameterAttribute).Namespace}; | ||
class TestComponent : ComponentBase | ||
{{ | ||
[SupplyParameterFromForm(FormName = ""main-form"")] public string MyProperty {{ get; set; }} = ""default""; | ||
}} | ||
}}" + TestDeclarations; | ||
|
||
var expected = new DiagnosticResult | ||
{ | ||
Id = "BL0008", | ||
Message = "Property 'ConsoleApplication1.TestComponent.MyProperty' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts.", | ||
Severity = DiagnosticSeverity.Warning, | ||
Locations = new[] | ||
{ | ||
new DiagnosticResultLocation("Test0.cs", 7, 77) | ||
} | ||
}; | ||
|
||
VerifyCSharpDiagnostic(test, expected); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this test relevant to the feature we are implementing? We don't look at the name at all, do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct - that test was not relevant since the analyzer doesn't examine the FormName parameter at all. It only checks if the [SupplyParameterFromForm] attribute is present, regardless of what parameters are passed to it. I removed the redundant test in commit 3c8fd5a.
…ing lifecycle method recommendation Co-authored-by: javiercn <[email protected]>
Co-authored-by: javiercn <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new Roslyn analyzer (BL0008) that prevents developers from accidentally using property initializers with [SupplyParameterFromForm]
attributes, which can lead to null reference exceptions when form submissions overwrite the initialized values.
Key changes:
- Adds a new analyzer that detects the problematic pattern and warns developers
- Provides comprehensive test coverage for various scenarios
- Includes proper resource strings for diagnostic messages
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
SupplyParameterFromFormAnalyzer.cs | Main analyzer implementation that detects properties with both [SupplyParameterFromForm] and initializers |
SupplyParameterFromFormAnalyzerTest.cs | Comprehensive test suite covering valid and invalid scenarios |
DiagnosticDescriptors.cs | Adds the BL0008 diagnostic descriptor with proper severity and messaging |
Resources.resx | Localizable strings for the diagnostic title, format, and description |
ComponentsApi.cs | API constants for SupplyParameterFromForm and ComponentBase types |
ComponentSymbols.cs | Symbol resolution for the new analyzer types |
ComponentFacts.cs | Helper methods to identify SupplyParameterFromForm attributes and ComponentBase inheritance |
@copilot there are some template test failures that are caused by issues in the current templates that don't follow the analyzer guidance. Look at the errors and update the template code following the analyzer guidance to avoid them Project new blazor --no-restore -int None -au Individual failed to publish. Exit code 1.\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\dotnet\dotnet.exe publish -c Release /bl \nStdErr: \nStdOut: Determining projects to restore...\r\n All projects are up-to-date for restore.\r\nD:\a\_work\1\s\.dotnet\sdk\10.0.100-preview.7.25322.101\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(335,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\ForgotPassword.razor(38,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.ForgotPassword.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\ExternalLogin.razor(57,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.ExternalLogin.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\Login.razor(82,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.Login.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\ResetPassword.razor(49,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.ResetPassword.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\LoginWith2fa.razor(52,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.LoginWith2fa.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\LoginWithRecoveryCode.razor(41,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.LoginWithRecoveryCode.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\Manage\ChangePassword.razor(51,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.Manage.ChangePassword.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\Manage\Email.razor(66,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.Manage.Email.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\Manage\Index.razor(44,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.Manage.Index.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\Manage\EnableAuthenticator.razor(81,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.Manage.EnableAuthenticator.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\Manage\DeletePersonalData.razor(49,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.Manage.DeletePersonalData.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\Manage\Passkeys.razor(68,31): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.Manage.Passkeys.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\Manage\SetPassword.razor(47,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.Manage.SetPassword.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\Manage\RenamePasskey.razor(44,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.Manage.RenamePasskey.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\Register.razor(61,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.Register.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\nD:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\Components\Account\Pages\ResendEmailConfirmation.razor(40,24): error BL0008: Property 'AspNet.og4bddmkhnnv.Components.Account.Pages.ResendEmailConfirmation.Input' has [SupplyParameterFromForm] and a property initializer. This can be overwritten with null during form posts. [D:\a\_work\1\s\src\ProjectTemplates\test\Templates.Blazor.Tests\bin\Release\net10.0\TestTemplates\AspNet.og4bddmkhnnv\AspNet.og4bddmkhnnv.csproj]\r\n |
Co-authored-by: javiercn <[email protected]>
Fixed all template test failures by updating 16 Blazor template files to remove |
@copilot I was wrong. We want to ignore the cases where the initializer initializes the property to a default value (null or default).
|
…ate changes - Reverted template files to original state with `= default!` initializers - Modified SupplyParameterFromFormAnalyzer to ignore default value initializers (null, null!, default, default!) - Added comprehensive tests for ignored cases: = null, = null!, = default, = default! - All 49 analyzer tests pass, confirming correct behavior Co-authored-by: javiercn <[email protected]>
Updated the analyzer to ignore default value initializers as requested (commit 41245e2). The analyzer now ignores |
src/Components/Analyzers/test/SupplyParameterFromFormAnalyzerTest.cs
Outdated
Show resolved
Hide resolved
src/Components/Analyzers/test/SupplyParameterFromFormAnalyzerTest.cs
Outdated
Show resolved
Hide resolved
…alyzer tests Co-authored-by: javiercn <[email protected]>
using {typeof(ParameterAttribute).Namespace}; | ||
class NotAComponent | ||
{{ | ||
[SupplyParameterFromForm] public string MyProperty {{ get; set; }} = ""default""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use default here, use "initial-value"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the remaining string literal "default" usage in the test. Changed it to "initial-value" to avoid confusion with the default keyword (commit 9a23dd5).
…alyzer tests Co-authored-by: javiercn <[email protected]>
This PR adds a new Roslyn analyzer (BL0008) that detects when properties decorated with
[SupplyParameterFromForm]
have non-default initializers, which can lead to unexpected null values during form posts.Problem
When using
[SupplyParameterFromForm]
with non-default property initializers like this:The initializer can be overwritten with
null
during form submission, causingEditForm
to fail with:Solution
The analyzer warns developers when this pattern is detected, while safely ignoring default value initializers (
= null
,= null!
,= default
,= default!
) that don't cause the same issue.Warning Message:
Safe patterns (ignored by analyzer):
Recommended fix for problematic cases:
Implementation Details
[SupplyParameterFromForm]
and non-default initializers= null
,= null!
,= default
,= default!
)Test Coverage
The implementation includes comprehensive tests covering:
[SupplyParameterFromForm]
(ignored)[SupplyParameterFromForm]
without initializers (ignored)= null
,= null!
,= default
,= default!
(ignored)All 49 analyzer tests pass.
Fixes #51420.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.