ILLinker removes type forwarders when they are still needed

[fadimounir]

To repro:

1. dotnet new console (using the Preview6 at least)
2. dotnet publish -r win-x64 /p:PublishReadyToRun=true /p:PublishTrimmed=true

Crossgen will emit warnings. They are due to missing types. Here's an example:
System.Console.dll has a type SyncTextReader. The base type for it is TextReader, which is declared as a forwarded type in System.Runtime.Extensions.dll, and really lives in System.Private.Corelib.dll

After linking, the linked System.Runtime.Extensions.dll doesn't have the TextReader forwarder anymore, causing crossgen to fail to compile type SyncTextReader when compiling System.Console.dll, because it can't find type TextReader.

Note: crossgen will emit a warning and move on when it fails to compile a type/method. At runtime, if we hit a codepath that causes the typeloader to load a type through these type forwarders, it would most likely throw exceptions)

[fadimounir]

cc @sbomer

[sbomer]

Here's what the linker is doing that causes this. During the sweep phase:

• Some typeforwarder assembly referenced by System.Runtime.Extensions is removed
• System.Runtime.Extensions ("copyused") gets references to used types directed to the target, instead of through that typeforwarder. Its action is changed to "save".
• Later, because it is now a "save" assembly, its own exported types (unused ones) are removed. This normally would not happen for a "copy/copyused" assembly.
  Because we never fix up typerefs for unused types, the result is that the unused type SyncTextReader in System.Console references the removed forwarder.

It's generally expected that "copyused" assemblies will end up with references to removed types in other assemblies. However what's unexpected here is that the "copyused" assembly itself has exports removed. At least IMO "copyused" should mean that all exports are kept (even to types in removed assemblies).

On the other hand, we already remove assemblyrefs from "copy" assemblies (but not "copyused" assemblies, I think - this seems like a bug), so maybe we should also remove unused exports.

I'd like to be more clear about what we want "copy" and "copyused" to mean. Clearly we are already modifying such assemblies in some cases. We need to modify used typerefs to removed forwarders at least. @marek-safar:

• do we need to be removing assemblyrefs to removed assemblies?
• are you ok with my suggestion to keep all exported types in "copy" and "copyused" assemblies?

[mrvoorhe]

How we handle various combinations of AssemblyAction's is kind of a mess. If you start writing tests with various combinations of code behaviors and assembly actions is not hard to run into results that make you as "does this make sense? Or is this a bug?

do we need to be removing assemblyrefs to removed assemblies?

I don't recall exactly why the linker does this. I can say that for our use cases, after linking, we don't want any missing things. No references to assemblies that are gone or types that don't resolve. The current behaviors do help us achieve this.

What if instead of keeping around references to assemblies that are gone, or exported types that won't resolved, we

• Said having an assembly with "copy/copyused" caused all the assemblies it references to at least survive as empty assemblies. There are going to be some complications with this, there are some special attribute behaviors that might be impacted by such a change.

• Said having an assembly with "copy/copyused" caused the type of all TypeForwards in that assembly to be marked?

[sbomer]

"copy/copyused" caused all the assemblies it references to at least survive as empty assemblies

I think this would be confusing - for .NET Core at least we are trying to reduce size and number of files, and seeing empty shells for unused assemblies wouldn't be great.

assembly with "copy/copyused" caused the type of all TypeForwards in that assembly to be marked

I think marking them might be too much. I'd expect that rooting an assembly would mark all TypeForwards in it, but to me it seems useful to have the ability to set the action to "copy" and have it keep the assembly (with the TypeForwards) without necessarily marking all of them.

[marek-safar]

I cleaned up the copy/save handling with forwarders but they are not enough to control the process. We also have --keep-facades option which is by default off so copy with remove facades mode can lead to save mode.