diff --git a/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs b/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs index aab6839ea24..ee3f50b4796 100644 --- a/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs +++ b/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseSafeAccessRuleTests.cs @@ -51,6 +51,24 @@ param foo object """, """ param foo object var test = foo[?'oh-dear'] ?? 'baz' +"""); + + [TestMethod] + public void Codefix_is_aware_of_operator_precedence() => AssertCodeFix(""" +param foo object +var test = contai|ns(foo, 'bar') ? foo.bar : contains(foo, 'baz') ? foo.baz : 'qux' +""", """ +param foo object +var test = foo.?bar ?? (contains(foo, 'baz') ? foo.baz : 'qux') +"""); + + [TestMethod] + public void Codefix_is_aware_of_operator_precedence_when_not_needed() => AssertCodeFix(""" +param foo object +var test = contai|ns(foo, 'bar') ? foo.bar : 1 + 2 +""", """ +param foo object +var test = foo.?bar ?? 1 + 2 """); [TestMethod] @@ -71,5 +89,63 @@ param notTarget string public void Rule_ignores_syntax_which_cannot_be_simplified_array_access() => AssertNoDiagnostics(""" param foo object var test = contains(foo, 'bar') ? bar.bar : 'baz' +"""); + + [TestMethod] + // https://github.com/Azure/bicep/issues/14705 + public void Codefix_handles_issue14705_correctly() => AssertCodeFix(""" +@description('Optional. Array of role assignments to create.') +param roleAssignments roleAssignmentType + +var builtInRoleNames = { + Contributor: subscriptionResourceId('Microsoft.Authorization/roleDefinitions', 'b24988ac-6180-42a0-ab88-20f7382dd24c') + Owner: subscriptionResourceId('Microsoft.Authorization/roleDefinitions', '8e3af657-a8ff-443c-a75c-2fe8c4bcb635') + Reader: subscriptionResourceId('Microsoft.Authorization/roleDefinitions', 'acdd72a7-3385-48ef-bd42-f606fba81ae7') +} + +output formattedRoles array = [ + for (roleAssignment, index) in (roleAssignments ?? []): union(roleAssignment, { + roleDefinitionId: conta|ins(builtInRoleNames, roleAssignment.roleDefinitionIdOrName) + ? builtInRoleNames[roleAssignment.roleDefinitionIdOrName] + : contains(roleAssignment.roleDefinitionIdOrName, '/providers/Microsoft.Authorization/roleDefinitions/') + ? roleAssignment.roleDefinitionIdOrName + : subscriptionResourceId('Microsoft.Authorization/roleDefinitions', roleAssignment.roleDefinitionIdOrName) + }) +] + +// ================ // +// Definitions // +// ================ // + +type roleAssignmentType = { + @description('Required. The role to assign. You can provide either the display name of the role definition, the role definition GUID, or its fully qualified ID in the following format: \'/providers/Microsoft.Authorization/roleDefinitions/c2f4ef07-c644-48eb-af81-4b1b4947fb11\'.') + roleDefinitionIdOrName: string +}[]? +""", """ +@description('Optional. Array of role assignments to create.') +param roleAssignments roleAssignmentType + +var builtInRoleNames = { + Contributor: subscriptionResourceId('Microsoft.Authorization/roleDefinitions', 'b24988ac-6180-42a0-ab88-20f7382dd24c') + Owner: subscriptionResourceId('Microsoft.Authorization/roleDefinitions', '8e3af657-a8ff-443c-a75c-2fe8c4bcb635') + Reader: subscriptionResourceId('Microsoft.Authorization/roleDefinitions', 'acdd72a7-3385-48ef-bd42-f606fba81ae7') +} + +output formattedRoles array = [ + for (roleAssignment, index) in (roleAssignments ?? []): union(roleAssignment, { + roleDefinitionId: builtInRoleNames[?roleAssignment.roleDefinitionIdOrName] ?? (contains(roleAssignment.roleDefinitionIdOrName, '/providers/Microsoft.Authorization/roleDefinitions/') + ? roleAssignment.roleDefinitionIdOrName + : subscriptionResourceId('Microsoft.Authorization/roleDefinitions', roleAssignment.roleDefinitionIdOrName)) + }) +] + +// ================ // +// Definitions // +// ================ // + +type roleAssignmentType = { + @description('Required. The role to assign. You can provide either the display name of the role definition, the role definition GUID, or its fully qualified ID in the following format: \'/providers/Microsoft.Authorization/roleDefinitions/c2f4ef07-c644-48eb-af81-4b1b4947fb11\'.') + roleDefinitionIdOrName: string +}[]? """); } diff --git a/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs b/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs index c86a0bf539f..098fbfd16aa 100644 --- a/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs +++ b/src/Bicep.Core/Analyzers/Linter/Rules/UseSafeAccessRule.cs @@ -44,10 +44,19 @@ public override IEnumerable AnalyzeInternal(SemanticModel model, Di continue; } + // See https://github.com/Azure/bicep/issues/14705 for context on why this is necessary + var rhsNeedsParentheses = ternary.FalseExpression switch + { + TernaryOperationSyntax => true, + BinaryOperationSyntax binaryOperation => + TokenTypeHelper.GetOperatorPrecedence(binaryOperation.OperatorToken.Type) < TokenTypeHelper.GetOperatorPrecedence(TokenType.DoubleQuestion), + _ => false, + }; + var replacement = SyntaxFactory.CreateBinaryOperationSyntax( SyntaxFactory.CreateSafeAccess(truePropertyAccess.BaseExpression, functionCall.Arguments[1].Expression), TokenType.DoubleQuestion, - ternary.FalseExpression); + rhsNeedsParentheses ? SyntaxFactory.CreateParenthesized(ternary.FalseExpression) : ternary.FalseExpression); yield return CreateFixableDiagnosticForSpan( diagnosticLevel, diff --git a/src/Bicep.Core/Parsing/BaseParser.cs b/src/Bicep.Core/Parsing/BaseParser.cs index 3c19fcd38ae..b41a6de18a2 100644 --- a/src/Bicep.Core/Parsing/BaseParser.cs +++ b/src/Bicep.Core/Parsing/BaseParser.cs @@ -63,34 +63,6 @@ TokenType.NewLine or private static bool CheckKeyword(Token? token, string keyword) => token?.Type == TokenType.Identifier && token.Text == keyword; - private static int GetOperatorPrecedence(TokenType tokenType) => tokenType switch - { - // the absolute values are not important here - TokenType.Modulo or - TokenType.Asterisk or - TokenType.Slash => 100, - - TokenType.Plus or - TokenType.Minus => 90, - - TokenType.RightChevron or - TokenType.GreaterThanOrEqual or - TokenType.LeftChevron or - TokenType.LessThanOrEqual => 80, - - TokenType.Equals or - TokenType.NotEquals or - TokenType.EqualsInsensitive or - TokenType.NotEqualsInsensitive => 70, - - // if we add bitwise operators in the future, they should go here - TokenType.LogicalAnd => 50, - TokenType.LogicalOr => 40, - TokenType.DoubleQuestion => 30, - - _ => -1, - }; - protected static RecoveryFlags GetSuppressionFlag(SyntaxBase precedingNode) { // local function @@ -246,7 +218,7 @@ private SyntaxBase BinaryExpression(ExpressionFlags expressionFlags, int precede // it could also be the end of file or some other token that is actually valid in this place Token candidateOperatorToken = this.reader.Peek(); - int operatorPrecedence = GetOperatorPrecedence(candidateOperatorToken.Type); + int operatorPrecedence = TokenTypeHelper.GetOperatorPrecedence(candidateOperatorToken.Type); if (operatorPrecedence <= precedence) { diff --git a/src/Bicep.Core/Parsing/TokenTypeHelper.cs b/src/Bicep.Core/Parsing/TokenTypeHelper.cs new file mode 100644 index 00000000000..9a28dd912c8 --- /dev/null +++ b/src/Bicep.Core/Parsing/TokenTypeHelper.cs @@ -0,0 +1,34 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +namespace Bicep.Core.Parsing; + +public static class TokenTypeHelper +{ + public static int GetOperatorPrecedence(TokenType tokenType) => tokenType switch + { + // the absolute values are not important here + TokenType.Modulo or + TokenType.Asterisk or + TokenType.Slash => 100, + + TokenType.Plus or + TokenType.Minus => 90, + + TokenType.RightChevron or + TokenType.GreaterThanOrEqual or + TokenType.LeftChevron or + TokenType.LessThanOrEqual => 80, + + TokenType.Equals or + TokenType.NotEquals or + TokenType.EqualsInsensitive or + TokenType.NotEqualsInsensitive => 70, + + // if we add bitwise operators in the future, they should go here + TokenType.LogicalAnd => 50, + TokenType.LogicalOr => 40, + TokenType.DoubleQuestion => 30, + + _ => -1, + }; +} \ No newline at end of file diff --git a/src/Bicep.Core/Syntax/SyntaxFactory.cs b/src/Bicep.Core/Syntax/SyntaxFactory.cs index 8407fa9e26d..62c5a77e9fc 100644 --- a/src/Bicep.Core/Syntax/SyntaxFactory.cs +++ b/src/Bicep.Core/Syntax/SyntaxFactory.cs @@ -386,5 +386,8 @@ public static BinaryOperationSyntax CreateBinaryOperationSyntax(SyntaxBase left, left, CreateToken(operatorType, SingleSpaceTrivia, SingleSpaceTrivia), right); + + public static ParenthesizedExpressionSyntax CreateParenthesized(SyntaxBase inner) + => new(LeftParenToken, inner, RightParenToken); } }