Skip to content

Commit

Permalink
Re-implement CodeAnalysisTreatWarningsAsErrors with globalconfig files
Browse files Browse the repository at this point in the history 
Fixes #6281

Prior implementation for `CodeAnalysisTreatWarningsAsErrors` passed `/warnaserror+:CAxxxx` to the command line for each CA rule ID. This led to couple of issues:
1. All disabled by default CA warnings seem to get enabled as errors
2. Editorconfig/globalconfig based configuration of the enabled CA rules stops working.

The new implementation updates our globalconfig tooling to generate parallel globalconfigs with `_warnaserror` suffix, such that rules which would have had effective warning severity are set to error severity in the new globalconfig files. The targets/props that picks the appropriate globalconfig based on the `AnalysisLevel` property now chooses the file with `_warnaserror` suffix if user has enabled code analysis warnings to be treated as errors.

Another couple of notes:
1. Given that the buggy implementation of `CodeAnalysisTreatWarningsAsErrors` has already shipped in .NET7 and prior SDKs, we use a separate property `EffectiveCodeAnalysisTreatWarningsAsErrors`, so users using the newer NuGet package, but older .NET SDK can still get this functionality. Users that move to the newer .NET8 preview SDK with this fix can use `CodeAnalysisTreatWarningsAsErrors` itself.
2. I have adjusted the tooling to generate the config files into buildtransitive folder instead of build folder. This is needed as a follow-up to #6325 for globalconfig files to be correctly mapped.
3. I have also renamed all the tooling generated globalconfig files to have .globalconfig extension instead of .editorconfig extension, as that was misleading.

I have verified that `CodeAnalysisTreatWarningsAsErrors` works fine with the locally built package when `EffectiveCodeAnalysisTreatWarningsAsErrors` is set to true. If I manually edit the targets/props shipped inside .NET7 SDK to delete all the prior shipped `CodeAnalysisTreatWarningsAsErrors` logic, then setting `CodeAnalysisTreatWarningsAsErrors` also works fine.
  • Loading branch information
@mavasani
mavasani committed Jan 12, 2023
1 parent 3168c39 commit 6c028d3
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 58 deletions.
8 changes: 4 additions & 4 deletions src/Tools/GenerateAnalyzerNuspec/Program.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,15 +235,15 @@

if (globalAnalyzerConfigsDir.Length > 0 && Directory.Exists(globalAnalyzerConfigsDir))
{
foreach (string editorconfig in Directory.EnumerateFiles(globalAnalyzerConfigsDir))
foreach (string globalconfig in Directory.EnumerateFiles(globalAnalyzerConfigsDir))
{
if (Path.GetExtension(editorconfig) == ".editorconfig")
if (Path.GetExtension(globalconfig) == ".globalconfig")
{
result.AppendLine(FileElement(Path.Combine(globalAnalyzerConfigsDir, editorconfig), $"build\\config"));
result.AppendLine(FileElement(Path.Combine(globalAnalyzerConfigsDir, globalconfig), $"buildtransitive\\config"));
}
else
{
throw new InvalidDataException($"Encountered a file with unexpected extension: {editorconfig}");
throw new InvalidDataException($"Encountered a file with unexpected extension: {globalconfig}");
}
}
}
Expand Down
105 changes: 51 additions & 54 deletions src/Tools/GenerateDocumentationAndConfigFiles/Program.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ void createPropsFiles()

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

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

string getCompilerVisibleProperties()
{
return analyzerPackageName switch
Expand Down Expand Up @@ -794,12 +780,15 @@ void CreateGlobalConfigsForVersion(
{
var analysisLevelVersionString = GetNormalizedVersionStringForEditorconfigFileNameSuffix(version);

foreach (var analysisMode in Enum.GetValues(typeof(AnalysisMode)))
foreach (var warnAsError in new[] { true, false })
{
CreateGlobalConfig(version, isShippedVersion, analysisLevelVersionString, (AnalysisMode)analysisMode!, releaseTrackingData, category: null);
foreach (var category in categories!)
foreach (var analysisMode in Enum.GetValues(typeof(AnalysisMode)))
{
CreateGlobalConfig(version, isShippedVersion, analysisLevelVersionString, (AnalysisMode)analysisMode!, releaseTrackingData, category);
CreateGlobalConfig(version, isShippedVersion, analysisLevelVersionString, (AnalysisMode)analysisMode!, warnAsError, releaseTrackingData, category: null);
foreach (var category in categories!)
{
CreateGlobalConfig(version, isShippedVersion, analysisLevelVersionString, (AnalysisMode)analysisMode!, warnAsError, releaseTrackingData, category);
}
}
}
}
Expand All @@ -809,26 +798,38 @@ void CreateGlobalConfig(
bool isShippedVersion,
string analysisLevelVersionString,
AnalysisMode analysisMode,
bool warnAsError,
ImmutableArray<ReleaseTrackingData> releaseTrackingData,
string? category)
{
var analysisLevelPropName = "AnalysisLevel";
var title = $"Rules from '{version}' release with '{analysisMode}' analysis mode";
var description = $"Rules with enabled-by-default state from '{version}' release with '{analysisMode}' analysis mode. Rules that are first released in a version later than '{version}' are disabled.";

if (category != null)
{
analysisLevelPropName += category;
title = $"'{category}' {title}";
description = $"'{category}' {description}";
}

CreateGlobalconfig(
analyzerGlobalconfigsDir,
#pragma warning disable CA1308 // Normalize strings to uppercase
$"{analysisLevelPropName}_{analysisLevelVersionString}_{analysisMode!.ToString()!.ToLowerInvariant()}.editorconfig",
var globalconfigFileName = $"{analysisLevelPropName}_{analysisLevelVersionString}_{analysisMode!.ToString()!.ToLowerInvariant()}";
#pragma warning restore CA1308 // Normalize strings to uppercase
title,

if (warnAsError)
{
globalconfigFileName += "_warnaserror";
title += " escalated to 'error' severity";
description += " Enabled rules with 'warning' severity are escalated to 'error' severity to respect 'CodeAnalysisTreatWarningsAsErrors' MSBuild property.";
}

CreateGlobalconfig(
analyzerGlobalconfigsDir,
$"{globalconfigFileName}.globalconfig",
title,
description,
warnAsError,
analysisMode,
category,
allRulesById,
Expand Down Expand Up @@ -1158,34 +1159,37 @@ private static void Validate(string fileWithPath, string fileContents, List<stri

private static void CreateGlobalconfig(
string folder,
string editorconfigFileName,
string editorconfigTitle,
string editorconfigDescription,
string fileName,
string title,
string description,
bool warnAsError,
AnalysisMode analysisMode,
string? category,
SortedList<string, DiagnosticDescriptor> sortedRulesById,
(ImmutableArray<ReleaseTrackingData> releaseTrackingData, Version version, bool isShippedVersion) releaseTrackingDataAndVersion)
{
Debug.Assert(editorconfigFileName.EndsWith(".editorconfig", StringComparison.Ordinal));
Debug.Assert(fileName.EndsWith(".globalconfig", StringComparison.Ordinal));

var text = GetGlobalconfigText(
editorconfigTitle,
editorconfigDescription,
title,
description,
warnAsError,
analysisMode,
category,
sortedRulesById,
releaseTrackingDataAndVersion);
var directory = Directory.CreateDirectory(folder);
#pragma warning disable CA1308 // Normalize strings to uppercase - Need to use 'ToLowerInvariant' for file names in non-Windows platforms
var editorconfigFilePath = Path.Combine(directory.FullName, editorconfigFileName.ToLowerInvariant());
var configFilePath = Path.Combine(directory.FullName, fileName.ToLowerInvariant());
#pragma warning restore CA1308 // Normalize strings to uppercase
File.WriteAllText(editorconfigFilePath, text);
File.WriteAllText(configFilePath, text);
return;

// Local functions
static string GetGlobalconfigText(
string editorconfigTitle,
string editorconfigDescription,
string title,
string description,
bool warnAsError,
AnalysisMode analysisMode,
string? category,
SortedList<string, DiagnosticDescriptor> sortedRulesById,
Expand All @@ -1200,8 +1204,8 @@ void StartGlobalconfig()
{
result.AppendLine(@"# NOTE: Requires **VS2019 16.7** or later");
result.AppendLine();
result.AppendLine($@"# {editorconfigTitle}");
result.AppendLine($@"# Description: {editorconfigDescription}");
result.AppendLine($@"# {title}");
result.AppendLine($@"# Description: {description}");
result.AppendLine();
result.AppendLine($@"is_global = true");
result.AppendLine();
Expand Down Expand Up @@ -1240,6 +1244,11 @@ bool AddRule(DiagnosticDescriptor rule, string? category)
}

var (isEnabledByDefault, severity) = GetEnabledByDefaultAndSeverity(rule, analysisMode);
if (warnAsError && severity == DiagnosticSeverity.Warning && isEnabledByDefault)
{
severity = DiagnosticSeverity.Error;
}

if (rule.IsEnabledByDefault == isEnabledByDefault &&
severity == rule.DefaultSeverity)
{
Expand Down Expand Up @@ -1395,7 +1404,6 @@ static string GetCommonContents(string packageName, IOrderedEnumerable<string> c
}

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

Expand Down Expand Up @@ -1443,8 +1451,14 @@ static string GetGlobalAnalyzerConfigTargetContents(string packageName, string?
<_GlobalAnalyzerConfigAnalysisMode_{trimmedPackageName} Condition=""'$(_GlobalAnalyzerConfigAnalysisMode_{trimmedPackageName})' == 'AllDisabledByDefault'"">{nameof(AnalysisMode.None)}</_GlobalAnalyzerConfigAnalysisMode_{trimmedPackageName}>
<_GlobalAnalyzerConfigAnalysisMode_{trimmedPackageName} Condition=""'$(_GlobalAnalyzerConfigAnalysisMode_{trimmedPackageName})' == ''"">{nameof(AnalysisMode.Default)}</_GlobalAnalyzerConfigAnalysisMode_{trimmedPackageName}>
<!-- Default 'EffectiveCodeAnalysisTreatWarningsAsErrors' to 'CodeAnalysisTreatWarningsAsErrors' for escalating relevant code analysis warnings to errors. -->
<!-- We use a separate property to allow users to override 'CodeAnalysisTreatWarningsAsErrors' implementation from .NET7 or older SDK, which had a known issue: https://github.com/dotnet/roslyn-analyzers/issues/6281 -->
<EffectiveCodeAnalysisTreatWarningsAsErrors Condition=""'$(EffectiveCodeAnalysisTreatWarningsAsErrors)' == ''"">$(CodeAnalysisTreatWarningsAsErrors)</EffectiveCodeAnalysisTreatWarningsAsErrors>
<!-- Choose GlobalAnalyzerConfig file with '_warnaserror' suffix if 'EffectiveCodeAnalysisTreatWarningsAsErrors' is 'true'. -->
<_GlobalAnalyzerConfigFileName_{trimmedPackageName}_WarnAsErrorSuffix Condition=""'$(EffectiveCodeAnalysisTreatWarningsAsErrors)' == 'true'"">_warnaserror</_GlobalAnalyzerConfigFileName_{trimmedPackageName}_WarnAsErrorSuffix>
<!-- GlobalAnalyzerConfig file name based on user specified package version '{packageVersionPropName}', if any. We replace '.' with '_' to map the version string to file name suffix. -->
<_GlobalAnalyzerConfigFileName_{trimmedPackageName} Condition=""'$({packageVersionPropName})' != ''"">{analysisLevelPropName}_$({packageVersionPropName}.Replace(""."",""_""))_$(_GlobalAnalyzerConfigAnalysisMode_{trimmedPackageName}).editorconfig</_GlobalAnalyzerConfigFileName_{trimmedPackageName}>
<_GlobalAnalyzerConfigFileName_{trimmedPackageName} Condition=""'$({packageVersionPropName})' != ''"">{analysisLevelPropName}_$({packageVersionPropName}.Replace(""."",""_""))_$(_GlobalAnalyzerConfigAnalysisMode_{trimmedPackageName})$(_GlobalAnalyzerConfigFileName_{trimmedPackageName}_WarnAsErrorSuffix).globalconfig</_GlobalAnalyzerConfigFileName_{trimmedPackageName}>
<_GlobalAnalyzerConfigFileName_{trimmedPackageName}>$(_GlobalAnalyzerConfigFileName_{trimmedPackageName}.ToLowerInvariant())</_GlobalAnalyzerConfigFileName_{trimmedPackageName}>
<_GlobalAnalyzerConfigDir_{trimmedPackageName} Condition=""'$(_GlobalAnalyzerConfigDir_{trimmedPackageName})' == ''"">$(MSBuildThisFileDirectory)config</_GlobalAnalyzerConfigDir_{trimmedPackageName}>
Expand Down Expand Up @@ -1580,23 +1594,6 @@ static void AddMSBuildContentForItemOptions(StringBuilder builder)
}
}

static string GetCodeAnalysisTreatWarningsAsErrorsTargetContents()
{
return $@"
<!--
Design-time target to handle 'CodeAnalysisTreatWarningsAsErrors' for the CA rule ids implemented in this package.
Note that similar 'WarningsNotAsErrors' and 'WarningsAsErrors'
property groups are 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'"">$(WarningsNotAsErrors);$(CodeAnalysisRuleIds)</WarningsNotAsErrors>
<WarningsAsErrors Condition=""'$(CodeAnalysisTreatWarningsAsErrors)' == 'true' and '$(TreatWarningsAsErrors)' != 'true'"">$(WarningsAsErrors);$(CodeAnalysisRuleIds)</WarningsAsErrors>
</PropertyGroup>
</Target>
";
}

static string GetPackageSpecificContents(string packageName)
=> packageName switch
{
Expand Down

0 comments on commit 6c028d3

Please sign in to comment.