dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.56k stars 4.54k forks source link

AOT/analyzer should not warn for MakeGenericType of reference type #97408

Open sbomer opened 5 months ago

sbomer commented 5 months ago
void Foo(Type t) {
    if (!t.IsValueType)
        typeof(G<>).MakeGenericType(t); // currently warns with IL3050
}

void Bar([ReferenceType] Type t) { typeof(G<>).MakeGenericType(t); }



(The latter idea is something that has come up in discussion with @vitek-karas and @MichalStrehovsky, not sure if there's another tracking issue for it already.)

Context: https://github.com/dotnet/maui/pull/20058/files/7f4d26ca0aee7dc9d995e9776573125b35ad44b6#r1463169163
ghost commented 5 months ago

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

Issue Details
`MakeGenericType` over reference types is supported in native AOT, so the following should not warn: ```csharp void Foo(Type t) { if (!t.IsValueType) typeof(G<>).MakeGenericType(t); // currently warns with IL3050 } ``` Probably the `class` constraint should be supported as well: ```csharp void Foo() where T : class { typeof(G<>).MakeGenericType(typeof(T)); } ``` We may also want to introduce an annotation to propagate the fact that a type is a reference type across methods, for example: ```csharp void Foo(Type t) { if (!t.IsValueType) Bar(t); } void Bar([ReferenceType] Type t) { typeof(G<>).MakeGenericType(t); } ``` (The latter idea is something that has come up in discussion with @vitek-karas and @MichalStrehovsky, not sure if there's another tracking issue for it already.) Context: https://github.com/dotnet/maui/pull/20058/files/7f4d26ca0aee7dc9d995e9776573125b35ad44b6#r1463169163
Author: sbomer
Assignees: -
Labels: `untriaged`, `area-NativeAOT-coreclr`, `needs-area-label`
Milestone: -
MichalPetryka commented 5 months ago

Does NativeAOT always generate a shared generic impl for this or would it need to scan for MakeGenericType and generate it then?

sbomer commented 5 months ago

Based on some experimentation, it looks like it doesn't always generate the shared impl. I'm not sure exactly what is enough to generate the shared impl. Just calling G<SomeOtherReferenceType>.Method() won't do it - but accessing some of the generic type's methods via reflection will. For example MakeGenericType fails here:

typeof(G<>).MakeGenericType(typeof(C));
// typeof(G<>).GetMethod("Foo");

class G<T> {
    public static void Foo() {
        Console.WriteLine(typeof(T));
    }
}

class C {}

but succeeds if you uncomment the GetMethod("Foo").

@MichalStrehovsky could probably give a better answer.

MichalStrehovsky commented 5 months ago

Based on some experimentation, it looks like it doesn't always generate the shared impl. I'm not sure exactly what is enough to generate the shared impl. Just calling G<SomeOtherReferenceType>.Method() won't do it - but accessing some of the generic type's methods via reflection will. For example MakeGenericType fails here:

typeof(G<>).MakeGenericType(typeof(C));
// typeof(G<>).GetMethod("Foo");

class G<T> {
    public static void Foo() {
        Console.WriteLine(typeof(T));
    }
}

class C {}

but succeeds if you uncomment the GetMethod("Foo").

@MichalStrehovsky could probably give a better answer.

Looks like we don't treat MakeGenericType specially in dataflow analysis - the only special handling is to trigger the AOT warning here:

https://github.com/dotnet/runtime/blob/135fec006e727a31763271984cd712f1659ccbd3/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs#L376-L382

It works if a method/constructor/field is subsequently accessed because dataflow will report "G<>.Foo is reflection accessed" and then the compiler will try to make G<>.Foo work on at least something.

It shouldn't be hard to make this work for an unused type.