Consider removing IsTrimmable project-level MSBuild property from documentation

[sbomer]

Summarizing discussion with @vitek-karas:

IsTrimmable (the project-level MSBuild property intended for library authors) has two behaviors:

1. Turns on the analyzer
2. Adds an assembly-level attribute

In the 6.0 timeframe, the default trimming behavior was to trim only those assemblies with the attribute. So IsTrimmable was the recommended way for library authors to address trim-compatibility warnings, and to ensure that their libraries would be trimmed by default, so as not to contribute unused code to the app size.

@vitek-karas pointed out that tying the two behaviors together is confusing, because even if a given library is properly trim-annotated, apps which reflect over it may still break when trimmed. In other words, the assembly-level [AssemblyMetadata ("IsTrimmable", "True")] doesn't say anything about whether it is "safe" to trim this library in the context of an app that consumes it.

This especially creates problems for scenarios like blazor where trim analysis warnings are off by default, since app authors won't hit these issues until runtime. The model where library authors are guided to put IsTrimmable in their project makes it more likely that blazor apps will be broken when consuming such libraries, for example when serializing types from these libraries using trim-incompatible serializers.

@vitek-karas suggested to remove IsTrimmable from the public documentation, and instead only recommend turning on the analyzers. Essentially the idea is that the assembly-level attribute should primarily be used by the framework libraries, and we don't want to encourage third-party libraries to use it.

[sbomer]

Some context on why we introduced this behavior in the first place:

The main motivation for introducing the assembly-level attribute was https://github.com/dotnet/linker/issues/1269. Blazor used to maintain logic that opts framework assemblies into trimming, but this logic was fragile, and started missing OOB framework assemblies. We needed a way for the OOB assemblies to be trimmed by default in the blazor scenario, but we didn't want blazor to own the logic.

Then in https://github.com/dotnet/sdk/issues/17201, we added the MSBuild property which made it easy for library authors (framework and third-party) to use the attribute. At the time, we wanted to encourage library authors both to annotate their code, and to give a hint that allowed the SDK defaults to trim the library.

Now in 7.0 we have more reasonable ("full"/"aggressive") defaults, but the blazor SDK still relies on the attribute (TrimMode "partial").

[sbomer]

My personal take at the moment:

There is still an incentive for library authors to opt into trimming via the attribute in 7.0, because of those app models using "partial" defaults (and because of app authors staying on the 6.0 SDK). If we discourage its use, then those app models will not see size wins from library authors doing the work to make their libraries trim-compatible.

It's not clear to me how much third-party IsTrimmable really hurts. I suspect that many of the libraries which opt into trimming will not be the target of reflection/serialization. Even if they are so in some blazor scenario, I'd find it unfortunate to prevent them from being trimmed in other scenarios (xamarin, or apps built with the 6.0 SDK).

I'd like to see some examples where IsTrimmable helps size or hurts correctness for blazor. If we see this breaking blazor scenarios, I see that as a signal telling us where we/blazor need to invest more effort in improving the tooling - but if we discourage IsTrimmable we won't get that signal. If developers are blindly sticking IsTrimmable in their libraries without understanding what it does, that is a problem we need to address - but I would look for a way to make it more understandable, not discourage it outright.

Until we get to a point where "partial" TrimMode is a niche or deprecated scenario, I think we should still be encouraging the ecosystem to produce libraries that will get trimmed in more scenarios.

[MichalStrehovsky]

If you dig through the issues in the runtime repo, you'll see issues where this is breaking Blazor. I would say we get one once a month but I didn't bother collecting the exact data; here's two examples specifically for Blazor in the past couple of months: https://github.com/dotnet/runtime/issues/78776, https://github.com/dotnet/runtime/issues/74141. (I'm posting the first two I could quickly find).

IsTrimmable is a false promise. It makes sense as a stopgap, if we want to trim at least something, but it doesn't scale - we won't be able to support people running into this and people will not trust trimming as long as it exists.

[vitek-karas]

@MichalStrehovsky just to make sure - you want to get rid of IsTrimmable, right? :wink: (you didn't directly say what you would prefer we do here...)

[marek-safar]

Essentially the idea is that the assembly-level attribute should primarily be used by the framework libraries, and we don't want to encourage third-party libraries to use it.

What is framework libraries in this context?

[MichalStrehovsky]

@MichalStrehovsky just to make sure - you want to get rid of IsTrimmable, right? 😉 (you didn't directly say what you would prefer we do here...)

I was only responding to the "I'd like to see some examples where IsTrimmable helps size or hurts correctness for blazor" part - IsTrimmable in our own framework hurts correctness in Blazor :).

The correctness angle probably won't help us solve this. We know we can't have correctness with partial trimming and warnings off.

"Being smaller on Blazor WASM" might be an additional selling point why 3rd parties could start caring about trimming. We know it doesn't amount to much (given how much was this adopted since we introduced it), but every little bit could help. E.g. ImageSharp is a pretty big library and it's possible https://github.com/SixLabors/ImageSharp/pull/2160 was motivated by one of the SKUs that do partial trimming (since the change itself probably doesn't fix anything that would be broken with full trimming that we were just rolling out as another forcing function).

[vitek-karas]

The scenario which make me particularly nervous is:

• Blazor app with some NuGet dependency... works
• The NuGet package is updated and adds IsTrimmable=true - "Because it helps with size", maybe there are even no warnings from analyzer in this library.
• The app updates to a newer package version
• Now it's broken (because something in the app was using serialization over a type from NuGet for example)

Per our documentation both the developer of the NuGet package as well as the developer of the app didn't do anything wrong, but it's still broken, while it worked before. I know that we technically don't make any promises in this area, but this still doesn't sound right.

Note that the proposal is not to remove the ability to mark assemblies as IsTrimmable=true, just so that we don't document it as "fixing trimming for the library" solution.

[MichalStrehovsky]

If someone adds IsTrimmable and there are warnings, this is just a bug in the NuGet like any other (we should do something with the netstandard2.1 case that runs analyzer against unannotated framework though).

If someone adds IsTrimmable and there are no warnings and the app gets broken because someone else is reflecting on it, the someone else just ran out of luck. This is no different from e.g. https://github.com/dotnet/runtime/issues/78776#issuecomment-1326059800 - here running out of luck just means we lost a static reference to something that was keeping something alive and now it's garbage collected.

I don't view IsTrimmable on 3rd party NuGets any more risky than IsTrimmable on our framework.

[vitek-karas]

I don't view IsTrimmable on 3rd party NuGets any more risky than IsTrimmable on our framework.

I agree in principal, but I'm worried about motivations and due diligence. We try to mark only libraries where the likelyhood of breaking somebody is relatively low. And if that's not the case we try to limit the impact (as in the JSON case where we do root the most common collection types basically for this reason). Unfortunately I've seen examples of libraries where the sole motivation was size - completely disregarding warnings and/or understanding potential problems caused by it.

I'm not "hardcore" about this, I just think it's worth having a discussion and hopefully at least updating our docs in some way.

[marek-safar]

Would it be possible to turn the analyzer into source generator that emits AssemblyMetadataAttribute if there are 0 warnings?

[vitek-karas]

Would it be possible to turn the analyzer into source generator that emits AssemblyMetadataAttribute if there are 0 warnings?

Yes - but that doesn't really solve the problem. The fact that there are no warnings in any given assembly doesn't say anything about trim correctness of the app (the warnings might be in a different assembly, but trimming the first one breaks the app).