From f1f39852306ab7dc0a64c4f7d300999c5832ca17 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 2 Dec 2020 11:39:03 -0800 Subject: [PATCH] hclsyntax: Explicit AST node for parentheses So far the expression parentheses syntax has been handled entirely in the parser and has been totally invisible in the AST. That's fine for typical expression evaluation, but over the years it's led to a few quirky behaviors in less common situations where we've assumed that all expressions are covered by the AST itself or by the source ranges that the AST captures. In particular, hclwrite assumes that all expressions will have source ranges that cover their tokens, and it generates an incorrect physical syntax tree when the AST doesn't uphold that. After resisting through a few other similar bugs, this commit finally introduces an explicit AST node for parentheses, which makes the parentheses explicit in the AST and captures the larger source range that includes the TokenOParen and the TokenCParen. This means that parentheses will now be visible as a distinct node when walking the AST, as reflected in the updated tests here. That may cause downstream applications that traverse the tree to exhibit different behaviors but we're not considering that as a "breaking change" because the Walk function doesn't make any guarantees about the specific AST shape. --- hclsyntax/expression.go | 26 ++++++++++++ hclsyntax/parser.go | 14 ++++++- hclsyntax/structure_at_pos_test.go | 2 +- hclsyntax/walk_test.go | 2 + hclwrite/parser_test.go | 64 ++++++++++++++++++++++++++++++ 5 files changed, 105 insertions(+), 3 deletions(-) diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index ca59461..f32f0d8 100644 --- a/hclsyntax/expression.go +++ b/hclsyntax/expression.go @@ -27,6 +27,32 @@ type Expression interface { // Assert that Expression implements hcl.Expression var assertExprImplExpr hcl.Expression = Expression(nil) +// ParenthesesExpr represents an expression written in grouping +// parentheses. +// +// The parser takes care of the precedence effect of the parentheses, so the +// only purpose of this separate expression node is to capture the source range +// of the parentheses themselves, rather than the source range of the +// expression within. All of the other expression operations just pass through +// to the underlying expression. +type ParenthesesExpr struct { + Expression + SrcRange hcl.Range +} + +var _ hcl.Expression = (*ParenthesesExpr)(nil) + +func (e *ParenthesesExpr) Range() hcl.Range { + return e.SrcRange +} + +func (e *ParenthesesExpr) walkChildNodes(w internalWalkFunc) { + // We override the walkChildNodes from the embedded Expression to + // ensure that both the parentheses _and_ the content are visible + // in a walk. + w(e.Expression) +} + // LiteralValueExpr is an expression that just always returns a given value. type LiteralValueExpr struct { Val cty.Value diff --git a/hclsyntax/parser.go b/hclsyntax/parser.go index 858bb7c..0998cc4 100644 --- a/hclsyntax/parser.go +++ b/hclsyntax/parser.go @@ -911,7 +911,7 @@ func (p *parser) parseExpressionTerm() (Expression, hcl.Diagnostics) { switch start.Type { case TokenOParen: - p.Read() // eat open paren + oParen := p.Read() // eat open paren p.PushIncludeNewlines(false) @@ -937,9 +937,19 @@ func (p *parser) parseExpressionTerm() (Expression, hcl.Diagnostics) { p.setRecovery() } - p.Read() // eat closing paren + cParen := p.Read() // eat closing paren p.PopIncludeNewlines() + // Our parser's already taken care of the precedence effect of the + // parentheses by considering them to be a kind of "term", but we + // still need to include the parentheses in our AST so we can give + // an accurate representation of the source range that includes the + // open and closing parentheses. + expr = &ParenthesesExpr{ + Expression: expr, + SrcRange: hcl.RangeBetween(oParen.Range, cParen.Range), + } + return expr, diags case TokenNumberLit: diff --git a/hclsyntax/structure_at_pos_test.go b/hclsyntax/structure_at_pos_test.go index fb87274..ead77bf 100644 --- a/hclsyntax/structure_at_pos_test.go +++ b/hclsyntax/structure_at_pos_test.go @@ -282,7 +282,7 @@ func TestOutermostExprAtPos(t *testing.T) { "parens": { `a = (1 + 1)`, hcl.Pos{Byte: 6}, - `1 + 1`, // The parser trims the parens off, so they aren't considered as part of the expression :( + `(1 + 1)`, }, "tuple cons": { `a = [1, 2, 3]`, diff --git a/hclsyntax/walk_test.go b/hclsyntax/walk_test.go index 5a24260..9200199 100644 --- a/hclsyntax/walk_test.go +++ b/hclsyntax/walk_test.go @@ -45,12 +45,14 @@ func TestWalk(t *testing.T) { { `(1 + 1)`, []testWalkCall{ + {testWalkEnter, "*hclsyntax.ParenthesesExpr"}, {testWalkEnter, "*hclsyntax.BinaryOpExpr"}, {testWalkEnter, "*hclsyntax.LiteralValueExpr"}, {testWalkExit, "*hclsyntax.LiteralValueExpr"}, {testWalkEnter, "*hclsyntax.LiteralValueExpr"}, {testWalkExit, "*hclsyntax.LiteralValueExpr"}, {testWalkExit, "*hclsyntax.BinaryOpExpr"}, + {testWalkExit, "*hclsyntax.ParenthesesExpr"}, }, }, { diff --git a/hclwrite/parser_test.go b/hclwrite/parser_test.go index ed17303..6edebc7 100644 --- a/hclwrite/parser_test.go +++ b/hclwrite/parser_test.go @@ -189,6 +189,70 @@ func TestParse(t *testing.T) { }, }, }, + { + "a = (\n 1 + 2\n)\nb = 3\n", + TestTreeNode{ + Type: "Body", + Children: []TestTreeNode{ + { + Type: "Attribute", + Children: []TestTreeNode{ + {Type: "comments"}, + { + Type: "identifier", + Val: "a", + }, + { + Type: "Tokens", + Val: " =", + }, + { + Type: "Expression", + Children: []TestTreeNode{ + { + Type: "Tokens", + Val: " (\n 1 + 2\n)", + }, + }, + }, + {Type: "comments"}, + { + Type: "Tokens", + Val: "\n", + }, + }, + }, + { + Type: "Attribute", + Children: []TestTreeNode{ + {Type: "comments"}, + { + Type: "identifier", + Val: "b", + }, + { + Type: "Tokens", + Val: " =", + }, + { + Type: "Expression", + Children: []TestTreeNode{ + { + Type: "Tokens", + Val: " 3", + }, + }, + }, + {Type: "comments"}, + { + Type: "Tokens", + Val: "\n", + }, + }, + }, + }, + }, + }, { "b {}\n", TestTreeNode{