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

Fix GetWrappedType implementation(s) to not return null #560

Merged
merged 2 commits into from
Mar 10, 2023

Conversation

Metadorius
Copy link
Contributor

@Metadorius Metadorius commented Feb 28, 2023

Motivation for this change:
I tried to resolve IContainer through third-party Splat.DryIoc dependency resolver in my app (bad practice but WinForms is a bad practice by itself, so eh). It errored with a NullReferenceException here (none of the params were generic):

DryIoc/src/DryIoc/Container.cs

Lines 5072 to 5077 in e6a4e33

var requiredItemType = container.GetWrappedType(itemType, details.RequiredServiceType);
var items = container.GetServiceRegisteredAndDynamicFactories(requiredItemType) // todo: @bug check for the unregistered values
.Map(requiredItemType, (t, x) => new ServiceRegistrationInfo(x.Value, t, x.Key));
if (requiredItemType.IsClosedGeneric())

Upon further inspection I found out that the methods shouldn't ever return null, according to documentation:

/// <returns>Unwrapped service type in case it corresponds to registered generic wrapper, or input type in all other cases.</returns>

And here:
/// Otherwise, method returns the input <paramref name="serviceType"/>.</summary>

Yet one of the branches was seemingly erratically returning null, so I fixed that.

@dadhi
Copy link
Owner

dadhi commented Feb 28, 2023

Thanks, I will look a bit later.

@Metadorius
Copy link
Contributor Author

Metadorius commented Mar 6, 2023

Any updates on that? It should be a pretty small oversight, as the documentation (and code usage) assumes the methods will never return null, and it clearly looks like an oversight that it returns null in one case when in another it returns the initial type properly. It's breaking in some of the usage scenarios of the container, particularly resolving the container through Splat which calls the method in question.

@dadhi
Copy link
Owner

dadhi commented Mar 6, 2023

I am on vacation till 8th of March, so I will look when I have time.

@dadhi dadhi self-requested a review March 10, 2023 12:44
@dadhi dadhi added this to the v5.3.4 milestone Mar 10, 2023
@dadhi
Copy link
Owner

dadhi commented Mar 10, 2023

Hi, thanks for PR, merging.

@dadhi dadhi merged commit 0e73df0 into dadhi:master Mar 10, 2023
@dadhi
Copy link
Owner

dadhi commented Mar 13, 2023

@Metadorius The v5.3.4 containing the PR is out. Thanks!

@Metadorius Metadorius deleted the fix-getwrappedtype-impl branch March 13, 2023 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants