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

[FUSE] Component attribute nameof() #10581

Merged
merged 15 commits into from
Jul 16, 2024

Conversation

chsienki
Copy link
Contributor

@chsienki chsienki commented Jul 4, 2024

This PR changes the way we emit component parameter names in runtime to use nameof(ParameterName) rather than a string. This allows things like Find All References to work in the IDE as we have an actual reference to the parameter.

Note that this is technically a breaking change if a user has a method called nameof( that is in scope to the generated component. This was discussed previously and decided to be an appropriate trade off. If we still feel strongly about this, we could a) add a specific razor error for that case, or b) ship an analyzer that identifies it (see also: 9369a14#r1037660470)

There are still a couple of edge cases around bind-[get/set] that aren't handled by this as it seemed big enough already. I'll submit those in a follow up PR.

I strongly suggest reviewing commit-by-commit.

Fixes: #10403

@chsienki chsienki force-pushed the fuse/component_attribute_nameof branch 2 times, most recently from 0f18d2a to 3df065d Compare July 9, 2024 18:34
@chsienki chsienki force-pushed the fuse/component_attribute_nameof branch from 3df065d to f60e10b Compare July 9, 2024 19:07
@chsienki chsienki changed the title Fuse/component attribute nameof [FUSE] Component attribute nameof() Jul 9, 2024
@chsienki chsienki added the area-compiler Umbrella for all compiler issues label Jul 9, 2024
@chsienki chsienki added this to the 17.12 Preview 1 milestone Jul 9, 2024
@chsienki chsienki marked this pull request as ready for review July 9, 2024 22:23
@chsienki chsienki requested review from a team as code owners July 9, 2024 22:23
@chsienki chsienki force-pushed the fuse/component_attribute_nameof branch from 94d7580 to 7858734 Compare July 9, 2024 22:35
@chsienki
Copy link
Contributor Author

chsienki commented Jul 9, 2024

@dotnet/razor-compiler for reviews please :)

@chsienki chsienki force-pushed the fuse/component_attribute_nameof branch from 7858734 to 9bcef7e Compare July 9, 2024 22:40
- Adds a test the demonstrates what happens if you declare a function called nameof in a component
@chsienki chsienki force-pushed the fuse/component_attribute_nameof branch from 9bcef7e to fe90192 Compare July 9, 2024 23:28
@chsienki chsienki force-pushed the fuse/component_attribute_nameof branch from b70c901 to 394da97 Compare July 10, 2024 00:03
@@ -1709,6 +1709,48 @@ public class MyComponent : ComponentBase
CompileToAssembly(generated);
}

[IntegrationTestFact]
public void BindToComponent_SpecifiesValue_WithMatchingProperties_WithNameof()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have tests for situations like this

@using TheirComponent=Test.MyComponent;

<TheirComponent @bind-Value=""ParentValue"" />
@code {
    public int ParentValue { get; set; } = 42;

    public string nameof(string s) => string.Empty;
}");

Copy link
Contributor Author

@chsienki chsienki Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, this isn't supported today anyway: #7670

But I would expect it to work when we do. We get the containing type name from the actual roslyn INamedTypeSymbol so we would already have had to do de-aliasing at that point and have the real name in hand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, this isn't supported today anyway: #7670

Locally it compiled fine on my machine. What am I missing?

Copy link
Contributor Author

@chsienki chsienki Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll compile, but you should get a warning warning RZ10012: Found markup element with unexpected name 'TheirComponent'. If this is intended to be a component, add a @using directive for its namespace.

You can write the using, and it even looks like it should work, but our tag helper discovery logic doesn't consider aliases, so it won't find it.


// Act
var generated = CompileToCSharp(@"
<MyComponent @bind-Value=""ParentValue"" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to think about all the quirky behavior with nameof and how it could intersect with this feature.

Ensure nameof doesn't bind to a keyword.

public class @int : ComponentBase
{
    [Parameter]
    public bool BoolParameter { get; set; }
}
<int BoolParameter="true" />

nameof(dynamic) cannot bind to the type dynamic (late bound type) but can bind to other types named dynamic.

public class dynamic : ComponentBase
{
    [Parameter]
    public bool BoolParameter { get; set; }
}
<dynamic BoolParameter="true" />

Note: all of these samples compile today.

Copy link
Contributor Author

@chsienki chsienki Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both @int and dynamic work as expected. Added tests to verify.

// nameof(containingType.PropertyName)
context.CodeWriter.Write("nameof(");
TypeNameHelper.WriteGloballyQualifiedName(context.CodeWriter, containingType);
context.CodeWriter.Write(".");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the type is in the global namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is broken already today :(

#10609

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think it should work right? The attribute must always be on some kind of type, and while the type itself might not have a namespace we're not constructing that part, that's handled by roslyn for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so the issue is actually only with @bind- syntax. Components in top level namespaces work otherwise. Confirmed it's not an issue here and added a test to cover it.

@chsienki
Copy link
Contributor Author

Integration test failure is unrelated to these changes. @dotnet/razor-compiler for reviews please :)

@@ -9,5 +9,5 @@ internal static class SerializationFormat
// or any of the types that compose it changes. This includes: RazorConfiguration,
// ProjectWorkspaceState, TagHelperDescriptor, and DocumentSnapshotHandle.
// NOTE: If this version is changed, a coordinated insertion is required between Roslyn and Razor for the C# extension.
public const int Version = 4;
public const int Version = 5;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we need to do a coordinated insertion per the note above?

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.

FUSE - Component attributes are written as strings
6 participants