cty: add CanListFunc, CanSetFunc, CanMapFunc #91
Conversation
Codecov Report
@@ Coverage Diff @@
## main #91 +/- ##
==========================================
+ Coverage 70.85% 70.88% +0.02%
==========================================
Files 79 79
Lines 6636 6656 +20
==========================================
+ Hits 4702 4718 +16
- Misses 1490 1491 +1
- Partials 444 447 +3
Continue to review full report at Codecov.
|
cty/value_init.go
Outdated
| @@ -186,6 +184,23 @@ func ListValEmpty(element Type) Value { | |||
| } | |||
| } | |||
|
|
|||
| // CanListFunc returns an error if the given Values can not be coalesced | |||
| // into a single List due to inconsistent element types. | |||
| func CanListFunc(vals []Value) error { | |||
There was a problem hiding this comment.
I think I created this confusion by making a typo when we were originally talking about this, but I was actually proposing for these to be named CanListVal, CanMapVal, etc, to represent that they each determine whether the corresponding Can... function can succeed.
Given that there's only one possible panic situation here, and for consistency with the other Can...-prefixed functions elsewhere in cty, I think I'd rather have these be functions that return bool to indicate that a subsequent call would succeed, rather than returning an error, and then let the ListVal/etc functions be responsible for constructing a suitable panic message as they were before.
This does mean that the convert codepaths will also need to construct themselves a suitable error value to return, but I think that's justified because the error messages there ought to be written with an end-user audience in mind, rather than a Go developer audience in mind. We don't really have enough context to return a great error message at that convert layer (cause we're already in a "shouldn't really happen" situation anyway), so I'd go for something pretty generic like this:
List element types don't match
A higher layer can replace that with a better error message if it has more interesting context to add, but we can save that for a later change.
There was a problem hiding this comment.
Got it! I reverted the corresponding constructor functions (so they maintain the element-specific error message) and refactored the Can... funcs to return bools (that was my original plan anyway, so cool!). I also added test coverage for the new functions.
mildwonkey
force-pushed
the
panic-elems
branch
2 times, most recently
from
8bbb856
to
129cba3
Compare
Despite our best efforts to correctly unify element types during conversion, sometimes some inconsistent types slip through anyway and make the conversion panic. Although in the long run it would be better to fix these problems upstream, it's the responsibility of the caller of cty to not try to perform any invalid operations, and so here we make the convert package pre-verify that it's found a sensible result before trying to construct it, and thus it can return a proper error if not rather than panicking.
apparentlymart
force-pushed
the
panic-elems
branch
from
129cba3
to
139e794
Compare
|
Thanks! |
The logic which may result in a panic in List(), Map() and Set() have been extracted into these helper functions, which return errors instead. The original callers (still) panic if an error is returned.
These new functions are called in the appropriate conversion functions in the convert package to validate that the given elements can be used to create a single Type, to help reduce the number of panics that occur when calling Convert.
For additional background information on the (odd) scenarios that can lead to these panics, see hashicorp/terraform#26195
While working on this, I realized that
conversionCheckMapElementTypeswas doing much the same thing, so I removed it in favor ofCanMapFunc(). However since the logic for CanMapFunc() was taken from Map(), it's lost the path-specific error. If that's not good enough I can roll back its removal!