Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hclsyntax: Source range of IndexExpr, SplatExpr, and RelativeTraversalExpr must cover whole expression #328

Merged
merged 1 commit into from
Dec 6, 2019

Conversation

apparentlymart
Copy link
Contributor

@apparentlymart apparentlymart commented Dec 6, 2019

Some hclsyntax 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.

While investigating this I noticed that both SplatExpr (both full and attribute-only variations) and RelativeTraversalExpr had 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 hclwrite showing that this PR fixes #327, by allowing hclwrite's analyzer to determine the correct bounds of an index expression in order to partition the tokens correctly in its physical syntax tree.

@@ -867,8 +867,9 @@ Traversal:
Collection: ret,
Key: keyExpr,

SrcRange: rng,
OpenRange: open.Range,
SrcRange: hcl.RangeBetween(from.Range(), rng),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 apparentlymart changed the title hclsyntax: Source range of IndexExpr must cover whole expression hclsyntax: Source range of IndexExpr, SplatExpr, and RelativeTraversalExpr must cover whole expression Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants