-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Adding support for constrained open generics to DI #39540
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
Conversation
|
Tagging subscribers to this area: @eerhardt, @maryamariyan |
src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs
Show resolved
Hide resolved
|
Regarding test failures refer to the list here: https://dev.azure.com/dnceng/public/_build/results?buildId=735898&view=ms.vss-test-web.build-test-results-tab |
|
@maryamariyan fixed! |
|
@maryamariyan Anything else needed from my end? |
@jbogard the CI checks are green. Let's wait on @davidfowl 's approval on this. |
|
bump |
|
Reviewing this now |
… the generic type fails other than constraint failure
| { | ||
| closedType = descriptor.ImplementationType.MakeGenericType(serviceType.GenericTypeArguments); | ||
| } | ||
| catch (ArgumentException) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exception happens once per service type right? Will get try to create the service with the wrong generic arguments more than once per service type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly once per closed service type. Once it's correctly resolved the CallSite (well, IEnumerableCallSite), that result is cached like any other created CallSite.
davidfowl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, lets improve this in the future by solving #28033
|
Hello @maryamariyan! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Backed-out pull request: #34393
Previous pull request: dotnet/extensions#536
Original discussion: aspnet/DependencyInjection#471
Original pull request: aspnet/DependencyInjection#635
First naive pull request: aspnet/DependencyInjection#472
3rd4th time's the charm?All of the DI packages now support all of the specification tests added.