-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
convert: Fix panic: heterogeneous tuple with null #56
convert: Fix panic: heterogeneous tuple with null #56
Conversation
Tuples with elements of different types can be converted to homogeneous collections (sets or lists), so long as their elements are unifiable. For example: list("a", "b") // all elements have the same type list("a", 5) // "a" and 5 can be unified to string list("a", 5, null) // null is a valid value for string However, tuples with elements which are not unifiable cannot be converted to homogeneous collections: list(["a"], "b") // no common type for list(string) and string This commit fixes a panic for this failure case, when the tuple contains both non-unifiable types and a null value: list(["a"], "b", null) // should not panic The null value was causing the unification process to result in a list or set of dynamic type, which causes the conversion functions to pass through the original value. This meant that in the final conversion step, we would attempt to construct a list or set of different values, which panics.
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
==========================================
+ Coverage 70.54% 70.60% +0.06%
==========================================
Files 79 79
Lines 7153 7161 +8
==========================================
+ Hits 5046 5056 +10
+ Misses 1664 1662 -2
Partials 443 443
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, that looked "fun" to figure out 😂
@@ -156,34 +156,45 @@ func conversionCollectionToMap(ety cty.Type, conv conversion) conversion { | |||
// given tuple type and return a set of the given element type. | |||
// | |||
// Will panic if the given tupleType isn't actually a tuple type. | |||
func conversionTupleToSet(tupleType cty.Type, listEty cty.Type, unsafe bool) conversion { | |||
func conversionTupleToSet(tupleType cty.Type, setEty cty.Type, unsafe bool) conversion { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent change, tyvm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, nice! Thanks for digging into this. I have a memory of thinking something was a little off here when I originally wrote it but at the time I could quite get my head around it, but now you've pointed it out this makes perfect sense.
Tuples with elements of different types can be converted to homogeneous collections (sets or lists), so long as their elements are unifiable. For example:
However, tuples with elements which are not unifiable cannot be converted to homogeneous collections:
This commit fixes a panic for this failure case, when the tuple contains both non-unifiable types and a null value:
The null value was causing the unification process to result in a list or set of dynamic type, which causes the conversion functions to pass through the original value. This meant that in the final conversion step, we would attempt to construct a list or set of different values, which panics.
Fixes hashicorp/terraform#24377