From 1ae3143761422d397b61f9e20ada51a0914030bd Mon Sep 17 00:00:00 2001 From: Anthony Martin <38542602+anthony-c-martin@users.noreply.github.com> Date: Thu, 12 Dec 2024 16:32:10 -0500 Subject: [PATCH] `use-safe-access` linter rule: Expand to check for nullable properties (#15838) Some examples where a replacement will be suggested: ## Example 1 ### Input ```bicep param foo { bar: string? } output bar string? = foo.bar ``` ### Replacement ```bicep param foo { bar: string? } output bar string? = foo.?bar ``` ## Example 2 `module.bicep` has declared the `bar` output as a nullable string. ### Input ```bicep module foo 'module.bicep' = { name: 'foo' } output bar string? = foo.outputs.bar ``` ### Replacement ```bicep module foo 'module.bicep' = { name: 'foo' } output bar string? = foo.outputs.?bar ``` Closes #15823 --- .../LinterRuleTests/LinterRuleTestsBase.cs | 5 +- .../LinterRuleTests/UseSafeAccessRuleTests.cs | 96 ++++++++++++++++++- .../Utils/CompilationHelper.cs | 7 ++ .../Linter/Rules/UseSafeAccessRule.cs | 37 ++++++- src/Bicep.Core/CoreResources.Designer.cs | 15 ++- src/Bicep.Core/CoreResources.resx | 9 +- .../schemas/bicepconfig.schema.json | 2 +- 7 files changed, 159 insertions(+), 12 deletions(-) diff --git a/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/LinterRuleTestsBase.cs b/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/LinterRuleTestsBase.cs index f1c431d9259..44697355482 100644 --- a/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/LinterRuleTestsBase.cs +++ b/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/LinterRuleTestsBase.cs @@ -128,10 +128,11 @@ private static bool IsCompilerDiagnostic(IDiagnostic diagnostic) return diagnostic.Code.StartsWith("BCP"); } - protected static void AssertCodeFix(string expectedCode, string expectedFixTitle, string inputFile, string resultFile) + protected static void AssertCodeFix(string expectedCode, string expectedFixTitle, string inputFile, string resultFile, CompilationHelper.InputFile[]? supportingFiles = null) { + supportingFiles ??= []; var (file, cursor) = ParserHelper.GetFileWithSingleCursor(inputFile, '|'); - var result = CompilationHelper.Compile(file); + var result = CompilationHelper.Compile([..supportingFiles, new("main.bicep", file)]); using (new AssertionScope().WithVisualCursor(result.Compilation.GetEntrypointSemanticModel().SourceFile, cursor)) { diff --git a/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs b/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs index ee3f50b4796..0ea3c5fa381 100644 --- a/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs +++ b/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using Bicep.Core.Analyzers.Linter.Rules; +using Bicep.Core.UnitTests.Utils; using Microsoft.VisualStudio.TestTools.UnitTesting; namespace Bicep.Core.UnitTests.Diagnostics.LinterRuleTests; @@ -9,8 +10,8 @@ namespace Bicep.Core.UnitTests.Diagnostics.LinterRuleTests; [TestClass] public class UseSafeAccessRuleTests : LinterRuleTestsBase { - private void AssertCodeFix(string inputFile, string resultFile) - => AssertCodeFix(UseSafeAccessRule.Code, "Use the safe access (.?) operator", inputFile, resultFile); + private void AssertCodeFix(string inputFile, string resultFile, CompilationHelper.InputFile[]? supportingFiles = null) + => AssertCodeFix(UseSafeAccessRule.Code, "Use the safe access (.?) operator", inputFile, resultFile, supportingFiles); private void AssertNoDiagnostics(string inputFile) => AssertLinterRuleDiagnostics(UseSafeAccessRule.Code, inputFile, [], new(OnCompileErrors.Ignore, IncludePosition.None)); @@ -148,4 +149,95 @@ param roleAssignments roleAssignmentType roleDefinitionIdOrName: string }[]? """); + + [TestMethod] + public void Codefix_recommends_safe_access_for_nullable_property_access() => AssertCodeFix(""" +param foo { + bar: string? +} + +output bar string? = foo.b|ar +""", """ +param foo { + bar: string? +} + +output bar string? = foo.?bar +"""); + + [TestMethod] + public void Rule_ignores_non_nullable_property_access() => AssertNoDiagnostics(""" +param foo { + bar: string +} + +output bar string? = foo.bar +"""); + + [TestMethod] + public void Codefix_recommends_safe_access_for_nullable_module_output_property_access() => AssertCodeFix(""" +module foo 'module.bicep' = { + name: 'foo' +} + +output bar string? = foo.outputs.b|ar +""", """ +module foo 'module.bicep' = { + name: 'foo' +} + +output bar string? = foo.outputs.?bar +""", +supportingFiles: [new("module.bicep", """ +output bar string? = null +""")]); + + [TestMethod] + public void Codefix_recommends_safe_access_for_nullable_array_access() => AssertCodeFix(""" +param foo { + bar: string? +} + +output bar string? = foo['b|ar'] +""", """ +param foo { + bar: string? +} + +output bar string? = foo[?'bar'] +"""); + + [TestMethod] + public void Rule_ignores_non_nullable_array_access() => AssertNoDiagnostics(""" +param foo { + bar: string +} + +output bar string? = foo['bar'] +"""); + + [TestMethod] + public void Rule_ignores_array_access_on_array() => AssertNoDiagnostics(""" +param foo (string | null)[] + +output bar string? = foo[0] +"""); + + [TestMethod] + public void Codefix_recommends_safe_access_for_nullable_module_output_array_access() => AssertCodeFix(""" +module foo 'module.bicep' = { + name: 'foo' +} + +output bar string? = foo.outputs['b|ar'] +""", """ +module foo 'module.bicep' = { + name: 'foo' +} + +output bar string? = foo.outputs[?'bar'] +""", +supportingFiles: [new("module.bicep", """ +output bar string? = null +""")]); } diff --git a/src/Bicep.Core.UnitTests/Utils/CompilationHelper.cs b/src/Bicep.Core.UnitTests/Utils/CompilationHelper.cs index 61a8e348e94..f6d9374becb 100644 --- a/src/Bicep.Core.UnitTests/Utils/CompilationHelper.cs +++ b/src/Bicep.Core.UnitTests/Utils/CompilationHelper.cs @@ -18,6 +18,10 @@ namespace Bicep.Core.UnitTests.Utils { public static class CompilationHelper { + public record InputFile( + string FileName, + string Contents); + public interface ICompilationResult { IEnumerable Diagnostics { get; } @@ -140,6 +144,9 @@ public static CompilationResult Compile(ServiceBuilder services, IFileResolver f public static CompilationResult Compile(IResourceTypeLoader resourceTypeLoader, params (string fileName, string fileContents)[] files) => Compile(new ServiceBuilder().WithFeatureOverrides(BicepTestConstants.FeatureOverrides).WithAzResourceTypeLoader(resourceTypeLoader), files); + public static CompilationResult Compile(params InputFile[] files) + => Compile(files.Select(x => (x.FileName, x.Contents)).ToArray()); + public static CompilationResult Compile(params (string fileName, string fileContents)[] files) => Compile(new ServiceBuilder().WithFeatureOverrides(BicepTestConstants.FeatureOverrides), files); diff --git a/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs b/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs index 9efe27cc1c0..35b4f85d112 100644 --- a/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs +++ b/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs @@ -9,6 +9,8 @@ using Bicep.Core.Syntax; using Bicep.Core.Syntax.Comparers; using Bicep.Core.Syntax.Visitors; +using Bicep.Core.TypeSystem; +using Bicep.Core.TypeSystem.Types; namespace Bicep.Core.Analyzers.Linter.Rules; @@ -66,7 +68,40 @@ public override IEnumerable AnalyzeInternal(SemanticModel model, Di isPreferred: true, CodeFixKind.QuickFix, new CodeReplacement(ternary.Span, replacement.ToString())), - CoreResources.UseSafeAccessRule_MessageFormat); + CoreResources.UseSafeAccessRule_ContainsReplacement_MessageFormat); + } + + foreach (var access in SyntaxAggregator.AggregateByType(model.Root.Syntax)) + { + if (access.IsSafeAccess) + { + continue; + } + + var baseType = model.GetTypeInfo(access.BaseExpression); + if (baseType is not ObjectType) + { + // avoid incorrectly flagging array access - e.g. [null][0] + continue; + } + + var accessType = model.GetTypeInfo(access); + if (!TypeHelper.IsNullable(accessType)) + { + continue; + } + + var replacement = access.AsSafeAccess(); + + yield return CreateFixableDiagnosticForSpan( + diagnosticLevel, + access.Span, + new CodeFix( + CoreResources.UseSafeAccessRule_CodeFix, + isPreferred: true, + CodeFixKind.QuickFix, + new CodeReplacement(access.Span, replacement.ToString())), + CoreResources.UseSafeAccessRule_NullCheckReplacement_MessageFormat); } } } diff --git a/src/Bicep.Core/CoreResources.Designer.cs b/src/Bicep.Core/CoreResources.Designer.cs index 7cc67ea4135..c9e8ec99920 100644 --- a/src/Bicep.Core/CoreResources.Designer.cs +++ b/src/Bicep.Core/CoreResources.Designer.cs @@ -1069,7 +1069,7 @@ internal static string UseSafeAccessRule_CodeFix { } /// - /// Looks up a localized string similar to Use the safe access (.?) operator instead of checking object contents with the 'contains' function.. + /// Looks up a localized string similar to Use the safe access (.?) operator.. /// internal static string UseSafeAccessRule_Description { get { @@ -1080,9 +1080,18 @@ internal static string UseSafeAccessRule_Description { /// /// Looks up a localized string similar to The syntax can be simplified by using the safe access (.?) operator.. /// - internal static string UseSafeAccessRule_MessageFormat { + internal static string UseSafeAccessRule_ContainsReplacement_MessageFormat { get { - return ResourceManager.GetString("UseSafeAccessRule_MessageFormat", resourceCulture); + return ResourceManager.GetString("UseSafeAccessRule_ContainsReplacement_MessageFormat", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to The property being accessed may be null. Use the (.?) operator to handle this safely.. + /// + internal static string UseSafeAccessRule_NullCheckReplacement_MessageFormat { + get { + return ResourceManager.GetString("UseSafeAccessRule_NullCheckReplacement_MessageFormat", resourceCulture); } } diff --git a/src/Bicep.Core/CoreResources.resx b/src/Bicep.Core/CoreResources.resx index 03df2d8fd31..217a23e4878 100644 --- a/src/Bicep.Core/CoreResources.resx +++ b/src/Bicep.Core/CoreResources.resx @@ -512,14 +512,17 @@ Resource-derived types - Use the safe access (.?) operator instead of checking object contents with the 'contains' function. + Use the safe access (.?) operator. Use the safe access (.?) operator - + The syntax can be simplified by using the safe access (.?) operator. + + The property being accessed may be null. Use the (.?) operator to handle this safely. + Runtime values should not be used to determine resource IDs @@ -530,4 +533,4 @@ Secure outputs - + diff --git a/src/vscode-bicep/schemas/bicepconfig.schema.json b/src/vscode-bicep/schemas/bicepconfig.schema.json index e246e3df374..3fc508846cd 100644 --- a/src/vscode-bicep/schemas/bicepconfig.schema.json +++ b/src/vscode-bicep/schemas/bicepconfig.schema.json @@ -664,7 +664,7 @@ "use-safe-access": { "allOf": [ { - "description": "Use the safe access (.?) operator instead of checking object contents with the 'contains' function. Defaults to 'Warning'. See https://aka.ms/bicep/linter/use-safe-access" + "description": "Use the safe access (.?) operator. Defaults to 'Warning'. See https://aka.ms/bicep/linter/use-safe-access" }, { "$ref": "#/definitions/rule-def-level-warning"