cty: Allow list of vals with partly-dynamic types #85
Conversation
The ListVal constructor function previously required that all elements
had exactly the same type. In some cases involving partly-dynamic
element types, this was overly strict. This commit relaxes the
constraints which caused these cases to panic.
If we attempted to create a list of objects, some of which contained
dynamic pseudo-type fields, the constructor would panic. For example,
consider this (HCL) example:
list = [
{
ids = [1, 2],
},
{
ids = [for obj in some_dynamic_list: obj.id],
},
]
This panic happened because the two element types were different:
namely, Object({ ids: string }) and Object({ ids: dynamic }). However,
these types are unifiable, and the convert package calls ListVal after
verifying this.
Instead of strictly comparing all element types, we now only panic if
the list contains two or more elements with fully concrete types which
are inequal. Element types which are dynamic or partly dynamic are
permitted.
Codecov Report
@@ Coverage Diff @@
## main #85 +/- ##
==========================================
+ Coverage 70.64% 70.69% +0.05%
==========================================
Files 79 79
Lines 6547 6549 +2
==========================================
+ Hits 4625 4630 +5
+ Misses 1478 1475 -3
Partials 444 444
Continue to review full report at Codecov.
|
|
Thanks for looking into this, @alisdair! My approach for this sort of thing so far was to say that the main With that said, if possible I'd prefer to preserve that distinction and consider changing the conversion rules in ...in which case, I'd prefer to consider this to be a bug in With that said, I've not looked closely at the caller in |
|
I think that the change to Given that, I'm going to close this, though I'm happy to reopen it and reconsider if you disagree with my assertion that this has been covered by the other PR. |
The ListVal constructor function previously required that all elements had exactly the same type. In some cases involving partly-dynamic element types, this was overly strict. This commit relaxes the constraints which caused these cases to panic.
If we attempted to create a list of objects, some of which contained dynamic pseudo-type fields, the constructor would panic. For example, consider this (HCL) example:
This panic happened because the two element types were different: namely,
Object({ ids: string })andObject({ ids: dynamic }). However, these types are unifiable, and the convert package callsListValafter verifying this.Instead of strictly comparing all element types, we now only panic if the list contains two or more elements with fully concrete types which are inequal. Element types which are dynamic or partly dynamic are permitted.
Downstream issue, verified fixed with this change: hashicorp/terraform#27275
I'm not sure this is the right approach to solving this problem. It feels inelegant, but I can't think of a better solution. If we do want to change this behaviour, the change should probably also be applied to
MapValandSetVal, which follow a similar pattern for checking dynamic element types.