Repository navigation
Confusing use of Type.IsAssignableFrom in ArgumentConverter.CreateEnumerable #1768
Description
The ArgumentConverter.CreateEnumerable method calls Type.IsAssignableFrom here:
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.
Activity
KalleOlaviNiemitalo commentedon Jun 20, 2022
I hope Type.IsAssignableFrom is not used here for some COM type-equivalence purpose.
jonsequitur commentedon Jun 21, 2022
We would never! 😅
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
IsAssignableFromlooks 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 commentedon Jun 21, 2022
type.IsArrayis also true for multidimensional arrays… 😓fix dotnet#1768
fix dotnet#1768