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

Parse indexing [] as a postfix operator #34

Closed
wants to merge 2 commits into from
Closed

Parse indexing [] as a postfix operator #34

wants to merge 2 commits into from

Conversation

apparentlymart
Copy link
Contributor

Previously indexing was treated as a part of variable access, which meant it couldn't be applied to non-variable expressions such as the result of a function.

Now indexing is treated as a postfix operator in its own right, meaning that it can follow any expression term.

This change is backward-compatible because a postfix operator following a variable access produces an AST identical to the previous, and other uses of brackets were previously a syntax error.

The main reason for this is to allow constructions like split(" ", foo)[0] in Terraform. Currently such things must be done with the element function. With this in place it's likely that we can drop the element function from Terraform in a future release. (so we might want to mark it as deprecated now?)

@apparentlymart
Copy link
Contributor Author

I realized I hadn't actually added any eval tests, since I was just working in the parser. I added one and found that the type check doesn't work unless the target is a variable, so some rework is needed there.

I don't know the eval stuff so well so I need to study up on it before I can fix that. This parser change doesn't do any harm in the mean time, though... just turns what was previously a syntax error into a type check error.

Previously indexing was treated as a part of variable access, which meant
it couldn't be applied to non-variable expressions such as the result of
a function.

Now indexing is treated as a postfix operator in its own right, meaning
that it can follow any expression term.

This change is backward-compatible because a postfix operator following
a variable access produces an AST identical to the previous, and other
uses of brackets were previously a syntax error.

This doesn't change the type checking or evaluation behavior, so this change
alone doesn't actually make indexing of function return values work, it just
changes it from being a parse error to being a type checker error. The
short-term value here is to enable a more helpful error message for this
scenario, rather than the raw syntax error we had previously. A later
improvement to the type system could then make this work for real.
Due to limitations of HIL's type system we are currently only able to do
static type checking when indexing is applied directly to variables. We
can't, for example, type check an expression like split(" ", foo)[0]
because we only know that split returns a list, and not what it's a
list *of*.

We will hopefully support this later, but for now we can have an error
message that explicitly describes the problem.
@apparentlymart
Copy link
Contributor Author

After digging into the type checker a bit I came to the realization that HIL's type system is not currently sophisticated enough to support what I was trying to do here. Since array and map types are not parameterized by their element types, if we ask Terraform's split function for its return type it will just tell us "I return a list", but we won't know what it's a list of until the function is actually called.

So after some discussion with @mitchellh I decided to rescope this to just be a parser change along with a minor tweak to the type checker's error message. This means that indexing functions still doesn't work, but at least now if you try it you'll get an error message addressing that specific problem, rather than just a generic parse error.

Actually making indexing work will require a bit of an overhaul of how HIL thinks about types, which is too much to get done in time for Terraform 0.8.

@apparentlymart
Copy link
Contributor Author

I forgot I had already done this as a separate changeset and ended up redoing it as one of the commits on #42.

I'm going to close this to consolidate over there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant