Handling facade assemblies while trimming

[eerhardt]

In dotnet/runtime we have a few places where we are pointing to string Type names, but we are using a "stable" assembly identity that won't ever change from version-to-version, even if we decide to move the Type to another assembly.

This is not an exhaustive list, but here are some examples:

• TypeConverters
  • https://github.com/dotnet/runtime/blob/8a52f1e948b6f22f418817ec1068f07b8dae2aa5/src/libraries/System.Drawing.Primitives/src/System/Drawing/Color.cs#L14
  • https://github.com/dotnet/runtime/blob/8a52f1e948b6f22f418817ec1068f07b8dae2aa5/src/libraries/System.Drawing.Primitives/src/System/Drawing/Size.cs#L13-L14
• TypeDescriptionProvider
  • https://github.com/dotnet/runtime/blob/54f6250216a8f73025ccd3340560f26455b3ec18/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XAttribute.cs#L20-L21
  • https://github.com/dotnet/runtime/blob/c87f7a664b250e44c2e3a303170e6b39666ecb67/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XElement.cs#L36-L37

At its heart, the reason we are pointing to these "stable" assemblies is because there are tools (for example Visual Studio's WinForms and WPF designers) that run in the context of .NET Framework. These strings need to point to the "stable" assembly name across TFMs and versions. Tools will also take these strings from our assemblies and write them into files (e.g. .resx, BAML, etc). So there are extraneous reasons why these attributes are pointing to the facade assembly.

Note that this pattern isn't exclusive to dotnet/runtime. Any library could have a TypeForwardedToAttribute in one assembly, and then a string Type name pointing to that "facade" assembly. We just happen to use it in dotnet/runtime a lot more than anyone else.

We need to decide how we want to handle this situation w.r.t. trimming. At a high-level I think there are two options for handling this in the trimmer:

1. Notice that a "facade" assembly is being referenced in a Type string name, and preserve the facade assembly in the output app.
2. Re-write the Type string name to "forward" the type name to the real location, just like we do for IL Type references.

This issue was discovered as part of investigating dotnet/runtime#48249.

cc @MichalStrehovsky @vitek-karas @marek-safar @sbomer @ericstj @safern

[marek-safar]

I think in general we should aim for removing such assemblies as they are primary compatibility facades and should not be present in the linked output.

The linker handles that correctly for any TypeReference and also for any attribute which uses System.Type argument.

The cases you pointed out are string values which the implementation is treating as type-references or assembly references. In the most general case, they can come from an external source that is totally invisible to the linker and we the user will have to handle that with custom build settings to root the assembly.

What we could do to improve the current state is

• Fix the cases where the string reference is used unnecessarily and use typeof instead, for example, https://github.com/dotnet/runtime/blob/8a52f1e948b6f22f418817ec1068f07b8dae2aa5/src/libraries/System.Drawing.Primitives/src/System/Drawing/Color.cs#L14
• Annotate any attribute that takes type/assembly string or Type value with DynamicallyAccessedMemberTypes so that the flow analysis kicks in and marks the assembly
• Consider expanding DynamicallyAccessedMemberTypes enum with something like Original which could be used where the implementation requires the forwarded type as well.

[MichalStrehovsky]

If customer code wants to do lightup (e.g. target NetStandard 2.0 but have optional behaviors that use APIs outside of that), Type.GetType with a documented reference assembly is how they would do this. We definitely don't want people to do lightup with SomeType, System.Private.CoreLib.

Rewriting the string feels dangerous because the string could be used for other purposes before it's consumed by Type.GetType. I would vote to keep the type forward.

[marek-safar]

That's how it works today and we didn't have a single issue reported about it. Disabling this would significantly increase linker output as none of the facades would be removed for netstandard/netfx targets (and we got bug reports about that angle)

[eerhardt]

Fix the cases where the string reference is used unnecessarily and use typeof instead, for example, https://github.com/dotnet/runtime/blob/8a52f1e948b6f22f418817ec1068f07b8dae2aa5/src/libraries/System.Drawing.Primitives/src/System/Drawing/Color.cs#L14

This cannot use typeof because that would create a circular reference between System.Drawing.Primitives and System.ComponentModel.TypeConverters. TypeConverters depends on Drawing.Prmitives today because there are converters in it for Color and Size.

Annotate any attribute that takes type/assembly string or Type value with DynamicallyAccessedMemberTypes so that the flow analysis kicks in and marks the assembly

We do that already for the above 2 attributes:

https://github.com/dotnet/runtime/blob/8a52f1e948b6f22f418817ec1068f07b8dae2aa5/src/libraries/System.ObjectModel/src/System/ComponentModel/TypeConverterAttribute.cs#L50
https://github.com/dotnet/runtime/blob/8a52f1e948b6f22f418817ec1068f07b8dae2aa5/src/libraries/System.ObjectModel/src/System/ComponentModel/TypeDescriptionProviderAttribute.cs#L14

That's how it works today and we didn't have a single issue reported about it

You might not have seen them. Here are the ones I've seen in this area:

dotnet/runtime#48249
dotnet/runtime#39709
#1469
#1536

[sbomer]

I agree that the correct behavior would be to keep the the referenced type forwarders - see also #1740 which mentions the same issue. I would suggest keeping only those forwarders which are detected, instead of the big-hammer behavior of --keep-facades which would keep all facade assemblies. This might mitigate the concern about extra files in the output - I would hope that at least simple console apps don't end up with any extra facade dlls (do we know if this is true?).

[marek-safar]

I disagree that we should keep all the facades all the time. This would render linker output unusable for most size-sensitive workloads. As soon as you add a library with only netstandard (which is what we build many runtime libraries for) you pull all kinds of type forwarders junk that is not needed (netstandard, System.Runtime, etc.) If you are unlucky and you have .net framework dependency like all today's Xamarin apps you pull even more useless assemblies.

The issues you are referencing are different problems where linker internally does not always recognize type forwarders but that's a separate issue.

I think we should think about the targeted approach for scenarios that need to retain the type forwarder. The only one I'm aware of potentially needed something like that is binary deserialization.

[vitek-karas]

• For assemblies which we own, we should avoid using pure facades as much as possible. If the code is used in designer as well as runtime, that is once on .NET Framework and then in .NET Core - I would expect that we already build the assembly twice and so could use different strings in each. I do agree that default templates should not require facades to run. This is not just a linker issue, it's a performance issue as well - the runtime has load the facade, parse it and apply the type forward, while fast, it's not free at all.
• For everything else, linker should continue to rewrite typerefs/assemblyrefs as it does today to avoid facades and trim them. If we detect a presence of a type name in string (through annotations) and that type name refers to a facade I think we should make an exception in that case and preserve the facade. Ideally we would trim it down to contain just the type forwarder for the cases we know about. I agree that we should not rewrite the strings.
• For cases where the type name comes from input data (binary serializer, configuration, ...) there's no good solution. These cases are already security holes anyway. If we want to improve the experience for these, we should probably seek runtime solutions which provide good exception messages with hints on how to fix this (linker descriptors probably).

[ericstj]

If the code is used in designer as well as runtime, that is once on .NET Framework and then in .NET Core - I would expect that we already build the assembly twice and so could use different strings in each

In some cases even the .NETCore design time component may be running on .NETFramework inside Visual studio. I believe this is the case for parts of the WinForms designer, and the Resource editor in project system. I think you'd need to chase down the stuff that breaks and try to fix it. There are still other cases where the result of the design time component is to write a shared source file which folks expect to be able to multi-target against and have the same source work for .NETFramework and .NETCore (eg: resx). I agree with you in principle, but you may find that doing this breaks a lot of things that are outside our ability to fix. The pattern with multi-targeting has been to honor the original .NETFramework identity: this is something that has been around since we started having multiple flavors of .NETFramework that folks wanted to interoperate (SilverLight, Windows Phone, etc).

These cases are already security holes anyway.

Not necessarily. They are only security holes if the input is untrusted. There are cases today where trusted input contains these type names.

[marek-safar]

I'm probably missing some history context here but what is the relation of .NET Framework designers and .NET6 trimming? Are you expecting to run linker for designer build?

[ericstj]

@vitek-karas was suggesting we change the value of these attributes in the assemblies we ship, this would impact all .NET6 application types, regardless if they run the linker or not. Some design time components (even for .NET 6) run inside visual studio on .NETFramework and may need to treat some of these as .NETFramework types. Additionally some of those design time components also emit assets which can multi-target between .NETFramework and .NET 6, those assets may contain type identities which need to resolve in both .NETFramework and .NET6.

[sbomer]

@mateoatr I think we can close this. Sound ok?

[mateoatr]

Yes. This should've been fixed by #1941.