function: Deeply unmark function arguments before calling, reapply afterwards #72
Conversation
For functions which do not explicitly support marks, we now deeply unmark arguments before calling, and reapply those marks to the return value. This ensures that these functions do not panic with arguments which are collection values with marked descendants. This does result in overly conservatively applying marks in some cases, but that can be fixed with later work to add explicit support for marks to those functions.
Codecov Report
@@ Coverage Diff @@
## master #72 +/- ##
==========================================
+ Coverage 70.09% 70.30% +0.20%
==========================================
Files 79 79
Lines 6534 6536 +2
==========================================
+ Hits 4580 4595 +15
+ Misses 1514 1498 -16
- Partials 440 443 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for working on this, @alisdair!
I have a little ambivalence inline that I'm curious to hear your opinion on. Other than some indecision about some earlier code of mine, this seems great to me and I'd be happy to merge it either way.
| resultMarks = append(resultMarks, marks) | ||
| args = newArgs | ||
| if !spec.AllowMarked { | ||
| unwrappedVal, marks := val.UnmarkDeep() |
There was a problem hiding this comment.
I was going to suggest using val.ContainsMarked rather than val.Unmark here on the grounds that the old code here was trying to avoid reconstructing data structures unless it actually encountered something marked. However, having reviewed both functions it seems like ContainsMarked is currently potentially rather expensive too -- at the very least, it will allocate an array deep enough to represent the deepest cty.Path within the data structure -- so maybe it's not a big deal which to use.
Given that though, I wonder if the complexity of only conditionally copying the args array (as described in the comment below) is warranted anymore. Maybe we should now just args in all cases and just conditionally assign over zero or more of its elements, to make this overall easier to follow.
What do you think? I'm not greatly bothered by this, but I didn't really like the complexity of this conditional copying in the first place and had only really done it because val.IsMarked() is a relatively cheap operation that avoided doing a bunch of work in the common case.
There was a problem hiding this comment.
My thinking here was that spec.AllowMarked is almost always going to be false. Of ~80 stdlib functions, only 9 currently support marked parameters. When UnmarkDeep is potentially so expensive, I felt the complexity trade-off was reasonable.
|
Thanks for the thoughts on my comment above! Having had a little more time to reflect I agree with what you said and also see that this is an implementation detail we can always adjust over time if it ends up being a performance bottleneck, but that's relatively unlikely. |
For functions which do not explicitly support marks, we now deeply unmark arguments before calling, and reapply those marks to the return value. This ensures that these functions do not panic with arguments which are collection values with marked descendants.
This does result in overly conservatively applying marks in some cases, but that can be fixed with later work to add explicit support for marks to those functions.
I've added tests for the
joinfunction, which I now think exhibits the correct behaviour.Replaces #71