Skip to content

Include all warnings under suppression setting #2930

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

Merged
merged 2 commits into from
Aug 4, 2022
Merged

Include all warnings under suppression setting #2930

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0178740
Include all warnings under suppression setting
sbomer Jul 29, 2022
08f494f
Merge branch 'main' into suppressAll
sbomer Aug 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 2 additions & 43 deletions src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,38 +59,8 @@ Copyright (c) .NET Foundation. All rights reserved.

<!-- Suppress warnings produced by the linker or by the ILLink Roslyn analyzer. -->
<PropertyGroup Condition="'$(SuppressTrimAnalysisWarnings)' == 'true'">
<!-- RequiresUnreferenceCodeAttribute method called -->
<NoWarn>$(NoWarn);IL2026;IL2116</NoWarn>
<!-- Invalid use of DynamicallyAccessedMembersAttribute -->
<NoWarn>$(NoWarn);IL2041;IL2042;IL2043;IL2056</NoWarn>
<!-- Reference to removed attribute type -->
<NoWarn>$(NoWarn);IL2045</NoWarn>
<!-- RequiresUnreferencedCodeAttribute mismatch on virtual override -->
<NoWarn>$(NoWarn);IL2046</NoWarn>
<!-- COM marshalling warning -->
<NoWarn>$(NoWarn);IL2050</NoWarn>
<!-- Reflection intrinsics with unknown arguments -->
<NoWarn>$(NoWarn);IL2032;IL2055;IL2057;IL2058;IL2059;IL2060;IL2061;IL2096</NoWarn>
<!-- Unknown values passed to locations with DynamicallyAccessedMemberTypes -->
<NoWarn>$(NoWarn);IL2062;IL2063;IL2064;IL2065;IL2066</NoWarn>
<!-- Unsatisfied DynamicallyAccessedMembers requirements -->
<NoWarn>$(NoWarn);IL2067;IL2068;IL2069;IL2070;IL2071;IL2072;IL2073;IL2074;IL2075;IL2076;IL2077;IL2078;IL2079;IL2080;IL2081;IL2082;IL2083;IL2084;IL2085;IL2086;IL2087;IL2088;IL2089;IL2090;IL2091</NoWarn>
<!-- DynamicallyAccessedMembersAttribute mismatch on virtual override -->
<NoWarn>$(NoWarn);IL2092;IL2093;IL2094;IL2095</NoWarn>
<!-- DynamicallyAccessedMembersAttribute used on unsupported member -->
<NoWarn>$(NoWarn);IL2097;IL2098;IL2099;IL2106</NoWarn>
<!-- Unknown value passed to Expression.Property -->
<NoWarn>$(NoWarn);IL2103</NoWarn>
<!-- Multiple methods associated with state machine type or user method -->
<NoWarn>$(NoWarn);IL2107;IL2117</NoWarn>
<!-- Unannotated type derived from base type with RequiresUnreferencedCode -->
<NoWarn>$(NoWarn);IL2109</NoWarn>
<!-- Reflection access to members with DynamicallyAccessedMembers requirements -->
<NoWarn>$(NoWarn);IL2110;IL2111;IL2114;IL2115</NoWarn>
<!-- Reflection access to members with RequiresUnreferencedCode -->
<NoWarn>$(NoWarn);IL2112;IL2113</NoWarn>
<!-- Reflection access to compiler-generated code -->
<NoWarn>$(NoWarn);IL2118;IL2119;IL2120</NoWarn>
<ILLinkWarningLevel Condition="'$(ILLinkWarningLevel)' == ''">0</ILLinkWarningLevel>
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member

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

Copy link
Member

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.

Copy link
Member Author

@sbomer sbomer Aug 2, 2022 •
edited
Loading

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.

Copy link
Member

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

Copy link
Member Author

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!

<EnableTrimAnalyzer Condition="'$(EnableTrimAnalyzer)' == ''">false</EnableTrimAnalyzer>
</PropertyGroup>

<!--
Expand Down Expand Up @@ -248,17 +218,6 @@ Copyright (c) .NET Foundation. All rights reserved.
<TrimmerSingleWarn Condition=" '$(TrimmerSingleWarn)' == '' ">true</TrimmerSingleWarn>
</PropertyGroup>

<!-- Suppressions to work around issues in previous versions of the framework. See https://github.com/dotnet/runtime/issues/40336 -->
<PropertyGroup Condition="'$(SuppressTrimAnalysisWarnings)' == 'true' And $([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '6.0'))">
<!-- Framework embedded XML descriptors reference windows-only members. -->
<NoWarn Condition=" !$(RuntimeIdentifier.StartsWith('win')) ">$(NoWarn);IL2008</NoWarn> <!-- Unresolved type referenced in XML. -->
<NoWarn Condition=" !$(RuntimeIdentifier.StartsWith('win')) ">$(NoWarn);IL2009</NoWarn> <!-- Unresolved member on type referenced in XML. -->
<!-- Framework has DynamicDependencyAttributes that reference windows-only members. -->
<NoWarn Condition=" !$(RuntimeIdentifier.StartsWith('win')) ">$(NoWarn);IL2037</NoWarn> <!-- Unresolved member for DynamicDependencyAttribute -->
<!-- Framework embedded XML descriptors reference 32-bit-only members. -->
<NoWarn Condition=" '$(PlatformTarget)' != 'x64' AND '$(PlatformTarget)' != 'arm64'">$(NoWarn);IL2009;IL2012</NoWarn> <!-- Unresolved field referenced in XML -->
</PropertyGroup>

<!-- Enable serialization discovery for compat in < 7.0 -->
<PropertyGroup Condition="$([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '7.0')">
<_ExtraTrimmerArgs>--enable-serialization-discovery $(_ExtraTrimmerArgs)</_ExtraTrimmerArgs>
Expand Down