dotnet / roslyn-analyzers

MIT License
1.59k stars 465 forks source link

CodeAnalysisTreatWarningsAsErrors broken in .NET 6 preview 2 #4916

Open jmarolf opened 3 years ago

jmarolf commented 3 years ago

Version: SDK 6.0.100-preview.2.21127.3 (internal only link)

Describe the bug

The following property is no longer respected in Visual Studio

<CodeAnalysisTreatWarningsAsErrors>false</CodeAnalysisTreatWarningsAsErrors>

Steps To Reproduce

  1. create a new console app with the following project file:

    
    <Project Sdk="Microsoft.NET.Sdk">
    
    <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
    <CodeAnalysisTreatWarningsAsErrors>false</CodeAnalysisTreatWarningsAsErrors>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
    </PropertyGroup>

and the following `Program.cs` file:
```csharp
using System;
class Program {
  static void Main() {
    while (true) {
      Span<char> c = stackalloc char[5]; // Warning CA2014  Potential stack overflow. Move the stackalloc out of the loop.
    }
  }
}

Expected behavior

Both IDE and Command line builds show this as a warning

Actual behavior

IDE shows as an error while commandline build shows as error

both dotnet build and (within a dev command prompt) msbuild /t:rebuild /v:m

produce the same output suggesting it is something workspace related.

Additional context

First thing I looked at was if there is a difference in the design-time-build info of Preview 1 vs Preview 2. The only difference is that preview 2 has the addition WarningsNotAsErrors entry for CA1418 everything else is identical

diff of build target evaluation between Preview 1 and Preview 2 (nothing of note)

Target Name=AddGlobalAnalyzerConfigForPackage_MicrosoftCodeAnalysisNetAnalyzers Project=net6-analyzers-bug.csproj
    MicrosoftCodeAnalysisNetAnalyzersRulesVersion = 5
    _GlobalAnalyzerConfigAnalysisMode_MicrosoftCodeAnalysisNetAnalyzers = 
    _GlobalAnalyzerConfigAnalysisMode_MicrosoftCodeAnalysisNetAnalyzers = 
    _GlobalAnalyzerConfigAnalysisMode_MicrosoftCodeAnalysisNetAnalyzers = Default
    _GlobalAnalyzerConfigFileName_MicrosoftCodeAnalysisNetAnalyzers = AnalysisLevel_5_Default.editorconfig
-   _GlobalAnalyzerConfigDir_MicrosoftCodeAnalysisNetAnalyzers = C:\Program Files\dotnet\sdk\6.0.100-preview.1.21103.13\Sdks\Microsoft.NET.Sdk\analyzers\build\config
+   _GlobalAnalyzerConfigDir_MicrosoftCodeAnalysisNetAnalyzers = C:\Program Files\dotnet\sdk\6.0.100-preview.2.21127.3\Sdks\Microsoft.NET.Sdk\analyzers\build\config
-   _GlobalAnalyzerConfigFile_MicrosoftCodeAnalysisNetAnalyzers = C:\Program Files\dotnet\sdk\6.0.100-preview.1.21103.13\Sdks\Microsoft.NET.Sdk\analyzers\build\config\AnalysisLevel_5_Default.editorconfig
+   _GlobalAnalyzerConfigFile_MicrosoftCodeAnalysisNetAnalyzers = C:\Program Files\dotnet\sdk\6.0.100-preview.2.21127.3\Sdks\Microsoft.NET.Sdk\analyzers\build\config\AnalysisLevel_5_Default.editorconfig

    EditorConfigFiles
-       C:\Program Files\dotnet\sdk\6.0.100-preview.1.21103.13\Sdks\Microsoft.NET.Sdk\analyzers\build\config\AnalysisLevel_5_Default.editorconfig
+       C:\Program Files\dotnet\sdk\6.0.100-preview.2.21127.3\Sdks\Microsoft.NET.Sdk\analyzers\build\config\AnalysisLevel_5_Default.editorconfig

binary log of design-time-builds can be found here

diff of AnalysisLevel_5_Default.editorconfig between Preview 1 and Preview 2

# NOTE: Requires **VS2019 16.7** or later

# Rules from '5.0' release with 'Default' analysis mode
# Description: Rules with enabled-by-default state from '5.0' release with 'Default' analysis mode. Rules that are first released in a version later than '5.0' are disabled.

is_global = true

  global_level = -2

+ # CA1418: Use valid platform string
+ dotnet_diagnostic.CA1418.severity = none

Changes in 6.0 Preview 2 that are not in Preview 1 (View Complete Diff of Changes)

Youssef1313 commented 3 years ago

@jmarolf The binlog of preview 2 shows that CA2014 is correctly specified in WarningsNotAsErrors:

image

Isn't the rest of work part of roslyn?

As .NET 6 preview 2 doesn't seem to be publicly available, can you pin the compiler version to an older version and keep NetAnalyzers version the same to figure if it's a compiler regression or not?

mavasani commented 3 years ago

Interesting, no repro with 6.0.100-preview.1.21102.7. Let me try with latest

mavasani commented 3 years ago

Extremely likely this is a regression in the compiler...

Youssef1313 commented 3 years ago

Extremely likely this is a regression in the compiler...

@mavasani Should this move to roslyn then?

mavasani commented 3 years ago

@jmarolf I was able to find a fix for the issue, but I am not sure why/how this is a reliable fix. The scenario works fine if I remove the BeforeTargets=""CoreCompile"" from https://github.com/dotnet/roslyn-analyzers/blob/9e5f533cbafcc5579e4d758bc9bde27b7611ca54/src/Tools/GenerateDocumentationAndConfigFiles/Program.cs#L1506-L1515

You can try this out locally by editing C:\Program Files\dotnet\sdk\6.0.100-preview.2.21153.5\Sdks\Microsoft.NET.Sdk\analyzers\build\Microsoft.CodeAnalysis.NetAnalyzers.targets

jmarolf commented 3 years ago

I ran msbuild /pp on the project file and I admit I am a little confused.

There are our properties in the project file:

<PropertyGroup>
  <OutputType>Exe</OutputType>
  <TargetFramework>net5.0</TargetFramework>
  <CodeAnalysisTreatWarningsAsErrors>false</CodeAnalysisTreatWarningsAsErrors>
  <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
  <RootNamespace>net6_analyzers_bug</RootNamespace>
</PropertyGroup>

Then we set the WarningsNotAsErrors property twice

C:\Program Files\dotnet\sdk\6.0.100-preview.2.21127.3\Sdks\Microsoft.NET.Sdk\analyzers\build\Microsoft.CodeAnalysis.NetAnalyzers.props

  <!-- 
    This property group prevents the rule ids implemented in this package to be bumped to errors when
    the 'CodeAnalysisTreatWarningsAsErrors' = 'false'.
  -->
  <PropertyGroup>
    <CodeAnalysisRuleIds>CA1000;CA1001;CA1002;CA1003;CA1005;CA1008;CA1010;CA1012;CA1014;CA1016;CA1017;CA1018;CA1019;CA1021;CA1024;CA1027;CA1028;CA1030;CA1031;CA1032;CA1033;CA1034;CA1036;CA1040;CA1041;CA1043;CA1044;CA1045;CA1046;CA1047;CA1050;CA1051;CA1052;CA1054;CA1055;CA1056;CA1058;CA1060;CA1061;CA1062;CA1063;CA1064;CA1065;CA1066;CA1067;CA1068;CA1069;CA1070;CA1200;CA1303;CA1304;CA1305;CA1307;CA1308;CA1309;CA1310;CA1401;CA1416;CA1417;CA1418;CA1501;CA1502;CA1505;CA1506;CA1507;CA1508;CA1509;CA1700;CA1707;CA1708;CA1710;CA1711;CA1712;CA1713;CA1715;CA1716;CA1720;CA1721;CA1724;CA1725;CA1802;CA1805;CA1806;CA1810;CA1812;CA1813;CA1814;CA1815;CA1816;CA1819;CA1820;CA1821;CA1822;CA1823;CA1824;CA1825;CA1826;CA1827;CA1828;CA1829;CA1830;CA1831;CA1832;CA1833;CA1834;CA1835;CA1836;CA1837;CA1838;CA2000;CA2002;CA2007;CA2008;CA2009;CA2011;CA2012;CA2013;CA2014;CA2015;CA2016;CA2100;CA2101;CA2109;CA2119;CA2153;CA2200;CA2201;CA2207;CA2208;CA2211;CA2213;CA2214;CA2215;CA2216;CA2217;CA2218;CA2219;CA2224;CA2225;CA2226;CA2227;CA2229;CA2231;CA2234;CA2235;CA2237;CA2241;CA2242;CA2243;CA2244;CA2245;CA2246;CA2247;CA2248;CA2249;CA2300;CA2301;CA2302;CA2305;CA2310;CA2311;CA2312;CA2315;CA2321;CA2322;CA2326;CA2327;CA2328;CA2329;CA2330;CA2350;CA2351;CA2352;CA2353;CA2354;CA2355;CA2356;CA2361;CA2362;CA3001;CA3002;CA3003;CA3004;CA3005;CA3006;CA3007;CA3008;CA3009;CA3010;CA3011;CA3012;CA3061;CA3075;CA3076;CA3077;CA3147;CA5350;CA5351;CA5358;CA5359;CA5360;CA5361;CA5362;CA5363;CA5364;CA5365;CA5366;CA5367;CA5368;CA5369;CA5370;CA5371;CA5372;CA5373;CA5374;CA5375;CA5376;CA5377;CA5378;CA5379;CA5380;CA5381;CA5382;CA5383;CA5384;CA5385;CA5386;CA5387;CA5388;CA5389;CA5390;CA5391;CA5392;CA5393;CA5394;CA5395;CA5396;CA5397;CA5398;CA5399;CA5400;CA5401;CA5402;CA5403</CodeAnalysisRuleIds>
    <WarningsNotAsErrors Condition="'$(CodeAnalysisTreatWarningsAsErrors)' == 'false'">$(WarningsNotAsErrors);$(CodeAnalysisRuleIds)</WarningsNotAsErrors>
  </PropertyGroup>

C:\Program Files\dotnet\sdk\6.0.100-preview.2.21127.3\Sdks\Microsoft.NET.Sdk\analyzers\build\Microsoft.CodeAnalysis.NetAnalyzers.targets

  <!--
    Design-time target to prevent the rule ids implemented in this package to be bumped to errors in the IDE
    when 'CodeAnalysisTreatWarningsAsErrors' = 'false'. Note that a similar 'WarningsNotAsErrors'
    property group is present in the generated props file to ensure this functionality on command line builds.
  -->
  <Target Name="_CodeAnalysisTreatWarningsNotAsErrors" BeforeTargets="CoreCompile" Condition="'$(CodeAnalysisTreatWarningsAsErrors)' == 'false' AND ('$(DesignTimeBuild)' == 'true' OR '$(BuildingProject)' != 'true')">
    <PropertyGroup>
      <WarningsNotAsErrors>$(WarningsNotAsErrors);$(CodeAnalysisRuleIds)</WarningsNotAsErrors>
    </PropertyGroup>
  </Target>

@mavasani I see no reason to have some special logic for design-time-builds in the targets files. Properties should be able to be evaluated once in Microsoft.CodeAnalysis.NetAnalyzers.props and be the same is both commandline compiles and design-time builds. I would recommend not writing this section to the target file at all.

mvacha commented 2 years ago

I was able to find even simple repro - just setting <CodeAnalysisTreatWarningsAsErrors>true</CodeAnalysisTreatWarningsAsErrors> (without <TreatWarningsAsErrors>true</TreatWarningsAsErrors>) causes that no code analysis warnings appear in neither dotnet build output (.net 7.0.100 preview 5) or Visual Studio Error List (17.3.0 preview 3).

cmenzi commented 1 year ago

Not sure if it goes into the same direction, but I also encountered an unexcepted behavior:

I would expect in both executions the opposite behavior.

I saw that in this file this evaluation was added this in .NET 7. // C:\Program Files\dotnet\sdk\7.0.100\Sdks\Microsoft.NET.Sdk\analyzers\build

<Project>
    ....
    <WarningsNotAsErrors Condition="'$(CodeAnalysisTreatWarningsAsErrors)' == 'false'">$(WarningsNotAsErrors);$(CodeAnalysisRuleIds)</WarningsNotAsErrors>
    <WarningsAsErrors Condition="'$(CodeAnalysisTreatWarningsAsErrors)' == 'true' and '$(TreatWarningsAsErrors)' != 'true'">$(WarningsAsErrors);$(CodeAnalysisRuleIds)</WarningsAsErrors>
  </PropertyGroup>
</Project>
bh-sijtnic commented 1 year ago

I also run into this issue, while i continue to dig deeper, i have the feeling this is caused by a deeper problem. My problem is that when setting CodeAnalysisTreatWarningsAsErrors to false, no CA warnings or errors appear anymore. I can see confirm in the Solution Explorer that the code analyzer rules are marked as warn only. image

Though when building the project, no warnings appear.

When i remove CodeAnalysisTreatWarningsAsErrors from the csproj, the warning appears. But i usually have TreatWarningsAsError enabled, so now i have an error, so i really need CodeAnalysisTreatWarningsAsErrors set to false.

From the SDK props and target i also found the piece cmenzi referred to. I dont think this is the problem though. This props basically adds all the results into WanringNotAsErrors, which should be fine.

I then removed CodeAnalysisTreatWarningsAsErrors from my project and copied the lines from the SDK props, so i added the WarningsNotAsErrors directly into the csproj, this results in the same problem.

In my sample project, adding these lines, with a .cs file generating warning CA1051, results in no warnings in the compiler or IDE

  <!-- .Net Analyzers -->
  <PropertyGroup>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
    <EnableNETAnalyzers>true</EnableNETAnalyzers>
    <AnalysisLevel>latest-recommended</AnalysisLevel>
    <WarningsNotAsErrors>CA1051</WarningsNotAsErrors>
  </PropertyGroup>

When i remove CA1051 from the WarningsNotAsErrors, the CA1051 error appears as expected. image

So i think the problem is more with how WarningsNotAsErrors is processed. It suppresses the message entirely instead of still logging the warning. Not sure if this is code analyzer or compiler related.