YAML: fix MergeYaml + skip CoalesceProperties when block scalars present#8157
Draft
steve-aom-elliott wants to merge 1 commit into
Draft
Conversation
MergeYaml.mergeScalar: when merging into an existing FOLDED/LITERAL block scalar, route the new body through BlockScalarUtils so the block envelope (chomp indicator, header newline, body indent, trailing whitespace) is preserved rather than clobbered. Previously the new value was assigned via Yaml.Scalar.withValue and corrupted siblings. BlockScalarUtils.withBody: drop the trailing-newline fallback. The previous fallback added a '\n' when the existing scalar's value had no trailing whitespace, which was wrong for scalars produced by autoFormat (in which the boundary newline lives in the next entry's prefix). Preserving the existing trailing whitespace exactly fixes a latent extra-blank-line bug visible through MergeYaml's addLiteralStyleBlockWhichDoesAlreadyExist path. CoalescePropertiesVisitor: skip the dot-joined key coalesce when the sub-mapping (transitively) contains a block scalar. The downstream ShiftFormatLeftVisitor cannot dedent block-scalar body indent (stored inside Yaml.Scalar.value) nor entries whose prefix has no '\n' because the boundary newline lives in a preceding block scalar's value. Conservatively leaving those mappings un-coalesced keeps the file structurally intact.
steve-aom-elliott
force-pushed
the
fix-yaml-merge-coalesce-block-scalars
branch
from
3890d10 to
464c76c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #8154 — review/merge that first.
Summary
Follow-up that addresses the two recipes left out of #8154:
MergeYaml:mergeScalarnow routes throughBlockScalarUtilsto preserve the block envelope when merging values into a FOLDED/LITERAL scalar.CoalescePropertiesVisitor: the dot-joined key coalesce is skipped when the sub-mapping (transitively) contains a block scalar, because the downstreamShiftFormatLeftVisitorcannot correctly dedent block-scalar body indent or entries whose preceding sibling is a block scalar.Root causes and fixes
MergeYaml.mergeScalarBefore:
y1.withValue(y2.getValue())clobbered the existing FOLDED/LITERAL envelope, producing corrupt one-line output likekey: >replacedafter: tail.After:
BlockScalarUtils.withBody(y1, BlockScalarUtils.getBody(y2))preserves y1's chomp indicator, header newline, body indent, and trailing whitespace; the new body lines are re-indented to match.The behavior choice: when merging a PLAIN value into an existing block scalar, preserve the existing block style. The user's choice of
>-or|+reflects intent (chomp semantics, line semantics); honoring it is more faithful than silently switching to PLAIN. For FOLDED>-the resulting YAML is semantically equivalent to PLAIN anyway; for LITERAL the preserved style retains the meaningful newline structure.CoalescePropertiesVisitor— guard against block scalars in subtreeThe coalesce flattens a single-entry sub-mapping (
outer: { inner: { ... } }→outer.inner: { ... }) and dedents the contents viaShiftFormatLeftVisitor. That visitor has two limitations against block scalars:Yaml.Scalar.value; the visitor only shifts entry/sequence prefixes, never scalar values, so the body stays at its original column.\n(the boundary newline lives in the previous scalar'svalue); the visitor'sprefix.contains("\n")guard skips it, leaving it at the original column.Result: a structurally-corrupt YAML at a stable state.
The conservative fix is to detect a block scalar anywhere in the subtree and decline to coalesce. The recipe still operates on the rest of the document; only the specific block-scalar-containing chains are left as-is.
Test plan
MergeYamlTest.addLiteralStyleBlockWhichDoesAlreadyExist(existing) still passesMergeYamlTestcases for overwriting>-and|+block scalarsCoalescePropertiesTestcases for direct/nested block-scalar avoidancerewrite-yamlsuite passes