We need to make sure reflection-accessing reflection surface area results in a trimming warning. Invoking reflection APIs with parameters that illink didn't see is not safe.

Cc @vitek-karas I filed dotnet/linker#1764.

Good call.

In this case, the usage is "safe" since the type arguments all come from the user's code. So any requirements on the types will be seen by the linker there.

Here was the original suppression:

Sigh, maybe DispatchProxy is not trimming safe after all.

interface IFoo
{
    [return: DynamicallyAccessedMembers(PublicConstructors)]
    Type GetTheType();
}

IFoo someFoo = GetItFromDispatchProxy();
Activator.CreateInstance(someFoo.GetTheType());

If we make a DispatchProxy for this, who is going to enforce that the Type returned from the implementation of GetTheType matches the requirements?

There was a discussion whether we need to mark the interface methods as reflected up here: #46715 (comment)

I think we have to. And if we do that and once dotnet/linker#1764 is fixed, this will generate a warning because we in fact cannot statically enforce that this will hold.

Sigh, maybe DispatchProxy is not trimming safe after all.

I meant to say "not trimming safe for all possible interfaces". This would only warn if the interface has the DynamicallyAccessed annotations.

We will probably have to take this into account - see dotnet/linker#1607 (comment). Originally I though that return value annotations are not affected, since they apply to the implementation not the caller, but with TypeBuilder I can provide implementation to annotated method without linker seeing it that way.

@MichalStrehovsky that would mean that we need to mark the interface type in the proxy with All as well - so that linker would "mark as reflection" all of the interfaces and all the members on them. With the fix in dotnet/linker#1764 that would lead to generating a warning in the above case.
If we don't mark it as All, we would not be able to catch this. Ideally we would need some annotation like "consider all used methods on this type/interface as accessed via reflection" - but that's way too specific annotation to create. So All is probably the best for now.

So to answer @eerhardt question:

• We need an issue tracking that this scenario is actually property handled (all up) - not sure if that should live in runtime or linker
• Annotate the interface type in dispatch proxy with All - I know we originally reverted that, but given the above, I don't see a better way currently. It's probably not a big size concern anyway, since it will only increase the size of the proxy itself, which is typically very small (each method's implementation is pretty trivial).

Annotate the interface type in dispatch proxy with All

This is the part I don't understand. If a method on an interface is marked as DynamicallyAccessed and isn't being used, why would we need to warn about it? In the scenario above, GetTheType() is called. So I guess I don't understand how annotating the interface as All should change anything here.

Annotating it as All will trigger a warning at the callsite to DispatchProxy's Create once dotnet/linker#1764 is addressed and the interface has any methods that have annotations. The purpose is just to trigger the warning when creating a DispatchProxy for an annotated interface. It's not about preserving stuff.

If it's not annotated and we're suppressing the warning within the method body there's no place that would warn in such situation.

Linker needs to see that the interface method is potentially reflection-accessed.

Basically, the problem is that there's a suppression on the call to TypeBuilder.AddInterfaceImplementation within the DispatchProxy code. This will never be safe to suppress if the interface is user-provided.

Thanks a lot @MichalStrehovsky for the explanation. Another way to look at it is: Each annotation has two sides to it. The code which assigns value to "it" and the code which uses the value from "it". Linker needs to validate both sides. In the case of an annotation on a return value, the code which assigns the value is the method's body, and the code which uses the value is the caller. In this case with dispatch proxy and the interface method, the caller is known (as you mention there will be call to this method somewhere in the app's code), but the method's body is not known to linker - it will be created at runtime via TypeBuilder. So linker can only validate one side - which is not enough.

Here's a sample I used to wrap my head around this:

using System;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;

IFooer f = DispatchProxy.Create<IFooer, MyFooer>();
Activator.CreateInstance(f.MakeFoo());

class MyFooer : DispatchProxy
{
    protected override object Invoke(MethodInfo targetMethod, object[] args)
    {
        return typeof(Unused);
    }
}

class Unused
{
}

interface IFooer
{
    [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
    Type MakeFoo();
}

Linker will not keep the ctor of Unused but there's also no warning. So I was thinking "what's the spot that should have warned". The spot was the TypeBuilder.AddInterfaceImplementation that we suppressed - linker needs to see that we reflection-access the interface methods. Propagating that annotation out to the public API surface will make sure there is a warning for this case (once the above mentioned bug is fixed), because indeed illink cannot prove that this will work.

Suppressions need a lot of scrutiny :(.