SuppressTrimAnalysisWarnings should also suppress warnings from the Roslyn trim analyzer

[eerhardt]

We have the property SuppressTrimAnalysisWarnings which tells the linker not to warn when PublishTrimmed=true. However, the Roslyn analyzer still warns because it is enabled by this code:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets

Lines 59 to 60 in 1ee1d24

	<EnableTrimAnalyzer Condition="'$(EnableTrimAnalyzer)' == '' And
	('$(IsTrimmable)' == 'true' Or '$(PublishTrimmed)' == 'true')">true</EnableTrimAnalyzer>

Setting SuppressTrimAnalysisWarnings=true has no effect on the Roslyn analyzer, but it should. We shouldn't have two settings to disable trim analysis warnings. If someone wants to disable the warnings, they should just need to set one property.

cc @vitek-karas @sbomer @agocke @mateoatr @tlakollo

[sbomer]

Which warnings were you seeing? I believe we just need to keep this list updated with any new shared warnings:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

Lines 55 to 62 in 58ce205

	<!-- Suppress warnings produced by the linker or by the ILLink Roslyn analyzer. Warnings produced
	exclusively by the linker should be set in PrepareForILLink, to avoid polluting the global NoWarn list. -->
	<PropertyGroup Condition="'$(SuppressTrimAnalysisWarnings)' == 'true'">
	<!-- RequiresUnreferenceCodeAttribute method called -->
	<NoWarn>$(NoWarn);IL2026</NoWarn>
	<!-- RequiresUnreferencedCodeAttribute mismatch on virtual override -->
	<NoWarn>$(NoWarn);IL2046</NoWarn>
	</PropertyGroup>

Or we could just put all of them here so we don't need to keep updating the list (it will just pass a bunch of NoWarns to csc that currently don't do anything).

[eerhardt]

Which warnings were you seeing?

IL2046. See https://github.com/dotnet/runtime/runs/2933733811 on PR dotnet/runtime#54741.

src/libraries/System.Runtime.Serialization.Xml/tests/SerializationTypes.RuntimeOnly.cs(2856,29): error IL2046: (NETCORE_ENGINEERING_TELEMETRY=Build) Base member 'System.Xml.Serialization.XmlSerializationReader.InitCallbacks()' with 'RequiresUnreferencedCodeAttribute' has a derived member 'MyReader.InitCallbacks()' without 'RequiresUnreferencedCodeAttribute'. Attributes must match across all interface implementations or overrides.

I believe we just need to keep this list updated with any new shared warnings

Is that really the best approach here? Having 2 properties feels wrong. What does it mean when I say EnableTrimAnalyzer= true and SuppressTrimAnalysisWarnings=false?

[agocke]

I think this is a good reason why we should move all the warnings from the linker up to the analyzer block, even if they're not produced by the analyzer yet.

We should also consider moving this code into the linker as part of a props file that gets inserted into the SDK, so when we add a new warning that should be suppressed by SuppressTrimAnalysisWarnings, we can fix it in the same commit that we make the change.

I think that the runtime problem that you were seeing @eerhardt is split-state, since the linker was updated but the SDK wasn't.

[sbomer]

The whole approach of putting a bunch of NoWarns behind an MSBuild property is flawed in my opinion - the linker/analyzer define the category of trim analysis warnings, so ideally the linker/analyzer would just take a boolean option to disable the whole category. But we couldn't agree on a way to do that, so we put them in MSBuild instead.

Having 2 properties feels wrong. What does it mean when I say EnableTrimAnalyzer= true and SuppressTrimAnalysisWarnings=false?

There are some warnings produced by the linker that don't get disabled by SuppressTrimAnalysisWarnings - this includes things like resolution failures from bad XML files. The analyzer could conceivably support these warnings too. I agree this is confusing. It might be better to do away with the distinction.