Delegate.CreateDelegate is not trimming safe

[MichalStrehovsky]

#45984 flipped public static Delegate? CreateDelegate(Type, Type, string, bool, bool) to being trimming safe, but from how I understand things, it isn't:

using System;

Delegate.CreateDelegate(typeof(Action), typeof(Derived), "Method").DynamicInvoke();

class Base
{
    private static void Method() => Console.WriteLine("Hello");
}

class Derived : Base { }

Linker can remove Base.Method since private methods declared on base types don't count into NonPublicMethods of the declaring type:

https://github.com/mono/linker/blob/7a095faf3925689f2a3450da1a76fdd79a070de9/src/linker/Linker.Dataflow/DynamicallyAccessedMembersBinder.cs#L125-L128

I've noticed this in NativeAOT since NativeAOT implements reflection stack in C# and annotations were not matching up.

Cc @vitek-karas @eerhardt

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF
See info in area-owners.md if you want to be subscribed.

Issue Details

---

#45984 flipped public static Delegate? CreateDelegate(Type, Type, string, bool, bool) to being trimming safe, but from how I understand things, it isn't:

using System;

Delegate.CreateDelegate(typeof(Action), typeof(Derived), "Method").DynamicInvoke();

class Base
{
    private static void Method() => Console.WriteLine("Hello");
}

class Derived : Base { }

Linker can remove Base.Method since private methods declared on base types don't count into NonPublicMethods of the declaring type:

https://github.com/mono/linker/blob/7a095faf3925689f2a3450da1a76fdd79a070de9/src/linker/Linker.Dataflow/DynamicallyAccessedMembersBinder.cs#L125-L128

I've noticed this in NativeAOT since NativeAOT implements reflection stack in C# and annotations were not matching up.

Cc @vitek-karas @eerhardt

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-System.Reflection, linkable-framework
Milestone:	-

[eerhardt]

Is this fix here to change

runtime/src/libraries/System.Private.CoreLib/src/System/Delegate.cs

Lines 52 to 53 in 5078dd6

	public static Delegate CreateDelegate(Type type, [DynamicallyAccessedMembers(AllMethods)] Type target, string method) => CreateDelegate(type, target, method, ignoreCase: false, throwOnBindFailure: true)!;
	public static Delegate CreateDelegate(Type type, [DynamicallyAccessedMembers(AllMethods)] Type target, string method, bool ignoreCase) => CreateDelegate(type, target, method, ignoreCase, throwOnBindFailure: true)!;

to

        public static Delegate CreateDelegate(Type type, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type target, string method) => CreateDelegate(type, target, method, ignoreCase: false, throwOnBindFailure: true)!;
        public static Delegate CreateDelegate(Type type, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type target, string method, bool ignoreCase) => CreateDelegate(type, target, method, ignoreCase, throwOnBindFailure: true)!;

This will preserve everything in the Type hierarchy, even Base.Method above.

[MichalStrehovsky]

I'll let Vitek speak to how All operates. In my head All should be a union of all the flags because that's how it's easy to reason about but I know linker does All differently - that behavior is accidental and undocumented as far as I'm concerned. AFAIK it keeps more than necessary because we don't rely on that definition of All in the class libraries. This would be the first spot that relies on that behavior.

I would just keep them as RequiresUnreferencedCode. People can still use the overload that takes MethodInfo. If we get feedback that it's unacceptable, we can look at our options again:

• do .All and document the way linker does it, or
• add a new MemberTypes to the enum, or
• intrinsically recognize the API in illink and make it safe if the parameters are known.

[vitek-karas]

The special All behavior is intentional - it was done in reaction to discussion in the API review. I think the main point was that we have a certain set of values on the DynamicallyAccessedMemberTypes which cover some portion of the "things which can be preserved" but not all. If we were to implement All as a simple union of everything it would need to change its behavior if/when we added more things to the enum.

The other argument was basically along the lines of this issue - that the other enum values do not cover everything (which was also intentional - we modeled them by BindingFlags both in naming and in functionality) and there was a desire to have one value which would simply preserve the whole type - no exceptions.

At least that's how I remember it - I can't find it in the API review issue though...

[eerhardt]

Given the axiom stated in https://github.com/dotnet/designs/blob/main/accepted/2020/linking-libraries.md

An application that works is preferred over a smaller application that doesn't.

My preference is to annotate the Type as All rather than marking it as RequiresUnreferencedCode. Developers don't see ILLink warnings by default today, so I'd rather be more conservative and ensure an app using this API still works. People can still use the overload that takes MethodInfo, if they decide the app is too large and this is causing size issues.

I can't find it in the API review issue though...

Here's where we discussed All on the video
https://www.youtube.com/watch?t=2900&v=1GkjoDSR7dE&feature=youtu.be