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.

[stephentoub]

It would be interesting to see what the linker actually trims out that results in 17KB being dropped.

Yes. 17k out of 53k? That's a huge percentage.

[vitek-karas]

Just to make it clear - I didn't mean that we should be hiding the linker bugs behind the changes to what gets trimmed. Just that it makes little sense to me to trim the PNSE assemblies, based on the description of what they are and how they're created.

[joperezr]

Yes. 17k out of 53k? That's a huge percentage.

I checked and most of that diff seems to be coming out from all internal System.SR static members representing resources which I guess makes sense to remove. This specific library has around 54 resources so removing all the metadata of all those internal properties from the SR class add up.

[marek-safar]

I checked and most of that diff seems to be coming out from all internal System.SR static members representing resources which I guess makes sense to remove.

It sounds like a build issue to me ;-)

[ViktorHofer]

Agree that this should be fixed on the build side.

[ViktorHofer]

We just recently noticed that this issue is affecting two libraries in runtime's main branch:

/Users/runner/work/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22159.9/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : CannotRemoveBaseTypeOrInterface : Type 'System.Security.Cryptography.CngKey' does not implement interface 'System.IDisposable' in the implementation but it does in the contract. [/Users/runner/work/1/s/src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj]
/Users/runner/work/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22159.9/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : CannotRemoveBaseTypeOrInterface : Type 'System.Net.NetworkInformation.IPAddressInformationCollection' does not implement interface 'System.Collections.Generic.ICollection<System.Net.NetworkInformation.IPAddressInformation>' in the implementation but it does in the contract. [/Users/runner/work/1/s/src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj]

ApiCompat has been comparing the contract against the unlinked implementation assembly which is why these issues weren't observed until now. dotnet/runtime#66706 fixes the sequencing to make sure that ApiCompat's input is the linked assembly.

@vitek-karas would you recommend that we wait for a linker fix as you already started looking into this or disable the linking of the two affected libraries meanwhile?

[vitek-karas]

I actually wasn't looking into this (probably dropped the ball here). I will do so now.

[ViktorHofer]

Sorry, I meant that you were looking into this in the past :)

[vitek-karas]

Did some investigation on the first one System.Net.NetworkInformation.dll. The difference exists only on Linux. The reason is basically the same as described in this issue above, even though this is not a PNSE assembly. The class in question IPAddressInformationCollection while being public has internal only .ctor. And in the Linux version of the assembly there are no callers which instantiate it. So linker removes the .ctor and also removes interfaces.

I think I agree that removing public interfaces on public classes is not a good idea (in the library mode). So we should fix this. The fix is likely going to be very similar (if not identical) to the one proposed above already.