From 7ace05a3bed93021f87190cbb1f2e58bde8d593c Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 11 Dec 2018 16:48:31 -0800 Subject: [PATCH] hcl/hclsyntax: Better handling of naked attribute keys with dots To make things read better in the normal case, we treat naked identifiers in the place of object keys as literal strings containing the identifier text rather than as references. However, this had a couple sub-optimal implications: - If a user would try to create a key containing a period, the evaluator would see that it wasn't a valid keyword and try to resolve it as a normal scope traversal, causing a confusing error that didn't align with the user's intent. - In the rarer case where the attempted key contains a period followed by a digit, the parser would trip over what seems to be an unexpected identifier following the colon and produce, again, a confusing error that doesn't align with what the user intended. To address the first of these problems, it is now invalid to use a naked traversal with more than one step as an object key, which allows us to produce a targeted error message that directs the user to either put the expression in parentheses to force interpretation as a scope traversal or in quotes to force interpretation as a literal. The second problem can't be addressed exactly due to it being a parser problem, but we improve the situation slightly here by adding an extra hint to the parse error message in this case so that a user making this mistake might understand better how the error relates to what they were trying to express. --- hcl/hclsyntax/expression.go | 20 +++++++++++++++++++ hcl/hclsyntax/expression_test.go | 34 ++++++++++++++++++++++++++++++++ hcl/hclsyntax/parser.go | 23 ++++++++++++++++++--- 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/hcl/hclsyntax/expression.go b/hcl/hclsyntax/expression.go index e57f81e..db3cbfd 100644 --- a/hcl/hclsyntax/expression.go +++ b/hcl/hclsyntax/expression.go @@ -797,6 +797,26 @@ func (e *ObjectConsKeyExpr) walkChildNodes(w internalWalkFunc) { } func (e *ObjectConsKeyExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { + // Because we accept a naked identifier as a literal key rather than a + // reference, it's confusing to accept a traversal containing periods + // here since we can't tell if the user intends to create a key with + // periods or actually reference something. To avoid confusing downstream + // errors we'll just prohibit a naked multi-step traversal here and + // require the user to state their intent more clearly. + // (This is handled at evaluation time rather than parse time because + // an application using static analysis _can_ accept a naked multi-step + // traversal here, if desired.) + if travExpr, isTraversal := e.Wrapped.(*ScopeTraversalExpr); isTraversal && len(travExpr.Traversal) > 1 { + var diags hcl.Diagnostics + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Ambiguous attribute key", + Detail: "If this expression is intended to be a reference, wrap it in parentheses. If it's instead intended as a literal name containing periods, wrap it in quotes to create a string literal.", + Subject: e.Range().Ptr(), + }) + return cty.DynamicVal, diags + } + if ln := e.literalName(); ln != "" { return cty.StringVal(ln), nil } diff --git a/hcl/hclsyntax/expression_test.go b/hcl/hclsyntax/expression_test.go index 0a9ee7f..aef22a0 100644 --- a/hcl/hclsyntax/expression_test.go +++ b/hcl/hclsyntax/expression_test.go @@ -409,6 +409,40 @@ upper( cty.DynamicVal, 1, // Incorrect key type; Can't use this value as a key: string required }, + { + `{"centos_7.2_ap-south-1" = "ami-abc123"}`, + nil, + cty.ObjectVal(map[string]cty.Value{ + "centos_7.2_ap-south-1": cty.StringVal("ami-abc123"), + }), + 0, + }, + { + // This is syntactically valid (it's similar to foo["bar"]) + // but is rejected during evaluation to force the user to be explicit + // about which of the following interpretations they mean: + // -{(foo.bar) = "baz"} + // -{"foo.bar" = "baz"} + // naked traversals as keys are allowed when analyzing an expression + // statically so an application can define object-syntax-based + // language constructs with looser requirements, but we reject + // this during normal expression evaluation. + `{foo.bar = "ami-abc123"}`, + nil, + cty.DynamicVal, + 1, // Ambiguous attribute key; If this expression is intended to be a reference, wrap it in parentheses. If it's instead intended as a literal name containing periods, wrap it in quotes to create a string literal. + }, + { + // This is a weird variant of the above where a period is followed + // by a digit, causing the parser to interpret it as an index + // operator using the legacy HIL/Terraform index syntax. + // This one _does_ fail parsing, causing it to be subject to + // parser recovery behavior. + `{centos_7.2_ap-south-1 = "ami-abc123"}`, + nil, + cty.EmptyObjectVal, // (due to parser recovery behavior) + 1, // Missing key/value separator; Expected an equals sign ("=") to mark the beginning of the attribute value. If you intended to given an attribute name containing periods or spaces, write the name in quotes to create a string literal. + }, { `{"hello" = "world", "goodbye" = "cruel world"}`, nil, diff --git a/hcl/hclsyntax/parser.go b/hcl/hclsyntax/parser.go index 5784f0c..ee1da09 100644 --- a/hcl/hclsyntax/parser.go +++ b/hcl/hclsyntax/parser.go @@ -273,7 +273,7 @@ Token: return &Block{ Type: blockType, Labels: labels, - Body: &Body{ + Body: &Body{ SrcRange: ident.Range, EndRange: ident.Range, }, @@ -1135,7 +1135,8 @@ func (p *parser) parseObjectCons() (Expression, hcl.Diagnostics) { next = p.Peek() if next.Type != TokenEqual && next.Type != TokenColon { if !p.recovery { - if next.Type == TokenNewline || next.Type == TokenComma { + switch next.Type { + case TokenNewline, TokenComma: diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Missing attribute value", @@ -1143,7 +1144,23 @@ func (p *parser) parseObjectCons() (Expression, hcl.Diagnostics) { Subject: &next.Range, Context: hcl.RangeBetween(open.Range, next.Range).Ptr(), }) - } else { + case TokenIdent: + // Although this might just be a plain old missing equals + // sign before a reference, one way to get here is to try + // to write an attribute name containing a period followed + // by a digit, which was valid in HCL1, like this: + // foo1.2_bar = "baz" + // We can't know exactly what the user intended here, but + // we'll augment our message with an extra hint in this case + // in case it is helpful. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Missing key/value separator", + Detail: "Expected an equals sign (\"=\") to mark the beginning of the attribute value. If you intended to given an attribute name containing periods or spaces, write the name in quotes to create a string literal.", + Subject: &next.Range, + Context: hcl.RangeBetween(open.Range, next.Range).Ptr(), + }) + default: diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Missing key/value separator",