razor icon indicating copy to clipboard operation
razor copied to clipboard

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

Open Jcparkyn opened this issue 3 years ago • 2 comments

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 avatar Oct 28 '22 02:10 Jcparkyn

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:

  • The value used for type inference (1 in the example above) would need to be stored in a local variable, to avoid evaluating it twice.
  • CreateInferred would need additional overloads to handle e.g. passing an already-constructed EventCallback<T>
  • This would only work if at least one of the component parameters is a plain T (as opposed to e.g., Func<T>).

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));

Jcparkyn avatar Oct 28 '22 03:10 Jcparkyn

Any updates on this?

Jinjinov avatar Feb 18 '24 18:02 Jinjinov