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
505 stars 196 forks source link

Automatic type inference won't work when an EventCallBack<T> is added #7069

Open Jcparkyn opened 2 years ago

Jcparkyn commented 2 years ago

This is an intentional duplicate of dotnet/razor-compiler#150 and https://github.com/dotnet/aspnetcore/issues/39734, which were both (as far as I can tell) closed prematurely, due to a separate (but related) issue being fixed.

Tested on SDK version 7.0.100-rc.2.22477.23

To recap the actual issue: Given the following generic component: GenericComponent.razor

@typeparam T
@code {
    [Parameter] public T Item { get; set; } = default!;
    [Parameter] public EventCallback<T> OnChange { get; set; }
    [Parameter] public Action<T>? OnChangeAction { get; set; }
}

Some of the following usages fail to compile, and neither of them give error messages that adequately explain the issue:

@*All of the following three lines fail to compile*@
<GenericComponent Item=1 OnChange="x => {}" /> @*Cannot convert from EventCallback to EventCallback<int>*@
<GenericComponent Item=1 OnChange=DoNothing /> @*Cannot convert from 'method group' to `EventCallback`*@
<GenericComponent Item=1 OnChange="(object x) => {}" /> @*Cannot convert from EventCallback to EventCallback<int>*@

@*First two versions work with an explicit type parameter:*@
<GenericComponent Item=1 T=int OnChange="x => {}" />
<GenericComponent Item=1 T=int OnChange=DoNothing />
@*...but this doesn't work if the lambda type doesn't match exactly (even when variance should be allowed):*@
<GenericComponent Item=1 T=int OnChange="(object x) => {}" /> @*Cannot convert lambda expression to type 'EventCallback' because it is not a delegate type*@

@*All versions work when using Action<T> instead of EventCallback<T>*@
<GenericComponent Item=1 OnChangeAction="x => {}" />
<GenericComponent Item=1 OnChangeAction=DoNothing />
<GenericComponent Item=1 OnChangeAction="(object x) => {}" />

@*Explicit lambda type parameter works, if the exact type is used:*@
@*(this is the regression that was fixed in dotnet/razor-compiler#150)*@
<GenericComponent Item=1 OnChange="(int x) => {}" />

@code {
    private void DoNothing(int x) {}
}

The rules for which scenarios work here are quite confusing, and the error messages don't help to guide the user. This is a particularly big problem when using more complicated (or multiple) type arguments, which are a lot harder to type explicitly.

https://github.com/dotnet/aspnetcore/issues/39734 has some explanation of why this issue occurs.

Jcparkyn commented 2 years ago

To summarize some of the possible fixes for this, any of the following should be possible, but they each have issues to work around:

1. Use Action<T> instead of EventCallback<T> in the generated TypeInference method

E.g.,

public static void CreateMyComponent_0<TItem>(TItem __arg0, object __component, Action<TItem> __arg1) // other params omitted
{
    __builder.OpenComponent<global::Test.MyComponent<TItem>>(seq);
    __builder.AddAttribute(__seq0, "Item", __arg0);
    __builder.AddAttribute(__seq1, "MyEvent", EventCallback.Factory.Create<TItem>(__component, __arg1));
    __builder.CloseComponent();
}

The big issue here is that it wouldn't work by default if the user passes an EventCallback<T> directly, rather than a delegate.

2. Type inference via EventCallback.Factory.CreateInferred

E.g., in the generated BuildRenderTree method, replace

global::Microsoft.AspNetCore.Components.EventCallback.Factory.Create(this, x => {})

with

global::Microsoft.AspNetCore.Components.EventCallback.Factory.CreateInferred(this, x => {}, 1) // 1 would come from the value of the first `T` parameter.

The issues with this approach are:

3. Type inference via target-typed new()

It is possible to get around most of the issues with the approaches above by using target-typed new() expressions inside BuildRenderTree. EventCallback<T> does not currently have constructors that would make this possible, so the options would be either:

  1. Add these constructors to EventCallback<T>. This could potentially be a breaking change, and I assume there's a reason they don't exist already?
  2. Create a new class/struct to wrap an EventCallback<T> and provide the necessary constructors. This would need to be used as the parameter for the TypeInference method. E.g.,
    private class EventCallbackTypeInference<T>
    {
        public EventCallback<T> Handler { get; }
        // Note: These would probably use EventCallback.Factory instead of new()
        public EventCallbackTypeInference(IHandleEvent? receiver, Action<T> handler) => Handler = new(receiver, handler);
        public EventCallbackTypeInference(IHandleEvent? receiver, Func<T, object> handler) => Handler = new(receiver, handler);
        public EventCallbackTypeInference(IHandleEvent? receiver, EventCallback<T> handler) => Handler = handler;
    }

Then, in the type inference method:

    static void CreateMyComponent_0<TItem>(TItem __arg0, EventCallbackTypeInference<TItem> __arg1) //etc
    {
        __builder.OpenComponent<global::Test.MyComponent<TItem>>(seq);
        __builder.AddAttribute(__seq0, "Item", __arg0);
        __builder.AddAttribute(__seq1, "MyEvent", __arg1.Handler);
        __builder.CloseComponent();
    }

BuildRenderTree can then use any of the following formats, without needing to know any of the types:

CreateMyComponent_0(3, new(this, x => { }));
CreateMyComponent_0(3, new(this, async x => { }));
CreateMyComponent_0(3, new(this, SomeMethodGroup));
CreateMyComponent_0(3, new(this, SomeMethodGroupAsync));
CreateMyComponent_0(3, new(this, x => x + 1));
var ec = new EventCallback<int>();
CreateMyComponent_0(3, new(this, ec));
Jinjinov commented 9 months ago

Any updates on this?