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

Update for new HIL type system #11704

Closed
wants to merge 6 commits into from
Closed

Update for new HIL type system #11704

wants to merge 6 commits into from

Conversation

apparentlymart
Copy link
Contributor

@apparentlymart apparentlymart commented Feb 4, 2017

This is the Terraform counterpart to hashicorp/hil#42.

After that change is merged, HIL's type system is extended to support parameterized types. The initial use of this is to represent explicitly the element types of lists and map values, where previously HIL merely represented the list-ness and map-ness of these values without speaking about the element type.

A lot of this change is simply adjusting the function definitions to use the new type definition syntax, where TypeList and TypeMap have become struct types rather than enumeration values as before.

However, this also presented an opportunity to remove some user-frustrating limitations in many of the functions. In Terraform 0.7 we introduced the idea of maps and lists but that initial implementation was slightly hobbled by this HIL type system limitation, leading to the introduction of the concepts of "flat lists" and "flat maps", where functions would take only lists or maps of string values due to being unable to operate generically over all types.

As a consequence of this, several list and map functions are now able to operate generically over arbitrary element types, including arbitrarily-nested structures as long as the element types remain homogeneous. For example, the concat function can now concatenate two lists of lists of strings into a single list of lists of strings, and lookup can allow defaulted lookups into maps of lists. Full details on these changes can be found in the diff for the interpolation function documentation, and when merging this we may wish to enumerate these separately in CHANGELOG to present this change in a user-oriented manner, rather than a type-system-nerd-oriented manner.

This also inherits the updated HIL functionality that allows the index syntax to be a true operator in its own right, allowing chaining of indexing and indexing function return values:

  • something[i][j]
  • somefunc()[i]

Note that the vendor update for HIL is included in this PR. This will need to be rebased if there are any updates to the hashicorp/hil#42 branch during its review.

@apparentlymart
Copy link
Contributor Author

(There are still some bugs in the HIL change that need to be resolved before the tests will pass here.)

@apparentlymart apparentlymart changed the title Update interpolation functions for new HIL type system Update for new HIL type system Feb 5, 2017
@apparentlymart
Copy link
Contributor Author

As well as the conflict that Github already detected, this will likely also need an adjustment to the implementation of the new slice function that was added in #9729, which will need to be altered to use ReturnTypeFunc.

HIL now has a first-class representation of the element types of lists and
maps, allowing the index operator to be properly type-checked without
relying on dynamic data.
Since each of these tests contains several distinct test cases, this makes
the test output easier to understand when one fails in a non-graceful
way (e.g. a panic).
@apparentlymart
Copy link
Contributor Author

I have fixed the merge config (which was in docs) and updated the slice function implementation to be compatible with the new approach.

It looks like this is now getting tripped up on a test added for #12210 in the mean time:

--- FAIL: TestResourceGH12183_basic (0.03s)
	testing.go:265: Step 0 error: Error applying: 1 error(s) occurred:
		
		* test_resource_gh12183.b: inconsistent map element types; previously string but found list of string at key rules
FAIL
FAIL	github.com/hashicorp/terraform/builtin/providers/test	0.740s

I didn't yet have a chance to dig into what's going on here. Probably it's a bug that was always there and this new test is just exposing it for the first time. @mitchellh if you happen to have a good guess as to what's going on here I'd appreciate it, but if it's not immediately obvious I'll try to dig in next weekend and debug in more detail.

HIL now requires lists and maps to be parameterized with their element
types, so we need to go back and specify what sorts of collections each
of our functions deals with.

In some cases our functions operate generically over collection types.
Previously this was handled within the implementation by inspecting the
element values, but new ReturnTypeFunc and CallbackFunc attributes allow
us to express the type relationships explicitly and thus allow type
checking of values that aren't known until evaluation, such as function
return values.

This change removes some element type limitations that previously existed
for some of the list and map functions. The full details of these
changes can be seen in the diff of the interpolation syntax documentation.
This also eliminates the idea of "flat maps" and "flat lists" from the
documentation, instead referring to "lists of strings" in cases where
that still matters, and to just "maps" and "lists" in cases where
the functions are now element-type-generic.
The new version of HIL stringifies its types in a slightly different way
because they are no longer just enumerated int values.
Using t.Run allows us to run all of the test cases even if one of them
fails. The output in the event of a failure is also adjusted to be more
useful for debugging.
We now need to specify the element type of a list.
@apparentlymart
Copy link
Contributor Author

This is unlikely to land in its current state and I don't have the energy to keep it mergeable for a long period so I am going to close this and plan to revisit at some later date when it's more of a core priority.

@ghost
Copy link

ghost commented Apr 15, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants