martinothamar / Mediator

A high performance implementation of Mediator pattern in .NET using source generators.
MIT License
2.11k stars 70 forks source link

Error in generated code with generic parameter #76

Open marinasundstrom opened 1 year ago

marinasundstrom commented 1 year ago

I just attempted at moving over to this library but encountered some strange stuff in generated code.

I have decorated my NotificationHandlers with IDomainEventHandler, implementing INotificationHandler, taking generic parameters with TDomainEventbeing constrained to DomainEvent (implements INotification)

... Mediator.SourceGenerator.Roslyn40/Mediator.SourceGenerator.IncrementalMediatorGenerator/Mediator.g.cs(1214,74,1214,86): 
error CS0246: The type or namespace name 'TDomainEvent' could not be found (are you missing a using directive or an assembly reference?)
public interface IDomainEventHandler<TDomainEvent>
    : INotificationHandler<TDomainEvent>
    where TDomainEvent : DomainEvent
{

}
services.AddMediator(options =>
{
    options.ServiceLifetime = ServiceLifetime.Scoped;
});
marinasundstrom commented 1 year ago

I can provide a link to my code if necessary.

martinothamar commented 1 year ago

Hi, thanks for the report! A link to the code would definitely help. The handlers themselves should work, but I think there might be some issues if the notifications/events themselves are generic

marinasundstrom commented 1 year ago

Exactly my though, @martinothamar. It doesn't handle those correctly.

Here's the branch: https://github.com/marinasundstrom/todo-app/tree/Mediator

martinothamar commented 1 year ago

Thanks! I'm creating a sample and a test for this, and am looking into supporting generic messages type across the different abstractions, I don't remember if there were any difficult problems here, but will post back here when I find out :)

martinothamar commented 1 year ago

Yeah this will be a bit tricky unfortunately. Lots of the generated code relies on knowing/checking for the concrete type, which in this case necessitates a generic context where your generic type parameter exists. One of the reasons for this is that I don't want to rely on reflection at all, and when it comes to generic messages, I'm having a hard time coming up with a solutions that doesnt use reflection along with Type.MakeGenericType (which potentially generates code, making it not viable for AOT scenarios). Another reason to avoid reflection is performance, I would quickly end up in the same place as MediatR performance wise with the solutions I came up with investigating this.

One path could be to enforce generic costraints for generic messages, so that we could generate code that accounted for any reachable type matching the generic constraints of a message, then everything could still be "monomorphized" and performant but could potentially generate a lot of code depending on how strict the rules around generic constraints would be (there is a reason the JIT doesnt monomorphize everything by default).

Or we could accept that it's hard to be AoT friendly and super performant with generic messages and just live with that - users that have specific requirements around AoT or performence could just not use generic messages.

One thing I can mention is that I am planning a bit of a refactoring of the generated mediator implementation to allow for better performance in large projects, and I would like to consider generic messages as part of that refactoring so that it is hopefully easier to add on later. I think that would be the best use of my time currently since this appears to be tricky to implement.

WasimAhmad commented 1 year ago

having same issue

antunesl commented 1 year ago

I'm having the exact same problem. Is there already a plan to address (or not) this use case of generic notifications?

martinothamar commented 1 year ago

This is still my plan:

One thing I can mention is that I am planning a bit of a refactoring of the generated mediator implementation to allow for better performance in large projects, and I would like to consider generic messages as part of that refactoring so that it is hopefully easier to add on later. I think that would be the best use of my time currently since this appears to be tricky to implement.

One possible solution would be to accept a perf hit (essentially have a slower path for generic messages spesifically). Could end up with reflection + ConcurrentDictionary<,> which is how MediatR works last I checked. I for sure want to investigate faster alternatives, but that will have to be done in the context of the planned refactoring mentioned above. I also need to spend some time with all the improvements that have been made around AoT in .NET to understand what the constraints are, and how other libraries dealing with runtime reflection adapt to running on NativeAOT - for example ASP.NET Core runs in NativeAOT now in the latest NET 8 preview.

I don't have a ton of time these days, but that will change soon. If anyone else feel like helping out and contributing then thats also a possibility 😃