Implement: Finalize default levels for linting rules

src/Bicep.Core.UnitTests/Configuration/BicepConfigSchemaTests.cs

@@ -1,7 +1,9 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
+using System.Collections.Immutable;
 using System.Diagnostics.CodeAnalysis;
+using System.Reflection;
 using System.Text.RegularExpressions;
 using Bicep.Core.Analyzers.Interfaces;
 using Bicep.Core.Analyzers.Linter;
@@ -23,6 +25,25 @@ public class BicepConfigSchemaTests
         [NotNull]
         public TestContext? TestContext { get; set; }
 
+        [AttributeUsage(AttributeTargets.Method, AllowMultiple = false)]
+        public class RuleAndSchemaTestDataAttribute : Attribute, ITestDataSource
+        {
+            public IEnumerable<object[]> GetData(MethodInfo methodInfo)
+            {
+                var analyzer = new LinterAnalyzer();
+                var ruleSet = analyzer.GetRuleSet().ToArray();
+
+                return AllRulesAndSchemasById.Values.Select(value => new object[] { value.Rule.Code, value.Rule, value.Schema});
+            }
+
+            public string? GetDisplayName(MethodInfo methodInfo, object?[]? data)
+            {
+                var id = (data?[0] as string)!;
+
+                return $"{methodInfo.Name} ({id})";
+            }
+        }
+
         private const string BicepRootConfigFilePath = "src/Bicep.Core/Configuration/bicepconfig.json";
         private const string BicepConfigSchemaFilePath = "src/vscode-bicep/schemas/bicepconfig.schema.json";
 
@@ -34,7 +55,7 @@ public class BicepConfigSchemaTests
                 "providerRegistry",
             };
 
-        private string GetBicepConfigSchemaContents()
+        private static string GetBicepConfigSchemaContents()
         {
             var configStream = typeof(BicepConfigSchemaTests).Assembly.GetManifestResourceStream(
                 $"{typeof(BicepConfigSchemaTests).Assembly.GetName().Name}.bicepconfig.schema.json");
@@ -44,55 +65,56 @@ private string GetBicepConfigSchemaContents()
             return configContents;
         }
 
-        private JObject BicepConfigSchema => JObject.Parse(GetBicepConfigSchemaContents());
+        private static JObject BicepConfigSchema => JObject.Parse(GetBicepConfigSchemaContents());
 
-        private JSchema BicepConfigSchemaAsJSchema => JSchema.Parse(GetBicepConfigSchemaContents());
+        private static JSchema BicepConfigSchemaAsJSchema => JSchema.Parse(GetBicepConfigSchemaContents());
 
-        private IBicepAnalyzerRule[] AllRules
+        private static ImmutableArray<IBicepAnalyzerRule> AllRules
         {
             get
             {
                 var linter = new LinterAnalyzer();
                 var ruleSet = linter.GetRuleSet();
                 ruleSet.Should().NotBeEmpty();
 
-                return ruleSet.ToArray();
+                return ruleSet.ToImmutableArray();
             }
         }
 
-        private (string Id, JObject Schema)[] AllRuleSchemas =>
+        private static (string Id, JObject Schema)[] AllRuleSchemas =>
             (BicepConfigSchema.SelectToken("properties.analyzers.properties.core.properties.rules.properties") as JObject)!
             .Children<JProperty>()
             .Select(prop => (prop.Name, (JObject)prop.Value))
             .ToArray();
 
-        private IDictionary<string, JObject> AllRuleSchemasById =>
-            BicepConfigSchema.SelectToken("properties.analyzers.properties.core.properties.rules.properties")!.ToObject<IDictionary<string, JObject>>()!;
+        private static IImmutableDictionary<string, JObject> AllRuleSchemasById =>
+            BicepConfigSchema.SelectToken("properties.analyzers.properties.core.properties.rules.properties")!.ToObject<IDictionary<string, JObject>>()!
+            .ToImmutableDictionary();
 
-        private IDictionary<string, (IBicepAnalyzerRule Rule, JObject Schema)> AllRulesAndSchemasById =>
+        private static IImmutableDictionary<string, (IBicepAnalyzerRule Rule, JObject Schema)> AllRulesAndSchemasById =>
                 AllRules
                     .Join(AllRuleSchemas,
                           rule => rule.Code,
                           ruleSchema => ruleSchema.Id,
                           (rule, ruleSchema) => new { Id = rule.Code, Rule = rule, ruleSchema.Schema })
-                    .ToDictionary(rs => rs.Id, rs => (rs.Rule, rs.Schema));
+                    .ToImmutableDictionary(rs => rs.Id, rs => (rs.Rule, rs.Schema));
 
-        private IEnumerable<JProperty> GetRuleCustomConfigurationProperties(JObject ruleConfigSchema)
+        private static IEnumerable<JProperty> GetRuleCustomConfigurationProperties(JObject ruleConfigSchema)
         {
             var properties = ruleConfigSchema.SelectToken("allOf[0].properties")?.OfType<JProperty>();
             return properties ?? Enumerable.Empty<JProperty>();
         }
 
-        private IDictionary<string, JObject> GetExperimentalFeaturesFromSchema()
+        private IImmutableDictionary<string, JObject> GetExperimentalFeaturesFromSchema()
         {
             IDictionary<string, JObject>? experimentalFeatures = BicepConfigSchema.SelectToken("properties.experimentalFeaturesEnabled.properties")!.ToObject<IDictionary<string, JObject>>();
             Assert.IsNotNull(experimentalFeatures);
-            return experimentalFeatures;
+            return experimentalFeatures.ToImmutableDictionary();
         }
 
-        private ICollection<string> GetExperimentalFeatureIdsFromSchema()
+        private IEnumerable<string> GetExperimentalFeatureIdsFromSchema()
         {
-            IDictionary<string, JObject>? experimentalFeatures = GetExperimentalFeaturesFromSchema();
+            IImmutableDictionary<string, JObject>? experimentalFeatures = GetExperimentalFeaturesFromSchema();
             return experimentalFeatures.Keys;
         }
 
@@ -278,7 +300,7 @@ public void NoHardCodedEnvUrls_DefaultsShouldMatchInConfigAndSchema()
         public void ExperimentalFeatures_ShouldHaveDescription()
         {
             // From schema
-            IDictionary<string, JObject>? experimentalFeatures = GetExperimentalFeaturesFromSchema();
+            IImmutableDictionary<string, JObject>? experimentalFeatures = GetExperimentalFeaturesFromSchema();
 
             foreach (var (featureName, featureValue) in experimentalFeatures)
             {
@@ -326,7 +348,7 @@ public void ExperimentalFeatures_ShouldHaveDescription()
         public void ExperimentalFeatures_DescriptionsShouldEndWithLinkToHelpPage()
         {
             // From schema
-            IDictionary<string, JObject>? experimentalFeatures = GetExperimentalFeaturesFromSchema();
+            IImmutableDictionary<string, JObject>? experimentalFeatures = GetExperimentalFeaturesFromSchema();
 
             foreach (var (featureName, featureValue) in experimentalFeatures)
             {
@@ -407,16 +429,14 @@ public void UserConfig_SysProviderIsProhibited()
         }
 
         [TestMethod]
-        public void RuleConfigs_DefaultLevelShouldMatchRuleDefinition()
+        [RuleAndSchemaTestData]
+        public void RuleConfigs_DefaultLevelShouldMatchRuleDefinition(string id, IBicepAnalyzerRule rule, JObject ruleSchema)
         {
-            foreach (var (id, (rule, ruleSchema)) in AllRulesAndSchemasById)
-            {
-                var defaultLevelInRuleDefinition = rule.DefaultDiagnosticLevel.ToString();
-                var defaultLevelInSchema = GetRuleDefaultLevel(ruleSchema);
+            var defaultLevelInRuleDefinition = rule.DefaultDiagnosticLevel.ToString();
+            var defaultLevelInSchema = GetRuleDefaultLevel(ruleSchema);
 
-                defaultLevelInSchema.Should().Be(defaultLevelInRuleDefinition,
-                    $"the default diagnostic level of a rule's config schema should match that defined in the rule's class definition. Make sure rule {id} #/definitions/rule-def-level-xxx reference is correct");
-            }
+            defaultLevelInSchema.Should().Be(defaultLevelInRuleDefinition,
+                $"the default diagnostic level of a rule's config schema should match that defined in the rule's class definition (make sure rule {id} #/definitions/rule-def-level-xxx reference is correct)");
         }
     }
 }

src/Bicep.Core.UnitTests/Diagnostics/LinterAnalyzerTests.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
+using System.Reflection;
 using Bicep.Core.Analyzers;
 using Bicep.Core.Analyzers.Interfaces;
 using Bicep.Core.Analyzers.Linter;
@@ -17,6 +18,25 @@ namespace Bicep.Core.UnitTests.Diagnostics
     [TestClass]
     public class LinterAnalyzerTests
     {
+        [AttributeUsage(AttributeTargets.Method, AllowMultiple = false)]
+        public class TestDataAttribute : Attribute, ITestDataSource
+        {
+            public IEnumerable<object[]> GetData(MethodInfo methodInfo)
+            {
+                var analyzer = new LinterAnalyzer();
+                var ruleSet = analyzer.GetRuleSet().ToArray();
+
+                return ruleSet.Select(rule => new object[] { rule });
+            }
+
+            public string? GetDisplayName(MethodInfo methodInfo, object?[]? data)
+            {
+                var baselineData = (data?[0] as IBicepAnalyzerRule)!;
+
+                return $"{methodInfo.Name} ({baselineData.Code})";
+            }
+        }
+
         [TestMethod]
         public void HasBuiltInRules()
         {
@@ -38,7 +58,7 @@ public void BuiltInRulesExistSanityCheck(string ruleCode)
         }
 
         [TestMethod]
-        public void AllDefinedRulesAreListInLinterRulesProvider()
+        public void AllDefinedRulesAreListedInLinterRulesProvider()
         {
             var linter = new LinterAnalyzer();
             var ruleTypes = linter.GetRuleSet().Select(r => r.GetType()).ToArray();
@@ -78,17 +98,35 @@ public void MostRulesEnabledByDefault()
             numberEnabled.Should().BeGreaterThan(ruleSet.Length / 2, "most rules should probably be enabled by default");
         }
 
-        [TestMethod]
-        public void AllRulesHaveDescription()
+        [DataTestMethod]
+        [TestData]
+        public void AllRulesHaveDescription(IBicepAnalyzerRule rule)
         {
-            var analyzer = new LinterAnalyzer();
-            var ruleSet = analyzer.GetRuleSet();
-            ruleSet.Should().OnlyContain(r => r.Description.Length > 0);
+            rule.Description.Length.Should().BeGreaterThan(0);
+        }
+
+        [DataTestMethod()]
+        [TestData]
+        public void RulesShouldNotSpecifyOverriddenDiagnosticLevel_UnlessDifferingFromCategoryDefault(IBicepAnalyzerRule rule)
+        {
+            if (rule is LinterRuleBase ruleBase)
+            {
+                if (ruleBase.OverrideCategoryDefaultDiagnosticLevel.HasValue)
+                {
+                    ruleBase.DefaultDiagnosticLevel.Should().NotBe(LinterRuleBase.GetDefaultDiagosticLevelForCategory(ruleBase.Category),
+                        "Do not specify a value for OverrideCategoryDefaultDiagnosticLevel unless it is overriding the default diagnostic level for that rule's category " +
+                            "(and usually that should not be done).");
+
+                    ruleBase.DefaultDiagnosticLevel.Should().Be(DiagnosticLevel.Off,
+                        "I think the reason for overriding the default diagnostic level of a rule's category should only be to turn it to Off by default " +
+                            "(if there turn out to be valid reasons for something different, this test will need to be changed)");
+                }
+            }
         }
 
         public class LinterThrowsTestRule : LinterRuleBase
         {
-            public LinterThrowsTestRule() : base("ThrowsRule", "Throws an exception when used", null, DiagnosticLevel.Warning) { }
+            public LinterThrowsTestRule() : base("ThrowsRule", "Throws an exception when used", LinterRuleCategory.Style) { }
 
             public override IEnumerable<IDiagnostic> AnalyzeInternal(SemanticModel model, DiagnosticLevel diagnosticLevel)
             {

src/Bicep.Core/Analyzers/Linter/LinterRuleBase.cs

@@ -15,30 +15,39 @@ public abstract class LinterRuleBase : IBicepAnalyzerRule
         public LinterRuleBase(
             string code,
             string description,
+            LinterRuleCategory category,
             Uri? docUri = null,
-            DiagnosticLevel diagnosticLevel = DiagnosticLevel.Warning,
-            DiagnosticStyling diagnosticStyling = DiagnosticStyling.Default)
+            DiagnosticStyling diagnosticStyling = DiagnosticStyling.Default,
+            // This should normally be left unspecified so that the default diagnostic level is set based on the category.  Only specify
+            //   if it needs to default to something other than the category's default diagnostic level.
+            DiagnosticLevel? overrideCategoryDefaultDiagnosticLevel = default)
         {
             this.AnalyzerName = LinterAnalyzer.AnalyzerName;
             this.Code = code;
             this.Description = description;
             this.Uri = docUri;
-            this.DefaultDiagnosticLevel = diagnosticLevel;
+            this.Category = category;
             this.DiagnosticStyling = diagnosticStyling;
+            this.OverrideCategoryDefaultDiagnosticLevel = overrideCategoryDefaultDiagnosticLevel;
         }
 
         public string AnalyzerName { get; }
 
         public string Code { get; }
 
+        public LinterRuleCategory Category { get; }
+
         public readonly string RuleConfigSection = $"{LinterAnalyzer.AnalyzerName}.rules";
 
-        public DiagnosticLevel DefaultDiagnosticLevel { get; }
+        public DiagnosticLevel DefaultDiagnosticLevel =>
+            OverrideCategoryDefaultDiagnosticLevel.HasValue ? OverrideCategoryDefaultDiagnosticLevel.Value : GetDefaultDiagosticLevelForCategory(this.Category);
 
         public string Description { get; }
 
         public Uri? Uri { get; }
 
+        public DiagnosticLevel? OverrideCategoryDefaultDiagnosticLevel { get; }
+
         // If specified, adds the given diagnostic label to every diagnostic created for this rule (such as for unnecessary or obsolete code).
         // Should be left as None/null for most rules.
         public DiagnosticStyling DiagnosticStyling { get; }
@@ -145,5 +154,23 @@ protected virtual AnalyzerFixableDiagnostic CreateFixableDiagnosticForSpan(Diagn
                 documentationUri: this.Uri,
                 codeFixes: fixes,
                 styling: this.DiagnosticStyling);
+
+        public static DiagnosticLevel GetDefaultDiagosticLevelForCategory(LinterRuleCategory category) =>
+            category switch
+            {
+                // Note: In general the default diagnostic level for a category should be either Warning or Off
+                LinterRuleCategory.BestPractice => DiagnosticLevel.Warning,
+                LinterRuleCategory.Portability => DiagnosticLevel.Off,
+                LinterRuleCategory.PotentialCodeIssues => DiagnosticLevel.Warning,
+                LinterRuleCategory.ResourceLocationRules => DiagnosticLevel.Off,
+                LinterRuleCategory.Security => DiagnosticLevel.Warning,
+                LinterRuleCategory.Style => DiagnosticLevel.Warning,
+
+                // This is an exception to the "Warning" or "Off" only rule - these will cause actual deployment errors, so default level is Error
+                LinterRuleCategory.DeploymentError => DiagnosticLevel.Error,
+
+                // Unexpected values
+                _ => throw new ArgumentOutOfRangeException($"LinterRuleCategory (unexpected value \"{category}\")")
+            };
     }
 }

src/Bicep.Core/Analyzers/Linter/LinterRuleCategory.cs

@@ -0,0 +1,23 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+using Bicep.Core.Analyzers.Interfaces;
+using Bicep.Core.CodeAction;
+using Bicep.Core.Configuration;
+using Bicep.Core.Diagnostics;
+using Bicep.Core.Parsing;
+using Bicep.Core.Semantics;
+
+namespace Bicep.Core.Analyzers.Linter;
+
+// Note: These aren't currently exposed to users (but might be later), they're currently used just to determine the default diagnostic level for a rule
+public enum LinterRuleCategory
+{
+    Style,
+    Portability,
+    BestPractice,
+    PotentialCodeIssues,
+    ResourceLocationRules,
+    Security,
+    DeploymentError,
+}

src/Bicep.Core/Analyzers/Linter/Rules/AdminUsernameShouldNotBeLiteralRule.cs

@@ -15,6 +15,7 @@ public sealed class AdminUsernameShouldNotBeLiteralRule : LinterRuleBase
         public AdminUsernameShouldNotBeLiteralRule() : base(
             code: Code,
             description: CoreResources.AdminUsernameShouldNotBeLiteralRuleDescription,
+            LinterRuleCategory.Security,
             docUri: new Uri($"https://aka.ms/bicep/linter/{Code}")
         )
         {

src/Bicep.Core/Analyzers/Linter/Rules/ArtifactsParametersRule.cs

@@ -10,7 +10,7 @@
 
 namespace Bicep.Core.Analyzers.Linter.Rules
 {
-    public sealed partial class ArtifactsParametersRule : LocationRuleBase
+    public sealed partial class ArtifactsParametersRule : LinterRuleBase
     {
         public new const string Code = "artifacts-parameters";
 
@@ -24,6 +24,7 @@ public sealed partial class ArtifactsParametersRule : LocationRuleBase
         public ArtifactsParametersRule() : base(
             code: Code,
             description: "Follow best practices when including the _artifactsLocation and _artifactsLocationSasToken parameters.",
+            LinterRuleCategory.BestPractice,
             docUri: new Uri($"https://aka.ms/bicep/linter/{Code}"))
         {
             Debug.Assert(ArtifactsLocationName.StartsWith("_"));

src/Bicep.Core/Analyzers/Linter/Rules/DecompilerCleanupRule.cs

@@ -9,7 +9,7 @@
 namespace Bicep.Core.Analyzers.Linter.Rules
 {
     // Mark decompiler imperfections that should be manually cleaned up
-    public sealed class DecompilerCleanupRule : LocationRuleBase
+    public sealed class DecompilerCleanupRule : LinterRuleBase
     {
         public new const string Code = "decompiler-cleanup";
 
@@ -19,6 +19,7 @@ public sealed class DecompilerCleanupRule : LocationRuleBase
         public DecompilerCleanupRule() : base(
             code: Code,
             description: CoreResources.DecompilerImperfectionsRule_Description,
+            LinterRuleCategory.BestPractice,
             docUri: new Uri($"https://aka.ms/bicep/linter/{Code}"))
         {
         }

src/Bicep.Core/Analyzers/Linter/Rules/LocationRuleBase.cs

@@ -4,6 +4,7 @@
 using System.Collections.Immutable;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
+using System.Reflection;
 using Bicep.Core.Diagnostics;
 using Bicep.Core.Semantics;
 using Bicep.Core.Semantics.Namespaces;
@@ -25,10 +26,9 @@ public LocationRuleBase(
             string code,
             string description,
             Uri docUri,
-            DiagnosticLevel diagnosticLevel = DiagnosticLevel.Warning,
             DiagnosticStyling diagnosticStyling = DiagnosticStyling.Default
             )
-        : base(code, description, docUri, diagnosticLevel, diagnosticStyling) { }
+        : base(code, description, LinterRuleCategory.ResourceLocationRules, docUri, diagnosticStyling) { }
 
         /// <summary>
         /// Retrieves the literal text value of a syntax node if that node is either a string literal or a reference (possibly indirectly)