diff --git a/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs b/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs new file mode 100644 index 00000000000..8598020dd59 --- /dev/null +++ b/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs @@ -0,0 +1,57 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Bicep.Core.Analyzers.Linter.Rules; +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 AssertNoDiagnostics(string inputFile) + => AssertLinterRuleDiagnostics(UseSafeAccessRule.Code, inputFile, [], new(OnCompileErrors.Ignore, IncludePosition.None)); + + [TestMethod] + public void Codefix_fixes_syntax_which_can_be_simplified() => AssertCodeFix(""" +param foo object +var test = contai|ns(foo, 'bar') ? foo.bar : 'baz' +""", """ +param foo object +var test = foo.?bar ?? 'baz' +"""); + + [TestMethod] + public void Codefix_fixes_syntax_which_can_be_simplified_array_access() => AssertCodeFix(""" +param foo object +param target string +var test = contai|ns(foo, target) ? foo[target] : 'baz' +""", """ +param foo object +param target string +var test = foo[?target] ?? 'baz' +"""); + + [TestMethod] + public void Rule_ignores_syntax_which_cannot_be_simplified() => AssertNoDiagnostics(""" +param foo object +var test = contains(foo, 'bar') ? foo.baz : 'baz' +"""); + + [TestMethod] + public void Rule_ignores_syntax_which_cannot_be_simplified_2() => AssertNoDiagnostics(""" +param foo object +param target string +param notTarget string +var test = contains(foo, target) ? bar[notTarget] : 'baz' +"""); + + [TestMethod] + public void Rule_ignores_syntax_which_cannot_be_simplified_array_access() => AssertNoDiagnostics(""" +param foo object +var test = contains(foo, 'bar') ? bar.bar : 'baz' +"""); +} diff --git a/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs b/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs new file mode 100644 index 00000000000..abb15fa80b6 --- /dev/null +++ b/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs @@ -0,0 +1,63 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Bicep.Core.CodeAction; +using Bicep.Core.Diagnostics; +using Bicep.Core.Parsing; +using Bicep.Core.Semantics; +using Bicep.Core.Semantics.Namespaces; +using Bicep.Core.Syntax; +using Bicep.Core.Syntax.Comparers; +using Bicep.Core.Syntax.Visitors; + +namespace Bicep.Core.Analyzers.Linter.Rules; + +public sealed class UseSafeAccessRule : LinterRuleBase +{ + public new const string Code = "use-safe-access"; + + public UseSafeAccessRule() : base( + code: Code, + description: CoreResources.UseSafeAccessRule_Description, + LinterRuleCategory.BestPractice, + docUri: new Uri($"https://aka.ms/bicep/linter/{Code}")) + { } + + public override IEnumerable AnalyzeInternal(SemanticModel model, DiagnosticLevel diagnosticLevel) + { + foreach (var ternary in SyntaxAggregator.AggregateByType(model.Root.Syntax)) + { + if (SemanticModelHelper.TryGetNamedFunction(model, SystemNamespaceType.BuiltInName, "contains", ternary.ConditionExpression) is not {} functionCall || + functionCall.Arguments.Length != 2) + { + continue; + } + + if (ternary.TrueExpression is not AccessExpressionSyntax truePropertyAccess || + !SyntaxIgnoringTriviaComparer.Instance.Equals(functionCall.Arguments[0].Expression, truePropertyAccess.BaseExpression)) + { + continue; + } + + if (!truePropertyAccess.AccessExpressionMatches(functionCall.Arguments[1].Expression)) + { + continue; + } + + var replacement = SyntaxFactory.CreateBinaryOperationSyntax( + SyntaxFactory.CreateSafeAccess(truePropertyAccess.BaseExpression, functionCall.Arguments[1].Expression), + TokenType.DoubleQuestion, + ternary.FalseExpression); + + yield return CreateFixableDiagnosticForSpan( + diagnosticLevel, + ternary.Span, + new CodeFix( + CoreResources.UseSafeAccessRule_CodeFix, + isPreferred: true, + CodeFixKind.QuickFix, + new CodeReplacement(ternary.Span, replacement.ToString())), + CoreResources.UseSafeAccessRule_MessageFormat); + } + } +} \ No newline at end of file diff --git a/src/Bicep.Core/CoreResources.Designer.cs b/src/Bicep.Core/CoreResources.Designer.cs index 3ae6d009ad2..be6814707b9 100644 --- a/src/Bicep.Core/CoreResources.Designer.cs +++ b/src/Bicep.Core/CoreResources.Designer.cs @@ -1058,5 +1058,23 @@ internal static string UseStableVMImageRuleFixMessageFormat { return ResourceManager.GetString("UseStableVMImageRuleFixMessageFormat", resourceCulture); } } + + internal static string UseSafeAccessRule_Description { + get { + return ResourceManager.GetString("UseSafeAccessRule_Description", resourceCulture); + } + } + + internal static string UseSafeAccessRule_MessageFormat { + get { + return ResourceManager.GetString("UseSafeAccessRule_MessageFormat", resourceCulture); + } + } + + internal static string UseSafeAccessRule_CodeFix { + get { + return ResourceManager.GetString("UseSafeAccessRule_CodeFix", resourceCulture); + } + } } } diff --git a/src/Bicep.Core/CoreResources.resx b/src/Bicep.Core/CoreResources.resx index 9b617ad6a97..3a19b6c9979 100644 --- a/src/Bicep.Core/CoreResources.resx +++ b/src/Bicep.Core/CoreResources.resx @@ -495,4 +495,13 @@ Resource-derived types + + Use the safe access (.?) operator instead of checking object contents with the 'contains' function. + + + Use the safe access (.?) operator + + + The syntax can be simplified by using the safe access (.?) operator. + \ No newline at end of file diff --git a/src/Bicep.Core/Semantics/SemanticModelHelper.cs b/src/Bicep.Core/Semantics/SemanticModelHelper.cs index edb3f530584..491850558bf 100644 --- a/src/Bicep.Core/Semantics/SemanticModelHelper.cs +++ b/src/Bicep.Core/Semantics/SemanticModelHelper.cs @@ -21,6 +21,18 @@ public static IEnumerable GetFunctionsByName(SemanticMod .Where(s => SemanticModelHelper.TryGetFunctionInNamespace(model, @namespace, s) is { }); } + public static FunctionCallSyntaxBase? TryGetNamedFunction(SemanticModel model, string @namespace, string functionName, SyntaxBase syntax) + { + if (syntax is FunctionCallSyntaxBase functionCall && + functionCall.NameEquals(functionName) && + SemanticModelHelper.TryGetFunctionInNamespace(model, @namespace, functionCall) is { }) + { + return functionCall; + } + + return null; + } + public static FunctionCallSyntaxBase? TryGetFunctionInNamespace(SemanticModel semanticModel, string @namespace, SyntaxBase syntax) { if (semanticModel.GetSymbolInfo(syntax) is FunctionSymbol function && diff --git a/src/Bicep.Core/Syntax/SyntaxExtensions.cs b/src/Bicep.Core/Syntax/SyntaxExtensions.cs index 4d5704ad990..9a1263e3d70 100644 --- a/src/Bicep.Core/Syntax/SyntaxExtensions.cs +++ b/src/Bicep.Core/Syntax/SyntaxExtensions.cs @@ -2,55 +2,79 @@ // Licensed under the MIT License. using Bicep.Core.Parsing; using Bicep.Core.Semantics; +using Bicep.Core.Syntax.Comparers; using Bicep.Core.TypeSystem.Types; -namespace Bicep.Core.Syntax +namespace Bicep.Core.Syntax; + +public static class SyntaxExtensions { - public static class SyntaxExtensions + private static TypeProperty? TryGetTypeProperty(SemanticModel model, SyntaxBase objectSyntax, string propertyName) { - private static TypeProperty? TryGetTypeProperty(SemanticModel model, SyntaxBase objectSyntax, string propertyName) + // Cannot use assigned type here because it won't handle the case where the property value + // is an array accesss or a string interpolation. + return model.TypeManager.GetDeclaredType(objectSyntax) switch { - // Cannot use assigned type here because it won't handle the case where the property value - // is an array accesss or a string interpolation. - return model.TypeManager.GetDeclaredType(objectSyntax) switch - { - ObjectType { Properties: var properties } - when properties.TryGetValue(propertyName, out var typeProperty) => typeProperty, - DiscriminatedObjectType { DiscriminatorKey: var discriminatorKey, DiscriminatorProperty: var typeProperty } - when LanguageConstants.IdentifierComparer.Equals(propertyName, discriminatorKey) => typeProperty, - _ => null, - }; - } + ObjectType { Properties: var properties } + when properties.TryGetValue(propertyName, out var typeProperty) => typeProperty, + DiscriminatedObjectType { DiscriminatorKey: var discriminatorKey, DiscriminatorProperty: var typeProperty } + when LanguageConstants.IdentifierComparer.Equals(propertyName, discriminatorKey) => typeProperty, + _ => null, + }; + } - public static TypeProperty? TryGetTypeProperty(this ObjectPropertySyntax syntax, SemanticModel model) + public static TypeProperty? TryGetTypeProperty(this ObjectPropertySyntax syntax, SemanticModel model) + { + if (syntax.TryGetKeyText() is not string propertyName || model.Binder.GetParent(syntax) is not ObjectSyntax objectSyntax) { - if (syntax.TryGetKeyText() is not string propertyName || model.Binder.GetParent(syntax) is not ObjectSyntax objectSyntax) - { - return null; - } - - return TryGetTypeProperty(model, objectSyntax, propertyName); + return null; } - /// - /// If a chain of accesses starts with a "safe" access (e.g., base[?0].property or base.?some.deeply.nested.property), - /// it may short-circuit at runtime, meaning that .deeply.nested.property will only be evaluated if base.?some returns a non-null value. - /// The upshot of this is that we will need to mark base.?some as non-nullable when evaluating any chained property accesses, then - /// mark the resultant type as nullable iff the original "safe" access might return null. - /// Because of this requirement, it's necessary to evaluate the full access chain and determine if it is kicked off by a .? or [?] operator rather than - /// just evaluating syntax.BaseExpression recursively - /// - public static Stack ToAccessExpressionStack(this AccessExpressionSyntax syntax) + return TryGetTypeProperty(model, objectSyntax, propertyName); + } + + /// + /// If a chain of accesses starts with a "safe" access (e.g., base[?0].property or base.?some.deeply.nested.property), + /// it may short-circuit at runtime, meaning that .deeply.nested.property will only be evaluated if base.?some returns a non-null value. + /// The upshot of this is that we will need to mark base.?some as non-nullable when evaluating any chained property accesses, then + /// mark the resultant type as nullable iff the original "safe" access might return null. + /// Because of this requirement, it's necessary to evaluate the full access chain and determine if it is kicked off by a .? or [?] operator rather than + /// just evaluating syntax.BaseExpression recursively + /// + public static Stack ToAccessExpressionStack(this AccessExpressionSyntax syntax) + { + Stack chainedAccesses = new(); + chainedAccesses.Push(syntax); + + while (chainedAccesses.TryPeek(out var current) && !current.IsSafeAccess && current.BaseExpression is AccessExpressionSyntax baseAccessExpression) { - Stack chainedAccesses = new(); - chainedAccesses.Push(syntax); + chainedAccesses.Push(baseAccessExpression); + } + + return chainedAccesses; + } - while (chainedAccesses.TryPeek(out var current) && !current.IsSafeAccess && current.BaseExpression is AccessExpressionSyntax baseAccessExpression) - { - chainedAccesses.Push(baseAccessExpression); - } + /// + /// Checks whether the a particular access expression is accessing an expression matching the target expression. + /// For example, if the target expression is `'bar'`, this would return true for either `foo['bar']` or `foo.bar`. + /// If the target expression is `myExpr`. this would return true for `foo[myExpr]`. + /// + public static bool AccessExpressionMatches(this AccessExpressionSyntax access, SyntaxBase targetExpression) + { + if (access is ArrayAccessSyntax arrayAccess && + SyntaxIgnoringTriviaComparer.Instance.Equals(arrayAccess.IndexExpression, targetExpression)) + { + return true; + } - return chainedAccesses; + if (access is PropertyAccessSyntax propertyAccess && + targetExpression is StringSyntax stringSyntax && + stringSyntax.TryGetLiteralValue() is { } literalValue && + propertyAccess.PropertyName.NameEquals(literalValue)) + { + return true; } + + return false; } } diff --git a/src/Bicep.Core/Syntax/SyntaxFactory.cs b/src/Bicep.Core/Syntax/SyntaxFactory.cs index 3a6ee0319f2..e10501982af 100644 --- a/src/Bicep.Core/Syntax/SyntaxFactory.cs +++ b/src/Bicep.Core/Syntax/SyntaxFactory.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. using System.Collections.Immutable; +using System.ComponentModel; using Bicep.Core.Extensions; using Bicep.Core.Parsing; @@ -64,6 +65,7 @@ public static Token GetCommaToken(IEnumerable? leadingTrivia = nul => CreateToken(TokenType.Comma, leadingTrivia, trailingTrivia); public static Token DotToken => CreateToken(TokenType.Dot); public static Token QuestionToken => CreateToken(TokenType.Question); + public static Token DoubleQuestionToken => CreateToken(TokenType.DoubleQuestion); public static Token ColonToken => CreateToken(TokenType.Colon); public static Token SemicolonToken => CreateToken(TokenType.Semicolon); public static Token AssignmentToken => CreateToken(TokenType.Assignment, EmptyTrivia, SingleSpaceTrivia); @@ -356,7 +358,18 @@ public static LambdaSyntax CreateLambdaSyntax(IReadOnlyList parameterNam }; public static PropertyAccessSyntax CreatePropertyAccess(SyntaxBase @base, string propertyName) - => new(@base, DotToken, null, CreateIdentifier(propertyName)); + => CreatePropertyAccess(@base, false, propertyName); + + public static PropertyAccessSyntax CreatePropertyAccess(SyntaxBase @base, bool safe, string propertyName) + => new(@base, DotToken, safe ? QuestionToken : null, CreateIdentifier(propertyName)); + + public static ArrayAccessSyntax CreateArrayAccess(SyntaxBase @base, bool safe, SyntaxBase accessExpression) + => new(@base, LeftSquareToken, safe ? QuestionToken : null, accessExpression, RightSquareToken); + + public static SyntaxBase CreateSafeAccess(SyntaxBase @base, SyntaxBase accessExpression) + => (accessExpression is StringSyntax stringAccess && stringAccess.TryGetLiteralValue() is {} stringValue) ? + CreatePropertyAccess(@base, true, stringValue) : + CreateArrayAccess(@base, true, accessExpression); public static ParameterAssignmentSyntax CreateParameterAssignmentSyntax(string name, SyntaxBase value) => new( @@ -364,5 +377,11 @@ public static ParameterAssignmentSyntax CreateParameterAssignmentSyntax(string n CreateIdentifierWithTrailingSpace(name), AssignmentToken, value); + + public static BinaryOperationSyntax CreateBinaryOperationSyntax(SyntaxBase left, TokenType operatorType, SyntaxBase right) + => new( + left, + CreateToken(operatorType, SingleSpaceTrivia, SingleSpaceTrivia), + right); } } diff --git a/src/vscode-bicep/schemas/bicepconfig.schema.json b/src/vscode-bicep/schemas/bicepconfig.schema.json index 45b2a002111..95497036ea0 100644 --- a/src/vscode-bicep/schemas/bicepconfig.schema.json +++ b/src/vscode-bicep/schemas/bicepconfig.schema.json @@ -700,6 +700,16 @@ } ] }, + "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" + }, + { + "$ref": "#/definitions/rule-def-level-warning" + } + ] + }, "use-recent-api-versions": { "allOf": [ {