Embedded XML not processed for assemblies referenced only via PreserveDependency attribute

[vitek-karas]

PreserveDependency attributes are processed in PreserveDependencyLookupStep which runs after BlacklistStep. As such any assemblies referenced only via PreserveDependency attributes are not processed by the BlacklistStep and thus any embedded XML in those assemblies is not processed.

This was obviously an intentional "omission" as per comment on the relevant test: https://github.com/mono/linker/blob/ab427fd29fd1ecfac81b878abd1043a0ae61f0de/test/Mono.Linker.Tests.Cases/PreserveDependencies/PreserveDependencyMethodInNonReferencedAssemblyWithEmbeddedXml.cs#L8

Unfortunately fixing this is not simple. A related desired behavior is that if the PreserveDependency attribute is not actually used (that is the item it's on is trimmed), the embedded XML of its referenced assembly should not be processed (or rather applied). This is shown in test https://github.com/mono/linker/blob/ab427fd29fd1ecfac81b878abd1043a0ae61f0de/test/Mono.Linker.Tests.Cases/PreserveDependencies/PreserveDependencyOnUnusedMethodInNonReferencedAssemblyWithEmbeddedXml.cs#L8

Real fix would have to be able to either add assemblies to the closure later on (during MarkStep), or delay application of embedded XML to that point (during MarkStep). Both approaches are rather tricky to handle correctly as they break assumptions made by existing code in the linker.

[joperezr]

While working on adding substitution files to introduce a new feature switch to remove Resources from System.* assemblies, I stumbled upon this issue. Basically some of the assemblies that do have embedded substitution files that will remove all resources from them when the feature switch is on won't be applied because they were only kept because of a DynamicDependency attribute. Special thanks to @sbomer who helped debug the linker and figure out what the issue was. It will be really important to fix this on our efforts to reduce app size as much as possible since resources might end up taking a lot of space on the assemblies.

FYI: @eerhardt

I have a simple repro project in case it may help in any way with the investigation, so if needed let me know and I can add this to the issue here.

[MichalStrehovsky]

A related desired behavior is that if the PreserveDependency attribute is not actually used (that is the item it's on is trimmed), the embedded XML of its referenced assembly should not be processed (or rather applied). This is shown in test

I think this behavior is the same to having an AssemblyRef to an assembly that ends up being unused because MarkStep didn't mark anything that used that specific AssemblyRef. I wouldn't worry about that aspect too much. It falls out from the global prepass design (that feels like a mistake in general).

This issue also has another angle that if the assemblies added by PreserveDependency have other PreserveDependency attributes, linker would not look at those either because they got added too late (PreserveDependencyLookupStep is already running and won't run again).

We basically need to be able to rerun the steps that potentially add new references over an over for each new discovered assembly until we reach a fixpoint (i.e. no new assembly was added). (This probably includes the B***kListStep because the XML also seems to have the ability to include new things into the closure.)

It almost feels like they shouldn't be IStep-based because that interface doesn't have the granularity to say "only run this on the following assembly").

[vitek-karas]

I would not mind changing these assembly-pre-processors into a separate interface and handle them separately.

[sbomer]

Fixed in #1666