Introduce 'RenderTreeBuilder.AddComponentParameter()'

[MackinnonBuck]

Introduce RenderTreeBuilder.AddComponentParameter()

Fixes an issue where user-defined implicit conversion operators may cause undesirable RenderTreeBuilder.AddAttribute() overloads to be resolved.

Description

Suppose you have defined the following class:

public class Foo
{
    public string Value { get; set; } = string.Empty;
    public static implicit operator string(Foo foo) => foo.Value;
}

Now let's say you have a Blazor component MyComponent that has a parameter of type Foo. The generated RenderTreeBuilder.AddAttribute(...) call to set the parameter value will look something like this:

// ...
__builder.AddAttribute(1, "Foo", _foo); // '_foo' is of type 'Foo' here

We want the AddAttribute(int sequence, string name, object? value) overload to be resolved, but since Foo defines an implicit conversion to string, the AddAttribute(int sequence, string name, string value) overload gets resolved instead. As a result, the framework later tries to set the component parameter of type Foo to a value of type string, and an exception is thrown.

To solve this problem, this PR adds an AddComponentParameter(int sequence, string name, object? value) method to RenderTreeBuilder. There is only one overload to choose from, so we don't experience the same problem as we did with AddAttribute(). Also, since component parameter values always get boxed eventually (to be added to a RenderTreeFrame), we're not losing anything by performing the boxing earlier.

A corresponding change will have to be made in the Razor compiler so that generated code makes use of the new AddComponentParameter() method.

Validation

• Manual
• Automated tests

Contributes to #18042

[MackinnonBuck]

Note to reviewers: This PR will be easier to review by looking at individual commits 🙂

[MackinnonBuck]

We'll also want to do an API review of this once we've confirmed this is the general strategy we'll be taking.

[SteveSandersonMS]

This looks good to me. Nice job, @MackinnonBuck!

One thing I'd still like to check: do you think we still have adequate unit test coverage for the case where existing RCLs continue to use AddAttribute for component parameters? I'm not saying we need 100% the same level of coverage as for AddComponentParameter, but we do want to have a realistic chance of being notified if some future change to AddAttribute was going to break back-compat with being used for component parameters. We only need to test that AddAttribute does still result in the expected RenderTreeFrame structure for each of the main component-related scenarios.

It's quite possible this coverage is already handled in this PR by virtue of not modifying certain existing tests. But that's very difficult to establish by reading a diff which only shows the things you have modified :) So is this something you could clarify?

[javiercn]

How does this approach work when the parameters are splatted?

[SteveSandersonMS]

That calls AddMultipleAttributes(int sequence, IEnumerable<KeyValuePair<string, object>>? attributes) which doesn't have the same overload resolution issue.

If you're suggesting adding a AddMultipleComponentParameters method we could do but it would have no difference in behavior. I agree the aesthetics of still having to use AddMultipleAttributes for components are a bit strange, but if RenderTreeBuilder is only intended as a compiler target it's less of a priority.

[SteveSandersonMS]

Oh one other idea: would it technically be valid to mark the AddAttribute(int, string, object) overload as obsolete? I haven't dug into this in detail but at first glance it seems like the object overload wouldn't make much sense for HTML elements, unless it results in calling ToString on the value automatically.

Perhaps that's too dramatic and too soon, especially given how we never actually intend to remove the method since we never want to break older RCLs.

[javiercn]

Is there a way in which we can solve this without requiring a compiler change?

I am a bit worried that this is a lot of work to support a niche scenario like implicit type conversions. It seems more trouble than it is worth.

[SteveSandersonMS]

Is it a lot of work? It looks like the PR here is already at a production standard, so what remains is the Razor compiler change. It should be a really trivial change to the actual compiler logic. The only nontrivial bit is that a lot of test baselines will get updated. And there's no time constraint, as the runtime will retain back-compat so the Razor compiler change can be done any time during the 8.0 timeline.

Unless I'm missing something of course!

[MackinnonBuck]

@SteveSandersonMS: One thing I'd still like to check: do you think we still have adequate unit test coverage for the case where existing RCLs continue to use AddAttribute for component parameters?

I think we do - we still have all the AddAttribute_Component_SomethingSomething tests that fully validate each overload of AddAttribute when applied to a component frame.

[MackinnonBuck]

Regarding marking AddAttribute(int, string, object) obsolete - I wonder if it might be fine to wait on this for now. If there does turn out to be some reason why a developer is boxing the attribute's value parameter (maybe via some custom abstraction), the obsoleteness of that method might get in the way?

[SteveSandersonMS]

Regarding marking AddAttribute(int, string, object) obsolete - I wonder if it might be fine to wait on this for now

Totally fine. It would have been an unnecessary extra point of friction for us to obsolete it without a clear need.

[SteveSandersonMS]

I'm fine with this based on the discussion here.

Before merging, do you think it's worth trying to get some kind of signal from the compiler team that they will likely be able to make the corresponding change on their side within the next couple of months? If for some reason they had a problem with this that might force us to re-think. But if they are unconcerned we could be pretty confident.

[MackinnonBuck]

@jaredpar Do you think the Razor Compiler team can make the changes corresponding to those in this PR?
This would involve generating calls to RenderTreeBuilder.AddComponentParameter(int, string, object) in place of any calls to RenderTreeBuilder.AddAttribute(...) when adding attributes to a Razor component.

[MackinnonBuck]

API review issue: #46575

[davidwengier]

Random drive-by thought: If there is going to need to be a compiler change anyway, couldn't the compiler change to add (object?) in front of the third parameter of AddAttribute and this PR is then unnecessary?

[jaredpar]

@davidwengier had similar thoughts. I think understanding why that would or wouldn't work would help us prioritize.

@chsienki, @333fred, @jjonescz

[chsienki]

Yep had the same thought also. Given the want to obsolete the existing method though, I wasn't sure if there were other issues with it that this API fixes.

[MackinnonBuck]

@jaredpar, @davidwengier, @chsienki,

The (object?) cast was something we considered, and it would work in the sense that it solves the exact issue at hand. However, doing so would prevent us from making certain types of improvements to this area in the future. For example, let's say we wanted to introduce a new, generic AddComponentParameter<T>(...) method that completely avoids boxing the component parameter value. We wouldn't be able to do something like this if we erase the type of the value argument.

Even in the current AddAttribute() implementation, each overload has specialized logic that checks whether we're dealing with a component parameter or an element attribute. Arguably, we should have had a separate AddComponentParameter() method from the start to allow us to take advantage of the differences between how components and elements are treated. So, this change is also a step toward long-term correctness in addition to fixing the bug linked in the PR.

cc @SteveSandersonMS

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.