Fix trivia slot shift that drops comments in literals

[garrettr]

Fixes a case where buf format silently deletes a comment, and a related case where it silently moves or deletes blank lines. Reported downstream as bufbuild/buf#4620.

The bug

The trivia walker records one detached-trivia slot per element in a literal scope, and the printers look those slots up by element index. An element whose value is a brace ends at the brace rather than at its own comma, so that comma opens a slot of its own and shifts every later slot by one.

Dumping the trivia index for the repro below, a two-element array produces four slots, with the comment in slot[2] — the index the printer treats as "final slot, emit before ]". On the next pass it lands in slot[3], which no printer path reads, so it is dropped. Each brace-valued element adds one slot, so a longer list delays the loss by a pass.

syntax = "proto3";

package example.v1;

import "google/protobuf/descriptor.proto";

message Item {
  optional string name = 1;
}

message ItemList {
  repeated Item items = 1;
}

extend google.protobuf.MessageOptions {
  optional ItemList item_list = 50001;
}

message Example {
  option (example.v1.item_list) = {
    items: [
      {name: "first"},
      /* detached comment */

      {name: "second"}
    ]
  };
}

Pass 1 moves the comment past the element it annotated, down to the closing bracket and dedented to the bracket's level. Pass 2 deletes it. Pass 3 onward is stable, so the file settles with the comment permanently gone. buf format -d on the unmodified input shows the first step:

@@
     items: [
       {name: "first"},
-      /* detached comment */
-
       {name: "second"}
+    /* detached comment */
+
     ]

Same defect, no comments involved

Blank lines are tracked by the same shifted index. With the blank line before {name: "b"}:

    items: [
      {name: "a"},

      {name: "b"},
      {name: "c"},
      {name: "d"}
    ]

the blank line comes back before {name: "c"}. Move it one element further down in the input and it is deleted outright, because it shifts past the last element. This needs no repeated passes and no comments — a single buf format reflows the file.

The fix

End the element at its comma when it has one. Separators are optional between message literal fields, so a brace with no following comma must still end the element.

Because the element now runs through to its comma, trivia that previously landed on the closing brace can land on the comma instead. A dict elides its commas when printing and emits only their trailing trivia, so an inline comment between the brace and the comma would be dropped; it is hoisted onto the brace, which is where it was attached before. Bracketed lists print their commas and keep emitting that trivia themselves, so the two literal flavors are now distinguished by scope mode.

I also replaced the local peek-and-rewind helper with the existing Cursor.Peek(). The hand-rolled rewind advanced with NextSkippable and unwound with PrevSkippable, which returns the close token for a fused token and parks the cursor there — Peek() copies the cursor by value and has no such state.

Testing

TestFormat already asserts two-pass idempotence, so the new fixture is enough to catch this class of shift; with the trivia.go change reverted it fails on both the legacy and default goldens. It covers array elements, compact options, and separator-less fields.

I also formatted every .proto in this repo and in bufbuild/buf (~1,500 files) with a stock v1.72.0 binary and a patched one. Three files differ, and in all three the patched output matches the source's blank-line structure where the current output shifts every blank line in a message literal down by one element. Comparing comment-marker counts before and after, the set of files that lose a marker is byte-identical between the two binaries, so this introduces no new loss.

Noted but not fixed here

• A semicolon is also a legal message-literal separator, so the same slot shift is still reachable with ; in place of ,. Pre-existing and unchanged by this PR.
• testdata/bufformat/proto2/option/v1/option_message_field.proto has a comment named Leading comment on ',' that the current golden drops: an elided comma's leading trivia is never emitted. An earlier revision of this patch repaired that incidentally, but not idempotently, so I narrowed the change to only what it disturbed. Happy to file it separately.
• emitTriviaSlot returns silently when the slot index exceeds the slot count, which is why this desync lost comments instead of failing a test. An assertion at the printer's scope entry points, where the element count is known, would turn any future desync into a loud failure.

[emcfarlane]

Thanks for the fix and detailed response. Fix looks good to me, just some small comments.

[emcfarlane]

From lint, nextNonSkippableIs is no longer needed.

[garrettr]

Removed in d5aa3bb. It lost its only caller when the brace lookahead moved to Cursor.Peek, and I missed it because my local lint run predated that refactor. Sorry for the red CI.

[emcfarlane]

Suggested change

	// scopeModeLiteral is a bracketed list: `[...]`, whether a compact
	// option list or an array literal. Commas are printed.
	scopeModeLiteral
	// scopeModeList is a bracketed list: `[...]`, whether a compact
	// option list or an array literal. Commas are printed.
	scopeModeList

This is maybe a nicer name now we have the division.

[garrettr]

Agreed, applied in d5aa3bb — scopeModeLiteral read as the umbrella term for both halves of the division.

Worth noting it caught something: walkFused stated its bracket-to-mode table twice, once in the doc comment and again inline, and the two had drifted (the inline copy still described the dict case as "dict literal (literal mode)"). I dropped the inline copy rather than syncing it.

[emcfarlane]

Suggested change

	// isLiteral reports whether the scope holds a comma-separated element
	// list, in either of the two literal flavors.
	func (m scopeMode) isLiteral() bool {
	return m == scopeModeLiteral || m == scopeModeDict
	// isLiteral reports whether the scope holds a comma-separated element
	// list, in either of the two literal flavors.
	func (m scopeMode) isLiteral() bool {
	return m == scopeModeList || m == scopeModeDict

[garrettr]

Applied in d5aa3bb. I kept the isLiteral name, since "literal" is still the accurate umbrella for list-plus-dict and renaming it would widen the diff without changing behavior — happy to rename it too if you would rather it echoed the constants.