🐛 Keep an empty indentless sequence entry before a sibling key#96
Conversation
`9:\n-\nq:` decoded to {9: [], q: null}, silently dropping the list element; it
should be {9: [null], q: null} (the indented form `9:\n -\nq:` already is). The
bug is reachable from plain hand-written YAML; the emit-options fuzzer surfaced
it via indentless output.
An indentless block sequence shares its parent mapping's block level, so an
empty `-` entry whose sibling mapping key sits at the sequence's own column
dedents straight to a `Key` event with no `BlockEnd`. decode_block_sequence's
empty-entry detection did not list `Key`, so the null entry was not recognized,
decode_node returned None on the `Key`, and the entry was lost. Add `Key` to the
set. A non-empty `- key: val` entry opens with `MappingStart`, never a bare
`Key`, so this cannot swallow real content.
frenck
added
the
bugfix
|
Warning Review limit reached
More reviews will be available in 9 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a fast-path decoding bug in YAMLRocks where an empty indentless block sequence entry (-) was silently dropped when followed by a sibling mapping key at the same column. Because an indentless block sequence shares its parent mapping's block level, dedenting to the sibling key emits a Key event with no BlockEnd; the empty-entry detection in decode_block_sequence did not list Key, so decode_node returned None and the null element was lost (e.g. 9:\n-\nq: decoded to {9: [], "q": None} instead of {9: [None], "q": None}). The fix adds EventKind::Key to the empty-entry match set.
Changes:
- Add
EventKind::Keyto the empty-entry detection indecode_block_sequence, with an expanded explanatory comment. - Add a regression test asserting
loads(b"9:\n-\nq:\n") == {9: [None], "q": None}.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/decode/mod.rs |
Adds Key to the indentless empty-entry detection so a null sequence entry before a sibling key is preserved on the fast path. |
tests/core/test_loads.py |
Adds a fast-path regression test pinning the empty indentless entry to [None]. |
Note: The fast-path change is correct and well-reasoned. However, the parallel round-trip composer (compose_block_entry) carries the identical detection without Key, so the same input drops the null node in the round-trip AST and breaks byte-for-byte re-emit (OPT_ROUND_TRIP). That path is not fixed here and is not covered by the new test, which only exercises loads. See the inline comment for details.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR review flagged that the fast-decoder fix had a twin in the round-trip path: compose_block_entry used the same empty-entry set without `Key`, so for `9:\n-\nq:` the composer dedented to a bare `Key`, compose_node returned None, and the null entry was dropped from the AST. The unmodified document still re-emitted byte-for-byte (the emitter replays source spans), but the AST no longer matched the data, so navigating or editing the sequence lost the entry. Add `Key` to compose_block_entry's empty-entry match, mirroring decode_block_sequence, so both paths agree. Pinned by a composer unit test (AST has one null, re-emits byte-for-byte) and an OPT_ROUND_TRIP assertion in the Python test.
Breaking change
None, other than that affected inputs now decode correctly instead of silently dropping a list element.
Proposed change
decoded to
{9: [], "q": None}, silently dropping the list element. It should be{9: [None], "q": None}: the indented form9:\n -\nq:already decodes that way, and9:\n-\n(no sibling) already keeps the[None]. The bug is reachable from plain hand-written YAML; it surfaced through a new emit-options differential fuzzer (separate PR) that emits sequences in the indentless style.An indentless block sequence shares its parent mapping's block level, so an empty
-entry whose sibling mapping key sits at the sequence's own column dedents straight to aKeyevent with noBlockEnd(the sequence has no level of its own to close).decode_block_sequence's empty-entry detection listedSequenceEntry | SequenceEnd | MappingEnd | BlockEndbut notKey, so the null entry was not recognized,decode_nodereturnedNoneon theKey, and the entry was lost.Add
Keyto the set. A non-empty- key: valentry opens withMappingStart, never a bareKey, so this cannot swallow real content. Verified by the full suite (including the YAML test suite), a Python regression test, and a 1.05M-run differential fuzz pass with no divergence.Type of change
Additional information
Checklist
uv run pytestpasses locally. A pull request cannot be merged unless CI is green.uv run ruff check .anduv run ruff format --check .pass.cargo fmt --checkandcargo clippy --all-targets -- -D warningspass.If the change is user-facing:
docs/is added or updated, anddocs/verify_examples.pystill passes.