RicoSuter / Namotion.Reflection

.NET library with advanced reflection APIs.
MIT License
212 stars 44 forks source link

EnumerableItemType seems not working for Dictionary #67

Open n9 opened 3 years ago

n9 commented 3 years ago
typeof(Dictionary<string, int>).ToContextualType().EnumerableItemType

returns null.

The reason seems to be that

typeof(Dictionary<string, int>).GetRuntimeMethod("GetEnumerator", Array.Empty<Type>())
    .ReturnParameter?.ToContextualParameter().Type.ToString()

returns System.Collections.Generic.Dictionary`2+Enumerator[System.String,System.Int32].

And the current implementation looks like:

https://github.com/RicoSuter/Namotion.Reflection/blob/4279901776ed8cbcb9f2338cefd1ab5465948863/src/Namotion.Reflection/Context/ContextualType.cs#L160-L165

n9 commented 3 years ago

The following code "works", but nullability is lost:

GetInterface(typeof(IEnumerable<>).FullName).GetRuntimeMethods().Single()
   .ReturnParameter.ToContextualParameter()
jeremyVignelles commented 3 years ago

Not sure "EnumerableItemType" sould be used for that, we had a discussion about how we could detect the nullability of generic arguments in a generic way, but it turns out it's harder than expected, and I didn't find a way to make it agnostic enough...

I disliked the concept of EnumerableItemType as it looks like something specific to the NSwag case (detecting nullability for T[] and List). I'm not sure it would make sense to treat Dictionary<K, V> as a IEnumerable<KeyValuePair<K, V>>.

n9 commented 3 years ago

@jeremyVignelles Yes, as I am testing more cases, the nullability is broken somehow:

IEnumerable<KeyValuePair<int, string?>?> returns correct result.

But EnumerableItemType for IReadOnlyDictionary<int, string?> returns

KeyValuePair: NotNullable
  Int32: NotNullable
  String: NotNullable

Do you know why the nullability of string gets lost?

Btw. I understand that you dislike EnumerableItemType. There could be more generic method that would return generic arguments of generic interface for given type.

bool TryGetInterfaceGenericArguments(this ContextualType type, string genericInterfaceName, out ContextualType[] genericIntefaceArguments)

(This method could be used instead of EnumerableItemType.)

jeremyVignelles commented 3 years ago

The real design of how NRT works is really not intuitive. It's based on [Nullable] attributes that can be set on properties and types, but when it comes to generics, things are really complicated :

on Dictionary<K, V>, you can't say whether K it's nullable if you don't have the context in which it's used.

If you have public Dictionary<int, string?> Prop { get; set; }, then you know that you're having nullable string because you know how the class works.

Now imagine I have a public class MyStringDictionnary: IDictionary<string, string?>, or, to make a simple example : public class MyNullableStringCollection: IEnumerable<string?>. In those case, there is no [Nullable] attribute on the derived class, nor is there on a property of that type : the compiler know what is nullable by looking at the signature of the implementation: in our example, MyNullableStringCollection.GetEnumerator() would have a [Nullable] attribute and return a IEnumerator<string?>

In other words, if you have IMyInterface<T> that is implemented in a class, and want to see if the T is nullable, you would have to detect where the T is used in that interface, and detect the nullability at that place... It can only be on a case-by-case basis.

If you want, we can chat about it in private on gitter or discord if you have ideas or if you need more explanations.

n9 commented 3 years ago

@jeremyVignelles I am not sure about that. If I understand your statement correctly, ILSpy (https://github.com/icsharpcode/ILSpy/pull/1425) will not be able to determine nullability of the following:

interface IBar<T> { }

class Bar : IBar<string> { }
class BarN : IBar<string?> { }

But ILSpy decompiles it correctly.

(I known that is not easy problem, I have started implementing NRT for reflection, but after realizing the real size of it, I have stopped and tried to search whether someone else have already implemented it. Then, I have found https://github.com/dotnet/runtime/issues/29723, which points (back) to this library.)

jeremyVignelles commented 3 years ago

You can try it out here : There is no difference between Bar and BarN in the generated code.

Now, with this code:

#nullable enable
interface IBar<T> { 
    T GetT();
}

class Bar : IBar<string> { 
    public string GetT() => "";
}

class BarN : IBar<string?> {
    public string? GetT() => "";
}

the classes differ only on the GetT() annotations see here

Things get worse if you type BarN.GetT as a string: see here In that last case, both code generate the same thing, so you can't know what's been given as a generic parameter, just by looking at the generated code. In the case of the interface, only the type of the implementation matter, it's just a "contract" after all, and you're free to restrict it (as in "this is a valid implementation of that interface"). My guess is that NRT were not made for the disassembly/reflection use case.

n9 commented 3 years ago

"There is no difference between Bar and BarN in the generated code."

There are different flags, check the IL:

.class private auto ansi beforefieldinit Bar
    extends [System.Private.CoreLib]System.Object
    implements .custom instance void System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8[]) = (
        01 00 02 00 00 00 00 01 00 00
    )
    class IBar`1<string>
.class private auto ansi beforefieldinit BarN
    extends [System.Private.CoreLib]System.Object
    implements .custom instance void System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8[]) = (
        01 00 02 00 00 00 00 02 00 00
    )
    class IBar`1<string>

But I do not know, how to access custom attributes in the implements part via reflection.

"My guess is that NRT were not made for the disassembly/reflection use case."

I agree.

jeremyVignelles commented 3 years ago

Nullable attribute on the auto generated constructor. So yeah, we might be able to know parts of the story, but we would still need to know what we're trying to read.

Working with Nullable attributes is already done by this library, but knowing where attributes are placed and where to glue things together and how to access a value is really tricky