hclsyntax: Source range of IndexExpr, SplatExpr, and RelativeTraversalExpr must cover whole expression #328
Conversation
apparentlymart
force-pushed
the
b-hclwrite-parser-crash
branch
from
f040a8e
to
b84c5c7
Compare
| @@ -867,8 +867,9 @@ Traversal: | |||
| Collection: ret, | |||
| Key: keyExpr, | |||
|
|
|||
| SrcRange: rng, | |||
| OpenRange: open.Range, | |||
| SrcRange: hcl.RangeBetween(from.Range(), rng), | |||
There was a problem hiding this comment.
from in this function is the expression that the subsequent postfix operators are applying to, so in an expression like foo[bar] the from variable refers to the foo expression.
| End: hcl.Pos{Line: 1, Column: 26, Byte: 25}, | ||
| }, | ||
| }, | ||
| Each: &AnonSymbolExpr{ |
There was a problem hiding this comment.
SplatExpr is a bit of an odd one in that it evaluates this Each expression multiple times, each time applying a localized substitution for the AnonSymbolExpr given in Item below. In this test case there is nothing after the [*] in the input, so the "each" expression is just the anonymous symbol itself; a [*] without subsequent traversals on a list expression just redundantly re-produces the list it was applied to.
| @@ -555,6 +555,78 @@ func TestParse(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| { | |||
| "a = foo[bar]\n", | |||
There was a problem hiding this comment.
What hclwrite's "parser" is doing here is turning the raw token stream into a physical parse tree, as opposed to the abstract parse tree in hclsyntax.
It uses the source ranges from hclsyntax to find the bounds of the attribute definition and its expression, and then it further marks out the traversals inside the expression in order to enable "rename symbol"-type operations in text editors to surgically update the references without affecting the surrounding tokens.
This case was panicking before because the foo token was not being indicated by hclsyntax as part of the expression, and so when the traversal analyzer attempted to find the TokenIdent to learn the name it was looking into an empty token sequence.
| { | ||
| Type: "identifier", | ||
| Val: " foo", | ||
| }, |
There was a problem hiding this comment.
This is the identifier extracted from the foo token, which was previously excluded from the parent expression by the faulty source boundaries on the IndexExpr node.
| @@ -631,7 +632,7 @@ func (e *IndexExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { | |||
| diags = append(diags, collDiags...) | |||
| diags = append(diags, keyDiags...) | |||
|
|
|||
| val, indexDiags := hcl.Index(coll, key, &e.SrcRange) | |||
| val, indexDiags := hcl.Index(coll, key, &e.BracketRange) | |||
There was a problem hiding this comment.
This final argument is what hcl.Index uses as the subject of any diagnostics it produces if the index operation fails. I've changed this so that errors relating to incorrectly-typed or incorrectly-valued indices will continue to highlight only the bracketed part, making it clearer which part of the expression is faulty.
Some HCL callers make the (reasonable) assumption that the overall source range of an expression will be a superset of all of the ranges of its child expressions, for purposes such as extraction of source code snippets, parse tree annotation in hclwrite, text editor analysis functions like "go to reference", etc. The IndexExpr type was not previously honoring that assumption, since its source range was placed around only the bracket portion. That is a good region to use when reporting errors relating to the index operation, but it is not a faithful representation of the full extent of the expression. In order to meet both of these requirements at once, IndexExpr now has both SrcRange covering the entire expression and BracketRange covering the index part delimited by brackets. We can then use BracketRange in our error messages but return SrcRange as the result of the general Range method that is common to all expression types.
apparentlymart
force-pushed
the
b-hclwrite-parser-crash
branch
from
b84c5c7
to
84e71e9
Compare
apparentlymart
changed the title
Some
hclsyntaxcallers make the (reasonable) assumption that the overall source range of an expression will be a superset of all of the ranges of its child expressions, for purposes such as extraction of source code snippets, parse tree annotation in hclwrite, text editor analysis functions like "go to reference", etc.The
IndexExprtype was not previously honoring that assumption, since its source range was placed around only the bracket portion. That is a good region to use when reporting errors relating to the index operation, but it is not a faithful representation of the full extent of the expression.In order to meet both of these requirements at once,
IndexExprnow has bothSrcRangecovering the entire expression andBracketRangecovering the index part delimited by brackets. We can then useBracketRangein our error messages but returnSrcRangeas the result of the generalRangemethod that is common to all expression types.While investigating this I noticed that both
SplatExpr(both full and attribute-only variations) andRelativeTraversalExprhad a similar issue, and have also corrected those in a similar way. They were already separately tracking the "operator part" of the expression for error reporting purposes, so no additional field was required for these ones.This also includes a new test case for
hclwriteshowing that this PR fixes #327, by allowinghclwrite's analyzer to determine the correct bounds of an index expression in order to partition the tokens correctly in its physical syntax tree.