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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@ namespace Microsoft.AspNetCore.Razor.Language.Components;

internal class ComponentLoweringPass : ComponentIntermediateNodePassBase, IRazorOptimizationPass
{
private readonly int? _razorWarningLevel;

// This pass runs earlier than our other passes that 'lower' specific kinds of attributes.
public override int Order => 0;

public ComponentLoweringPass(RazorConfiguration configuration)
{
_razorWarningLevel = configuration.RazorWarningLevel;
}

protected override void ExecuteCore(RazorCodeDocument codeDocument, DocumentIntermediateNode documentNode)
{
if (!IsComponentDocument(documentNode))
Expand Down Expand Up @@ -137,7 +144,7 @@ static TagHelperDescriptor GetTagHelperOrAddDiagnostic(TagHelperIntermediateNode
}
}

private static ComponentIntermediateNode RewriteAsComponent(TagHelperIntermediateNode node, TagHelperDescriptor tagHelper)
private ComponentIntermediateNode RewriteAsComponent(TagHelperIntermediateNode node, TagHelperDescriptor tagHelper)
{
var component = new ComponentIntermediateNode()
{
Expand All @@ -163,6 +170,7 @@ private static ComponentIntermediateNode RewriteAsComponent(TagHelperIntermediat
}

ValidateRequiredAttributes(node, tagHelper, component);
WarnForUnnecessaryAt(component);

return component;
}
Expand Down Expand Up @@ -213,6 +221,27 @@ static bool IsPresentAsAttribute(string attributeName, ComponentIntermediateNode
}
}

private void WarnForUnnecessaryAt(ComponentIntermediateNode component)
{
if (_razorWarningLevel is not >= 9)
{
return;
}

foreach (var attribute in component.Attributes)
{
// IntParam="@x" has unnecessary `@`, can just use IntParam="x" -> warn
// StrParam="@x" is different than StrParam="x" -> don't warn
if (!attribute.BoundAttribute.IsStringProperty &&
attribute.Source is { } originalSource &&
attribute.Children is [CSharpExpressionIntermediateNode])
{
var source = originalSource.With(length: 1, endCharacterIndex: originalSource.CharacterIndex + 1);
attribute.Diagnostics.Add(RazorDiagnosticFactory.CreateComponentParameter_UnnecessaryAt(source));
}
}
}

private static MarkupElementIntermediateNode RewriteAsElement(TagHelperIntermediateNode node)
{
var result = new MarkupElementIntermediateNode()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ public sealed record class RazorConfiguration(
string ConfigurationName,
ImmutableArray<RazorExtension> Extensions,
LanguageServerFlags? LanguageServerFlags = null,
bool UseConsolidatedMvcViews = false)
bool UseConsolidatedMvcViews = false,
int? RazorWarningLevel = null)
{
public static readonly RazorConfiguration Default = new(
RazorLanguageVersion.Latest,
Expand All @@ -27,6 +28,7 @@ public bool Equals(RazorConfiguration? other)
ConfigurationName == other.ConfigurationName &&
LanguageServerFlags == other.LanguageServerFlags &&
UseConsolidatedMvcViews == other.UseConsolidatedMvcViews &&
RazorWarningLevel == other.RazorWarningLevel &&
Extensions.SequenceEqual(other.Extensions);

public override int GetHashCode()
Expand All @@ -37,6 +39,7 @@ public override int GetHashCode()
hash.Add(Extensions);
hash.Add(UseConsolidatedMvcViews);
hash.Add(LanguageServerFlags);
hash.Add(RazorWarningLevel);
return hash;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,14 @@ public static RazorDiagnostic CreateTagHelper_InconsistentTagStructure(SourceSpa
public static RazorDiagnostic CreateComponent_EditorRequiredParameterNotSpecified(SourceSpan? location, string tagName, string parameterName)
=> RazorDiagnostic.Create(Component_EditorRequiredParameterNotSpecified, location, tagName, parameterName);

internal static readonly RazorDiagnosticDescriptor ComponentParameter_UnnecessaryAt =
new($"{DiagnosticPrefix}2013",
Resources.ComponentParameter_UnnecessaryAt,
RazorDiagnosticSeverity.Warning);

public static RazorDiagnostic CreateComponentParameter_UnnecessaryAt(SourceSpan? location)
=> RazorDiagnostic.Create(ComponentParameter_UnnecessaryAt, location);

#endregion

#region TagHelper Errors
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ public static RazorProjectEngine Create(
NamespaceDirective.Register(builder);
AttributeDirective.Register(builder);

AddComponentFeatures(builder, configuration.LanguageVersion);
AddComponentFeatures(builder, configuration);
}

configure?.Invoke(builder);
Expand Down Expand Up @@ -448,8 +448,10 @@ private static void AddDefaultFeatures(ImmutableArray<IRazorFeature>.Builder fea
});
}

private static void AddComponentFeatures(RazorProjectEngineBuilder builder, RazorLanguageVersion razorLanguageVersion)
private static void AddComponentFeatures(RazorProjectEngineBuilder builder, RazorConfiguration configuration)
{
RazorLanguageVersion razorLanguageVersion = configuration.LanguageVersion;

// Project Engine Features
builder.Features.Add(new ComponentImportProjectFeature());

Expand Down Expand Up @@ -486,7 +488,7 @@ private static void AddComponentFeatures(RazorProjectEngineBuilder builder, Razo

// Optimization
builder.Features.Add(new ComponentComplexAttributeContentPass());
builder.Features.Add(new ComponentLoweringPass());
builder.Features.Add(new ComponentLoweringPass(configuration));
builder.Features.Add(new ComponentEventHandlerLoweringPass());
builder.Features.Add(new ComponentKeyLoweringPass());
builder.Features.Add(new ComponentReferenceCaptureLoweringPass());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,9 @@
<data name="Component_EditorRequiredParameterNotSpecified" xml:space="preserve">
<value>Component '{0}' expects a value for the parameter '{1}', but a value may not have been provided.</value>
</data>
<data name="ComponentParameter_UnnecessaryAt" xml:space="preserve">
<value>The '@' prefix is not necessary for component parameters whose type is not string.</value>
</data>
<data name="An_item_with_the_same_key_has_already_been_added_Key_0" xml:space="preserve">
<value>An item with the same key has already been added. Key: {0}</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ namespace Microsoft.NET.Sdk.Razor.SourceGenerators
internal static class DiagnosticIds
{
public const string InvalidRazorLangVersionRuleId = "RZ3600";
public const string InvalidRazorWarningLevelRuleId = "RZ3601";
public const string ReComputingTagHelpersRuleId = "RSG001";
public const string TargetPathNotProvidedRuleId = "RSG002";
public const string GeneratedOutputFullPathNotProvidedRuleId = "RSG003";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ internal static class RazorDiagnostics
DiagnosticSeverity.Error,
isEnabledByDefault: true);

public static readonly DiagnosticDescriptor InvalidRazorWarningLevelDescriptor = new DiagnosticDescriptor(
DiagnosticIds.InvalidRazorWarningLevelRuleId,
new LocalizableResourceString(nameof(RazorSourceGeneratorResources.InvalidRazorWarningLevelTitle), RazorSourceGeneratorResources.ResourceManager, typeof(RazorSourceGeneratorResources)),
new LocalizableResourceString(nameof(RazorSourceGeneratorResources.InvalidRazorWarningLevelMessage), RazorSourceGeneratorResources.ResourceManager, typeof(RazorSourceGeneratorResources)),
"RazorSourceGenerator",
DiagnosticSeverity.Error,
isEnabledByDefault: true);

public static readonly DiagnosticDescriptor ReComputingTagHelpersDescriptor = new DiagnosticDescriptor(
DiagnosticIds.ReComputingTagHelpersRuleId,
new LocalizableResourceString(nameof(RazorSourceGeneratorResources.RecomputingTagHelpersTitle), RazorSourceGeneratorResources.ResourceManager, typeof(RazorSourceGeneratorResources)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,13 @@
<value>Invalid RazorLangVersion</value>
</data>
<data name="InvalidRazorLangMessage" xml:space="preserve">
<value>Invalid value '{0}'' for RazorLangVersion. Valid values include 'Latest' or a valid version in range 1.0 to 8.0.</value>
<value>Invalid value '{0}' for RazorLangVersion. Valid values include 'Latest' or a valid version in range 1.0 to 8.0.</value>
</data>
<data name="InvalidRazorWarningLevelTitle" xml:space="preserve">
<value>Invalid RazorWarningLevel</value>
</data>
<data name="InvalidRazorWarningLevelMessage" xml:space="preserve">
<value>Invalid value '{0}' for RazorWarningLevel. Must be empty or an integer.</value>
</data>
<data name="RecomputingTagHelpersTitle" xml:space="preserve">
<value>Recomputing tag helpers</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using Microsoft.AspNetCore.Razor;
using Microsoft.CodeAnalysis;
Expand Down Expand Up @@ -39,12 +40,12 @@ internal static IncrementalValuesProvider<TSource> ReportDiagnostics<TSource>(th
return source.Where((pair) => pair.Item1 != null).Select((pair, ct) => pair.Item1!);
}

internal static IncrementalValueProvider<TSource> ReportDiagnostics<TSource>(this IncrementalValueProvider<(TSource?, Diagnostic?)> source, IncrementalGeneratorInitializationContext context)
internal static IncrementalValueProvider<TSource> ReportDiagnostics<TSource>(this IncrementalValueProvider<(TSource?, ImmutableArray<Diagnostic>)> source, IncrementalGeneratorInitializationContext context)
{
context.RegisterSourceOutput(source, (spc, source) =>
{
var (_, diagnostic) = source;
if (diagnostic != null)
var (_, diagnostics) = source;
foreach (var diagnostic in diagnostics)
{
spc.ReportDiagnostic(diagnostic);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Immutable;
using System.IO;
using System.Text;
using System.Threading;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.PooledObjects;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Diagnostics;
Expand All @@ -14,7 +16,7 @@ namespace Microsoft.NET.Sdk.Razor.SourceGenerators
{
public partial class RazorSourceGenerator
{
private (RazorSourceGenerationOptions?, Diagnostic?) ComputeRazorSourceGeneratorOptions(((AnalyzerConfigOptionsProvider, ParseOptions), bool) pair, CancellationToken ct)
private (RazorSourceGenerationOptions?, ImmutableArray<Diagnostic>) ComputeRazorSourceGeneratorOptions(((AnalyzerConfigOptionsProvider, ParseOptions), bool) pair, CancellationToken ct)
{
var ((options, parseOptions), isSuppressed) = pair;
var globalOptions = options.GlobalOptions;
Expand All @@ -31,18 +33,42 @@ public partial class RazorSourceGenerator
globalOptions.TryGetValue("build_property.SupportLocalizedComponentNames", out var supportLocalizedComponentNames);
globalOptions.TryGetValue("build_property.GenerateRazorMetadataSourceChecksumAttributes", out var generateMetadataSourceChecksumAttributes);

Diagnostic? diagnostic = null;
using var diagnostics = new PooledArrayBuilder<Diagnostic>();
if (!globalOptions.TryGetValue("build_property.RazorLangVersion", out var razorLanguageVersionString) ||
!RazorLanguageVersion.TryParse(razorLanguageVersionString, out var razorLanguageVersion))
{
diagnostic = Diagnostic.Create(
diagnostics.Add(Diagnostic.Create(
RazorDiagnostics.InvalidRazorLangVersionDescriptor,
Location.None,
razorLanguageVersionString);
razorLanguageVersionString));
razorLanguageVersion = RazorLanguageVersion.Latest;
}

var razorConfiguration = new RazorConfiguration(razorLanguageVersion, configurationName ?? "default", Extensions: [], UseConsolidatedMvcViews: true);
globalOptions.TryGetValue("build_property.RazorWarningLevel", out var razorWarningLevelString);
int? razorWarningLevel;
if (string.IsNullOrEmpty(razorWarningLevelString))
{
razorWarningLevel = null;
}
else if (!int.TryParse(razorWarningLevelString, out var razorWarningLevelInt))
{
diagnostics.Add(Diagnostic.Create(
RazorDiagnostics.InvalidRazorWarningLevelDescriptor,
Location.None,
razorWarningLevelString));
razorWarningLevel = null;
}
else
{
razorWarningLevel = razorWarningLevelInt;
}

var razorConfiguration = new RazorConfiguration(
razorLanguageVersion,
configurationName ?? "default",
Extensions: [],
UseConsolidatedMvcViews: true,
RazorWarningLevel: razorWarningLevel);

var razorSourceGenerationOptions = new RazorSourceGenerationOptions()
{
Expand All @@ -54,7 +80,7 @@ public partial class RazorSourceGenerator
TestSuppressUniqueIds = _testSuppressUniqueIds,
};

return (razorSourceGenerationOptions, diagnostic);
return (razorSourceGenerationOptions, diagnostics.DrainToImmutable());
}

private static (SourceGeneratorProjectItem?, Diagnostic?) ComputeProjectItems((AdditionalText, AnalyzerConfigOptionsProvider) pair, CancellationToken ct)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
Expand Down Expand Up @@ -274,6 +275,79 @@ public static class RuntimeHelpers
Assert.DoesNotContain("AddComponentParameter", source.SourceText.ToString());
}

[Theory]
[InlineData(null, false)]
[InlineData("", false)]
[InlineData("8", false)]
[InlineData("9", true)]
[InlineData("10", true)]
[InlineData("999", true)]
public async Task ComponentParameter_UnnecessaryAt(string warningLevel, bool hasWarnings)
{
// Arrange
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));
}
else
{
result.Diagnostics.VerifyRazor(project);
}
Assert.Equal(3, result.GeneratedSources.Length);
await VerifyRazorPageMatchesBaselineAsync(compilation, "Views_Home_Index");
}

[Fact, WorkItem("https://github.com/dotnet/razor/issues/8660")]
public async Task TypeArgumentsCannotBeInferred()
{
Expand Down
Loading