dotnet / razor

Compiler and tooling experience for Razor ASP.NET Core apps in Visual Studio, Visual Studio for Mac, and VS Code.
https://asp.net
MIT License
489 stars 190 forks source link

Improve Blazor CascadingTypeParameter support - Current compiler cannot handle common scenarios. #10757

Open audacode opened 4 weeks ago

audacode commented 4 weeks ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

I expected the CascadingTypeParameter feature to work in the most common and obvious scenarios, but it does not. If the cascaded type was a string, it should be possible to use <Widget Action="item => Console.WriteLine(item)" /> instead of <Widget Action="(string item) => Console.WriteLine(item)" />. If you were passing in a parameter, it should be possible to do this <Widget Item="Hello" /> instead of needing to do this <Widget Item="@("Hello")" />. The CascadingTypeParameter feature was intended to resolve this type of issue but needs more work.

Describe the solution you'd like

Imagine this repeater:

@attribute [CascadingTypeParameter(nameof(T))]
@typeparam T

@foreach (var item in Items)
{
    @ChildContent(item)
}
@code {
    [Parameter] public RenderFragment<T> ChildContent { get; set; }
    [Parameter] public IEnumerable<T> Items { get; set;}
}

Then this Widget component intended to be used inside the repeater:

@typeparam T
Type is @typeof(T)
@code {
    [Parameter] public EventCallback<T> Action { get; set; }
    [Parameter] public T? Item { get; set; }
}

Warning: Do not overthink this widget as it is not intended to do anything sensible, other than demonstrate compiler limitations.

Now imagine we use the component like such:

<Repeater Items="items">
    <Widget Action="(string i) => Console.WriteLine(i)" />
</Repeater>
@code {
List<string> items = ["David", "Fred", "Jimmy", "Banjo"];
}

The above code works, and <Widget T="string" Action="i => Console.WriteLine(i)" /> also works. However, it is completely reasonable to expect to be able to use the following syntax:

<Widget Action="i => Console.WriteLine(i)" />

Another example of the compiler's mediocre compilation process is the following example. Let us say we write this code:

<Widget Item=@("Hello") />

That works, but because T is a string, for consistency with the way the rest of blazor works, we would expect this to work (noting that it does work if the underlying type was a string instead of a type parameter, where the parameter was a string) :

<Widget Item="Hello" />

However, this does not work (it looks for a member called Hello, which is what blazor would normally do for anything other than a string. Even this does NOT work: <Widget T="string" Item="Hello" />.

@danroth27 and @SteveSandersonMS I had thought we were further along than this once we got to .NET 6 and added the CascadingTypeParameter feature. It is hard, not being on the compiler team, to understand why when an outer component uses CascadingTypeParameter, that that isn't the same as directly passing in the type using the old-school T="string" approach. But I do feel the current expectation means almost everyone will hit this issue and get stuck/frustrated.

Is there a concrete reason why the situation cannot be improved for .NET 9 or even .NET 10 if that is needed.

Additional context

No response

audacode commented 3 weeks ago

Hi @davidwengier nice train simulator! I am Melbourne based as well.

If it helps, I notice that if the Widget.razor file uses an Action like this:

    [Parameter] public Action<T> Action { get; set; }

Then you can use this code without specifying the type of i:

    <Widget Action="i => Console.WriteLine(i)" />

But if it is specified using EventCallback like this:

    [Parameter] public EventCallback<T> Action { get; set; }

Then you you must specify the type of i either like this <Widget Action="(string i) => Console.WriteLine(i)" /> or this <Widget T="string" Action="i => Console.WriteLine(i)" />

I have not looked under the covers, but note what WORKS and DOES NOT work below:

        EventCallbackFactory factory = new();
        // This works
        EventCallback<string> f1 = factory.Create<string>(null, i => Console.WriteLine(i));
        // This works
        EventCallback<string> f2 = factory.Create(null, (string i) => Console.WriteLine(i));
        // This DOT NOT work
        EventCallback<string> f3 = factory.Create(null, i => Console.WriteLine(i));

Thus, my assumption is that the Razor compiler is not emitting this as a call to .Create<TCascadedType> and is instead using `.Create causing the issue.

I am guessing if the compiler emits the call to the cascaded type parameter, such as to .Create<string> that would resolve the issue? I have to assume there is a way of getting the razor compiler to handle this situation. We want developers to be using EventCallback rather than Action because it plays better with the way Blazor interacts with StateHasChanged when the callback happens.

jjonescz commented 3 weeks ago

Thanks for writing this up. If I'm reading it correctly, you are reporting two separate issues:

  1. CascadingTypeParameter and EventCallback don't play nicely in all cases. I haven't investigated deeply, so this might be wrong, but as you said, it looks like the razor compiler should emit EventCallbackFactory.Create<T> if there is type inference involved - and where Action<T> works, EventCallback<T> should work similarly.
  2. Passing a string literal directly to a generic component parameter which we know has type string doesn't work. This is not related to CascadingTypeParameter, e.g., the following doesn't work either:
    <Widget T="string" Item="Hello" />

    Whereas if the Widget wasn't generic, i.e., the Item parameter type was simply string, that would work. Changing that would be a breaking change though - if users are today relying on this behavior and passing a variable, it would suddenly start to be interpreted as a string literal.

iphdav commented 3 weeks ago

@jjonescz Yes I agree with your summary of point 1 that "if there is type inference involved - and where Action works, EventCallback should work similarly."

I think this should be a simple change and would ideally get into .NET 9. I was surprised this did not work out of the box. I wasted a lot of time on it because I believed it must have been me doing something stupid instead of the compiler not supporting EventCallback in that particular and common case.

Regarding point 2: Thank you for clarification and I agree with that it would be a breaking change so will need to be done through good documentation rather than a change (and I should not have included point 2 in the same bug/issue report - sorry).

jjonescz commented 2 weeks ago

Hm, I've investigated the EventCallback<T> problem more and I don't think it's easy to fix.

The razor compiler doesn't know the T at compile-time, it very deliberately relies on the C# compiler to do the type inference.

For example, for an Action<T> parameter we generate this C#:

CreateWidget(i => Console.WriteLine(i));

void CreateWidget<T>(Action<T> a) { }

but for EventCallback<T> we need to call EventCallbackFactory (it has multiple overloads to support scenarios where user passes Func<Task> for example which is then converted to the EventCallback<T> etc):

CreateWidget(EventCallback.Factory.Create(i => Console.WriteLine(i)); // error: the factory creates EventCallback instead of EventCallback<T>

void CreateWidget<T>(EventCallback<T> e) { }

If runtime provided EventCallbackFactory.CreateGeneric which would only have overloads returning EventCallback<T> we could use that (when we know the parameter type is the generic EventCallback<T>) and it would work, I think. Otherwise, I don't see an easy solution we could do in the compiler.