dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.51k stars 10.04k forks source link

[Blazor] Analyzer to warn when using `required` or `init` on Blazor component properties #47864

Open MackinnonBuck opened 1 year ago

MackinnonBuck commented 1 year ago

Background

We've seen a few issue reports (e.g., #47824, #44974) where customers attempt to use the required modifier on Blazor component parameter properties to ensure that the parameter will definitely have a value assigned to it.

Why this is a problem

This is not a supported pattern (and it likely never will be) because Blazor component parameters do not get assigned upon component instantiation. Instead, a ParameterView object gets passed to the component's SetParametersAsync method, which updates parameter property values accordingly. The required modifier has no bearing on whether the parameter will have an entry in the ParameterView. Furthermore, even for parameters that do have ParameterView entries, their properties won't get values assigned until long after what the required modifier is supposed to guarantee.

Currently, using the required modifier in a Blazor component doesn't break anything, because any component extending ComponentBase (or otherwise manually invoking ParameterView.SetParameterProperties()) gets its properties set via refelction. But the required modifier can mislead component authors into thinking that it will make the property "required", especially considering it makes nullability warnings go away.

Likewise, init has no effect in any component whose parameters are set using ParameterView.SetParameterProperties(), because reflection bypasses the "init-only" restriction.

If we implement #29550, then using required or init could cause compilation errors, since we won't be using reflection to bypass the restrictions that required or init impose. Therefore, for future-proofing reasons, it's in our best interest to ensure that customers avoid using these keywords on Blazor component properties.

Proposed solution

For all these reasons, we should consider implementing an analyzer that emits a warning whenever component properties use required or init. The analyzer message can suggest using the [EditorRequired] attribute instead.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

NielsHoogeveen commented 1 year ago

Please consider mentioning in the analyzer warning what to do to get rid of nullability warnings when using blazor parameters. The reason for introducing "required" was in my case a desire to have that nullability warning go away.

Using nullable reference types as properties for Parameters communicates that a null value could be passed as a parameter value, even when this is not desired. So that is not an option.

Initializing all parameter properties that have non nullable reference types with default! leads to a warning free use of blazor parameters. I hope to learn that this is indeed the appropriate solution.

ScarletKuro commented 1 year ago

Hello! I have a question about this code snippet:

[Inject]
public required NavigationManager NavigationManager { get; set; }

I'm curious to know if this is a supported pattern or not. It caught my attention because I've noticed that few users seem to be utilizing it, which has raised questions in my mind about its validity.

egil commented 1 year ago

My go to has thus far been to do something like this:

[Parameter, EditorRequired]
public required string Arg { get; set; }

It shuts up the C# compiler when nullable annotations are enabled, and signals to the Razor editor that a parameter is required.

I much prefer that to e.g.:

[Parameter, EditorRequired]
public string Arg { get; set; } = default!;

The first is much cleaner and does communicate the intention of the component author. I bet many Blazor devs don't know that parameters are currently set via reflection, so that doesn't confuse them.

Instead of issuing a warning to users when using the required keyword together with EditorRequired, I prefer the opposite, i.e. that a warning is issued when the EditorRequired attribute is used on a property and the required keyword is not set (of course, only when nullable annotations are enabled).

Ps. I'm sure there is an excellent reason that we need the EditorRequired attribute and cannot just rely on the required keyword. Ideally, if nullable annotations are enabled then the EditorRequired attribute should be optional.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 11 months ago

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.