Decode change values with marks #28687
Conversation
Marks stored in a plans.ChangeSrc were not decoded along with the stored values. This was working in many cases by evaluation correctly re-evaluating the marks, but this cannot happen in all cases.
When an output change was decoded, we were decoding the raw value rather than the ChangeSrc, which lost any encoded marks for that value.
Make sure that this function can handle any unexpectedly marked values. The only remaining caller of this function is in the diff formatter, which uses it to suppress meaningless diffs created by legacy providers.
Marks are not needed when transcoding changes from the internal msgpack format to json.
jbardin
added
the
0.15-backport
|
Is this going to be released within 0.15.4? @jbardin |
| // We drop the marks from the change, as decoding is only an | ||
| // intermediate step to re-encode the values as json | ||
| changeV.Before, _ = changeV.Before.UnmarkDeep() | ||
| changeV.After, _ = changeV.After.UnmarkDeep() |
There was a problem hiding this comment.
Rather than mutating the struct, it might be clearer to bind the results of the UnmarkDeep calls to locals inside the if changeV.[Before|After] != cty.NilVal` blocks below. Those are already messily referring back to the change source to retrieve the marks.
One other worry here is that I think calling UnmarkDeep on cty.NilVal might crash. I'm not completely sure that this is the case, but moving the unmarking inside the if blocks feels safer.
(Same thought for the other changes in this package.)
There was a problem hiding this comment.
I verified that UnmarkDeep is a noop with a NilVal, and opted to overwrite the fields here to leave the rest of the code as unchanged as possible.
I also don't particularly like mutating the value, but since the changeV values have no way of leaking out from this package, I went with the smaller change.
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
When decoding change values, the stored marks should be applied to the decoded value, as this is the direct inverse of the
Encodeoperation. The missing marks were not noticed in most cases because they would be correctly re-evaluated on demand, however some cases like module output values can rely on the change source alone, resulting in the marks being lost during apply.Many call sites which were aware that the marks were missing would manually apply the marks after decoding. After removing the duplicate marking and verifying tests,
Decodecall sites were again statically located and checked for mark handling. Any locations found that were relying on the absence of marks now must manually strip the marks before processing the values.Fixes #28671