Include all warnings under suppression setting

[sbomer]

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.

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

[agocke]

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.

[agocke]

This might be different from EnableTrimAnalyzer, as I could see wanting to just disable the linker.

[sbomer]

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.

[agocke]

Hmm. That makes sense. Maybe warn for both?

[vitek-karas]

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).

[vitek-karas]

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.

[sbomer]

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.

[vitek-karas]

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 :-)

[sbomer]

If you agree it makes enough sense as-is, would you mind approving the PR? Thanks!