Linter MVP (Azure#2341)

* WiP: Intial work for Bicep Linter concept * Move DiagnosticExtensions to IDiagnostic * Update to linter messages to use "BCPL" prefix * WiP: merge fix up * fix up merge issues for IDiagnostic * WiP: linter syntax rewrite for concat to Interpolate * Linter Rule BCPL1080: interpolation of single string variable * BCPL1080: fix string empty check * Fix NameEquals comparison * BCPL1060: fix up nested string concatenation reduction * BCP1040: check 'location' properties for literal string definitions * add extension method for name of ObjectPropertySyntax matching * add TODO notes * Add check for IsSecure Parameter * BCPL1070 - remove unneeded dependsOn * BCPL1070 no fix just warning * BCPL1020 and dynamic configuration added * Add a sample settings file * Load linter rules via Reflection * Tests: basic linter analyzer tests * BCPL1000 - Add test harness * BCPL1010: Initial test * BCPL1030: Initial test * Updates for code review comments * clarify naming to indicate base class * Create linter RuleSet through reflection * Code review suggestions on description and error level * set configuration package version to 5.0.0 * revert whitespace changes * Move DependsOnRemovalRewriter to Core to unify and resolve circular dependency * Remove flags for IsCliInvoked in settings and linter rule interface * fix up test for BCPL1030 * Make Analyze in Linter Rule catch exceptions and return a single diagnostic when rule has catastrophic failure * Move pretty print of a syntax element to extension method for SyntaxBase * "Dim" unnecessary code (e.g unused vars) * fix fatal compilation error when lint rules throw * Discard linter rule codes in favor of readable text * fix failed rule code so it won't be localized * Localize rule descriptions * TODO for external linters * move settings within Linter analyzer section called "Core" * Rename tests to remove the old coding scheme * Rewrite Location by Param rule, add tests, part 1 * Add file watching to pick up config changes dynamically * Move default settings to resources and improve perf of configuration load * WiP: settings perf update remove RuleName since we have readable "Code" * WiP: remove debugger launch * Location Set By Param Rule part 2 * Fix crash in param loc rule, fix/restructure tests * add todo * Fix rule descriptions * rouch draft of linter markdown * Add rule docs stubs * Add arm-ttk mention, add rule descriptions and examples * typos * Add full path to docUri * Add markdown table for DiagnosticLevels * ttk blurb updates * linter readme updates * Add gif * correct location * use aka links, change codes to lower case with dashes * WiP: override config in unit tests to silence Linter disagnostics * Unit tests working with linter disabled * create unique aka links, update rule codes per suggestions * @secure decorator, exceptions * missed a reference * add link to create feature request for linter-rule * ttk guidance, prefer-interpolation tweaks * change rule folder to linter-rules, new rule guidance, required param description * fix up merge for IDiagnostic * updated designer generated * Add link to auto add linter-rule tag * fix label in default settings file * Remove ParametersRequiredRule per PM * armTtk347 testcase * Add unused var tests * Add some tests with modules and conditions * minor additions * LocationSetByParameterRuleTests * renamed * Make SecureParameterDefaultRule match TTK * Unit tests for SimplifyInterpolationRuleTests * Fix and add tests for Interpolate-not-concat * fix F5 * WiP: fixing up integration tests to expect linter diagnostics * WiP: fix up Integration tests to expect linter diagnostics * WiP: updating integration tests to handle linter diagnostic expectations * add doc link to linter diagnostics * Minor changes * all rules for saving * Remove LocationSetByParameterRule * another test fixup * Remove UnnecessaryDependsOnRule * remove unused docs * remove unintended change * remove parameters-required.md * Fix bicepsettings.json file in examples * Make CreateDiagnosticForSpan protected and expose AnalyzerDiagnostic as a public class * Remove Enable from Rule contract. Make it an extension method. * correct broken link in readme * typo * fix logic error for IsEnabled * Clean up rule discovery with IsClass, IsPublic, and check for 0 param constructor * Simplify rule class reflection * change bicepsettings.json to bicepconfig.json * Handle exceptions during rule creation * Change level setting in config file Update bicepconfig.json remove InternalsVisibleTo * Update baselines for integration tests with expectation of linter messages * Simplify new line to fix cross-platform testing * baseline diagnostics expectations updates to match newline fix * Update src/Bicep.Core/Analyzers/Linter/LinterAnalyzer.cs * fix build * More linter exception hardening * Use LanguageConstants.IdentifierComparer * fix case-sensitiviy of newGuid * use single quotes, ttk verbage, DiagnosticLevel to level * Guard empy path string * Align resx with rule description markdown files * add output of theAnswer to avoid Linter warning * remove unused lambda * remove testing config files * Change defaults in example config to warnings * use default of Warning for all rule creation * DiagnosticCollectionExtensions => IDiagnosticCollectionExtensions * Fix condition to use established IsInterpolated() method * remove unused extension methods and convert to use IdentifierComparer * clarify Omnisharp diagnostic extensions * fix up baselines for text changes in rules Co-authored-by: jofleish <[email protected]> Co-authored-by: Stephen Weatherford <[email protected]> Co-authored-by: Marcus Felling <[email protected]> Co-authored-by: Marcus Felling <[email protected]> Co-authored-by: Stephen Weatherford (MSFT) <[email protected]>
geekswagg · May 24, 2021 · 5499a7b · 5499a7b
1 parent 064081c
commit 5499a7b
Show file tree

Hide file tree

Showing 129 changed files with 3,954 additions and 282 deletions.
diff --git a/docs/examples/bicepconfig.json b/docs/examples/bicepconfig.json
@@ -0,0 +1,55 @@
+{
+ "analyzers": {
+ "core": {
+ "verbose": true,
+ "enabled": true,
+ "rules": {
+ // If not specified, all the following rules default to warnings
+ "no-hardcoded-env-urls": {
+ "level": "warning",
+ // This only needs to be specified if it should be modified
+ "disallowedhosts": [
+ "management.core.windows.net",
+ "gallery.azure.com",
+ "management.core.windows.net",
+ "management.azure.com",
+ "database.windows.net",
+ "core.windows.net",
+ "login.microsoftonline.com",
+ "graph.windows.net",
+ "trafficmanager.net",
+ "vault.azure.net",
+ "datalake.azure.net",
+ "azuredatalakestore.net",
+ "azuredatalakeanalytics.net",
+ "vault.azure.net",
+ "api.loganalytics.io",
+ "api.loganalytics.iov1",
+ "asazure.windows.net",
+ "region.asazure.windows.net",
+ "api.loganalytics.iov1",
+ "api.loganalytics.io",
+ "asazure.windows.net",
+ "region.asazure.windows.net",
+ "batch.core.windows.net"
+ ]
+ },
+ "no-unused-params": {
+ "level": "warning"
+ },
+ "no-unused-vars": {
+ "level": "warning"
+ },
+ "prefer-interpolation": {
+ "level": "warning"
+ },
+ "secure-paramenter-default": {
+ "level": "warning"
+ },
+ "simplify-interpolation": {
+ "level": "warning"
+ }
+ }
+ }
+ }
+}
diff --git a/docs/images/linter.gif b/docs/images/linter.gif
diff --git a/docs/linter-rules/no-hardcoded-env-urls.md b/docs/linter-rules/no-hardcoded-env-urls.md
@@ -0,0 +1,24 @@
+# Environment URL hardcoded
+
+**Code**: no-hardcoded-env-urls
+
+**Description**: Do not hardcode environment URLs in your template. Instead, use the environment function to dynamically get these URLs during deployment. For a list of the URL hosts that are blocked, see the default list of DisallowedHosts in [`bicepconfig.json`](./src/Bicep.Core/Configuration/bicepconfig.json).
+
+The following example fails this test because the URL is hardcoded.
+
+```bicep
+var AzureURL = 'https://management.azure.com'
+```
+
+The test also fails when used with concat or uri.
+
+```bicep
+var AzureSchemaURL1 = concat('https://','gallery.azure.com')
+var AzureSchemaURL2 = uri('gallery.azure.com','test')
+```
+
+The following example passes this test.
+
+```bicep
+var AzureSchemaURL = environment().gallery
+```
diff --git a/docs/linter-rules/no-unused-params.md b/docs/linter-rules/no-unused-params.md
@@ -0,0 +1,5 @@
+# Parameters must be used
+
+**Code**: no-unused-params
+
+**Description**: To reduce confusion in your template, delete any parameters that are defined but not used. This test finds any parameters that aren't used anywhere in the template. Eliminating unused parameters also makes it easier to deploy your template because you don't have to provide unnecessary values.
diff --git a/docs/linter-rules/no-unused-vars.md b/docs/linter-rules/no-unused-vars.md
@@ -0,0 +1,5 @@
+# Unused variable
+
+**Code**: no-unused-vars
+
+**Description**: To reduce confusion in your template, delete any variables that are defined but not used. This test finds any variables that aren't used anywhere in the template.
diff --git a/docs/linter-rules/prefer-interpolation.md b/docs/linter-rules/prefer-interpolation.md
@@ -0,0 +1,29 @@
+# Interpolation preferred
+
+**Code**: prefer-interpolation
+
+**Description**: String interpolation should be used instead of the concat function.
+
+The following example fails this test because the concat function is used.
+
+```bicep
+param suffix string = '001'
+var vnetName = concat('vnet-', suffix)
+
+resource vnet 'Microsoft.Network/virtualNetworks@2018-10-01' = {
+ name: vnetName
+ ...
+}
+```
+
+The following example passes this test.
+
+```bicep
+param suffix string = '001'
+var vnetName = 'vnet-${suffix}'
+
+resource vnet 'Microsoft.Network/virtualNetworks@2018-10-01' = {
+ name: vnetName
+ ...
+}
+```
diff --git a/docs/linter-rules/secure-paramenter-default.md b/docs/linter-rules/secure-paramenter-default.md
@@ -0,0 +1,23 @@
+# Secure paramenter default
+
+**Code**: secure-paramenter-default
+
+**Description**: Don't provide a hard-coded default value for a secure parameter in your template, unless it is empty or an expression containing a call to newGuid().
+
+You use the [@secure() decorator](../spec/parameters.md) on parameters that contain sensitive values, like passwords. When a parameter uses a secure decorator, the value of the parameter isn't logged or stored in the deployment history. This action prevents a malicious user from discovering the sensitive value.
+
+However, when you provide a default value, that value is discoverable by anyone who can access the template or the deployment history.
+
+The following example fails this test:
+
+```bicep
+@secure()
+param adminPassword string = 'HardcodedPassword'
+```
+
+The following example passes this test.
+
+```bicep
+@secure()
+param adminPassword string
+```
diff --git a/docs/linter-rules/simplify-interpolation.md b/docs/linter-rules/simplify-interpolation.md
@@ -0,0 +1,27 @@
+# Simplify interpolation
+
+**Code**: simplify-interpolation
+
+**Description**: It isn't necessary to use interpolation to reference a parameter or variable.
+
+The following example fails this test.
+
+```bicep
+param AutomationAccountName string
+
+resource AutomationAccount 'Microsoft.Automation/automationAccounts@2020-01-13-preview' = {
+ name: '${AutomationAccountName}'
+ ...
+}
+```
+
+The following example passes this test.
+
+```bicep
+param AutomationAccountName string
+
+resource AutomationAccount 'Microsoft.Automation/automationAccounts@2020-01-13-preview' = {
+ name: AutomationAccountName
+ ...
+}
+```
diff --git a/docs/linter.md b/docs/linter.md
@@ -0,0 +1,38 @@
+# Linter
+
+The Bicep linter will inspect your code and catch a customizable set of authoring best practices. It will surface warnings, errors, or informational messages as you're typing in VS Code for immediate feedback. This means you don't have to build or deploy your code to find out that you've made a mistake. Some rules can also surface an automatic code fix through the VS Code light bulb. 
+
+The linter should make it easier to enforce team coding standards by providing guidance during the development inner loop, as well as the ability to break a build (if desired) during continuous integration (CI) for violations.
+
+![linter demo](./images/linter.gif)
+
+## Configuration
+
+[`bicepconfig.json`](../src/Bicep.Core/Configuration/bicepconfig.json) can be used to:
+
+- enable/disable analyzer
+- set rule-specific values (e.g. DisallowedHosts for [`no-hardcoded-env-urls`](./linter-rules/no-hardcoded-env-urls.md) rule)
+- set level of rules:
+
+| **level** | **Build-time behavior** | **Editor behavior** |
+|--|--|--|
+| `Error` | Violations appear as Errors in command-line build output, and cause builds to fail. | Offending code is underlined with a red squiggle and appears in Problems tab. |
+| `Warning` | Violations appear as Warnings in command-line build output, but do not cause builds to fail. | Offending code is underlined with a yellow squiggle and appears in Problems tab. |
+| `Info` | Violations do not appear in command-line build output. | Offending code is underlined with a blue squiggle and appears in Problems tab. |
+| `Off` | Suppressed completely. | Suppressed completely. |
+
+`bicepconfig.json` can be placed alongside your templates in the same directory. The closest configuration file found up the tree will be used.
+
+## Default rules
+
+There are a set of core rules that are enabled by default, set to `Warning` level. You can find their descriptions in the [`./linter-rules`](./linter-rules) folder.
+
+## Future
+
+The linter is being designed to be extensible so new rules can be added by either the Bicep team or the community. In the 0.5 milestone, we will be focusing more on extensibility, making it as easy as possible to contribute new rules and/or analyzers.
+
+If you have an idea for a new rule, please [submit a feature request](https://github.com/Azure/bicep/issues/new?assignees=&labels=enhancement,linting-rule&template=feature_request.md&title=).
+
+## ARM template test toolkit (arm-ttk)
+
+We've ported over most of the rules from the [ARM template test toolkit (arm-ttk)](https://docs.microsoft.com/azure/azure-resource-manager/templates/test-toolkit) that make sense for Bicep (more to come). We recommend using the Bicep linter when working with Bicep, and arm-ttk when working with ARM templates. It is not advised to run arm-ttk on Bicep generated templates. Bicep output is not intended to pass the arm-ttk rules even if it passes the Bicep rules.
diff --git a/src/Bicep.Cli.IntegrationTests/DecompileTests.cs b/src/Bicep.Cli.IntegrationTests/DecompileTests.cs
@@ -13,13 +13,14 @@
 using FluentAssertions.Execution;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using Bicep.Core.FileSystem;
+using Bicep.Core.Analyzers.Linter.Rules;
 
 namespace Bicep.Cli.IntegrationTests
 {
  [TestClass]
  public class DecompileTests
  {
- private const string ValidTemplate =  @"{
+ private const string ValidTemplate = @"{
  ""$schema"": ""https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#"",
  ""contentVersion"": ""1.0.0.0"",
  ""parameters"": {},
@@ -180,7 +181,8 @@ public void Decompilation_of_file_with_no_errors()
  error.Should().BeEquivalentTo(
  "WARNING: Decompilation is a best-effort process, as there is no guaranteed mapping from ARM JSON to Bicep.",
  "You may need to fix warnings and errors in the generated bicep file(s), or decompilation may fail entirely if an accurate conversion is not possible.",
- "If you would like to report any issues or inaccurate conversions, please see https://github.com/Azure/bicep/issues.");
+ "If you would like to report any issues or inaccurate conversions, please see https://github.com/Azure/bicep/issues."
+ );
  result.Should().Be(0);
  }
 
@@ -235,7 +237,8 @@ public void Decompilation_of_file_with_no_errors_to_stdout()
  error.Should().BeEquivalentTo(
  "WARNING: Decompilation is a best-effort process, as there is no guaranteed mapping from ARM JSON to Bicep.",
  "You may need to fix warnings and errors in the generated bicep file(s), or decompilation may fail entirely if an accurate conversion is not possible.",
- "If you would like to report any issues or inaccurate conversions, please see https://github.com/Azure/bicep/issues.");
+ "If you would like to report any issues or inaccurate conversions, please see https://github.com/Azure/bicep/issues."
+ );
  result.Should().Be(0);
  }
  }
@@ -247,7 +250,6 @@ public void Decompilation_of_file_with_no_errors_to_outfile()
  File.WriteAllText(fileName, ValidTemplate);
 
  var bicepFileName = Path.ChangeExtension(fileName, "bicep");
-
  var (output, error, result) = ExecuteProgram("decompile", "--outfile", bicepFileName, fileName);
 
  using (new AssertionScope())
@@ -256,7 +258,8 @@ public void Decompilation_of_file_with_no_errors_to_outfile()
  error.Should().BeEquivalentTo(
  "WARNING: Decompilation is a best-effort process, as there is no guaranteed mapping from ARM JSON to Bicep.",
  "You may need to fix warnings and errors in the generated bicep file(s), or decompilation may fail entirely if an accurate conversion is not possible.",
- "If you would like to report any issues or inaccurate conversions, please see https://github.com/Azure/bicep/issues.");
+ "If you would like to report any issues or inaccurate conversions, please see https://github.com/Azure/bicep/issues."
+ );
  result.Should().Be(0);
  }
 
@@ -282,7 +285,8 @@ public void Decompilation_of_file_with_no_errors_to_outdir()
  error.Should().BeEquivalentTo(
  "WARNING: Decompilation is a best-effort process, as there is no guaranteed mapping from ARM JSON to Bicep.",
  "You may need to fix warnings and errors in the generated bicep file(s), or decompilation may fail entirely if an accurate conversion is not possible.",
- "If you would like to report any issues or inaccurate conversions, please see https://github.com/Azure/bicep/issues.");
+ "If you would like to report any issues or inaccurate conversions, please see https://github.com/Azure/bicep/issues."
+ );
  result.Should().Be(0);
  }
 
@@ -341,7 +345,7 @@ public void Decompilation_of_invalid_input_paths_to_stdout_should_produce_expect
  var (output, error, result) = ExecuteProgram("decompile", "--stdout", badPath);
  var expectedErrorBadPath = Path.GetFullPath(badPath);
  var expectedErrorBadUri = PathHelper.FilePathToFileUrl(expectedErrorBadPath);
- 
+
  using (new AssertionScope())
  {
  output.Should().BeEmpty();

diff --git a/src/Bicep.Cli.IntegrationTests/ProgramTests.cs b/src/Bicep.Cli.IntegrationTests/ProgramTests.cs
@@ -6,6 +6,7 @@
 using System.IO;
 using System.Linq;
 using Bicep.Cli.UnitTests;
+using Bicep.Core.Analyzers.Linter.Rules;
 using Bicep.Core.FileSystem;
 using Bicep.Core.Samples;
 using Bicep.Core.Semantics;
@@ -112,6 +113,8 @@ public void ZeroFilesToBuildShouldProduceError()
  error.Should().Be($"The input file path was not specified{Environment.NewLine}");
  }
 
+
+ // TODO: handle variant linter messaging for each data test
  [DataTestMethod]
  [DynamicData(nameof(GetValidDataSets), DynamicDataSourceType.Method, DynamicDataDisplayNameDeclaringType = typeof(DataSet), DynamicDataDisplayName = nameof(DataSet.GetDisplayName))]
  public void BuildSingleFileShouldProduceExpectedTemplate(DataSet dataSet)

diff --git a/src/Bicep.Cli/Logging/BicepDiagnosticLogger.cs b/src/Bicep.Cli/Logging/BicepDiagnosticLogger.cs
@@ -22,7 +22,7 @@ public BicepDiagnosticLogger(ILogger logger)
  this.HasLoggedErrors = false;
  }
 
- public void LogDiagnostic(Uri fileUri, Diagnostic diagnostic, ImmutableArray<int> lineStarts)
+ public void LogDiagnostic(Uri fileUri, IDiagnostic diagnostic, ImmutableArray<int> lineStarts)
  {
  (int line, int character) = TextCoordinateConverter.GetPosition(lineStarts, diagnostic.Span.Position);
  string message = $"{fileUri.LocalPath}({line + 1},{character + 1}) : {diagnostic.Level} {diagnostic.Code}: {diagnostic.Message}";

diff --git a/src/Bicep.Cli/Logging/IDiagnosticLogger.cs b/src/Bicep.Cli/Logging/IDiagnosticLogger.cs
@@ -9,7 +9,7 @@ namespace Bicep.Cli.Logging
 {
  public interface IDiagnosticLogger
  {
- void LogDiagnostic(Uri fileUri, Diagnostic diagnostic, ImmutableArray<int> lineStarts);
+ void LogDiagnostic(Uri fileUri, IDiagnostic diagnostic, ImmutableArray<int> lineStarts);
 
  bool HasLoggedErrors { get; }
  }

diff --git a/src/Bicep.Core.IntegrationTests/DecoratorTests.cs b/src/Bicep.Core.IntegrationTests/DecoratorTests.cs
@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation.
+// Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 using System;
 using System.Collections.Generic;
@@ -7,6 +7,7 @@
 using Bicep.Core.Semantics;
 using Bicep.Core.UnitTests.Assertions;
 using Bicep.Core.UnitTests.Utils;
+using Bicep.Core.Analyzers.Linter.Rules;
 using FluentAssertions;
 using FluentAssertions.Execution;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
@@ -84,6 +85,7 @@ public void ParameterDecorator_AttachedToOtherKindsOfDeclarations_CannotBeUsedAs
  {
  diagnosticsByFile[mainUri].Should().HaveDiagnostics(new[] {
  ("BCP126", DiagnosticLevel.Error, "Function \"maxLength\" cannot be used as a variable decorator."),
+ (UnusedVariableRule.Code, DiagnosticLevel.Warning, new UnusedVariableRule().GetMessage()),
  ("BCP127", DiagnosticLevel.Error, "Function \"allowed\" cannot be used as a resource decorator."),
  ("BCP128", DiagnosticLevel.Error, "Function \"secure\" cannot be used as a module decorator."),
  ("BCP129", DiagnosticLevel.Error, "Function \"minValue\" cannot be used as an output decorator."),
@@ -104,6 +106,7 @@ public void NonDecoratorFunction_MissingDeclaration_CannotBeUsedAsDecorator()
  template.Should().NotHaveValue();
  diagnostics.Should().HaveDiagnostics(new[] {
  ("BCP152", DiagnosticLevel.Error, "Function \"concat\" cannot be used as a decorator."),
+ (InterpolateNotConcatRule.Code, DiagnosticLevel.Warning, new InterpolateNotConcatRule().GetMessage()),
  ("BCP132", DiagnosticLevel.Error, "Expected a declaration after the decorator."),
  ("BCP152", DiagnosticLevel.Error, "Function \"resourceId\" cannot be used as a decorator.")
  });
@@ -164,7 +167,10 @@ public void NonDecoratorFunction_AttachedToDeclaration_CannotBeUsedAsDecorator()
  {
  diagnosticsByFile[mainUri].Should().HaveDiagnostics(new[] {
  ("BCP152", DiagnosticLevel.Error, "Function \"resourceId\" cannot be used as a decorator."),
+ (ParametersMustBeUsedRule.Code, DiagnosticLevel.Warning, new ParametersMustBeUsedRule().GetMessage()),
  ("BCP152", DiagnosticLevel.Error, "Function \"concat\" cannot be used as a decorator."),
+ (InterpolateNotConcatRule.Code, DiagnosticLevel.Warning, new InterpolateNotConcatRule().GetMessage()),
+ (UnusedVariableRule.Code, DiagnosticLevel.Warning, new UnusedVariableRule().GetMessage()),
  ("BCP152", DiagnosticLevel.Error, "Function \"environment\" cannot be used as a decorator."),
  ("BCP152", DiagnosticLevel.Error, "Function \"union\" cannot be used as a decorator."),
  ("BCP152", DiagnosticLevel.Error, "Function \"guid\" cannot be used as a decorator."),