fix(pest_bridge): correctly attach type-choice and group-choice comments#617
Open
SebastienGllmt wants to merge 1 commit into
Open
fix(pest_bridge): correctly attach type-choice and group-choice comments#617SebastienGllmt wants to merge 1 commit into
SebastienGllmt wants to merge 1 commit into
Conversation
SebastienGllmt
force-pushed
the
fix-comments
branch
from
June 30, 2026 18:19
228a5f8 to
d6516fa
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
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.
Fix comment attachment for type-choice and group-choice comments
Summary
#613 added comment capture to the Pest-bridge AST. It handles rule-level and group-entry comments, but drops or misplaces comments on type-choice alternatives (
a / b) and group choices (//).This PR replaces the line-bucketing post-pass with the standard "trivia merge" model used by Roslyn, Prettier, and rustfmt, so that every comment is retrievable from the exact AST node it documents. The change is confined to
src/pest_bridge.rs— no AST, grammar, or public-API changes.1. The problem
The
; commentconvention documents the construct it follows (trailing) or precedes (leading). Today, several common shapes lose or misattach those comments.Union of named types — the single most common CDDL choice form:
Neither comment is retrievable from the alternative it documents: the type-choice comment fields are never populated, so the comments are dropped (or, when an alternative is a container, misattached to an entry inside it). A consumer (e.g.
cddl-deriveemitting per-variant///docs) cannot tell which alternative; a round shapebelongs to.Array/map entries (off-by-one) — leading-comma layout:
xreceivesvertical(the next entry's comment) andyreceives nothing.Group choices — leading comments before
//alternatives:; @name okand; @name errare dropped entirely.Root cause
The post-pass keys each comment to an AST node by a line number derived from that node's greedy parser span — and Pest spans for
type1/group_entryrun forward to the start of the next token, so the computed line overshoots onto the following node. On top of that, two node kinds (type-choice, group-choice) are never visited at all, and the line index is non-consuming, so one comment can be claimed by several nodes. These compound into four distinct failure modes (trailing off-by-one, type-choice never filled, group-choice never filled, duplication).2. Why a principled fix instead of an ad-hoc patch
We could keep the line-bucketing post-pass and patch around it with ad-hoc fixes in various places, but it leaves the "guess the owner from a line number derived from a greedy span" in place, which is fragile
Comment attachment is a solved problem. Every serious formatter/compiler treats comments as trivia and binds them with the same model. We looked at three:
attach.jsclassifies each comment exactly once against three neighbours (enclosing / preceding / following node) as leading, trailing, or dangling, in a single pass over a source-ordered list.The shared, battle-tested model: one source-ordered pass that binds each comment to exactly one node by position, consuming it once — never re-deriving ownership from greedy spans.
3. The approach
This PR implements that trivia merge for the CDDL AST:
COMMENTtokens, so;inside text/byte strings is never mistaken for a comment), each tagged leading (alone on its line) or trailing.This systematically fixes the 4 bugs that motivated this PR by construction rather than via ad-hoc patches.
A few choices worth calling out for review:
]/}) and leaf tokens (identifiers, literals) have tight spans even when their parenttype1/group_entryspans are greedy. Anchors key off those tight ends, so a trailing comment binds to the construct it actually follows. (The one subtlety: a fewType2variants — bare typename,~unwrap,&group— store a greedy span because their grammar alternative ends in an optionalgeneric_args?; we recover a tight end from the inner identifier token, the same way group entries already do.)4. Scope, edge cases, and open questions
Deliberately out of scope (current behaviour noted):
[]/{}, or after the last entry before the close bracket — don't bind to any AST node. They are collected as orphans and dropped. Retaining them would need a newcomments_inside-style field.#6.x/#(data-major-type / any) alternatives can carry the same greedy span as the typename family but have no inner token to recover a tight end from; a trailing comment directly on a bare#-form may misbind. These are rare as doc-hint targets.Round-trip caveat:
impl Displayalready normalizes layout (e.g. it reflows/to new lines for >2 choices and keeps 2-choice unions inline). So this PR guarantees comments are retrievable and re-emittable, not byte-exact. Concretely, a trailing comment on a 2-choice union emitted inline (a ; doc / b) would have the;comment swallow the rest of the line on re-parse; one alternative per line round-trips cleanly. Tests assert round-trip on the forms that surviveDisplay, not byte equality.Questions for the maintainer:
pest_bridge::teststhe right home for these tests?Testing
The original single acceptance test is split into per-construct tests. All run under the default
cargo test --libfeature set (ast-comments + ast-span).