-
Notifications
You must be signed in to change notification settings - Fork 601
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
Panic during nested splat evaluation with nulls #723
Comments
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 {
`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
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 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:
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: Lines 1934 to 1944 in b48ba6e
...although it also has a counterpart in the Lines 1849 to 1875 in b48ba6e
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 Perhaps the rule should be: if the current splat's 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. |
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. |
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
The text was updated successfully, but these errors were encountered: