convert: Remove nested optional attributes when converting empty lists and sets

[liamcervante]

For hashicorp/terraform#31924

This PR makes the Convert function explicitly strip out any nested optional attributes from empty lists and sets as they are converted.

I added three unit tests, one each for list, set and map. Interestingly, map conversion works even without the change applied. I wonder if there's something else about the way maps work that means they're not affected by this. I've left the unit test in but haven't made any change to the map logic.

[jbardin]

There are quite a few places in the collection conversion code where empty collections are instantiated. Do we also need to strip out optional attributes in any of the other locations?

[liamcervante]

There are quite a few places in the collection conversion code where empty collections are instantiated. Do we also need to strip out optional attributes in any of the other locations?

I checked this, and I don't think so. Both List and Set are done in the main case and Map doesn't seem to be affected by this particular case (not sure why).

The other times empty lists or sets are instantiated are:

• when the target type has a dynamic type
  • in which case, it pulls the concrete type from the value directly which does have the optional attributes removed
• when a collection is being converted from a tuple
  • in which case, you can't create dynamic tuples so you can't specify that a list contains an object with a non-empty tuple and an object with an empty tuple as a non-empty tuple and an empty tuple are different types - therefore this edge case cannot be reproduced using tuples.

@apparentlymart - could you verify my logic here?

[apparentlymart]

I must admit I'm not sure why this isn't necessary for maps. My understanding of the current state of things is as follows:

cty currently has cty.Type overloaded to mean three different overlapping concepts:

1. A concrete type.
2. A type constraint, which is used for a few different purposes:
   • To describe as closely as possible the possible concrete types of concrete values that an unknown value could be replaced with later.
   • To describe the possible types that a null is substituting for, mainly just to allow the type system to remain coherent when null values are present.
   • To describe the range of possible types that an empty collection could have as its collection type if there were one or more elements in the collection.
3. A target type constraint for type conversion, which is a superset of the previous two which also includes the possibility of optional attributes. This particular meaning of cty.Type is valid only as the second argument to convert.Convert and the other similar functions in the convert package.

The third case here was a late addition specifically to support the optional attributes concept; prior to this, the convert package just used type constraints (case 2). Therefore it seems unsurprising to me that there are some leftover spots in the convert package that don't correctly handle the distinction between case 3 and case 2, and therefore end up using "target type constraints" in situations where normal "type constraints" are expected, such as in the collection element types.

Because optional attributes started off as an experimental thing I didn't want to go infect all of the other codepaths with it, but it seems that Terraform is effectively forcing it to become stable now and so it's time to implement it more robustly. The change here seems like a good step in that direction and I think it's worth merging even though it's probably incomplete.

There are some other things I should add to my todo list to fix up in subsequent changes, building on this:

• Add some new methods to cty.Type which explicitly model whether a particular cty.Type value is a concrete type, a type constrant, or a target type constraint. (I'll probably want to find a better term for "target type constraint" when doing that, because it's confusingly similar to "type constraint").
• Make functions like cty.ListValEmpty, cty.SetValEmpty, cty.MapValEmpty, cty.UnknownVal, and cty.NullVal (is that all of them?) reject with a panic any cty.Type value that is a target type constraint, since those are not valid in that context.
• Update any codepaths which panic as a result of the previous point. I expect this will force the issue of making the convert package also strip optional attributes when the target is a map, even though that is apparently somehow working out okay right now.
• Remove the note from the cty.ObjectWithOptionalAttrs documentation saying that this mechanism is experimental.

In doing the above I may choose to make use of the fact that I marked this as experimental before to make some breaking changes before committing to a final API. However, in doing so I'll aim to make the necessary updates to Terraform/HCL just a matter of search and replace of some different function/method names to try to make the relationships between these things clearer, rather than a significant redesign.

For now though I'm going to merge this as-is because I think it is a good tactical fix to a specific known problem, which need not wait for the more comprehensive changes I described above. Thanks!

[apparentlymart]

Small note for future PRs: I prefer to update the changelog separately from each PR because otherwise PRs tend to get conflicted when I merge them in a different order than they were submitted in. This one worked out okay because I'm merging it immediately, so no harm done here. 😀