ILLinker is trimming interfaces from some public types when on "library" mode

[joperezr]

For additional context, you can look into: dotnet/runtime#57816 (comment)

We are using "library" mode to trim some of our assemblies in dotnet/runtime repo, and just found out that when a public type has an internal constructor, and this constructor doesn't get called inside the assembly, the linker seems to be trimming out non public members from that type, including explicit implementation of interfaces as well as declaration of interface implementations themselves. The more specific scenario we have in dotnet/runtime is a generated source code that looks like this:

    public sealed partial class AceEnumerator : System.Collections.IEnumerator
    {
        internal AceEnumerator() { throw new System.PlatformNotSupportedException(System.SR.PlatformNotSupported_AccessControl);  }
        public System.Security.AccessControl.GenericAce Current { get { throw new System.PlatformNotSupportedException(System.SR.PlatformNotSupported_AccessControl);  } }
        object System.Collections.IEnumerator.Current { get { throw new System.PlatformNotSupportedException(System.SR.PlatformNotSupported_AccessControl);  } }
        public bool MoveNext() { throw new System.PlatformNotSupportedException(System.SR.PlatformNotSupported_AccessControl);  }
        public void Reset() { throw new System.PlatformNotSupportedException(System.SR.PlatformNotSupported_AccessControl);  }
    }

In this case, AceEnumerator ctor is internal and because the whole source code of the assembly is generated (it all throws PlatformNotSupportedException) then nobody calls this internal constructor, so the Linker is trimming parts of the type and removing the interface from the type. The resulting type post-trimming looks like this:

After speaking about this offline with @sbomer, we reached the conclusion that ideally the linker shouldn't be removing interfaces from public types when in "library" mode, independently from whether they could be instantiated or not. Interface implementations are part of the public visible contract of a type, so in library mode they should be kept.

cc: @eerhardt @sbomer @vitek-karas

[sbomer]

@vitek-karas this is similar to the issues with DynamicallyAccessedMembers-on-type marking which also had to be fixed for library mode. Library mode shouldn't use any of the static analysis rules which that require multiple "reasons" for a thing to be marked - it should only look at the public surface of a library, and should never conditionally drop public surface that doesn't happen to be referenced in the implementation.

[marek-safar]

This sounds to me like a bug. The linker is already setup not to remove an interface from a type when trimming in library mode so it looks like some checks are missing somewhere.

[vitek-karas]

Debugged through this a little. We don't treat the type as instantiated. Which from a pure correctness perspective is correct - it has no public constructor, so it can't be instantiated using the public APIs and since the library itself doesn't instantiate it either, there's no way it can be instantiated ever.

That said - I guess in library mode linker should be more flexible and simply preserve all public aspects of a public type - interfaces are that.

The linker is already setup not to remove an interface from a type when trimming in library mode so it looks like some checks are missing somewhere.

Linker will not remove an interface in library mode if regardless if the interface is used or not, but it still requires the type to be instantiated to keep any interfaces on it.

[vitek-karas]

I'm looking at fixing this - the simplest fix seems to be to consider all public types in a library as instantiated. As already noted, for full implementation assemblies this should not be necessary, but for the ref/fake assemblies this will be needed.

Candidate fix: https://github.com/vitek-karas/linker/tree/FixLibraryInterface

[marek-safar]

@vitek-karas Would unusedinterfaces optimization fix not remove interface in this case be better?

[vitek-karas]

@marek-safar you mean instead of looking at the "library" flag, use the "unusedinterfaces" flag to condition this new behavior?

[marek-safar]

yes, I think if you use unusedinterfaces option linker should not remove unused interfaces in any mode.

[vitek-karas]

OK - I updated the implementation per the suggestion: https://github.com/mono/linker/compare/main...vitek-karas:FixLibraryInterface?expand=1

It's definitely more complicated now, but I guess it makes more sense this way.

[MichalStrehovsky]

but for the ref/fake assemblies this will be needed

If we want trimming ref assemblies to be a scenario, we'll also need to consider dotnet/runtime#16402 (TL;DR: trimming of private fields on public structs needs to be done very carefully). AFAIK we would only trim fields on structs with Auto layout, but still this would need special handling in library mode if trimming ref assemblies is a scenario we want to support.

I chatted with Eric and since we have dedicated ref assemblies that are never trimmed (i.e. Roslyn never sees any of the assemblies we trimmed in library mode), trimming fields is probably not a problem.

I think the problem with trimming fields is similar to trimming the interfaces - it's only observable through reflection at runtime, because ref assemblies shield us from causing a build break (or miscompile).

[vitek-karas]

I'm honestly not sold we should be trying to trim ref/fake assemblies at all. I don't see the point (and it breaks the tooling because the assemblies are "weird").

[ViktorHofer]

I'm honestly not sold we should be trying to trim ref/fake assemblies at all. I don't see the point (and it breaks the tooling because the assemblies are "weird").

cc @stephentoub regarding trimming PNSE assemblies (others refer to them as "fake assemblies").

[stephentoub]

I agree trimming ref assemblies is unlikely to be beneficial. After all, the goal of refs is to define exactly the metadata you want included, so there should be little to nothing to trim out (I'd be interested in knowing if anything at all gets trimmed), and the risk/reward equation doesn't pan out. Are we actually trimming refs today? I thought we weren't.

I'm not sure for PNSE assemblies. They should in theory contain the exact same metadata as the ref assemblies, just with all methods containing bodies that throw PNSE. If that's all they are and nothing is getting trimmed from them (when the linker is performing correctly), then there's little point in spending build time trimming them, especially if there's increased risk due to bugs like the one called out in this issue. So, if based purely on the yield from trimming these assemblies it's deemed not worth it, that's a fine decision to make. However, I don't want us to make such a decision based on an issue like this one, i.e. we shouldn't paper over bugs by not using the linker in the scenarios it's currently causing issues (I'm not saying that's what folks are suggesting we do, just being explicit, since the discussion is in the same thread).

As for this issue, it sounds like it's getting fixed, but even beyond reflection an interface implementation is meaningful even on types that aren't instantiated. For example, the interface can be used as a generic constraint, and removing the interface implementation will then cause potential build and runtime issues. This will be much more impactful in the face of static abstract interface methods.

[ViktorHofer]

Are we actually trimming refs today? I thought we weren't.

Nope, we don't and I agree that there is very little value in trimming them. The reference assemblies that we produce in dotnet/runtime are already the smallest possible unit to represent public API surface areas when using csc. Related: dotnet/runtime#58163 which talks about using Roslyn's RefOut feature to generate reference assemblies from source.

They should in theory contain the exact same metadata as the ref assemblies, just with all methods containing bodies that throw PNSE.

Exactly. Today, the reference source is fed into GenFacades which then invokes Roslyn to transform the method bodies and property getter / setts to throw PNSEs.

If that's all they are and nothing is getting trimmed from them (when the linker is performing correctly), then there's little point in spending build time trimming them, especially if there's increased risk due to bugs like the one called out in this issue.

from dotnet/runtime#58345 (comment), apparently the linker does trim out some KBs from PNSE assemblies:

From a size perspective, System.Security.AccessControl's reference assembly is 28KB small vs. the linked PNSE source assembly which is 36KB small vs. the unlinked PNSE source assembly which is 53KB small.

That said, from an infrastructure perspective I would prefer if we don't leverage the linker for these assemblies as the cost of invoking it likely outmatches the size gains. It would be interesting to see what the linker actually trims out that results in 17KB being dropped.