Make TrimmerDefaultAction public

[sbomer]

Fixes #16140
Relevant for dotnet/docs#23766

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[eerhardt]

Are we ready to expose this publicly? We now have a really easy way to "full trim" your app. Are we ready to support this mode?

[sbomer]

That would be my recommendation, to get the behavior described in the WIP library trimming docs:

<TrimmerDefaultAction>link</TrimmerDefaultAction> ensures that only used parts of dependencies are analyzed. Without this option, you would see warnings originating from any part of a dependency that doesn't set [AssemblyMetadata("IsTrimmable", "True")], including parts that are unused by your library.

I can note that this isn't recommended for app authors yet (or just not mention it anywhere else in the docs). If we really don't want to expose this I can change the library mode recommendation to:

<Target Name="TrimDependencies" BeforeTargets="PrepareForILLink">
  <ItemGroup>
    <ManagedAssemblyToLink IsTrimmable="true" />
  </ItemGroup>
</Target>

[vitek-karas]

While I do agree that we don't want people to use this too often (as it comes with a relatively high risk), I think that the current way to do this (via target) looks like a big hack.

Maybe we should change it so that when this is set, we always enable trim analysis warnings (even if SDK says otherwise). This would also get us the desired behavior where if I'm in Xamarin and I ask for full trimming, I get warnings by default (it being an advanced scenario).

[sbomer]

when this is set, we always enable trim analysis warnings (even if SDK says otherwise).

This sounds like a good default, but I think it should be up to each SDK to decide (instead of the .NET SDK overriding what the Xamarin SDK or user tells us). Xamarin users who have been successful with LinkAll might not be interested in the warnings.

I think a good way to accomplish this would be for the Xamarin SDKs to suppress warnings only when TrimmerDefaultAction is copy. If we agree, I can file issues recommending this.

[marek-safar]

Xamarin users who have been successful with LinkAll might not be interested in the warnings.

It's quite unlikely that app that works with LinkAll today (presumably because it also has several things manually persisted) will just work after migration to net6.

I think we should enable the warnings, especially if we issue only 1 per assembly by default, for anyone trimming with "link-all" setup.

[eerhardt]

Maybe we should change it so that when this is set, we always enable trim analysis warnings (even if SDK says otherwise). This would also get us the desired behavior where if I'm in Xamarin and I ask for full trimming, I get warnings by default (it being an advanced scenario).

I like this idea. 👍.

[sbomer]

Should there be a way to override that default (a way to to turn off warnings when using LinkAll)?

[vitek-karas]

I would expect the user provided value for SuppressTrimAnalysisWarnings to always be honored. But I guess this is pretty tricky with SDK interactions... can we make that work anyway?

[sbomer]

It might be possible by setting a default in early properties, then detecting if it has been changed after SDK props are imported... but I am not a fan of that pattern. I think just updating the places that set SuppressTrimAnalysisWarnings to true (so they only do it when the TrimmerDefaultAction is copy) is a simpler way to get the behavior we want.

[vitek-karas]

Alternatively we could change the SDKs to set a private property instead and combine the public property with the private property in one place, taking the TrimmerDefaultAction into consideration.

[marek-safar]

a way to to turn off warnings when using LinkAll

Why would not standard warning (1 in this case) suppression work?

[vitek-karas]

Which "1" warning so you have in mind. The idea is that is I turn on agressive trimming for everything by default I get the detailed warnings - that might be 100s for complex apps. The end of the discussion is if there's a way to suppress them - they are suppresable via NoWarn, but the list is like 80 different warning codes, so not ideal.

[sbomer]

That would work for nuget dependencies of the app, but we still show detailed warnings for app code (with different codes).

[sbomer]

The idea is that is I turn on agressive trimming for everything by default I get the detailed warnings

Just to clarify - we would still collapse warnings from nugets, right?

[vitek-karas]

yes - sorry I didn't make that clear

[marek-safar]

The idea is that is I turn on agressive trimming for everything by default I get the detailed warnings - that might be 100s for complex apps. The end of the discussion is if there's a way to suppress them - they are suppresable via NoWarn, but the list is like 80 different warning codes, so not ideal.

I think the scenario is a bit different. You turn it on and you have code you can modify (basically you have multiple ways to resolve the warnings) and you have tons of dependencies you cannot modify. If you are going to publish your app with aggressive trimming all unaddressed warnings are "wrong" and you would like to suppress them. I'd suspect these will come from the nuget assemblies and my understanding is that we are going to fold the trim analysis warning from them by default. Any warnings in the app code can be suppressed with pragma(s) or resolved.

[sbomer]

I'd suspect these will come from the nuget assemblies and my understanding is that we are going to fold the trim analysis warning from them by default. Any warnings in the app code can be suppressed

This is still true with the latest change, and will be the recommended way to suppress warnings (though note that pragmas don't work for trim warnings) - so I think we agree on this.

The question is only about SuppressTrimAnalysisWarnings, which suppress warnings even from the app, and was set by default for some SDKs in #16327. The latest commit just avoids suppressing them for "LinkAll". I think this matches what you were asking for in #16865 (comment):

I think we should enable the warnings, especially if we issue only 1 per assembly by default, for anyone trimming with "link-all" setup.

[eerhardt]

Note that these .props files get evalutated before the .csproj is evaluated.

So if I have a Blazor or Web .csproj that contains:

<TrimmerDefaultAction>true</TrimmerDefaultAction>

This check is going to run too soon, and the warnings will still be suppressed.

[sbomer]

Thanks for catching this, I moved it to the targets.