Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable EnableTrimAnalyzer by default #21293

Open
rolfbjarne opened this issue Sep 24, 2024 · 5 comments · May be fixed by #21351
Open

Enable EnableTrimAnalyzer by default #21293

rolfbjarne opened this issue Sep 24, 2024 · 5 comments · May be fixed by #21351
Assignees
Labels
enhancement The issue or pull request is an enhancement
Milestone

Comments

@rolfbjarne
Copy link
Member

The idea is to set EnableTrimAnalyzers=true by default in our targets somewhere, so that customers get trimmer warnings by default in all configurations (even if they've disabled linking in some configurations).

There are a few thoughts here:

  • Our end goal is a fully trimmable world (100% trimmable won't happen, but we want to get as close as we can).
  • Mobile projects have been trimmed for many years, and as such mobile developers are more accustomed to trimming than desktop developers. Mobile code base is also often more trimmable than desktop code, for the same reason.
  • In theory we'd like the trim analyzers to only be enabled when the project trims in any configuration, but we can't reliably detect whether the trimmer is fully enabled (TrimMode=full) in any configuration, any logic to try to reason about this or compute an accurate value is bound to not work for someone, and as the complexity grows, it'll be harder to reason about when the trim analyzers are enabled.

So the proposal is to just set EnableTrimAnalyzers=true by default.

  • It's very simple.
  • It's easy to override in a customer's project (set EnableTrimAnalyzers=false).
  • It hopefully moves us towards a fully trimmable world.

In order to minimize impact, we could enable it early on for .NET 10 to capture feedback. If it turns out too disruptive, we can also back the change out again (which is easy, because it's a very simple change).

CC @jonathanpeppers

@rolfbjarne rolfbjarne added the enhancement The issue or pull request is an enhancement label Sep 24, 2024
@rolfbjarne rolfbjarne added this to the .NET 10 milestone Sep 24, 2024
@dalexsoto
Copy link
Member

Agreed, we should enable this in the early previews of .NET 10 and get feedback if any 👍

@jonathanpeppers
Copy link
Member

For Android, I think we will have this change in .NET 9 RC 2 (if merged in time):

So, the project template will have TrimMode=full for both Debug & Release, you get the same warnings on both, and we keep the default behavior of no trimming in Debug mode.

A customer could remove TrimMode=full to get the behavior from .NET 8 or older.

@ivanpovazan
Copy link
Contributor

/cc: @vitek-karas @simonrozsival

@vitek-karas
Copy link

Do we set SuppressTrimAnalysisWarnings to true anywhere - that will effectively disable the analyzer even if EnableTrimAnalyzer is set to true since it will not produce any warnings due to the suppression.

@rolfbjarne
Copy link
Member Author

Do we set SuppressTrimAnalysisWarnings to true anywhere - that will effectively disable the analyzer even if EnableTrimAnalyzer is set to true since it will not produce any warnings due to the suppression.

No we don't set SuppressTrimAnalysisWarnings (minor caveat: when building for NativeAOT, we set it to true when running ILLink, and then back to the user's value when running NativeAOT).

@rolfbjarne rolfbjarne self-assigned this Sep 26, 2024
rolfbjarne added a commit that referenced this issue Oct 1, 2024
Our own code shouldn't produce trim analysis warnings anymore (see #10405), so
we don't need to suppress trim analysis warnings for everyone.

Fixes #21293.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue or pull request is an enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants