Confusing use of Type.IsAssignableFrom in ArgumentConverter.CreateEnumerable

[KalleOlaviNiemitalo]

The ArgumentConverter.CreateEnumerable method calls Type.IsAssignableFrom here:

command-line-api/src/System.CommandLine/Binding/ArgumentConverter.DefaultValues.cs

Lines 49 to 59 in eaad234

	var x = type.GetGenericTypeDefinition() switch
	{
	{ } enumerable when typeof(IEnumerable<>).IsAssignableFrom(enumerable) =>
	CreateArray(itemType, capacity),
	{ } array when typeof(IList<>).IsAssignableFrom(array) ||
	typeof(ICollection<>).IsAssignableFrom(array) =>
	CreateArray(itemType, capacity),
	{ } list when list == typeof(List<>) =>
	CreateEmptyList(type),
	_ => null
	};

These IsAssignable calls appear to be written the wrong way around, but the code works in practice anyway.

For example, if typeof(IEnumerable<>).IsAssignableFrom(enumerable) returns true, then CreateEnumerable calls CreateArray. For that, enumerable.IsAssignableFrom(typeof(IEnumerable<>)) would seem more appropriate, because if the application defines some interface IDerivedEnumerable<out T> : IEnumerable<T> and attempts to use Argument<IDerivedEnumerable<string>>, then IEnumerable<string> is assignable from IDerivedEnumerable<string>, but IDerivedEnumerable<string> is not assignable from string[], and thus CreateEnumerable should not call CreateArray.

However, CreateEnumerable indeed does not call CreateArray in this scenario, because the target of the Type.IsAssignableFrom call is typeof(IEnumerable<>) rather than typeof(IEnumerable<string>), and that apparently causes Type.IsAssignableFrom to return false unless the types match exactly:

Microsoft (R) Roslyn C# Compiler version 2.10.0.0
Loading context from 'CSharpInteractive.rsp'.
Type "#help" for more information.
> typeof(IEnumerable<>).IsAssignableFrom(typeof(IList<>))
false
> typeof(IList<>).IsAssignableFrom(typeof(IEnumerable<>))
false
> typeof(IEnumerable<object>).IsAssignableFrom(typeof(IList<object>))
true
> typeof(IList<object>).IsAssignableFrom(typeof(IEnumerable<object>))
false
> typeof(IEnumerable<object>).IsAssignableFrom(typeof(IList<>))
false
> typeof(IEnumerable<>).IsAssignableFrom(typeof(IList<object>))
false
> typeof(System.Collections.IEnumerable).IsAssignableFrom(typeof(IList<>))
true
> typeof(IEnumerable<>).IsAssignableFrom(typeof(IEnumerable<>))
true

I think, if the comparison were changed to enumerable == typeof(IEnumerable<>), and likewise for IList<> and ICollection<>, then that would have the same result as the current comparison, but would be easier for me to understand.

[KalleOlaviNiemitalo]

I hope Type.IsAssignableFrom is not used here for some COM type-equivalence purpose.

[jonsequitur]

I hope Type.IsAssignableFrom is not used here for some COM type-equivalence purpose.

We would never! 😅

I think, if the comparison were changed to enumerable == typeof(IEnumerable<>), and likewise for IList<> and ICollection<>, then that would have the same result as the current comparison, but would be easier for me to understand.

Good find!

I looked at this code and was trying to understand why it's written this way, and refactored a bit to make it easier to understand. It turns out that the reason IsAssignableFrom looks like it's being used backwards is because the implementation doesn't need it at all. In every case, == produces the same result.

Here's an updated version of the code with test coverage visible:

[KalleOlaviNiemitalo]

type.IsArray is also true for multidimensional arrays… 😓