Panic during nested splat evaluation with nulls

[cam72cam]

Initially reported in opentofu/opentofu#2327, it appears that null handling with nested splats can cause panics in seemingly valid HCL.

I traced it down to https://github.com/hashicorp/hcl/blob/b48ba6e/hclsyntax/expression.go#L1803. Instead of returning a tuple with a null element, it returns an empty tuple. When patched to instead return cty.TupleVal([]cty.Value{sourceVal}).WithSameMarks(sourceVal), it functions as expected in this particular scenario.

What I have not yet investigated is what the intended code path here is, should it be an error caught earlier in the process or should the patch above be accepted as the "correct" functionality here.

It looks like it was originally introduced in 2b184ca

[apparentlymart]

I've pushed a simpler reproduction of this into my fork: apparentlymart@ead0f8c

That commit is likely to vanish from my repository in the future, so here are the two new cases for TestExpressionParseAndValue in expression_test.go:

		{
			`listofobj[*].scalar[*]`,
			&hcl.EvalContext{
				Variables: map[string]cty.Value{
					"listofobj": cty.ListVal([]cty.Value{
						cty.ObjectVal(map[string]cty.Value{
							"scalar": cty.StringVal("foo"),
						}),
						cty.ObjectVal(map[string]cty.Value{
							"scalar": cty.StringVal("bar"),
						}),
					}),
				},
			},
			cty.ListVal([]cty.Value{
				// The second-level splat promotes the scalars to single-element tuples.
				cty.TupleVal([]cty.Value{cty.StringVal("foo")}),
				cty.TupleVal([]cty.Value{cty.StringVal("bar")}),
			}),
			0,
		},
		{
			`listofobj[*].scalar[*]`,
			&hcl.EvalContext{
				Variables: map[string]cty.Value{
					"listofobj": cty.ListVal([]cty.Value{
						cty.ObjectVal(map[string]cty.Value{
							"scalar": cty.NullVal(cty.String),
						}),
						cty.ObjectVal(map[string]cty.Value{
							"scalar": cty.StringVal("bar"),
						}),
					}),
				},
			},
			cty.ListVal([]cty.Value{
				// FIXME: The expected result of this is actually impossible because
				// a list can't contain both an empty tuple and a single-element tuple
				// at the same time. Therefore this actually panics.
				cty.EmptyTupleVal,
				cty.TupleVal([]cty.Value{cty.StringVal("bar")}),
			}),
			0,
		},

Unfortunately I think fixing this is not as simple as just deciding that the expression should return a single-element tuple in this case.

The rule for applying [*] to a scalar value (that is: anything except a list, tuple, or set) is that it either becomes a one-element tuple or a single-element tuple depending on whether the scalar value is null. The OpenTofu/Terraform-level documentation describes this behavior (slightly-misleadingly) as Single Values as Lists:

Splat expressions have a special behavior when you apply them to a value that isn't a list, set, or tuple.

If the value is anything other than a null value then the splat expression will transform it into a single-element list, or more accurately a single-element tuple value. If the value is null then the splat expression will return an empty tuple.

Despite the heading saying "lists", the subsequent text does at least clarify that it's actually generating tuples instead, as we see here.

Unfortunately in this case that interacts with another rule about splat expressions: if you apply [*] to a list (as opposed to a tuple) then its result is also a list, under the assumption that all elements of a list must have the same type and therefore usually all of the results of applying the splat also have the same type.

Combining those two rules together by nesting splats unfortunately violates the assumption made in the splat-on-list rule: it suddenly becomes possible for the nested splat to return different types depending on the value of the top-level list element, rather than only on the type of the top-level list element.

Resolving this seems to require some sort of special concession. The two main options I see are:

1. If the first-level splat is on a list but the nested splat produces elements of different types then force the top-level splat to return a tuple rather than a list, so that the elements can then have different types.
2. If the first-level splat is on a list then a nested splat involving a scalar value would produce either a zero or one element list instead of tuple, thereby making the result be a list of lists with homogenous element types rather than a list of tuples of different types.

The first of these options seems the most viable to me because it doesn't require the inner splat to know anything about the outer splat. The special case of returning a list when the splat source is a collection is already a special exception done right at the end anyway:

hcl/hclsyntax/expression.go

Lines 1934 to 1944 in b48ba6e

	switch {
	case sourceTy.IsListType() || sourceTy.IsSetType():
	if len(vals) == 0 {
	ty, tyDiags := resultTy()
	diags = append(diags, tyDiags...)
	return cty.ListValEmpty(ty.ElementType()).WithMarks(marks), diags
	}
	return cty.ListVal(vals).WithMarks(marks), diags
	default:
	return cty.TupleVal(vals).WithMarks(marks), diags
	}

...although it also has a counterpart in the resultTy closure, to make sure that the two agree:

hcl/hclsyntax/expression.go

Lines 1849 to 1875 in b48ba6e

	resultTy := func() (cty.Type, hcl.Diagnostics) {
	chiCtx := ctx.NewChild()
	var diags hcl.Diagnostics
	switch {
	case sourceTy.IsListType() || sourceTy.IsSetType():
	ety := sourceTy.ElementType()
	e.Item.setValue(chiCtx, cty.UnknownVal(ety))
	val, itemDiags := e.Each.Value(chiCtx)
	diags = append(diags, itemDiags...)
	e.Item.clearValue(chiCtx) // clean up our temporary value
	return cty.List(val.Type()), diags
	case sourceTy.IsTupleType():
	etys := sourceTy.TupleElementTypes()
	resultTys := make([]cty.Type, 0, len(etys))
	for _, ety := range etys {
	e.Item.setValue(chiCtx, cty.UnknownVal(ety))
	val, itemDiags := e.Each.Value(chiCtx)
	diags = append(diags, itemDiags...)
	e.Item.clearValue(chiCtx) // clean up our temporary value
	resultTys = append(resultTys, val.Type())
	}
	return cty.Tuple(resultTys), diags
	default:
	// Should never happen because of our promotion to list above.
	return cty.DynamicPseudoType, diags
	}
	}

The case where the top-level list is unknown presents a problem, because if we don't know the source list's value then we can't know whether the values inside it would cause a nested splat to return differing tuple types. Returning an unknown value of the wrong type would violate the rules for unknown values, so it seems like this would need to be defined structurally rather than value-wise. Similarly, retroactively making this case return cty.DynamicVal (unknown value of unknown type) instead of a more constrained unknown value would violate the rule that subsequent releases can't make any result "less known" than it was before.

Perhaps the rule should be: if the current splat's Each is also a SplatExpr then the result is never a cty.List value. However, that is potentially a breaking change since I expect it's possible today to write a nested splat where both levels are lists and thus get a list of lists as the result.

Seems like there's a tricky tradeoff to be made here, one way or another. In the meantime though, perhaps we can compromise by making this case return an error explaining that the result is impossible instead of crashing, since that would just make this get handled a little more robustly without taking away anything that's already working. I'm going to work on a little patch for that variation as a starting point, though hopefully we can find a better answer to make this work and return a valid result in the long run.

[apparentlymart]

Looking back on the history here, it seems that unfortunately I introduced most of both of these conflicting rules in the same commit: df9794b. 🤦‍♂️

As I'd guessed in my previous comments, this was motivated by making splat expressions return better type information when unknown values are used as input.

Where things seem to have gone really wrong was in this later change: 2b184ca. That would've been a valid change if we hadn't previously switched splat-on-scalar from returning lists to returning tuples, but it inadvertently introduced the first situation where splat over a list could produce individual elements of different types even though all of the original list elements are always of the same type.

Unfortunately the issue link on that second commit seems to be incorrect, since it refers to an issue that was opened long before I even started working on ZCL / HCL 2. Since that second change made HCL 2 behave more like Terraform v0.11's treatment of splats I'm taking this as not-quite-confirmation of my hypothesis that this was something we did during the Terraform v0.12 development hell to try to preserve compatibility with some existing usage patterns. But the reason for that change doesn't really matter now that the behavior has been in place for this long: it's still unclear to me what design change could be made here to make this work without risking a behavior change for currently-valid input.