Update the severities value of BuildCheck results (#10330)

dotnet · Jul 4, 2024 · 07be3a1 · 07be3a1
1 parent 06bb1c2
commit 07be3a1
Show file tree

Hide file tree

Showing 14 changed files with 232 additions and 79 deletions.
diff --git a/documentation/specs/proposed/BuildCheck.md b/documentation/specs/proposed/BuildCheck.md
@@ -75,15 +75,13 @@ Majority of following cases are included in appropriate context within the scena
 ```ini
 # I expect this to apply to all projects within my solution, but not to projects which are not part of the solution
 [ContosoFrontEnd.sln]
-build_check.BC0101.IsEnabled=true
 build_check.BC0101.Severity=warning
 ```
 * Attributing `.editorconfig` configurations to lower granularity than whole projects. E.g.:
 ```ini
 # I expect this to apply only to a scope of the imported file. Or possibly I expect this to apply to all projects importing this project.
 [ContosoCommonImport.proj]
-buildcheck.BC0101.IsEnabled=true
-buildcheck.BC0101.Severity=warning
+build_check.BC0101.Severity=warning
 ```
 * Respecting `.editorconfig` file in msbuild import locations (unless they are in the parent folders hierarchy of particular project file).
 * CodeFixes are not supported in V1

diff --git a/src/Build/BuildCheck/API/BuildAnalyzerConfiguration.cs b/src/Build/BuildCheck/API/BuildAnalyzerConfiguration.cs
@@ -21,8 +21,7 @@ public class BuildAnalyzerConfiguration
     public static BuildAnalyzerConfiguration Default { get; } = new()
     {
         EvaluationAnalysisScope = BuildCheck.EvaluationAnalysisScope.ProjectOnly,
-        Severity = BuildAnalyzerResultSeverity.Info,
-        IsEnabled = false,
+        Severity = BuildAnalyzerResultSeverity.None
     };
 
     public static BuildAnalyzerConfiguration Null { get; } = new();
@@ -45,7 +44,18 @@ public class BuildAnalyzerConfiguration
     /// If all rules within the analyzer are not enabled, it will not be run.
     /// If some rules are enabled and some are not, the analyzer will be run and reports will be post-filtered.
     /// </summary>
-    public bool? IsEnabled { get; internal init; }
+    public bool? IsEnabled {
+        get
+        {
+            // Do not consider Default as enabled, because the default severity of the rule coule be set to None
+            if (Severity.HasValue && Severity.Value != BuildAnalyzerResultSeverity.Default)
+            {
+                return !Severity.Value.Equals(BuildAnalyzerResultSeverity.None);
+            }
+
+            return null;
+        }
+    }
 
     /// <summary>
     /// Creates a <see cref="BuildAnalyzerConfiguration"/> object based on the provided configuration dictionary.
@@ -59,8 +69,7 @@ internal static BuildAnalyzerConfiguration Create(Dictionary<string, string>? co
         return new()
         {
             EvaluationAnalysisScope = TryExtractValue(nameof(EvaluationAnalysisScope), configDictionary, out EvaluationAnalysisScope evaluationAnalysisScope) ? evaluationAnalysisScope : null,
-            Severity = TryExtractValue(nameof(Severity), configDictionary, out BuildAnalyzerResultSeverity severity) ? severity : null,
-            IsEnabled = TryExtractValue(nameof(IsEnabled), configDictionary, out bool isEnabled) ? isEnabled : null,
+            Severity = TryExtractValue(nameof(Severity), configDictionary, out BuildAnalyzerResultSeverity severity) ? severity : null
         };
     }
 
@@ -83,31 +92,6 @@ private static bool TryExtractValue<T>(string key, Dictionary<string, string>? c
         return isParsed;
     }
 
-    private static bool TryExtractValue(string key, Dictionary<string, string>? config, out bool value)
-    {
-        value = default;
-
-        if (config == null || !config.TryGetValue(key.ToLower(), out var stringValue) || stringValue is null)
-        {
-            return false;
-        }
-
-        bool isParsed = false;
-
-        if (bool.TryParse(stringValue, out bool boolValue))
-        {
-            value = boolValue;
-            isParsed = true;
-        }
-
-        if (!isParsed)
-        {
-            ThrowIncorrectValueException(key, stringValue);
-        }
-
-        return isParsed;
-    }
-
     private static void ThrowIncorrectValueException(string key, string value)
     {
         // TODO: It will be nice to have the filename where the incorrect configuration was placed. 

diff --git a/src/Build/BuildCheck/API/BuildAnalyzerResultSeverity.cs b/src/Build/BuildCheck/API/BuildAnalyzerResultSeverity.cs
@@ -8,7 +8,28 @@ namespace Microsoft.Build.Experimental.BuildCheck;
 /// </summary>
 public enum BuildAnalyzerResultSeverity
 {
-    Info,
+    /// <summary>
+    /// When set, the default value of the BuildCheck rule will be used.
+    /// </summary>
+    Default,
+
+    /// <summary>
+    /// When set to None the rule will not run.
+    /// </summary>
+    None,
+
+    /// <summary>
+    /// Information level message.
+    /// </summary>
+    Suggestion,
+
+    /// <summary>
+    /// Results a warning in build if the BuildCheck rule applied.
+    /// </summary>
     Warning,
-    Error,
+
+    /// <summary>
+    /// Results an error in build if the BuildCheck rule applied.
+    /// </summary>
+    Error
 }
diff --git a/src/Build/BuildCheck/API/BuildCheckResult.cs b/src/Build/BuildCheck/API/BuildCheckResult.cs
@@ -31,7 +31,7 @@ public BuildCheckResult(BuildAnalyzerRule buildAnalyzerRule, ElementLocation loc
     internal BuildEventArgs ToEventArgs(BuildAnalyzerResultSeverity severity)
         => severity switch
         {
-            BuildAnalyzerResultSeverity.Info => new BuildCheckResultMessage(this),
+            BuildAnalyzerResultSeverity.Suggestion => new BuildCheckResultMessage(this),
             BuildAnalyzerResultSeverity.Warning => new BuildCheckResultWarning(this),
             BuildAnalyzerResultSeverity.Error => new BuildCheckResultError(this),
             _ => throw new ArgumentOutOfRangeException(nameof(severity), severity, null),

diff --git a/src/Build/BuildCheck/Analyzers/DoubleWritesAnalyzer.cs b/src/Build/BuildCheck/Analyzers/DoubleWritesAnalyzer.cs
@@ -23,7 +23,7 @@ internal sealed class DoubleWritesAnalyzer : BuildAnalyzer
     public static BuildAnalyzerRule SupportedRule = new BuildAnalyzerRule("BC0102", "DoubleWrites",
         "Two tasks should not write the same file",
         "Tasks {0} and {1} from projects {2} and {3} write the same file: {4}.",
-        new BuildAnalyzerConfiguration() { Severity = BuildAnalyzerResultSeverity.Warning, IsEnabled = true });
+        new BuildAnalyzerConfiguration() { Severity = BuildAnalyzerResultSeverity.Warning });
 
     public override string FriendlyName => "MSBuild.DoubleWritesAnalyzer";
 

diff --git a/src/Build/BuildCheck/Analyzers/SharedOutputPathAnalyzer.cs b/src/Build/BuildCheck/Analyzers/SharedOutputPathAnalyzer.cs
@@ -18,7 +18,7 @@ internal sealed class SharedOutputPathAnalyzer : BuildAnalyzer
     public static BuildAnalyzerRule SupportedRule = new BuildAnalyzerRule("BC0101", "ConflictingOutputPath",
         "Two projects should not share their OutputPath nor IntermediateOutputPath locations",
         "Projects {0} and {1} have conflicting output paths: {2}.",
-        new BuildAnalyzerConfiguration() { Severity = BuildAnalyzerResultSeverity.Warning, IsEnabled = true });
+        new BuildAnalyzerConfiguration() { Severity = BuildAnalyzerResultSeverity.Warning });
 
     public override string FriendlyName => "MSBuild.SharedOutputPathAnalyzer";
 

diff --git a/src/Build/BuildCheck/Infrastructure/BuildAnalyzerConfigurationInternal.cs b/src/Build/BuildCheck/Infrastructure/BuildAnalyzerConfigurationInternal.cs
@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System;
 using Microsoft.Build.Experimental.BuildCheck;
 
 namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure;
@@ -10,25 +11,31 @@ namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 /// </summary>
 internal sealed class BuildAnalyzerConfigurationInternal
 {
-    public BuildAnalyzerConfigurationInternal(string ruleId, EvaluationAnalysisScope evaluationAnalysisScope, BuildAnalyzerResultSeverity severity, bool isEnabled)
+    public BuildAnalyzerConfigurationInternal(string ruleId, EvaluationAnalysisScope evaluationAnalysisScope, BuildAnalyzerResultSeverity severity)
     {
+        if (severity == BuildAnalyzerResultSeverity.Default)
+        {
+            throw new ArgumentOutOfRangeException(nameof(severity), severity, "Severity 'Default' is not recognized by the BuildCheck reports infrastructure");
+        }
+
         RuleId = ruleId;
         EvaluationAnalysisScope = evaluationAnalysisScope;
         Severity = severity;
-        IsEnabled = isEnabled;
     }
 
     public string RuleId { get; }
+
     public EvaluationAnalysisScope EvaluationAnalysisScope { get; }
+
     public BuildAnalyzerResultSeverity Severity { get; }
-    public bool IsEnabled { get; }
+
+    public bool IsEnabled => Severity >= BuildAnalyzerResultSeverity.Suggestion;
 
     // Intentionally not checking the RuleId
     //  as for analyzers with multiple rules, we can squash config to a single one,
     //  if the ruleId is the only thing differing.
     public bool IsSameConfigurationAs(BuildAnalyzerConfigurationInternal? other) =>
         other != null &&
         Severity == other.Severity &&
-        IsEnabled == other.IsEnabled &&
         EvaluationAnalysisScope == other.EvaluationAnalysisScope;
 }
diff --git a/src/Build/BuildCheck/Infrastructure/ConfigurationProvider.cs b/src/Build/BuildCheck/Infrastructure/ConfigurationProvider.cs
@@ -33,7 +33,6 @@ internal sealed class ConfigurationProvider
 
     private readonly string[] _infrastructureConfigurationKeys = new string[] {
         nameof(BuildAnalyzerConfiguration.EvaluationAnalysisScope).ToLower(),
-        nameof(BuildAnalyzerConfiguration.IsEnabled).ToLower(),
         nameof(BuildAnalyzerConfiguration.Severity).ToLower()
     };
 
@@ -263,8 +262,7 @@ internal BuildAnalyzerConfigurationInternal MergeConfiguration(
         => new BuildAnalyzerConfigurationInternal(
             ruleId: ruleId,
             evaluationAnalysisScope: GetConfigValue(editorConfig, defaultConfig, cfg => cfg.EvaluationAnalysisScope),
-            isEnabled: GetConfigValue(editorConfig, defaultConfig, cfg => cfg.IsEnabled),
-            severity: GetConfigValue(editorConfig, defaultConfig, cfg => cfg.Severity));
+            severity: GetSeverityValue(editorConfig, defaultConfig));
 
     private BuildAnalyzerConfigurationInternal GetMergedConfiguration(
         string projectFullPath,
@@ -280,6 +278,22 @@ private T GetConfigValue<T>(
            propertyGetter(defaultValue) ??
            EnsureNonNull(propertyGetter(BuildAnalyzerConfiguration.Default));
 
+    private BuildAnalyzerResultSeverity GetSeverityValue(BuildAnalyzerConfiguration editorConfigValue, BuildAnalyzerConfiguration defaultValue)
+    {
+        BuildAnalyzerResultSeverity? resultSeverity = null;
+
+        // Consider Default as null, so the severity from the default value could be selected.
+        // Default severity is not recognized by the infrastructure and serves for configuration purpuses only. 
+        if (editorConfigValue.Severity != null && editorConfigValue.Severity != BuildAnalyzerResultSeverity.Default)
+        {
+            resultSeverity = editorConfigValue.Severity;
+        }
+
+        resultSeverity ??= defaultValue.Severity ?? EnsureNonNull(BuildAnalyzerConfiguration.Default.Severity);
+
+        return resultSeverity.Value;
+    }
+
     private static T EnsureNonNull<T>(T? value) where T : struct
     {
         if (value is null)

diff --git a/src/Build/CompatibilitySuppressions.xml b/src/Build/CompatibilitySuppressions.xml
@@ -0,0 +1,88 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!-- https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/diagnostic-ids -->
+<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Info</Target>
+    <Left>lib/net472/Microsoft.Build.dll</Left>
+    <Right>lib/net472/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Info</Target>
+    <Left>lib/net8.0/Microsoft.Build.dll</Left>
+    <Right>lib/net8.0/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Info</Target>
+    <Left>ref/net472/Microsoft.Build.dll</Left>
+    <Right>ref/net472/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Info</Target>
+    <Left>ref/net8.0/Microsoft.Build.dll</Left>
+    <Right>ref/net8.0/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0011</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Error</Target>
+    <Left>lib/net472/Microsoft.Build.dll</Left>
+    <Right>lib/net472/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0011</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Warning</Target>
+    <Left>lib/net472/Microsoft.Build.dll</Left>
+    <Right>lib/net472/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0011</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Error</Target>
+    <Left>lib/net8.0/Microsoft.Build.dll</Left>
+    <Right>lib/net8.0/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0011</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Warning</Target>
+    <Left>lib/net8.0/Microsoft.Build.dll</Left>
+    <Right>lib/net8.0/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0011</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Error</Target>
+    <Left>ref/net472/Microsoft.Build.dll</Left>
+    <Right>ref/net472/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0011</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Warning</Target>
+    <Left>ref/net472/Microsoft.Build.dll</Left>
+    <Right>ref/net472/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0011</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Error</Target>
+    <Left>ref/net8.0/Microsoft.Build.dll</Left>
+    <Right>ref/net8.0/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0011</DiagnosticId>
+    <Target>F:Microsoft.Build.Experimental.BuildCheck.BuildAnalyzerResultSeverity.Warning</Target>
+    <Left>ref/net8.0/Microsoft.Build.dll</Left>
+    <Right>ref/net8.0/Microsoft.Build.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+</Suppressions>
diff --git a/src/BuildCheck.UnitTests/BuildAnalyzerConfigurationInternalTests.cs b/src/BuildCheck.UnitTests/BuildAnalyzerConfigurationInternalTests.cs
@@ -5,34 +5,59 @@
 using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 using Microsoft.Build.Experimental.BuildCheck;
 using Shouldly;
+using System;
 
 namespace Microsoft.Build.BuildCheck.UnitTests;
 
 public class BuildAnalyzerConfigurationInternalTests
 {
     [Theory]
-    [InlineData("ruleId", EvaluationAnalysisScope.ProjectOnly, BuildAnalyzerResultSeverity.Warning, true, true)]
-    [InlineData("ruleId2", EvaluationAnalysisScope.ProjectOnly, BuildAnalyzerResultSeverity.Warning, true, true)]
-    [InlineData("ruleId", EvaluationAnalysisScope.ProjectOnly, BuildAnalyzerResultSeverity.Error, true, false)]
+    [InlineData("ruleId", EvaluationAnalysisScope.ProjectOnly, BuildAnalyzerResultSeverity.Warning,  true)]
+    [InlineData("ruleId2", EvaluationAnalysisScope.ProjectOnly, BuildAnalyzerResultSeverity.Warning,  true)]
+    [InlineData("ruleId", EvaluationAnalysisScope.ProjectOnly, BuildAnalyzerResultSeverity.Error, false)]
     public void IsSameConfigurationAsTest(
         string secondRuleId,
         EvaluationAnalysisScope secondScope,
         BuildAnalyzerResultSeverity secondSeverity,
-        bool secondEnabled,
         bool isExpectedToBeSame)
     {
         BuildAnalyzerConfigurationInternal configuration1 = new BuildAnalyzerConfigurationInternal(
                        ruleId: "ruleId",
                        evaluationAnalysisScope: EvaluationAnalysisScope.ProjectOnly,
-                       severity: BuildAnalyzerResultSeverity.Warning,
-                       isEnabled: true);
+                       severity: BuildAnalyzerResultSeverity.Warning);
 
         BuildAnalyzerConfigurationInternal configuration2 = new BuildAnalyzerConfigurationInternal(
             ruleId: secondRuleId,
             evaluationAnalysisScope: secondScope,
-            severity: secondSeverity,
-            isEnabled: secondEnabled);
+            severity: secondSeverity);
 
         configuration1.IsSameConfigurationAs(configuration2).ShouldBe(isExpectedToBeSame);
     }
+
+    [Theory]
+    [InlineData( BuildAnalyzerResultSeverity.Warning, true)]
+    [InlineData(BuildAnalyzerResultSeverity.Suggestion, true)]
+    [InlineData(BuildAnalyzerResultSeverity.Error, true)]
+    [InlineData(BuildAnalyzerResultSeverity.None, false)]
+    public void BuildAnalyzerConfigurationInternal_Constructor_SeverityConfig(BuildAnalyzerResultSeverity severity, bool isEnabledExpected)
+    {
+        BuildAnalyzerConfigurationInternal configuration = new BuildAnalyzerConfigurationInternal(
+                       ruleId: "ruleId",
+                       evaluationAnalysisScope: EvaluationAnalysisScope.ProjectOnly,
+                       severity: severity);
+
+        configuration.IsEnabled.ShouldBe(isEnabledExpected);
+    }
+
+    [Fact]
+    public void BuildAnalyzerConfigurationInternal_Constructor_SeverityConfig_Fails()
+    {
+        Should.Throw<ArgumentOutOfRangeException>(() =>
+        {
+            new BuildAnalyzerConfigurationInternal(
+                        ruleId: "ruleId",
+                        evaluationAnalysisScope: EvaluationAnalysisScope.ProjectOnly,
+                        severity: BuildAnalyzerResultSeverity.Default);
+        });
+    }
 }