Closed apparentlymart closed 7 years ago
I didn't do a review yet but I think regardless we'd hold this for Terraform 0.9 anyways.
This is great!
When you say "breaking change" you mean just an API-level breaking change correct? That's totally fine with me at this point. I'll take a closer look and also talked to @jen20 today about getting a 2nd pair of eyes due to the type (har har) of change.
Yes, the breaking change here is the same thing that made this diff so noisy... any caller referring to TypeMap
and TypeList
will need to be updated. Otherwise, things should generally work the way they did before.
@apparentlymart @mitchellh what is the status of this issue? Will be available for 0.9?
Hi @mitchellh and @jen20!
I eventually got around to finishing this up. Sorry for the delay. I think this, and its Terraform counterpart hashicorp/terraform#11704, are now ready for review.
Thanks @apparentlymart I'll try to take a look this week!
@apparentlymart @mitchellh it would be great if we could get support for range expressions for lists too - not too sure if it'd be easy to slip in, but yeah that would be huge. TF ref #11950 but I was here seeing what kind of HIL work would be needed to do it as well. In the current master it looks like it wouldn't be too hard but I'd imagine that work on that should probably wait until the dust settles on this before anyone else starts work on more list or map stuff.
I have thought about this too :) but agreed that it would be better as a separate change since this one is quite big already.
This change makes it possible to implement an interpolation function for Terraform in the absence of first-class syntax and indeed I think we may have just merged one the other day IIRC.
The function was definitely added :) interpolationFuncSlice
So sounds like the only thing missing is the list expression form.
Closing this for now with the intent to revisit later when it's more of a priority.
We have use-cases that would make things a lot simpler if this was implemented.
Is there a plan to get this in the roadmap @apparentlymart ?
This approach was abandoned in favor of a more holistic change to merge HCL and HIL together into a single parser and evaluator whose type system includes the capabilities that were added here.
Work is in progress to integrate the new implementation into Terraform. It's still subject to change until we've finished gathering feedback on it via Terraform (an opt-in version will be available before then to gather this feedback) but eventually this new package will replace both HCL and HIL.
@apparentlymart I just had time to go over HCL2. Thanks for driving this!
THIS IS A BREAKING CHANGE for applications that embed HIL
Thus far the HIL type system has been able to represent lists and maps as if they were primitive types, with no means to represent the element types of these composites. This is problematic for accurately type-checking an expression that applies the indexing operator
[ ... ]
to an expression whose value isn't known:somefunc()[0]
- can't know what element typesomefunc
will return until it is executedsomevar[0][1]
- this was actually checkable before but not supported by the parserThis set of changes retools the HIL type system so that it can capture the element types of lists and maps, thus allowing static type checking of indexing when applied to non-variable expressions. It also adjusts the parser to treat indexing as a standalone operator rather than as a part of variable parsing.
The new Go types representing HIL types are carefully designed so that the convenience of being able to compare HIL types using Go
==
is preserved, and so for most cases this is not a breaking change for callers that directly refer to only the primitive types. However, since the representation of list and map types has changed, this is a breaking change for callers that directly refer toast.TypeList
andast.TypeMap
as if they are values, since they are now types.It is also, more subtly, a breaking change for callers that are not able to provide accurate element type information for lists and maps. Callers must be adjusted to correctly specify the element type, or else evaluation is likely to crash on an invalid type assertion. The functions in
convert.go
are updated to do the right thing when converting slice and map values, so for most callers this should not be an issue.The full changeset for this PR is rather noisy due to all the updates to convert
TypeList
andTypeMap
references to be struct literals rather than mere identifiers; the individual commits will likely be easier to follow.ast/type.go
is probably where a reviewer would want to start, to see how the type system is now represented.This includes an extension to the API for registering interpolation functions that allows functions to have type-generic behavior over collection types.
A registered function can use
ReturnTypeFunc
instead ofReturnType
in order to provide its return type dynamically based on its argument types. This can be used in conjunction with the typesTypeAny
,TypeList{TypeAny}
andTypeMap{TypeAny}
to provide strongly-typed generic behavior, such as a list-concatenation function that returns a list whose element type is the same as that of the lists given in arguments.Terraform is by far the most prolific user of HIL, and in particular has a large set of interpolation functions that operate on lists and maps which need to be updated. These updates, along with other adjustments for the compatibility breaks in this branch, are submitted in hashicorp/terraform#11704.
As well as updating for the new API, this also allowed us to address some seemingly-arbitrary (from the user's perspective) limitations on the builtin functions operating on collections. For more details, see the other PR.
This Terraform changeset will hopefully also serve as a guide for anyone applying similar updates to other HIL-consuming applications.