Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error in generated code with generic parameter #76

Open
marinasundstrom opened this issue Jan 14, 2023 · 8 comments
Open

Error in generated code with generic parameter #76

marinasundstrom opened this issue Jan 14, 2023 · 8 comments
Labels
enhancement New feature or request
Milestone

Comments

@marinasundstrom
Copy link

marinasundstrom commented Jan 14, 2023

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
Copy link
Author

I can provide a link to my code if necessary.

@martinothamar
Copy link
Owner

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
Copy link
Author

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

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

@martinothamar
Copy link
Owner

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
Copy link
Owner

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
Copy link

having same issue

@antunesl
Copy link

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

@martinothamar
Copy link
Owner

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 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants