From 610ba43b17ecbf2efeea453267f4b4e930e20d98 Mon Sep 17 00:00:00 2001 From: Anthony Martin Date: Fri, 22 Oct 2021 13:12:31 -0400 Subject: [PATCH 1/2] Improve parser handling of single-line array/object --- .../ScenarioTests.cs | 156 ++++++++++++++++++ src/Bicep.Core/Parsing/Parser.cs | 31 ++++ 2 files changed, 187 insertions(+) diff --git a/src/Bicep.Core.IntegrationTests/ScenarioTests.cs b/src/Bicep.Core.IntegrationTests/ScenarioTests.cs index 015d7669398..e0d1fd77eb8 100644 --- a/src/Bicep.Core.IntegrationTests/ScenarioTests.cs +++ b/src/Bicep.Core.IntegrationTests/ScenarioTests.cs @@ -2794,5 +2794,161 @@ public void Test_Issue1228() var evaluated = TemplateEvaluator.Evaluate(result.Template); evaluated.Should().HaveValueAtPath("$.resources[?(@.name == 'Default initiative')].properties.policyDefinitions[0].policyDefinitionId", "/providers/Microsoft.Management/managementGroups/3fc9f36e-8699-43af-b038-1c103980942f/providers/Microsoft.Authorization/policyDefinitions/Allowed locations"); } + + // https://github.com/Azure/bicep/issues/4955 + [TestMethod] + public void Test_Issue4955() + { + // missing new line at the start and end of the array + var result = CompilationHelper.Compile(@" +targetScope = 'subscription' + +resource myRg 'Microsoft.Resources/resourceGroups@2021-04-01' = { + name: 'asdf' + location: 'asdf' + tags: { + 'abc': ['hi'] + } +} + +var otherExp = noValidation +"); + + result.Template.Should().NotHaveValue(); + result.Should().HaveDiagnostics(new[] { + // diagnostics for both the start and end of the array missing new lines + ("BCP019", DiagnosticLevel.Error, "Expected a new line character at this location."), + ("BCP019", DiagnosticLevel.Error, "Expected a new line character at this location."), + // Ensure we see these two diagnostics as it means that parsing the rest of the document has succeeded + ("no-unused-vars", DiagnosticLevel.Warning, "Variable \"otherExp\" is declared but never used."), + ("BCP057", DiagnosticLevel.Error, "The name \"noValidation\" does not exist in the current context."), + }); + + // missing new line at the start of the array + result = CompilationHelper.Compile(@" +targetScope = 'subscription' + +resource myRg 'Microsoft.Resources/resourceGroups@2021-04-01' = { + name: 'asdf' + location: 'asdf' + tags: { + 'abc': ['hi' + ] + } +} + +var otherExp = noValidation +"); + + result.Template.Should().NotHaveValue(); + result.Should().HaveDiagnostics(new[] { + ("BCP019", DiagnosticLevel.Error, "Expected a new line character at this location."), + // Ensure we see these two diagnostics as it means that parsing the rest of the document has succeeded + ("no-unused-vars", DiagnosticLevel.Warning, "Variable \"otherExp\" is declared but never used."), + ("BCP057", DiagnosticLevel.Error, "The name \"noValidation\" does not exist in the current context."), + }); + + // missing new line at the end of the array + result = CompilationHelper.Compile(@" +targetScope = 'subscription' + +resource myRg 'Microsoft.Resources/resourceGroups@2021-04-01' = { + name: 'asdf' + location: 'asdf' + tags: { + 'abc': [ + 'hi'] + } +} + +var otherExp = noValidation +"); + + result.Template.Should().NotHaveValue(); + result.Should().HaveDiagnostics(new[] { + ("BCP019", DiagnosticLevel.Error, "Expected a new line character at this location."), + // Ensure we see these two diagnostics as it means that parsing the rest of the document has succeeded + ("no-unused-vars", DiagnosticLevel.Warning, "Variable \"otherExp\" is declared but never used."), + ("BCP057", DiagnosticLevel.Error, "The name \"noValidation\" does not exist in the current context."), + }); + } + + // https://github.com/Azure/bicep/issues/4955 + [TestMethod] + public void Test_Issue4955_objects() + { + // missing new line at the start and end of the object + var result = CompilationHelper.Compile(@" +targetScope = 'subscription' + +resource myRg 'Microsoft.Resources/resourceGroups@2021-04-01' = { + name: 'asdf' + location: 'asdf' + tags: { + 'abc': {foo: 'bar'} + } +} + +var otherExp = noValidation +"); + + result.Template.Should().NotHaveValue(); + result.Should().HaveDiagnostics(new[] { + // diagnostics for both the start and end of the object missing new lines + ("BCP019", DiagnosticLevel.Error, "Expected a new line character at this location."), + ("BCP019", DiagnosticLevel.Error, "Expected a new line character at this location."), + // Ensure we see these two diagnostics as it means that parsing the rest of the document has succeeded + ("no-unused-vars", DiagnosticLevel.Warning, "Variable \"otherExp\" is declared but never used."), + ("BCP057", DiagnosticLevel.Error, "The name \"noValidation\" does not exist in the current context."), + }); + + // missing new line at the start of the object + result = CompilationHelper.Compile(@" +targetScope = 'subscription' + +resource myRg 'Microsoft.Resources/resourceGroups@2021-04-01' = { + name: 'asdf' + location: 'asdf' + tags: { + 'abc': {foo: 'bar' + } + } +} + +var otherExp = noValidation +"); + + result.Template.Should().NotHaveValue(); + result.Should().HaveDiagnostics(new[] { + ("BCP019", DiagnosticLevel.Error, "Expected a new line character at this location."), + // Ensure we see these two diagnostics as it means that parsing the rest of the document has succeeded + ("no-unused-vars", DiagnosticLevel.Warning, "Variable \"otherExp\" is declared but never used."), + ("BCP057", DiagnosticLevel.Error, "The name \"noValidation\" does not exist in the current context."), + }); + + // missing new line at the end of the object + result = CompilationHelper.Compile(@" +targetScope = 'subscription' + +resource myRg 'Microsoft.Resources/resourceGroups@2021-04-01' = { + name: 'asdf' + location: 'asdf' + tags: { + 'abc': { + foo: 'bar'} + } +} + +var otherExp = noValidation +"); + + result.Template.Should().NotHaveValue(); + result.Should().HaveDiagnostics(new[] { + ("BCP019", DiagnosticLevel.Error, "Expected a new line character at this location."), + // Ensure we see these two diagnostics as it means that parsing the rest of the document has succeeded + ("no-unused-vars", DiagnosticLevel.Warning, "Variable \"otherExp\" is declared but never used."), + ("BCP057", DiagnosticLevel.Error, "The name \"noValidation\" does not exist in the current context."), + }); + } } } \ No newline at end of file diff --git a/src/Bicep.Core/Parsing/Parser.cs b/src/Bicep.Core/Parsing/Parser.cs index 39970165868..aa35fd93a13 100644 --- a/src/Bicep.Core/Parsing/Parser.cs +++ b/src/Bicep.Core/Parsing/Parser.cs @@ -1044,6 +1044,11 @@ private SyntaxBase Array() var itemsOrTokens = new List(); + if (!Check(TokenType.NewLine)) + { + itemsOrTokens.Add(SkipEmpty(x => x.ExpectedNewLine())); + } + while (!this.IsAtEnd() && this.reader.Peek().Type != TokenType.RightSquare) { // this produces an item node, skipped tokens node, or just a newline token @@ -1065,6 +1070,16 @@ private SyntaxBase Array() itemsOrTokens.Add(skippedSyntax); } + // we've got a ']' immediately after a property. + // consume it and exit early with an error to avoid assuming we're still inside the object + if (Check(TokenType.RightSquare)) + { + itemsOrTokens.Add(SkipEmpty(x => x.ExpectedNewLine())); + var earlyCloseBracket = this.reader.Read(); + + return new ArraySyntax(openBracket, itemsOrTokens, earlyCloseBracket); + } + // items must be followed by newlines var newLine = this.WithRecoveryNullable(this.NewLineOrEof, RecoveryFlags.ConsumeTerminator, TokenType.NewLine); if (newLine != null) @@ -1107,6 +1122,12 @@ private ObjectSyntax Object(ExpressionFlags expressionFlags) } var propertiesOrResourcesTokens = new List(); + + if (!Check(TokenType.NewLine)) + { + propertiesOrResourcesTokens.Add(SkipEmpty(x => x.ExpectedNewLine())); + } + while (!this.IsAtEnd() && this.reader.Peek().Type != TokenType.RightBrace) { // this produces a property node, skipped tokens node, or just a newline token @@ -1128,6 +1149,16 @@ private ObjectSyntax Object(ExpressionFlags expressionFlags) propertiesOrResourcesTokens.Add(skippedSyntax); } + // we've got a '}' immediately after a property. + // consume it and exit early with an error to avoid assuming we're still inside the object + if (Check(TokenType.RightBrace)) + { + propertiesOrResourcesTokens.Add(SkipEmpty(x => x.ExpectedNewLine())); + var earlyCloseBrace = this.reader.Read(); + + return new ObjectSyntax(openBrace, propertiesOrResourcesTokens, earlyCloseBrace); + } + // properties must be followed by newlines var newLine = this.WithRecoveryNullable(this.NewLineOrEof, RecoveryFlags.ConsumeTerminator, TokenType.NewLine); if (newLine != null) From fbf094958ef42deabce1d1bb81699ed1591e5b76 Mon Sep 17 00:00:00 2001 From: Bicep Automation Date: Fri, 22 Oct 2021 18:10:37 +0000 Subject: [PATCH 2/2] Update test baselines --- .../InvalidModules_LF/main.diagnostics.bicep | 4 +- .../Files/InvalidModules_LF/main.syntax.bicep | 44 ++++++++++--------- .../main.diagnostics.bicep | 5 ++- .../InvalidResources_CRLF/main.syntax.bicep | 44 ++++++++++--------- 4 files changed, 53 insertions(+), 44 deletions(-) diff --git a/src/Bicep.Core.Samples/Files/InvalidModules_LF/main.diagnostics.bicep b/src/Bicep.Core.Samples/Files/InvalidModules_LF/main.diagnostics.bicep index 148addcfb9e..8ccd8bc1553 100644 --- a/src/Bicep.Core.Samples/Files/InvalidModules_LF/main.diagnostics.bicep +++ b/src/Bicep.Core.Samples/Files/InvalidModules_LF/main.diagnostics.bicep @@ -127,10 +127,10 @@ module modWithListKeysInCondition './main.bicep' = if (listKeys('foo', '2020-05- module modANoName './modulea.bicep' = if ({ 'a': b }.a == true) { //@[7:17) [BCP028 (Error)] Identifier "modANoName" is declared multiple times. Remove or rename the duplicates. (CodeDescription: none) |modANoName| -//@[51:52) [BCP019 (Error)] Expected a new line character at this location. (CodeDescription: none) |}| +//@[44:44) [BCP019 (Error)] Expected a new line character at this location. (CodeDescription: none) || +//@[51:51) [BCP019 (Error)] Expected a new line character at this location. (CodeDescription: none) || } -//@[1:1) [BCP018 (Error)] Expected the ")" character at this location. (CodeDescription: none) || module modANoInputs './modulea.bicep' = { //@[7:19) [BCP035 (Error)] The specified "module" declaration is missing the following required properties: "params". (CodeDescription: none) |modANoInputs| diff --git a/src/Bicep.Core.Samples/Files/InvalidModules_LF/main.syntax.bicep b/src/Bicep.Core.Samples/Files/InvalidModules_LF/main.syntax.bicep index ae714be27ac..d2bcb06fe1d 100644 --- a/src/Bicep.Core.Samples/Files/InvalidModules_LF/main.syntax.bicep +++ b/src/Bicep.Core.Samples/Files/InvalidModules_LF/main.syntax.bicep @@ -518,31 +518,35 @@ module modANoName './modulea.bicep' = if ({ 'a': b }.a == true) { //@[36:37) Assignment |=| //@[38:68) IfConditionSyntax //@[38:40) Identifier |if| -//@[41:68) ParenthesizedExpressionSyntax +//@[41:63) ParenthesizedExpressionSyntax //@[41:42) LeftParen |(| -//@[42:68) ObjectSyntax -//@[42:43) LeftBrace |{| -//@[44:50) ObjectPropertySyntax -//@[44:47) StringSyntax -//@[44:47) StringComplete |'a'| -//@[47:48) Colon |:| -//@[49:50) VariableAccessSyntax -//@[49:50) IdentifierSyntax -//@[49:50) Identifier |b| -//@[51:67) SkippedTriviaSyntax -//@[51:52) RightBrace |}| +//@[42:62) BinaryOperationSyntax +//@[42:54) PropertyAccessSyntax +//@[42:52) ObjectSyntax +//@[42:43) LeftBrace |{| +//@[44:44) SkippedTriviaSyntax +//@[44:50) ObjectPropertySyntax +//@[44:47) StringSyntax +//@[44:47) StringComplete |'a'| +//@[47:48) Colon |:| +//@[49:50) VariableAccessSyntax +//@[49:50) IdentifierSyntax +//@[49:50) Identifier |b| +//@[51:51) SkippedTriviaSyntax +//@[51:52) RightBrace |}| //@[52:53) Dot |.| -//@[53:54) Identifier |a| -//@[55:57) Equals |==| +//@[53:54) IdentifierSyntax +//@[53:54) Identifier |a| +//@[55:57) Equals |==| +//@[58:62) BooleanLiteralSyntax //@[58:62) TrueKeyword |true| -//@[62:63) RightParen |)| -//@[64:65) LeftBrace |{| -//@[65:67) NewLine |\n\n| +//@[62:63) RightParen |)| +//@[64:68) ObjectSyntax +//@[64:65) LeftBrace |{| +//@[65:67) NewLine |\n\n| } -//@[0:1) RightBrace |}| -//@[1:1) SkippedTriviaSyntax -//@[1:1) SkippedTriviaSyntax +//@[0:1) RightBrace |}| //@[1:3) NewLine |\n\n| module modANoInputs './modulea.bicep' = { diff --git a/src/Bicep.Core.Samples/Files/InvalidResources_CRLF/main.diagnostics.bicep b/src/Bicep.Core.Samples/Files/InvalidResources_CRLF/main.diagnostics.bicep index 042d9a56dc4..f58ecad6765 100644 --- a/src/Bicep.Core.Samples/Files/InvalidResources_CRLF/main.diagnostics.bicep +++ b/src/Bicep.Core.Samples/Files/InvalidResources_CRLF/main.diagnostics.bicep @@ -69,11 +69,12 @@ resource foo 'Microsoft.Foo/foos@2020-02-02-alpha'= if (name == 'value') { resource foo 'Microsoft.Foo/foos@2020-02-02-alpha'= if ({ 'a': b }.a == 'foo') { //@[9:12) [BCP028 (Error)] Identifier "foo" is declared multiple times. Remove or rename the duplicates. (CodeDescription: none) |foo| +//@[9:12) [BCP035 (Error)] The specified "resource" declaration is missing the following required properties: "name". (CodeDescription: none) |foo| //@[13:50) [BCP081 (Warning)] Resource type "Microsoft.Foo/foos@2020-02-02-alpha" does not have types available. (CodeDescription: none) |'Microsoft.Foo/foos@2020-02-02-alpha'| +//@[58:58) [BCP019 (Error)] Expected a new line character at this location. (CodeDescription: none) || //@[63:64) [BCP057 (Error)] The name "b" does not exist in the current context. (CodeDescription: none) |b| -//@[65:66) [BCP019 (Error)] Expected a new line character at this location. (CodeDescription: none) |}| +//@[65:65) [BCP019 (Error)] Expected a new line character at this location. (CodeDescription: none) || } -//@[1:1) [BCP018 (Error)] Expected the ")" character at this location. (CodeDescription: none) || // simulate typing if condition resource foo 'Microsoft.Foo/foos@2020-02-02-alpha'= if diff --git a/src/Bicep.Core.Samples/Files/InvalidResources_CRLF/main.syntax.bicep b/src/Bicep.Core.Samples/Files/InvalidResources_CRLF/main.syntax.bicep index f593e4ee98d..14b28eede71 100644 --- a/src/Bicep.Core.Samples/Files/InvalidResources_CRLF/main.syntax.bicep +++ b/src/Bicep.Core.Samples/Files/InvalidResources_CRLF/main.syntax.bicep @@ -222,30 +222,34 @@ resource foo 'Microsoft.Foo/foos@2020-02-02-alpha'= if ({ 'a': b }.a == 'foo') { //@[50:51) Assignment |=| //@[52:83) IfConditionSyntax //@[52:54) Identifier |if| -//@[55:83) ParenthesizedExpressionSyntax +//@[55:78) ParenthesizedExpressionSyntax //@[55:56) LeftParen |(| -//@[56:83) ObjectSyntax -//@[56:57) LeftBrace |{| -//@[58:64) ObjectPropertySyntax -//@[58:61) StringSyntax -//@[58:61) StringComplete |'a'| -//@[61:62) Colon |:| -//@[63:64) VariableAccessSyntax -//@[63:64) IdentifierSyntax -//@[63:64) Identifier |b| -//@[65:82) SkippedTriviaSyntax -//@[65:66) RightBrace |}| +//@[56:77) BinaryOperationSyntax +//@[56:68) PropertyAccessSyntax +//@[56:66) ObjectSyntax +//@[56:57) LeftBrace |{| +//@[58:58) SkippedTriviaSyntax +//@[58:64) ObjectPropertySyntax +//@[58:61) StringSyntax +//@[58:61) StringComplete |'a'| +//@[61:62) Colon |:| +//@[63:64) VariableAccessSyntax +//@[63:64) IdentifierSyntax +//@[63:64) Identifier |b| +//@[65:65) SkippedTriviaSyntax +//@[65:66) RightBrace |}| //@[66:67) Dot |.| -//@[67:68) Identifier |a| -//@[69:71) Equals |==| +//@[67:68) IdentifierSyntax +//@[67:68) Identifier |a| +//@[69:71) Equals |==| +//@[72:77) StringSyntax //@[72:77) StringComplete |'foo'| -//@[77:78) RightParen |)| -//@[79:80) LeftBrace |{| -//@[80:82) NewLine |\r\n| +//@[77:78) RightParen |)| +//@[79:83) ObjectSyntax +//@[79:80) LeftBrace |{| +//@[80:82) NewLine |\r\n| } -//@[0:1) RightBrace |}| -//@[1:1) SkippedTriviaSyntax -//@[1:1) SkippedTriviaSyntax +//@[0:1) RightBrace |}| //@[1:5) NewLine |\r\n\r\n| // simulate typing if condition