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.

Sign up for GitHub

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

Jump to bottom

Enable EnableTrimAnalyzer by default #21293

Closed
rolfbjarne opened this issue Sep 24, 2024 · 6 comments · Fixed by #21351
Closed

Enable EnableTrimAnalyzer by default #21293

rolfbjarne opened this issue Sep 24, 2024 · 6 comments · Fixed by #21351
Assignees
@rolfbjarne
Labels
enhancement The issue or pull request is an enhancement
Milestone
.NET 10

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
Member

/cc: @vitek-karas @simonrozsival

@vitek-karas
Copy link
Member

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
@rolfbjarne
[dotnet] Stop suppressing trim analysis warnings. Fixes #21293.
cceb557 
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.
@rolfbjarne rolfbjarne mentioned this issue Oct 1, 2024
Merged
rolfbjarne added a commit that referenced this issue Oct 24, 2024
@rolfbjarne
[dotnet] Stop suppressing trim analysis warnings. Fixes #21293. (#21351)
51dae3b 
Our own code shouldn't produce trim analysis warnings anymore (see #10405), so we don't need to suppress trim analysis warnings for everyone. 

Except: we still want to suppress trim analysis warnings if no assemblies are trimmed,  because otherwise we'll get warnings for code that would otherwise be trimmed away.

On the other hand, we want trim analyzer always enabled, so that warnings are reported for user code.

The difference between "trim analyzer" and "trim analysis warnings" is that the former is an analyzer that analyzes the currently compiled code (which we always want), while the latter is reported by the trimmers, and as such will report warnings from *all* code, including all references.

This also required bumping Touch.Unit and MonoTouch.Dialog.

* New commits in xamarin/MonoTouch.Dialog:

    * xamarin/MonoTouch.Dialog@d157950 Exclude code that's not trimmer safe.

        Diff: [77b3337..d157950](https://github.com/xamarin/MonoTouch.Dialog/compare/77b3337dbbc9e3e2f1b06dab3d37d2822488b0b3..d157950b6e6ed32cf53d4074fe19223dc1f1e8fe)

* New commits in xamarin/Touch.Unit:

    * xamarin/Touch.Unit@957faca [Touch.Client] Disable features that aren't trimmer safe.

        Diff: [92a0726..957faca](https://github.com/xamarin/Touch.Unit/compare/92a072683b69b2f61f235a7a249c2c7f261236d4..957facad80e753310f52fcbb8cf85219d9d6d887)

Fixes #21293.
@rolfbjarne
Copy link
Member Author

Fixed in .NET 10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rolfbjarne rolfbjarne

Labels
enhancement The issue or pull request is an enhancement
Projects
None yet
Milestone
.NET 10
Development

Successfully merging a pull request may close this issue.

[dotnet] Stop suppressing trim analysis warnings. Fixes #21293. dotnet/macios
5 participants
@dalexsoto @rolfbjarne @jonathanpeppers @vitek-karas @ivanpovazan