search

dotnet / linker

389 stars 127 forks source link

Define UX for `IsTrimmable` in `netstandard2.1` libraries #3175

Closed sbomer closed 1 year ago

sbomer commented 1 year ago

Context: https://github.com/dotnet/aspnetcore/pull/45879#issuecomment-1372517140

The change to use a TFM-versioned linker (https://github.com/dotnet/sdk/pull/29441) means that the illink targets aren't available for netcoreapp2.1 (or any TFM earlier than 3.0 when trimming was introduced). Those targets include some shared logic (analyzer support, and adding an assembly-level attribute for IsTrimmable).

This is the configuration we shipped in 7.0: When building a netstandard2.1 app, IsTrimmable turns on the assembly-level attribute. This doesn't enable the analyzer by default, but it can be turned on by EnableTrimAnalyzer. However, the framework ref assemblies aren't annotated in 2.1, so warnings will be incomplete, and library authors will need to compile in the analysis attributes if they want to annotate their own libraries (like we did in dotnet/runtime).

Marking existing APIs as trim-incompatible in a new TFM is effectively introducing a breaking change for trimmed apps/libraries. Our recommendation so far to address this is to multi-target libraries to get the latest analysis warnings. We probably need to make this more explicit in the docs. Even so, it seems easy to fall into this trap by just setting IsTrimmable in a netstandard library that's single-targeted.

The question is: what is the intended UX for this scenario?

marek-safar commented 1 year ago

Why do we need extra UX for this? Can the common workaround of defining project/assembly internal IsTrimmableAttribute be enough?

sbomer commented 1 year ago

@marek-safar The problem is that the analyzer warnings give bad results in this scenario - so the UX question is whether we should do something to block the analyzer and/or IsTrimmable or prevent people from hitting this when targeting older TFMs.

@vitek-karas and I discussed this and we think that there should be a warning when the trim analyzer is enabled for netstandard2.x. @vitek-karas also suggested not to recommend IsTrimmable to library authors - see https://github.com/dotnet/linker/issues/3178.

JamesNK commented 1 year ago

I've encountered this issue in https://github.com/grpc/grpc-dotnet. When I tried to update to .NET 8, I got errors.

Some libraries target netstandard2.x for compatibility between .NET and .NET Framework. For example, Grpc.Core.Api only targets netstandard2.0 and netstandard2.1.

This is my workaround: https://github.com/grpc/grpc-dotnet/pull/2022. ~I'm not exactly sure about the consequences of the change. I still see [IsTrimmable(true)] on the assembly.~ Let me know if you have a better suggestion for a workaround. Update: Switched to the workaround to what ASP.NET Core project did.