Skip to content

fix(pest_bridge): correctly attach type-choice and group-choice comments#617

Open
SebastienGllmt wants to merge 1 commit into
anweiss:mainfrom
dcSpark:fix-comments
Open

fix(pest_bridge): correctly attach type-choice and group-choice comments#617
SebastienGllmt wants to merge 1 commit into
anweiss:mainfrom
dcSpark:fix-comments

Conversation

@SebastienGllmt

Copy link
Copy Markdown
Contributor

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 ; comment convention 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:

shape = circle ; a round shape
      / square ; a four-sided shape

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-derive emitting per-variant /// docs) cannot tell which alternative ; a round shape belongs to.

Array/map entries (off-by-one) — leading-comma layout:

point = [ x ; @name horizontal
        , y ; @name vertical
        ]

x receives vertical (the next entry's comment) and y receives nothing.

Group choices — leading comments before // alternatives:

result = [ ; @name ok
           value: int //
           ; @name err
           code: int
         ]

; @name ok and ; @name err are 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_entry run 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:

  • Roslyn (C#) — every token carries leading and trailing trivia. The rule: trivia attaches to the following token, except trailing trivia up to and including the end of the line, which attaches to the preceding token. Binding is by source position, decided once.
  • Prettierattach.js classifies 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.
  • rustfmt — a cautionary tale. Its historical approach recovered comments from per-node span gaps, which produced a long tail of dropped / moved / non-idempotent-formatting bugs. The lesson: don't reconstruct ownership from spans node-by-node; classify everything up front in one ordered pass.

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:

  1. One flat, source-ordered list of comment tokens (from the grammar's real COMMENT tokens, so ; inside text/byte strings is never mistaken for a comment), each tagged leading (alone on its line) or trailing.
  2. One flat, source-ordered list of anchors — the AST comment slots (type-choice before/after, group-choice leading, entry leading/trailing, rule leading), each tagged with a tight byte extent.
  3. One linear pass classifies each comment exactly once:
    • trailing → the nearest preceding anchor whose tight end is on the same line;
    • leading → the nearest following anchor;
    • otherwise orphan (dropped, never a panic).

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:

  • Tight extents are the pivot. Container brackets (]/}) and leaf tokens (identifiers, literals) have tight spans even when their parent type1/group_entry spans are greedy. Anchors key off those tight ends, so a trailing comment binds to the construct it actually follows. (The one subtlety: a few Type2 variants — bare typename, ~unwrap, &group — store a greedy span because their grammar alternative ends in an optional generic_args?; we recover a tight end from the inner identifier token, the same way group entries already do.)
  • One traversal, two phases. Collecting anchors and writing comments back both drive a single traversal function, so the two passes cannot desync.
  • Optimized for attachment. The main design of this is to allow query for any AST node what comment is attached (primarily useful for attaching DSLs into CDDL comments)

4. Scope, edge cases, and open questions

Deliberately out of scope (current behaviour noted):

  • Dangling comments — alone inside an empty []/{}, 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 new comments_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 Display already 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 survive Display, not byte equality.

Questions for the maintainer:

  1. Is byte-exact round-trip a goal? If so it needs a lossless CST / a non-normalizing printer — a much larger change than this PR, which targets correct attachment into the existing typed AST.
  2. Should dangling/structural comments be retained (new field) or is dropping them acceptable?
  3. is pest_bridge::tests the right home for these tests?

Testing

The original single acceptance test is split into per-construct tests. All run under the default cargo test --lib feature set (ast-comments + ast-span).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant