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

Type system to support list and map element types #42

Closed

Conversation

@apparentlymart
Copy link
Member

apparentlymart commented Dec 11, 2016


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 type somefunc will return until it is executed
  • somevar[0][1] - this was actually checkable before but not supported by the parser

This 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 to ast.TypeList and ast.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 and TypeMap 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 of ReturnType in order to provide its return type dynamically based on its argument types. This can be used in conjunction with the types TypeAny, TypeList{TypeAny} and TypeMap{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.

convert.go Outdated
@@ -94,7 +94,7 @@ func InterfaceToVariable(input interface{}) (ast.Variable, error) {
}

return ast.Variable{
Type: ast.TypeList,
Type: ast.TypeList{ast.TypeAny},

This comment has been minimized.

Copy link
@apparentlymart

apparentlymart Dec 11, 2016

Author Member

I expect we need to do something a little more elaborate here to correctly infer the appropriate type, but I'm not sure what exactly this codepath is used for so not sure what the most appropriate behavior would be.

This comment has been minimized.

Copy link
@mitchellh

mitchellh Dec 11, 2016

Member

This function is used heavily in Terraform for converting user input and decoded state values into ast.Variable.

This comment has been minimized.

Copy link
@apparentlymart

apparentlymart Dec 11, 2016

Author Member

Ahh, that makes sense... for some reason I'd previously assumed this was not used outside of HIL, but I'll dig into Terraform to understand the usage patterns there and then update this to do something more sensible.

@mitchellh

This comment has been minimized.

Copy link
Member

mitchellh commented Dec 12, 2016

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.

@apparentlymart

This comment has been minimized.

Copy link
Member Author

apparentlymart commented Dec 12, 2016

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.

@cemo

This comment has been minimized.

Copy link

cemo commented Feb 1, 2017

@apparentlymart @mitchellh what is the status of this issue? Will be available for 0.9?

Previously Type was just an enumeration of simple types, but that meant
that there was no way for the type system to represent the element type
of a list or a map value.

Here we change Type to instead be an interface, and then provide separate
concrete types for primitives, lists, and maps. This initial patch
doesn't actually change any behavior to *use* the list and map element
types, but rather just creates a place for the element types to live and
then generally ignores it.

This intentionally preserves the convenient ability to compare types by
the standard equality operator == and treat the primitive types as
opaque values accessed via globals. Thus code dealing with the primitive
types is generally unchanged by this, but this is a breaking change for
code that deals with list and map types, and it is the updates for this
that make up the bulk of this change.

The most interesting part of this change is in ast/type.go where the new
type system is defined as a selection of Go types.
Since the old type system could not represent the element type of a list
or map, the type check for the index operator [] previously required that
its operand be a variable access so that the variable's value could be
used to infer the element type.

We are now able to represent element types directly in the type system,
so we can eliminate this constraint and determine the element type based
only on the type information, without needing to inspect runtime data.
Previously we treated indexing as an extension of variable access syntax
for parsing purposes. Now we treat it as a separate operator that can
appear after any term that returns a list or map of something.

This also allows us to include a test for the index type checking that
was generalized in the previous commit, since the type checker tests
depend on the parser.
Previously, due to historical constraints with the type system, we allowed
indexing only of variables directly. The type system and parser are now
generalized to support arbitrary indexing, so this completes the picture
making indexing on non-variable expressions return the expected value.

In particular, this allows indexing arrays of arrays and other such
nested data structures, and allows indexing into the results of functions
that return lists and maps.
Now that lists and map types are parameterized by their element types we
expect that all collection values will carry a concrete element type.
However, some functions operate generically on collections of any type,
such as Terraform's "concat" function which should be able to accept
lists of any type (as long as they all match) and return a list of that
same type.

To support this we allow functions to optionally define their own type
checking logic, in addition to the simple type checking built in to HIL.
The use of TypeAny as a parameter type wildcard is extended to allow
functions to accept TypeList{TypeAny} and TypeMap{TypeAny} to express
generic list and map support, and then the function declaration can use
the new ReturnTypeFunc field to explain to HIL how the given argument
types map to a particular return type.

Returning to the "concat" example, this function would be defined as
being Variadic with a type of TypeList{TypeAny}, and then its
ReturnTypeFunc would check that the ElementType of all of the given lists
is identical and indicate that its return type is that ElementType.
@apparentlymart apparentlymart force-pushed the apparentlymart:composite-types branch from 53b14db to ac51c9d Feb 4, 2017
@apparentlymart apparentlymart force-pushed the apparentlymart:composite-types branch from ac51c9d to fb13224 Feb 5, 2017
The functions in convert.go allow conversion between Go values and HIL
variable instances and vice-versa. This is used by calling applications
such as Terraform to integrate with other code that is not aware of HIL.

Now that our collection types have specified element types, we must
populate these correctly when converting from Go values. This problem
is simplified because these conversion functions use only the collection
types and the string type, and thus we can assume that we'll always end
up eventually making a string variable.
Now that we support computed return types for functions, it's convenient
to retain the computed return type and pass it in to the function's
implementation so that it doesn't need to repeat the logic to determine
the result type when working with collections generically.

To support this we add a new CallbackTyped attribute to Function which
can be used instead of Callback when knowing the return type is desired.
A new AST node CallTyped is used to augment a Call node with its
computed result type from the type check phase, which can then be used
in the eval phase.
@apparentlymart apparentlymart force-pushed the apparentlymart:composite-types branch from fb13224 to 7cd522a Feb 5, 2017
@apparentlymart

This comment has been minimized.

Copy link
Member Author

apparentlymart commented Feb 5, 2017

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.

@mitchellh

This comment has been minimized.

Copy link
Member

mitchellh commented Feb 5, 2017

Thanks @apparentlymart I'll try to take a look this week!

@vancluever

This comment has been minimized.

Copy link
Member

vancluever commented Feb 15, 2017

@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.

@apparentlymart

This comment has been minimized.

Copy link
Member Author

apparentlymart commented Feb 15, 2017

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.

@vancluever

This comment has been minimized.

Copy link
Member

vancluever commented Feb 15, 2017

The function was definitely added :) interpolationFuncSlice

So sounds like the only thing missing is the list expression form.

@apparentlymart

This comment has been minimized.

Copy link
Member Author

apparentlymart commented Mar 13, 2017

Closing this for now with the intent to revisit later when it's more of a priority.

@pmoust

This comment has been minimized.

Copy link

pmoust commented Dec 12, 2017

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 ?

@apparentlymart

This comment has been minimized.

Copy link
Member Author

apparentlymart commented Dec 18, 2017

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.

@pmoust

This comment has been minimized.

Copy link

pmoust commented Dec 29, 2017

@apparentlymart I just had time to go over HCL2. Thanks for driving this!

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.