Make all of the trimmer aware of recursive interfaces

[jtschuster]

Renames InterfaceImplementor to RuntimeInterfaceImplementation and adds additional information such as the inflated interface type, which type has the first interfaceImpl, and the chain of interfaceImpls.

Uses RuntimeInterfaceImplementation in places that previously used InterfaceImplementation or InterfaceImplementation[]. The type can be used for marking an interface implementation and determining if it ever could implement an interface, but it should not be used to determine if an interface implementation "chain" is marked, since there may be many different 'chains' that end with the same interface type. For this, MarkStep.IsInterfaceImplementationMarkedRecursively.

Checks base types for additional interfaces that a type will implement at runtime.

Closes #98536

[jtschuster]

Now that the linker recognizes interfaces on all base types and recursive interfaces, these methods are recognized as overrides of IReflect, which are marked with DAM. There are already suppressions for other missing DAM annotations, so I decided to just suppress the warnings.

[sbomer]

These are warning due to interfaces declared on the base type (or recursively required by interfaces declared on the base type), right? For example:

using System;
using System.Diagnostics.CodeAnalysis;
_ = 0;
class Base : I {
    public virtual void M() {} // warning IL2046
}
class Derived : Base {
    public override void M() {} // is this change introducing a warning here?
}
interface I {
    [RequiresUnreferencedCode("RUC")]
    void M();
}

I think this example should only warn on the Base virtual method. If Derived also had an interfaceimpl of I, then I would expect to see a warning on Derived.M as well.

[jtschuster]

Yeah, that makes sense to me. But them should we warn if both have an interfaceimpl of I but Derived doesn't override M()? I lean towards yes in this case.

class Base : I
{
    public virtual void M() {} // warning IL2046
}

class Derived : Base, I // Warning IL2046 pointing to Base.M()?
{ }

interface I
{
    [RequiresUnreferencedCode("RUC")]
    void M();
}

Also, in IL it is valid for an abstract Base to implement I but provide no methods. I assume we should still warn on Derived.M() in this case.

abstract class Base : I
{
}

class Derived : Base
{
    public virtual void M() {} // warning IL2046
}

interface I
{
    [RequiresUnreferencedCode("RUC")]
    void M();
}

[sbomer]

But them should we warn if both have an interfaceimpl of I but Derived doesn't override M()? I lean towards yes in this case.

Agreed - in general I think a good model to follow is the compiler error behavior when types don't implement interface methods due to mismatching signatures.

Your second example is interesting, thanks for pointing that out. I agree that one should warn on Derived.M, but it kind of breaks the way I've been thinking about this. I wanted the rule to be roughly:
"When a type requires an interface (via explicit or recursive .interfaceimpl), there should be a warning on any mismatching annotations for the method implementing a method of that interface - pointing to either a method on the type, or the type (if the implementing method isn't on the type itself)."

Now I think we need to add something like:
"When a type requires an interface via an explicit/recursive .interfaceimpl on an abstract (maybe recursive) base class, and none of the base classes provides an implementation of one of the interface methods...".

I think it would be fine to implement the simpler rule in this PR and make a separate change to deal with the abstract case.

[jtschuster]

Implemented the simple warning behavior and filed #106552

[sbomer]

It looks like there are a couple places still using non-recursive interfaces in MarkStep, for example in MarkEntireType.

[jtschuster]

It looks like there are a couple places still using non-recursive interfaces in MarkStep, for example in MarkEntireType.

I only see MarkEntireType called for descriptor file references and from MarkEntireAssembly. I wasn't sure if that aligns with what we want "rooting an entire type" to mean. I'd expect that to only mark all the metadata within the type itself, not anything on base types or interfaces.

There are a couple other places, like DynamicallyAccessedMembersBinder, that could make use of this. I'll do those in a follow up.

[jtschuster]

Given the diff and complexity, this should wait to be merged until after the 9.0 snap.

[jtschuster]

Here's the diff of the runtime interfaces algorithm between 4fa8ec5 and 01e4518. It should only have renamed variables and inlined helpers.

-ImmutableArray<RuntimeInterfaceImplementation> GetRecursiveInterfaceImplementations (TypeDefinition originalType)
+public ImmutableArray<RuntimeInterfaceImplementation> GetRuntimeInterfaceImplementations (TypeDefinition originalType)
 {
        if (_runtimeInterfaceImpls.TryGetValue (originalType, out var runtimeIfaces)) {
                return runtimeIfaces;
        }

-       Dictionary<TypeReference, List<InterfaceImplementationChain>> interfaceTypeToImplChainMap = new (new TypeReferenceEqualityComparer (context));
+       Dictionary<TypeReference, List<InterfaceImplementationChain>> interfaceTypeToImplChainMap = new (new TypeReferenceEqualityComparer (_context));

        foreach (var explicitIface in originalType.Interfaces) {
-               // Add runtimeIface for this Iface
-               var resolvedInterfaceType = context.TryResolve (explicitIface.InterfaceType);
                interfaceTypeToImplChainMap.AddToList (explicitIface.InterfaceType, new InterfaceImplementationChain (originalType, [explicitIface]));

+               var resolvedInterfaceType = _context.TryResolve (explicitIface.InterfaceType);
                if (resolvedInterfaceType is null) {
                        continue;
                }

-               var recursiveIFaces = GetRecursiveInterfaceImplementations (resolvedInterfaceType);
-
-               foreach (var recursiveIface in recursiveIFaces) {
-                       var impls = recursiveIface.CreateImplementationChainForImplementingType (originalType, explicitIface, context);
-                       foreach (var impl in impls) {
-                               interfaceTypeToImplChainMap.AddToList (impl.InterfaceType, impl.Chain);
+               // Add the recursive interfaces for each explicit interface, prepending the explicit interface on `originalType` to the chain
+               var recursiveIFaces = GetRuntimeInterfaceImplementations (resolvedInterfaceType);
+               foreach (var runtimeImpl in recursiveIFaces) {
+                       // Inflate the interface type with the explicit interfaceImpl reference
+                       var inflatedInterfaceType = runtimeImpl.InflatedInterfaceType.InflateFrom (explicitIface.InterfaceType);
+                       foreach (var existingImpl in runtimeImpl.InterfaceImplementationChains) {
+                               interfaceTypeToImplChainMap.AddToList (inflatedInterfaceType, new InterfaceImplementationChain (originalType, existingImpl.InterfaceImplementations.Insert (0, explicitIface)));
                        }
                }
        }

-       if (originalType.BaseType is not null && context.TryResolve (originalType.BaseType) is { } baseTypeDef) {
-               var baseTypeIfaces = GetRecursiveInterfaceImplementations (baseTypeDef);
-               foreach (var recursiveIface in baseTypeIfaces) {
-                       var impls = recursiveIface.CreateImplementationChainsForDerivedType (originalType.BaseType, context);
-                       foreach (var impl in impls) {
-                               interfaceTypeToImplChainMap.AddToList (impl.InterfaceType, impl.Chain);
+       if (originalType.BaseType is not null && _context.TryResolve (originalType.BaseType) is { } baseTypeDef) {
+               var baseTypeIfaces = GetRuntimeInterfaceImplementations (baseTypeDef);
+               foreach (var runtimeImpl in baseTypeIfaces) {
+                       // Inflate the interface type with the base type reference
+                       var inflatedInterfaceType = runtimeImpl.InflatedInterfaceType.InflateFrom (originalType.BaseType);
+                       foreach (var impl in runtimeImpl.InterfaceImplementationChains) {
+                               // Inflate the provider for the first .impl - this could be a different recursive base type for each chain
+                               var inflatedImplProvider = impl.TypeWithInterfaceImplementation.InflateFrom (originalType.BaseType);
+                               interfaceTypeToImplChainMap.AddToList (inflatedInterfaceType, new InterfaceImplementationChain (inflatedImplProvider, impl.InterfaceImplementations));
                        }
                }
        }
@@ -38,5 +41,13 @@ ImmutableArray<RuntimeInterfaceImplementation> GetRecursiveInterfaceImplementati
        if (interfaceTypeToImplChainMap.Count == 0)
                return ImmutableArray<RuntimeInterfaceImplementation>.Empty;

-       return interfaceTypeToImplChainMap.Select (kvp => new RuntimeInterfaceImplementation (originalType, kvp.Key, context.TryResolve (kvp.Key), kvp.Value)).ToImmutableArray ();
-}
+       // Build the ImmutableArray and cache it
+       ImmutableArray<RuntimeInterfaceImplementation>.Builder builder = ImmutableArray.CreateBuilder<RuntimeInterfaceImplementation> (interfaceTypeToImplChainMap.Count);
+       foreach (var kvp in interfaceTypeToImplChainMap) {
+               builder.Add (new (originalType, kvp.Key, _context.TryResolve (kvp.Key), kvp.Value));
+       }
+       var runtimeInterfaces = builder.MoveToImmutable ();
+       _runtimeInterfaceImpls[originalType] = runtimeInterfaces;
+
+       return runtimeInterfaces;
+}

Removed helpers for reference:

internal sealed class RuntimeInterfaceImplementation
{

-    /// <summary>
-    /// Returns a list of InterfaceImplementationChains for a derived type of <see cref="Implementor"/>.
-    /// </summary>
-    public IEnumerable<(TypeReference InterfaceType, InterfaceImplementationChain Chain)> CreateImplementationChainsForDerivedType (TypeReference baseTypeRef, ITryResolveMetadata context)
-    {
-    	// This is only valid for classes
-    	Debug.Assert (Implementor.IsClass);
-    	Debug.Assert (Implementor == context.TryResolve (baseTypeRef));
-    
-    	var inflatedInterfaceType = InflatedInterfaceType.TryInflateFrom (baseTypeRef, context);
-    	Debug.Assert (inflatedInterfaceType is not null);
-    
-    	foreach (var impl in InterfaceImplementationChains) {
-    		var inflatedImplProvider = impl.TypeWithInterfaceImplementation.TryInflateFrom (baseTypeRef, context);
-    		Debug.Assert (inflatedImplProvider is not null);
-    
-    		yield return (inflatedInterfaceType, new InterfaceImplementationChain (inflatedImplProvider, impl.InterfaceImplementations));
-    	}
-    }
-    
-    /// <summary>
-    /// Returns a list of InterfaceImplementationChains for a type that has an explicit implementation of <see cref="Implementor"/>.
-    /// </summary>
-    public IEnumerable<(TypeReference InterfaceType, InterfaceImplementationChain Chain)> CreateImplementationChainForImplementingType (TypeDefinition typeThatImplementsImplementor, InterfaceImplementation impl, ITryResolveMetadata context)
-    {
-    	Debug.Assert (Implementor.IsInterface);
-    	Debug.Assert (typeThatImplementsImplementor.Interfaces.Contains (impl));
-    	Debug.Assert (context.TryResolve (impl.InterfaceType) == Implementor);
-    
-    	var inflatedInterfaceType = InflatedInterfaceType.TryInflateFrom (impl.InterfaceType, context);
-    	Debug.Assert (inflatedInterfaceType is not null);
-    
-    	foreach (var existingImpl in InterfaceImplementationChains) {
-    		yield return (inflatedInterfaceType, new InterfaceImplementationChain (typeThatImplementsImplementor, existingImpl.InterfaceImplementations.Insert (0, impl)));
-    	}
-    }

[jtschuster]

It looks like this is exacerbating the issues caused by the trimmer's interface method resolution (dotnet/linker#1187). That issue should be fixed before this is finished and merged.

[sbomer]

I see warnings like the following:

ILLink(0,0): error IL2046: (NETCORE_ENGINEERING_TELEMETRY=Build) System.Data.TypedTableBase: Member 'System.Data.DataTable.GetSchema()' with 'RequiresUnreferencedCodeAttribute' implements interface member 'System.Xml.Serialization.IXmlSerializable.GetSchema()' without 'RequiresUnreferencedCodeAttribute'. 'RequiresUnreferencedCodeAttribute' annotations must match across all interface implementations or overrides.

which is supposed to be suppressed here:

runtime/src/libraries/System.Data.Common/src/System/Data/DataTable.cs

Lines 6672 to 6673 in 380898a

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2046:UnrecognizedReflectionPattern",
	Justification = "https://github.com/mono/linker/issues/1187 Trimmer thinks this implements IXmlSerializable.GetSchema() and warns about not matching attributes.")]

It would be good to understand why the suppression isn't working.

[jtschuster]

It looks like this is the reason for the IL2046 warning not being suppressed.

class Base : IFoo
{
  [Ruc("Message")]
  [UnconditionalSuppressMessage("IL2046")]
  void M() { } // Suppressed IL2046
}

class Derived : Base, IFoo // Another 2046 (unsuppressed)
{
}

interface IFoo
{
  void M();
}

Since BaseType provides the interface implementation method for Derived, we decided to put the warning method on the type that implements the interface. I think that makes sense for if Derived implements IFoo and Base doesn't, but if both do (and both implementations are marked), it might make sense to report the warning for Derived on the implementation method, or not at all (assuming it would warn when analyzing BaseType).

[sbomer]

I see, so it sounds like it's due to a combination of this PR and #104753. I think it's fine to produce another warning on the derived type in that case - it's similar to the compiler error behavior.

I think we should just add the same suppressions to the derived types, citing dotnet/linker#1187, no need to block this PR on that issue.