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

EnumerableItemType seems not working for Dictionary #67

Open
n9 opened this issue Mar 19, 2021 · 8 comments
Open

EnumerableItemType seems not working for Dictionary #67

n9 opened this issue Mar 19, 2021 · 8 comments

Comments

@n9
Copy link

n9 commented Mar 19, 2021

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:

var returnParam = getEnumeratorMethod.ReturnParameter?.ToContextualParameter();
if (returnParam?.GenericArguments.Length == 1)
{
_enumerableItemType = returnParam.GenericArguments[0];
return _enumerableItemType;
}

@n9
Copy link
Author

n9 commented Mar 19, 2021

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

GetInterface(typeof(IEnumerable<>).FullName).GetRuntimeMethods().Single()
   .ReturnParameter.ToContextualParameter()

@jeremyVignelles
Copy link
Contributor

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
Copy link
Author

n9 commented Mar 19, 2021

@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
Copy link
Contributor

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
Copy link
Author

n9 commented Mar 19, 2021

@jeremyVignelles I am not sure about that. If I understand your statement correctly, ILSpy (icsharpcode/ILSpy#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 dotnet/runtime#29723, which points (back) to this library.)

@jeremyVignelles
Copy link
Contributor

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
Copy link
Author

n9 commented Mar 19, 2021

"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
Copy link
Contributor

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

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

No branches or pull requests

2 participants