DynamicallyAccessedMembers should transitively apply to GetInterfaces()

[eerhardt]

If an interface Type is annotated with [DynamicallyAccessedMembers(All)], then any Types returned from interfaceType.GetInterfaces() should also assume to be annotated with the same member types.

Repro

Trimming the following application:

using System;
using System.Diagnostics.CodeAnalysis;

namespace ConsoleApp2
{
    interface IMyInterface : IFormattable
    {
        void M1();
    }

    class Program
    {
        static void Main(string[] args)
        {
            GenerateProxyType(typeof(IMyInterface));
        }

        private static void GenerateProxyType(
            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type interfaceType)
        {
            foreach (Type t in interfaceType.GetInterfaces())
                AddInterfaceImpl(t);  // <--- Warning happens here

            AddInterfaceImpl(interfaceType);
        }

        static void AddInterfaceImpl([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type iface)
        {
        }
    }
}

Produces this warning:

Program.cs(22,17): Trim analysis warning IL2062: ConsoleApp2.Program.GenerateProxyType(Type): Value passed to parameter 'iface' of method 'ConsoleApp2.Program.AddInterfaceImpl(Type)' can not be statically determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements.

Note, for some reason, when the ILLinker runs as part of the shared framework build, I am getting a different warning, I'm not sure if I'm using a different version of the ILLinker or not, but here is the code in DispatchProxy:

        internal void AddInterfaceImpl([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type iface)
        {
            ...
        }

        private static Type GenerateProxyType(
            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type baseType,
            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type interfaceType)
        {
            // Parameter validation is deferred until the point we need to create the proxy.
            // This prevents unnecessary overhead revalidating cached proxy types.

            // The interface type must be an interface, not a class
            if (!interfaceType.IsInterface)
            {
                // "T" is the generic parameter seen via the public contract
                throw new ArgumentException(SR.Format(SR.InterfaceType_Must_Be_Interface, interfaceType.FullName), "T");
            }

            // The base type cannot be sealed because the proxy needs to subclass it.
            if (baseType.IsSealed)
            {
                // "TProxy" is the generic parameter seen via the public contract
                throw new ArgumentException(SR.Format(SR.BaseType_Cannot_Be_Sealed, baseType.FullName), "TProxy");
            }

            // The base type cannot be abstract
            if (baseType.IsAbstract)
            {
                throw new ArgumentException(SR.Format(SR.BaseType_Cannot_Be_Abstract, baseType.FullName), "TProxy");
            }

            // The base type must have a public default ctor
            if (baseType.GetConstructor(Type.EmptyTypes) == null)
            {
                throw new ArgumentException(SR.Format(SR.BaseType_Must_Have_Default_Ctor, baseType.FullName), "TProxy");
            }

            // Create a type that derives from 'baseType' provided by caller
            ProxyBuilder pb = s_proxyAssembly.CreateProxy("generatedProxy", baseType);

            foreach (Type t in interfaceType.GetInterfaces())
                pb.AddInterfaceImpl(t);  // <--- Warning happens here

            pb.AddInterfaceImpl(interfaceType);

            Type generatedProxyType = pb.CreateType();
            return generatedProxyType;
        }

and the warning:

DispatchProxyGenerator.cs(139,17): Trim analysis warning IL2072: System.Reflection.DispatchProxyGenerator.GenerateProxyType(Type,Type): 'iface' argument does not satisfy 'All' in call to 'System.Reflection.DispatchProxyGenerator.ProxyBuilder.AddInterfaceImpl(Type)'. The return value of method 'System.Collections.Generic.IEnumerator<T>.Current.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

cc @vitek-karas @MichalStrehovsky

[MichalStrehovsky]

Linker doesn't ensure this.

interface IFoo
{
    public static void Hello() => Console.WriteLine("Hello");

    public static int Field => 42;
}

interface IBar : IFoo
{
}

class Bar : IBar
{
}

Should DynamicallyAccessedMembers(All) on Bar really apply to the static methods and fields on IFoo?

Linker will also remove unused interface methods from the interface itself. So e.g. if an interface method got added to IBar with a default implementation and Bar doesn't implement it, it's fair game to remove it.

To make this safe, we would need to make linker keep more stuff than necessary.

[vitek-karas]

Is it actually needed to fully preserve these for the DispatchProxy scenario? The logic being - if a given interface member is never used anywhere in the app then is it actually necessary to generate a proxy member for it (it should never be used either)?

[eerhardt]

This same issue came up in System.ComponentModel.TypeConverter:

https://github.com/dotnet/runtime/blob/7ab65423373f04998f3d365d564b3fc0e55fdf2b/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs#L92-L106

I annotated _type to be [DynamicallyAccessedMembers(All)]. But ILLink warns in this case. TypeDescriptor.GetAttributes(Type) is annotated as [DynamicallyAccessedMembers(All)] on the Type parameter.

So now we have 2 cases where DynamicallyAccessedMembers(All) should mean "preserve all members of any implemented interfaces on this Type".

[MichalStrehovsky]

I think it should be safe to do that now - we just an intrinsic for this - MarkEntireType does mark all members on implemented interfaces. Back in January I was still hoping we could scale back what .All means to a saner set, but it basically means "all members, nested types and their members, base types and their members, interfaces and their members, and their attributes, recursively" and we decided that this is what we want for All to mean.

Linker currently doesn't model arrays of types though so that's a prerequisite (we should probably restrict this to intrinsics instead of generalizing it to allowing void Foo([DynamicallyAccessed(All)] Type[] mytypes) because that brings complexities).

[vitek-karas]

The case which @eerhardt points to would not need the array handling - it only accessed attributes on the interfaces, so there's no need to propagate the annotation to the interface types for this case.

[MichalStrehovsky]

ould not need the array handling

Maybe I'm missing something:

                    // `_type` annotated as `.All`
                    Type[] interfaces = _type.GetInterfaces();
                    for (int idx = 0; idx < interfaces.Length; idx++)
                    {
                        Type iface = interfaces[idx];

                         // <snip />

                        // Warns because `iface` doesn't meet `.All`
                        attributes.AddRange(TypeDescriptor.GetAttributes(iface).Attributes);
                    }

Iface is the result of a ldelem on the returned array so in order for the linker to understand that, it needs to propagate annotation from the array to the result of ldelem.

[vitek-karas]

Well the iface is only passed to GetAttributes which probably doesn't have any annotations and thus has no requirements on the passed in value. So technically if iface is tracked as "unknown" in the linker, it would still "Work".

[MichalStrehovsky]

Ah, I think you missed what Eric wrote above: "TypeDescriptor.GetAttributes(Type) is annotated as [DynamicallyAccessedMembers(All)] on the Type parameter"

[vitek-karas]

Right - my bad :-(

[eerhardt]

But I can suppress the ILLink warning for now, right? Until this bug is addressed in the linker?

[vitek-karas]

Yes - I think it can be suppressed - All will mark all interfaces and all members on the interfaces.