Library trimming steps should support feature switch substitutions

[agocke]

Our recommended way to get all warnings from a library is to use a sample app that references the library with the 'copy' action, as described in https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming

Unfortunately, it doesn't seem like feature switches work as expected in this scenario. Specifically, the properties don't get replaced and the warnings don't get suppressed.

[ghost]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Our recommended way to get all warnings from a library is to use a sample app that references the library with the 'copy' action, as described in https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming

Unfortunately, it doesn't seem like feature switches work as expected in this scenario. Specifically, the properties don't get replaced and the warnings don't get suppressed.

Author:	agocke
Assignees:	sbomer
Labels:	area-Tools-ILLink, needs-area-label
Milestone:	9.0.0

[sbomer]

TrimmerRootAsembly maps to -a, which has long used the copy action under the hood, and copy is designed to make no modifications to the assembly, but instead copy it as-is to the output.

To fix this I think -a should change behavior so that it roots every method/member in the assembly, but allows modifications. This is a potentially risky change if people were relying on TrimmerRootAssembly for behavior other than rooting. I suggest we change the default, but add an escape hatch that goes back to the "copy" behavior. Any opinion on this @vitek-karas @MichalStrehovsky?

[MichalStrehovsky]

The current behavior causes us to unnecessarily root facade assemblies (System.Runtime, netstandard, etc.) because we're not allowed to rewrite typeforwards to their definition in assemblies rooted with TrimmerRootAssembly. I think making this change would be overall goodness. I assume our C++/CLI logic doesn't depend on TrimmerRootAssembly.

This is a potentially risky change if people were relying on TrimmerRootAssembly for behavior other than rooting

Trying to think about where this would break, the only scenario I can think about is if this was an assembly full of type forwards - we should make sure type forwards are also kept. Looks like people like to TrimmerRootAssembly mscorlib 🤦.

but add an escape hatch that goes back to the "copy" behavior.

Would setting the TrimMode metadata on ManagedAssemblyToLink work as the escape hatch?

[vitek-karas]

If I understand this correctly - we will still root everything in the assembly, so the only changes are:

• Method body edits (like branch removal, constant prop and so on)
• Metadata edits (probably just type ref resolution/rewrite due to type forwarders)
• Metadata optimizations (removal of parameter names on non-reflectable methods)
• Type forwards (see Michal's comment)
• Global removals - like removal of some attributes as dictated by substitution XML

These all sound relatively minor. I agree we should give this a try - if we merge this into 9 relatively soon, we should have a long window of validation.

[MichalStrehovsky]

Metadata optimizations (removal of parameter names on non-reflectable methods)

I think TrimmerRootAssembly should assume everything is a reflection target (otherwise why root it?) so I'd not expect that one to happen.

[vitek-karas]

That's a good point - and it should work that way already.

[sbomer]

Would setting the TrimMode metadata on ManagedAssemblyToLink work as the escape hatch?

Good point, we already have an escape hatch, even if it's not as simple as setting some boolean flag. Sounds good to me!