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 Compiler Warning for Unknown Attributes in Razor Components #9144

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

Conversation

SparkyTD
Copy link

Summary

This PR introduces a new compiler warning that will be triggered when a Razor component uses an attribute that does not correspond to any known property on the component. This warning aids in early detection of potential mistakes, such as mistyping an attribute name, improving code quality and robustness.


Background

Razor components allow developers to define parameters to receive data from the parent component. Without any feedback, mistakes like mistyping an attribute name or using an attribute that doesn't correspond to any property can lead to unexpected behavior at runtime.


Changes

  • New Warning Diagnostic: Introduced a new Razor Diagnostic: RZ10021: The attribute 'AttributeName' does not correspond to any of the parent component's parameters.
  • New Diagnostic Pass: Added ComponentUnknownAttributeDiagnosticPass, a new intermediate node pass that checks for unknown attributes.
  • Tests: Included unit tests to verify that the warning is triggered in the correct scenarios and not triggered in valid cases.

Incomplete Feature: Handling Arbitrary Parameter Matching

I'm seeking guidance on handling a Razor component's ability to capture unmatched values using a parameter with type IDictionary<string, object> and the CaptureUnmatchedValues attribute set to true (Documentation).

In order to correctly handle this case in my diagnostic pass, I would need to access some form of Type information about the given component either via Reflection, or component metadata. However, I was so far unable to find relevant information in the IntermediateNodeWalker's component visitor mechanism. The relevant part of the code can be found here.


Request for Feedback

I'm particularly interested in feedback on:

  • The logic used to detect unknown attributes.
  • The wording and formatting of the warning message.
  • Potential edge cases that may lead to breaking changes (e.g., when <TreatWarningsAsErrors> is enabled).

This diagnostic will be added to Razor component attributes that cannot be mapped to a Property in the underlying Component
This new intermediate pass will check for any component attributes that cannot be mapped to a valid property in the underlying component type, for the purpose of issuing a warning diagnostic.
@SparkyTD SparkyTD requested a review from a team as a code owner August 21, 2023 19:01
@SparkyTD
Copy link
Author

@dotnet-policy-service agree

@jjonescz jjonescz added the area-compiler Umbrella for all compiler issues label Aug 22, 2023
@jaredpar
Copy link
Member

jaredpar commented Sep 8, 2023

Thanks for this contribution. I do agree that this warning would be valuable (can name specific times it would have saved me doing razor work).

Unfortunately we need to change the structure of this a bit. As written it's a breaking change as it introduces diagnostics for existing code on tooling upgrade. That is true even though it's a warning as many customers use /warnaserror in their builds. Need to find a way to hook this into a warning wave, target framework change, etc ... An action where the customer does a specific edit that enables the warning.

At the moment we don't have an established process for this as we do for say the C# compiler. Need to work with the team to figure out how we want to appraoch changes like this.

@jaredpar jaredpar added this to the Backlog milestone Sep 8, 2023
@333fred
Copy link
Member

333fred commented Sep 8, 2023

I'm seeking guidance on handling a Razor component's ability to capture unmatched values using a parameter with type IDictionary<string, object> and the CaptureUnmatchedValues attribute set to true

This part is going to be particularly difficult, if not straight up impossible, with the current structure of the razor compiler. Consider a partial example like this:

Component1.razor

<MyComponent UnmatchedAttribute="hi" />

MyComponent.razor

@code {
    [Parameter] public bool UnrelatedValue { get; set; }
}

MyComponent.razor.cs

public partial class MyComponent
{
    [Parameter(CaptureUnmatchedValues = true)]
    public IDictionary<string, object> UnmatchedValues { get; set; }
}

There is currently no way to understand that that partial component with the UnmatchedValues property exists from the guts of the razor compiler. While I have no problem with the concept of a warning here, and think it's a good idea, we're going to need some pretty major architectural changes in order to be able to implement this correctly.

@jjonescz
Copy link
Member

There is currently no way to understand that that partial component with the UnmatchedValues property exists from the guts of the razor compiler.

@333fred Why? The compiler certainly understands these partial definitions. For example:

Component1.razor:

<Component2 />

Component2.razor:

@code {
    [Parameter, EditorRequired] public int Param1 { get; set; }
}

Component2.razor.cs:

using Microsoft.AspNetCore.Components;

namespace BlazorApp1.Shared;

public partial class Component2
{
    [Parameter, EditorRequired] public int Param2 { get; set; }
}

Gives me two warnings - for missing Param1 and Param2.

I think information about all component parameters should be in the corresponding tag helper's BoundAttributeDescriptors.

This commit attempts to solve the issues regarding the handling of 'CaptureUnmatchedValues' in the diagnostic pass. It does this by storing the value of this attribute parameter in 'BoundAttributeDescriptor.Metadata', where it can be later retrieved by the diagnostic analyzer.
@SparkyTD
Copy link
Author

I believe I have found a solution that addresses the issues regarding CaptureUnmatchedValues. When the property bindings are created in ComponentTagHelperDescriptorProvider, we do have access to property attribute information, including the value of CaptureUnmatchedValues. This information can now be stored as a new key in BoundAttributeDescriptor.Metadata, which can then be retrieved by my diagnostic pass.

I have created a commit with a rough outline of how I would implement this. In its current state, it does pass all the unit tests I've written, and it works in all the use cases that I could think of.

I am not sure how to address @jaredpar's concerns regarding the breaking change. Maybe if we can introduce warning waves for the razor compiler in the future, this PR might be useful as an opt-in warning.

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.

4 participants