Skip to content

Commit

Permalink
Fix regression in CodeAnalysisTreatWarningsAsErrors when set to false
Browse files Browse the repository at this point in the history
Fixes [AB#1726853](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1726853)

dotnet#6427 re-implemented CodeAnalysisTreatWarningsAsErrors support using global config files instead of appending CA rule IDs to WarningsAsErrors and WarningsNotAsErrors properties. However, this new implementation only works as expected for `CodeAnalysisTreatWarningsAsErrors = true` case, but doesn't prevent escalating CA warnings to errors for `CodeAnalysisTreatWarningsAsErrors = false` and `TreatWarningsAsErrors = true` case. I am restoring the WarningsNotAsErrors logic deleted from dotnet#6427 to fix this case.

I am also planning to file a separate tracking issue in Roslyn to see if we can implement this whole CodeAnalysisTreatWarningsAsErrors functionality in the compiler layer itself, instead of the current hybrid implementations in this repo, which seems to be quite a bit fragile.
  • Loading branch information
mavasani committed Jan 20, 2023
1 parent 4512de6 commit 9632ff3
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 2 deletions.
31 changes: 30 additions & 1 deletion src/Tools/GenerateDocumentationAndConfigFiles/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ void createPropsFiles()

var fileContents =
$@"<Project>
{disableNetAnalyzersImport}{getCompilerVisibleProperties()}
{disableNetAnalyzersImport}{getCodeAnalysisTreatWarningsAsErrors()}{getCompilerVisibleProperties()}
</Project>";
var directory = Directory.CreateDirectory(propsFileDir);
var fileWithPath = Path.Combine(directory.FullName, propsFileName);
Expand Down Expand Up @@ -310,6 +310,19 @@ We rely on the additional props file '{propsFileToDisableNetAnalyzersInNuGetPack
}
}

string getCodeAnalysisTreatWarningsAsErrors()
{
var allRuleIds = string.Join(';', allRulesById.Keys);
return $@"
<!--
This property group handles 'CodeAnalysisTreatWarningsAsErrors = false' for the CA rule ids implemented in this package.
-->
<PropertyGroup>
<CodeAnalysisRuleIds>{allRuleIds}</CodeAnalysisRuleIds>
<WarningsNotAsErrors Condition=""'$(CodeAnalysisTreatWarningsAsErrors)' == 'false' and '$(TreatWarningsAsErrors)' == 'true'"">$(WarningsNotAsErrors);$(CodeAnalysisRuleIds)</WarningsNotAsErrors>
</PropertyGroup>";
}

string getCompilerVisibleProperties()
{
return analyzerPackageName switch
Expand Down Expand Up @@ -1404,6 +1417,7 @@ static string GetCommonContents(string packageName, IOrderedEnumerable<string> c
}

stringBuilder.Append(GetMSBuildContentForPropertyAndItemOptions());
stringBuilder.Append(GetCodeAnalysisTreatWarningsAsErrorsTargetContents());
return stringBuilder.ToString();
}

Expand Down Expand Up @@ -1594,6 +1608,21 @@ static void AddMSBuildContentForItemOptions(StringBuilder builder)
}
}

static string GetCodeAnalysisTreatWarningsAsErrorsTargetContents()
{
return $@"
<!--
Design-time target to handle 'CodeAnalysisTreatWarningsAsErrors = false' for the CA rule ids implemented in this package.
Note that a similar 'WarningsNotAsErrors' property group is present in the generated props file to ensure this functionality on command line builds.
-->
<Target Name=""_CodeAnalysisTreatWarningsAsErrors"" BeforeTargets=""CoreCompile"" Condition=""'$(DesignTimeBuild)' == 'true' OR '$(BuildingProject)' != 'true'"">
<PropertyGroup>
<WarningsNotAsErrors Condition=""'$(CodeAnalysisTreatWarningsAsErrors)' == 'false' and '$(TreatWarningsAsErrors)' == 'true'"">$(WarningsNotAsErrors);$(CodeAnalysisRuleIds)</WarningsNotAsErrors>
</PropertyGroup>
</Target>
";
}

static string GetPackageSpecificContents(string packageName)
=> packageName switch
{
Expand Down
3 changes: 2 additions & 1 deletion src/Tools/GenerateDocumentationAndConfigFiles/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,5 @@ Following are the precedence rules as per the values of these properties:
2. For CAxxxx rules:

1. If `CodeAnalysisTreatWarningsAsErrors` is set to true, enabled CA warnings are bulk escalated to errors by choosing the appropriate globalconfig file with the error severity settings.
2. Otherwise, if `TreatWarningsAsErrors` is set to true, this property translates to `/warnaserror` command line switch and the compiler bumps all warnings, including enabled CA warnings, to errors.
2. If `CodeAnalysisTreatWarningsAsErrors` is set to false and `TreatWarningsAsErrors` is set to true, we append all CA rule IDs to `WarningsNotAsErrors` to ensure they are not escalated to errors. Users can still bump individual rule IDs to errors by editorconfig/ruleset entry, etc.
3. Otherwise, if `TreatWarningsAsErrors` is set to true, this property translates to `/warnaserror` command line switch and the compiler bumps all warnings, including enabled CA warnings, to errors.

0 comments on commit 9632ff3

Please sign in to comment.