From 6bb287deebb6c1602a8c7577ad8c0ec950397dd4 Mon Sep 17 00:00:00 2001 From: karitham Date: Fri, 21 Aug 2026 22:56:28 +0200 Subject: [PATCH] formatter: fix enum '=' pad trailing into separator computeEnumAlign sized the name column from valued members, but enumValueContent padded every member; on valueless members the pad trailed straight into the emitted separator (STAR_COMPASS ,). Guard the pad on v.Value != nil, mirroring the struct-side fieldPads guard, and drop the now-unreachable nameOnlyComment workaround. Add a separator-hygiene sweep (13 fixtures x 4 separator modes x 3 aligns x 3 widths x 2 passes) asserting no structural space precedes an emitted , or ;, with placeholder-substituting literal stripping to avoid fusing comment seams. Mutation-checked: reverting the guard fails the sweep. --- formatter/comments.go | 19 --- formatter/field.go | 17 +-- formatter/sepspace_test.go | 250 +++++++++++++++++++++++++++++++++++++ 3 files changed, 256 insertions(+), 30 deletions(-) create mode 100644 formatter/sepspace_test.go diff --git a/formatter/comments.go b/formatter/comments.go index b3521f4..0221ffa 100644 --- a/formatter/comments.go +++ b/formatter/comments.go @@ -232,22 +232,3 @@ func (f *formatter) commentBreaksGroup(prevEnd, curStart int, sep syntax.TokenKi return false } - -// nameOnlyComment reports whether a comment follows the name of a -// name-only value (no content tokens) on the same source line, directly -// or after the separator: with the separator text dropped it renders -// against the name pad. -func (f *formatter) nameOnlyComment(name int) bool { - for c := name + 1; c < len(f.toks); c++ { - ct := f.token(c) - if isComment(ct.Kind) { - return ct.Line == f.token(name).Line - } - - if ct.Line != f.token(name).Line { - return false - } - } - - return false -} diff --git a/formatter/field.go b/formatter/field.go index bb07229..e8dc532 100644 --- a/formatter/field.go +++ b/formatter/field.go @@ -476,10 +476,12 @@ func (f *formatter) enumValueContent(v *syntax.EnumValue, align *columnAlign, pa o.skipText = []int{v.TokEnd()} } - if padded && align.enumAssign && align.nameWidth > 0 { - // A comment after the name (or anywhere before the value's end) - // makes the pad ambiguous; the value's own same-line comments - // render after the pads and are fine. + if padded && align.enumAssign && align.nameWidth > 0 && v.Value != nil { + // Only members with a value get the '=' alignment pad: a pad on a + // name-only member would trail straight into the separator. A + // comment between the name and the value's end makes the pad + // ambiguous; the value's own same-line comments render after the + // pads and are fine. contentEnd := v.TokEnd() if v.Sep != 0 { contentEnd = f.prevReal(v.TokEnd() - 1) @@ -495,13 +497,6 @@ func (f *formatter) enumValueContent(v *syntax.EnumValue, align *columnAlign, pa } } - // A name-only value renders a same-line comment (directly or - // after its separator) against the pad when no separator text - // separates them. - if clean && contentEnd == v.TokStart() && !sepEmits(v.Sep, sepMode) && f.nameOnlyComment(v.TokStart()) { - clean = false - } - if clean { o.pads = []padEntry{{v.Name.TokStart(), padRight("", align.nameWidth-len(v.Name.Text))}} } diff --git a/formatter/sepspace_test.go b/formatter/sepspace_test.go new file mode 100644 index 0000000..2ba124c --- /dev/null +++ b/formatter/sepspace_test.go @@ -0,0 +1,250 @@ +package formatter + +import ( + "strings" + "testing" +) + +// sweepSeparatorSpace asserts the invariant behind the "space before +// separator" bug report: a trailing field/enum separator must hug its +// preceding token. A space between content and an emitted ; or , is only +// legal inside string literals and comments, so the check strips those and +// fails on anything left over. +// +// The sweep covers the full option matrix: every construct's separator +// mode (comma/semicolon/none/preserve) crossed with align modes, forced +// breaks, print widths, and two format passes (some artifacts only appear +// when formatting already-formatted output). + +type sepCase struct { + name string + src string +} + +func sepSweepSources() []sepCase { + return []sepCase{ + {"struct basic", `struct S { + 1: required i32 id; + 2: optional string name, + 3: map m = {"a": 1}; + 4: bool flag +}`}, {"struct ragged widths", `struct R { + 1: i32 a, + 2: map> longer_field_name, + 3: i8 x = 5; + 4: set s +}`}, + {"struct no separators at all", `struct N { + 1: i32 a + 2: i32 b + 3: i32 c +}`}, + {"struct trailing last", `struct T { + 1: i32 a; + 2: i32 b; +}`}, + {"comments around separators", `struct C { + 1: i32 id /* c */ ; + // own line + 2: string name; // after sep + 3: i64 n + ; +}`}, + {"annotations then separator", `struct AN { + 1: i32 id (js.type = "i32"); + 2: string s (opt = "x") +}`}, + {"values with punctuation", `struct V { + 1: map> m = {"k": [1, 2]}; + 2: i64 big = SOME_CONST; +}`}, + {"single line struct", `struct Tiny { 1: i32 id; 2: string name }`}, + {"union and exception", `union U { + 1: string a; + 2: i32 b +} + +exception E { + 1: string msg; +}`}, + {"enum mixed separators", `enum M { + A = 1; + B = 2, + C +}`}, + {"function args and throws", `service S { + R get(1: i32 id; 2: string name) throws (1: E err); + void put(1: R r) +}`}, + {"nested containers", `struct D { + 1: map>>> deep; +}`}, + {"pre-spaced separators struct", `struct P { + 1: i32 id ; + 2: string name , + 3: bool flag ; + 4: i64 n ; +}`}, + {"pre-spaced separators enum", `enum PE { + A = 1 ; + B = 2 ; + C +}`}, + {"pre-spaced separators service", `service PS { + R get(1: i32 id ; 2: string name ) throws (1: E err ; 2: i32 code ); + void put(1: R r ) +}`}, + {"pre-spaced collections", `struct PC { + 1: list l = [1 , 2 , 3]; + 2: map m = {"a" : 1 , "b": 2 }; +}`}, + {"enum long names mixed valued", `enum RelicKind { + STAR_COMPASS, + BLAZE_REAP = 2, + UNHEARD_BELL, + GAVEL_OF_THE_ABYSS = 4, + CROSSED_STILLS +}`}, + } +} + +// stripLiteralsAndComments replaces string literal and comment contents +// with placeholders so the separator scan sees only structural text while +// token adjacency (and therefore bogus "space before ;" seams) survives. +func stripLiteralsAndComments(s string) string { + var b strings.Builder + + for i := 0; i < len(s); { + c := s[i] + switch { + case c == '"': + b.WriteString(`"S"`) + + j := i + 1 + for j < len(s) && s[j] != '"' { + if s[j] == '\\' { + j++ + } + + j++ + } + + i = j + 1 + case strings.HasPrefix(s[i:], "//"): + b.WriteString("#C") + + for j := i; j < len(s); j++ { + if s[j] == '\n' { + break + } + + i = j + } + + i++ + case strings.HasPrefix(s[i:], "/*"): + end := strings.Index(s[i+2:], "*/") + if end < 0 { + b.WriteString("#C") + i = len(s) + + break + } + + b.WriteString("#C#C") + i += end + 4 + default: + b.WriteByte(c) + i++ + } + } + + return b.String() +} + +// assertNoGapBeforeSep fails when a non-string/comment region contains +// whitespace immediately before an emitted , or ;. +func assertNoGapBeforeSep(t *testing.T, out string, label string) { + t.Helper() + + stripped := stripLiteralsAndComments(out) + + lines := strings.Split(stripped, "\n") + for li, line := range lines { + idx := 0 + for idx < len(line) { + p := strings.IndexAny(line[idx:], ",;") + if p < 0 { + break + } + + at := idx + p + + if at > 0 { + prev := line[at-1] + if prev == ' ' || prev == '\t' { + t.Errorf("%s: whitespace before %q at line %d:\n%s", label, rune(line[at]), li+1, out) + + return + } + } + + idx = at + 1 + } + } +} + +func TestNoSpaceBeforeSeparator(t *testing.T) { + modes := []struct { + name string + mode SeparatorMode + }{ + {"comma", SeparatorComma}, + {"semicolon", SeparatorSemicolon}, + {"none", SeparatorNone}, + {"preserve", SeparatorPreserve}, + } + + aligns := []struct { + name string + align AlignMode + }{ + {"field", AlignField}, + {"assign", AlignAssign}, + {"disable", AlignDisable}, + } + + widths := []int{20, 40, 80} + + for _, src := range sepSweepSources() { + for _, m := range modes { + for _, a := range aligns { + for _, w := range widths { + o := DefaultOptions() + o.PrintWidth = w + o.Separator.Set(ConstructStruct, m.mode) + o.Separator.Set(ConstructEnum, m.mode) + o.Separator.Set(ConstructArguments, m.mode) + o.Separator.Set(ConstructThrows, m.mode) + o.Align = a.align + + label := src.name + "/" + m.name + "/" + a.name + + got, err := Format(parseDoc(t, src.src), o) + if err != nil { + t.Fatalf("%s: Format: %v", label, err) + } + + assertNoGapBeforeSep(t, got, label+" pass1") + + again, err := Format(parseDoc(t, got), o) + if err != nil { + t.Fatalf("%s pass2: parse: %v\noutput was:\n%s", label, err, got) + } + + assertNoGapBeforeSep(t, again, label+" pass2") + } + } + } + } +} -- 2.51.2