IEnumerable<T> is defaulted to an empty array during DI resolution

Stabzs commented 3 years ago

Description

When resolving registered instances with IEnumerable<T> arg1 = null constructor dependencies, arg1 in unexpectedly initialized to an empty array, instead of null. This is inconsistent with other collection types such as IList<T>, List<T> or IList<T>.

This can be recreated with the following code:

public class Test : ITest
{
    private ISet<string> _defaults = new HashSet<string> { "one", "two", "three" };
    private ISet<string> _filters;

    public Test(IEnumerable<string> filters = null)
    {
        _filters = new HashSet<string>(filters.ToHashSet() ?? _defaults);
    }
}

public interface ITest { }

services.AddScoped<ITest, Test>();

in the above example, you would expect that _defaults would be used since there is no parameter registered via a factory to the concrete type. However, filters is passed as an empty array by the IoC resolve call.

This does not occur for the following example:

public class Test : ITest
{
    private ISet<string> _defaults = new HashSet<string> { "one", "two", "three" };
    private ISet<string> _filters;

    public Test(List<string> filters = null)
    {
        _filters = new HashSet<string>(filters.ToHashSet() ?? _defaults);
    }
}

public interface ITest { }

services.AddScoped<ITest, Test>();

in the second example, _defaults is used because filters is correctly passed as null.

Configuration

Which version of .NET is the code running on?
- .netcoreapp3.1 and .net5.0
What OS and version, and what distro if applicable?
- Reproduced on Windows 10, but I would the same reproduction on other OSes.
What is the architecture (x64, x86, ARM, ARM64)?
- Specifically reproduced on x64.
Do you know whether it is specific to that configuration?
- I do not know because I have not tested other configurations, but I wouldn't expect it to be limited to x64.

Regression?

This seems consistent across at least .netcore 3.1 and .net 5. I did not test further back.

Other information

This seems to be related to how the constructor/IEnumerable visitor works here:

dotnet-issue-labeler[bot] commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 3 years ago

Tagging subscribers to this area: @eerhardt, @maryamariyan See info in area-owners.md if you want to be subscribed.

Issue Details

### Description When resolving registered instances with `IEnumerable arg1 = null` constructor dependencies, `arg1` in unexpectedly initialized to an empty array, instead of null. This is inconsistent with other collection types such as `IList`, `List` or `IList`. This can be recreated with the following code: ``` public class Test : ITest { private ISet _defaults = new HashSet { "one", "two", "three" }; private ISet _filters; public Test(IEnumerable filters = null) { _filters = new HashSet(filters.ToHashSet() ?? _defaults); } } public interface ITest { } services.AddScoped(); ``` in the above example, you would expect that `_defaults` would be used since there is no parameter registered via a factory to the concrete type. However, `filters` is passed as an empty array by the IoC resolve call. This does not occur for the following example: ``` public class Test : ITest { private ISet _defaults = new HashSet { "one", "two", "three" }; private ISet _filters; public Test(List filters = null) { _filters = new HashSet(filters.ToHashSet() ?? _defaults); } } public interface ITest { } services.AddScoped(); ``` in the second example, `_defaults` is used because filters is correctly passed as null. ### Configuration * Which version of .NET is the code running on? * * .netcoreapp3.1 and .net5.0 * What OS and version, and what distro if applicable? * * Reproduced on Windows 10, but I would the same reproduction on other OSes. * What is the architecture (x64, x86, ARM, ARM64)? * * Specifically reproduced on x64. * Do you know whether it is specific to that configuration? * * I do not know because I have not tested other configurations, but I wouldn't expect it to be limited to x64. ### Regression? This seems consistent across at least .netcore 3.1 and .net 5. I did not test further back. ### Other information This seems to be related to how the constructor/IEnumerable visitor works [here](https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs#L161):

Author:	Stabzs
Assignees:	-
Labels:	`untriaged`, `area-Extensions-DependencyInjection`
Milestone:	-

davidfowl commented 3 years ago

Other collection types aren't supported at all so that's why you're seeing that behavior. I don't have a strong sense of what is the right thing to do here but this seems like a reasonable thing to expect if you explicitly ask for null instead of empty.

It's also a breaking change so I don't know if we'd do it. We can try with other DI containers as well to see what the behavior looks like there.

eerhardt commented 3 years ago

One reason for using an empty array is for performance. Take for example https://github.com/dotnet/runtime/pull/49970 where we assume that an array is always passed by DI.

Stabzs commented 3 years ago

@eerhardt that makes perfect sense from a performance perspective. And in your example, the factory is responsible for the defaulting behavior, not the DI resolver itself.

As far as other IoC containers go, SimpleInjector will not even permit this type of registration due to unregistered dependencies and would require a factory, preventing the issue in the first place.

Autofac has similar behavior of defaulting IEnumerable<T>, ICollection<T> and IList<T>, but not List<T> or ISet<T>, which throws an exception during resolution.

LightInject defaults IEnumerable<T>, ICollection<T> and IList<T>, and throws on List<T> or ISet<T>.

It seems odd that most container implementations are somewhat inconsistent on how they handle it. If nothing else, it seems like this behavior should potentially expand to other collection types and be better documented. And ideally, a container verification model would prevent this type of odd registration altogether.

dotnet / runtime