Skip to content
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

Add warning wave support and warn for unnecessary @ in component parameters #10346

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented May 6, 2024

Resolves #9740.
Related (internal): https://github.com/dotnet/Razor-Language-Design/discussions/5
Commit-by-commit review might be useful.

The warning waves feature needs an SDK counterpart change to fully work (prototype here: dotnet/sdk#40720), but it shouldn't need any coordinated merging - without the SDK passing RazorWarningLevel through, the warnings simply stay disabled.

@jjonescz jjonescz added the area-compiler Umbrella for all compiler issues label May 6, 2024
@jjonescz jjonescz marked this pull request as ready for review May 6, 2024 18:09
@jjonescz jjonescz requested review from a team as code owners May 6, 2024 18:09
@stsrki
Copy link

stsrki commented Jun 7, 2024

I find this feature really bad for the .razor future. The use of @ should be recommended in most of times. The work of today's razor editor is not that good. Removing the need for @ and adding a warning by using it is a bad design, in my opinion.

Here is a quick try at removing the @ from a very simple code.

image

As you can see, the variable behind it is not recognized by the editor.

  1. It doesn't respond to the F2 key to go to its reference.
  2. It has a different color.
  3. The color wrongly indicates it is a string, which is not.
  4. Hovering the variable also doesn't show any information.

Please revise your plans for the warnings. And if you want to go forward, then at least fix the existing problem with the editor.

@jjonescz
Copy link
Member Author

jjonescz commented Jun 10, 2024

@stsrki I think you misunderstood - this PR warns only about unnecessary @ symbols and suggests to remove those. The ones in your example are necessary, and hence removing them changes functionality - and the IDE is correct in showing them as strings because they no longer point to the variables without the @ transition - but we definitely won't warn about such necessary @ symbols.

Related docs: https://learn.microsoft.com/en-us/aspnet/core/blazor/components/?view=aspnetcore-8.0#component-parameters

@stsrki
Copy link

stsrki commented Jun 10, 2024

@stsrki I think you misunderstood - this PR warns only about unnecessary @ symbols and suggests to remove those. The ones in your example are necessary, and hence removing them changes functionality - and the IDE is correct in showing them as strings because they no longer point to the variables without the @ transition - but we definitely won't warn about such necessary @ symbols.

Related docs: https://learn.microsoft.com/en-us/aspnet/core/blazor/components/?view=aspnetcore-8.0#component-parameters

Can you give a few examples of such use cases that are going to be affected by this change?

@jjonescz
Copy link
Member Author

See this test for some examples:

var project = CreateTestProject(new()
{
["Views/Home/Index.cshtml"] = """
@(await Html.RenderComponentAsync<MyApp.Shared.Component1>(RenderMode.Static))
""",
["Shared/Component1.razor"] = """
@{
var hi = "str";
var x = 21;
}
<Component2 IntParam="42" StrParam="hi" />
<Component2
IntParam="@(43)"
StrParam="@hi" />
<Component2
IntParam="@x"
StrParam="@("lit")" />
<Component2 IntParam="x * 3" StrParam="@(hi + "hey")" />
<Component2 IntParam=@x />
""",
["Shared/Component2.razor"] = """
I: @(IntParam + 1), S: @StrParam?.ToUpperInvariant()
@code {
[Parameter] public int IntParam { get; set; }
[Parameter] public string? StrParam { get; set; }
}
"""
});
var compilation = await project.GetCompilationAsync();
var driver = await GetDriverAsync(project, options =>
{
if (warningLevel != null)
{
options.TestGlobalOptions["build_property.RazorWarningLevel"] = warningLevel;
}
});
// Act
var result = RunGenerator(compilation!, ref driver, out compilation);
// Assert
if (hasWarnings)
{
result.Diagnostics.VerifyRazor(project,
// Shared/Component1.razor(7,15): warning RZ2013: The '@' prefix is not necessary for component parameters whose type is not string.
// IntParam="@(43)"
Diagnostic("RZ2013", "@").WithLocation(7, 15),
// Shared/Component1.razor(10,15): warning RZ2013: The '@' prefix is not necessary for component parameters whose type is not string.
// IntParam="@x"
Diagnostic("RZ2013", "@").WithLocation(10, 15),
// Shared/Component1.razor(13,22): warning RZ2013: The '@' prefix is not necessary for component parameters whose type is not string.
// <Component2 IntParam=@x />
Diagnostic("RZ2013", "@").WithLocation(13, 22));

In short, if there's a component with attribute that's known to be of a non-string type, it's unnecessary to pass values with @. E.g., in the test above, there's Component2 with [Parameter] public int IntParam { get; set; } and you can write <Component2 IntParam="42" /> rather than <Component2 IntParam="@(42)" /> (both are equivalent), so you will get a warning for the second case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn when an @ is optional in an attribute value
2 participants