-
Notifications
You must be signed in to change notification settings - Fork 126
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
Include all warnings under suppression setting #2930
Conversation
SuppressTrimAnalysisWarnings now includes all warning codes produced by the linker or trim analyzer. Previously, only warnings in the "Trim analysis" category (see MessageSubCategory.cs) were suppressed by this setting. This makes the logic more future-proof so we don't need to keep adding new warning codes to the list.
<NoWarn>$(NoWarn);IL2112;IL2113</NoWarn> | ||
<!-- Reflection access to compiler-generated code --> | ||
<NoWarn>$(NoWarn);IL2118;IL2119;IL2120</NoWarn> | ||
<ILLinkWarningLevel Condition="'$(ILLinkWarningLevel)' == ''">0</ILLinkWarningLevel> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe warn or error instead of not setting it? I think these settings are in conflict and we should probably signal there's something wrong.
Also, I think SuppressTrimAnalysisWarnings should take precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be different from EnableTrimAnalyzer, as I could see wanting to just disable the linker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I was thinking about it it is that SuppressTrimAnalysisWarnings is the high-level user-visible property that influences the behavior of two different underlying tools. EnableTrimAnalyzer and ILLinkWarningLevel are more like escape hatches for the individual tools that people can set if they know what they're doing, so I was ok with not warning about potentially conflicting settings.
To make sure I follow, is this the suggestion?
<SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings>
<ILLinkWarningLevel>5</ILLinkWarningLevel>
Warns that the settings are in conflict, SuppressTrimAnalysisWarnings wins.
<SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings>
<EnableTrimAnalyzer>true</EnableTrimAnalyzer>
No warning about conflicting settings, trimmer warnings are suppressed but analyzer warnings are shown.
Why would we warn about conflicting settings in the first case but not the second? If anything, SuppressTrimAnalysisWarnings and EnableTrimAnalyzer sound like they are more in conflict to me, and as a user it wouldn't be obvious what that combination means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. That makes sense. Maybe warn for both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have some mechanism where WarningLevel
influences ILLinkWarningLevel
, or are those completely separate?
(Honestly I thought we didn't have ILLinkWarningLevel
at all - since we only have NoWarn
and there's no ILLinkNoWarn
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW: I agree that if we should warn about something, it should be the SuppressTrimAnalysisWarnings
versus EnableTrimAnalyzer
first - as those are more "high-level" then ILLinkWarningLevel
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have some mechanism where WarningLevel influences ILLinkWarningLevel, or are those completely separate?
Any of this is easy enough to tweak (for example, make linker respect WarningLevel
directly, add an ILLinkNoWarn
that by default gets set to NoWarn
, etc), but here's how it works today:
WarningLevel
is like ILLinkWarningLevel
but for the compiler. Both are influenced by AnalysisLevel
(which by default is set based on the TFM). NoWarn
is a general mechanism for silencing warnings from any tool, whereas the SDK logic around WarningLevel
assumes it's specifically for the C# compiler.
Maybe warn for both?
if we should warn about something, it should be the SuppressTrimAnalysisWarnings versus EnableTrimAnalyzer first
Regarding warnings:
What should be the preferred way to enable analyzer warnings but not linker warnings? Would we recommend people set
<SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings>
<EnableTrimAnalyzer>true</EnableTrimAnalyzer>
or would we recommend this?
<ILLinkWarningLevel>0</ILLinkWarningLevel>
This is kind of an advanced scenario so maybe it's ok to rely on ILLinkWarningLevel
for this, but if we expect people to do the former then I wouldn't want that combination to warn. My current preference is not to produce any warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should be the preferred way to enable analyzer warnings but not linker warnings?
I don't think this needs to be "easy" - should be rare to want this
But I guess the SuppressTrimAnalysisWarnings
/EnableTrimAnalyzer
makes more sense in this case.
I think I came around to what the code does in this PR as-is. It's not perfect, but at least it makes logical sense :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you agree it makes enough sense as-is, would you mind approving the PR? Thanks!
SuppressTrimAnalysisWarnings now includes all warning codes produced by the linker or trim analyzer. Previously, only warnings in the "Trim analysis" category (see MessageSubCategory.cs) were suppressed by this setting. This makes the logic more future-proof so we don't need to keep adding new warning codes to the list. This does change the behavior a bit. It includes warnings that weren't previously part of the list. Additionally, previously if you set: <EnableTrimAnalyzer>true</EnableTrimAnalyzer> <SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings> you wouldn't see trim analysis warnings from the analyzer. With this change, you will. Similarly, previously if you set <ILLinkWarningLevel>5</ILLinkWarningLevel> <SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings> you wouldn't see trim analysis warnings, but after this change you will. Commit migrated from dotnet/linker@82c6dc6
SuppressTrimAnalysisWarnings
now includes all warning codes produced bythe linker or trim analyzer. Previously, only warnings in the
"Trim analysis" category (see MessageSubCategory.cs) were suppressed by
this setting.
This makes the logic more future-proof so we don't need to keep adding
new warning codes to the list.
This does change the behavior a bit. It includes warnings that weren't previously
part of the list. Additionally, previously if you set:
you wouldn't see trim analysis warnings from the analyzer. With this change, you will.
Similarly, previously if you set
you wouldn't see trim analysis warnings, but after this change you will.
The SDK defaults are imported after the ILLink targets so I think no SDK
changes are needed:
https://github.com/dotnet/sdk/blob/main/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L80-L81