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

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))
         {

src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs

@@ -2,15 +2,16 @@
 // 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;
 
 [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
+""")]);
 }

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<IDiagnostic> 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);
 

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<IDiagnostic> 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<AccessExpressionSyntax>(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);            
         }
     }
 }

src/Bicep.Core/CoreResources.resx

@@ -512,14 +512,17 @@
     <value>Resource-derived types</value>
   </data>
   <data name="UseSafeAccessRule_Description" xml:space="preserve">
-    <value>Use the safe access (.?) operator instead of checking object contents with the 'contains' function.</value>
+    <value>Use the safe access (.?) operator.</value>
   </data>
   <data name="UseSafeAccessRule_CodeFix" xml:space="preserve">
     <value>Use the safe access (.?) operator</value>
   </data>
-  <data name="UseSafeAccessRule_MessageFormat" xml:space="preserve">
+  <data name="UseSafeAccessRule_ContainsReplacement_MessageFormat" xml:space="preserve">
     <value>The syntax can be simplified by using the safe access (.?) operator.</value>
   </data>
+  <data name="UseSafeAccessRule_NullCheckReplacement_MessageFormat" xml:space="preserve">
+    <value>The property being accessed may be null. Use the (.?) operator to handle this safely.</value>
+  </data>
   <data name="WhatIfShortCircuitingRuleDescription" xml:space="preserve">
     <value>Runtime values should not be used to determine resource IDs</value>
   </data>
@@ -530,4 +533,4 @@
   <data name="ExperimentalFeatureNames_SecureOutputs" xml:space="preserve">
     <value>Secure outputs</value>
   </data>
-</root>
+</root>

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"