fix(format): keep every token of a line in the canonical output

Assisted-by: GLM 5.3

format/format.go

@@ -35,7 +35,12 @@ func Source(src string) string {
 		inf := info{kind: kBlank, funcID: funcID}
 		if len(line) > 0 {
 			switch {
-			case line[0].Kind == token.Comment:
+			// A whole-line comment is layout of its own.  A block comment
+			// ahead of code on the same line ("/* head */ MOVQ AX, BX") is
+			// legal assembly and must not swallow the statement after it, so
+			// only a lone comment is classified as one; anything else renders
+			// as an instruction line that carries the comment inline.
+			case line[0].Kind == token.Comment && len(line) == 1:
 				inf.kind = kComment
 			case line[0].Kind == token.Hash:
 				inf.kind = kPreproc
@@ -87,6 +92,10 @@ func Source(src string) string {
 	for i, line := range lines {
 		inf := infos[i]
 		var out string
+		var last token.Token
+		if len(line) > 0 {
+			last = line[len(line)-1]
+		}
 		switch inf.kind {
 		case kBlank:
 			out = ""
@@ -99,7 +108,10 @@ func Source(src string) string {
 		case kPreproc:
 			out = renderPreproc(line)
 		case kDirective:
-			out = line[0].Text + " " + renderOps(line[1:])
+			out = line[0].Text
+			if ops := renderOps(line[1:]); ops != "" {
+				out += " " + ops
+			}
 			inBody = line[0].Text == "TEXT"
 		case kLabel:
 			// Every label, and a trailing instruction, becomes its own
@@ -121,9 +133,9 @@ func Source(src string) string {
 				// A bare directive cannot start a line of its own (the
 				// parser wants a symbol per line), so a directive sharing
 				// the label's line stays there.
-				outs[len(outs)-1].text += " " + strings.TrimRight(renderOps(rest), " \t")
+				outs[len(outs)-1].text += " " + trimLineRight(renderOps(rest), rest[len(rest)-1])
 			} else if len(rest) > 0 && rest[0].Kind == token.Ident {
-				outs = append(outs, outLine{kind: kInstr, text: strings.TrimRight(renderInstr(rest, maxWidth[inf.funcID]), " \t")})
+				outs = append(outs, outLine{kind: kInstr, text: trimLineRight(renderInstr(rest, maxWidth[inf.funcID]), rest[len(rest)-1])})
 				if strings.EqualFold(rest[0].Text, "RET") {
 					inBody = false
 				}
@@ -140,11 +152,31 @@ func Source(src string) string {
 				inBody = false
 			}
 		}
-		outs = append(outs, outLine{kind: inf.kind, text: strings.TrimRight(out, " \t")})
+		if len(line) == 0 {
+			outs = append(outs, outLine{kind: inf.kind, text: ""})
+			continue
+		}
+		outs = append(outs, outLine{kind: inf.kind, text: trimLineRight(out, last)})
 	}
 	return normalizeSpacing(outs)
 }
 
+// trimLineRight removes trailing spaces and tabs from a rendered line, which
+// are layout the canonical form drops.  The trim never eats into the text of
+// the line's final token: a string or rune literal may legitimately end in
+// whitespace, and that whitespace is the token's content, not spacing between
+// tokens.  A comment is the exception, because the lexer itself trims a
+// comment token's trailing whitespace, so removing it cannot change what the
+// next pass reads.  s must end with the final token's text.
+func trimLineRight(s string, last token.Token) string {
+	start := len(s) - len(last.Text)
+	t := strings.TrimRight(s, " \t")
+	if last.Kind == token.Comment || len(t) <= start {
+		return t
+	}
+	return s[:start] + last.Text
+}
+
 // Line classification, shared by the formatting passes.
 const (
 	kBlank = iota
@@ -259,7 +291,14 @@ func renderPreproc(line []token.Token) string {
 	// "#" directive [args]
 	if len(line) >= 3 && line[1].Kind == token.Ident && line[1].Text == "include" &&
 		line[2].Kind == token.String {
-		return "#include " + line[2].Text
+		// Whatever follows the header name is stray, but it is the file's
+		// stray text: it renders after the name rather than vanishing, so
+		// re-lexing the output sees exactly the tokens the input carried.
+		out := "#include " + line[2].Text
+		if rest := renderOps(line[3:]); rest != "" {
+			out += " " + rest
+		}
+		return out
 	}
 	// The body of a directive, a macro definition included, is an ordinary
 	// token run: rendering it through renderOps applies the same punctuation

format/fuzz_test.go

@@ -4,16 +4,24 @@
 package format
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
+	"reflect"
+	"strings"
 	"testing"
 
+	"sourcedock.dev/petrbalvin/gasm-sdk/ast"
+	"sourcedock.dev/petrbalvin/gasm-sdk/lexer"
 	"sourcedock.dev/petrbalvin/gasm-sdk/parser"
+	"sourcedock.dev/petrbalvin/gasm-sdk/token"
 )
 
 // FuzzFormatIdempotency hammers the formatter with arbitrary input.  The
-// contract: formatting twice equals formatting once, and input that parses
-// cleanly still parses cleanly after formatting.  The seed corpus carries the
+// contract: formatting twice equals formatting once, the output re-lexes to
+// the same tokens as the input (so formatting changes layout, never meaning),
+// input that parses cleanly still parses cleanly after formatting, and its
+// tree, positions aside, is unchanged.  The seed corpus carries the
 // repository's kernels, so a plain `go test` run replays every seed as a
 // regression case and CI exercises them without any fuzzing budget.
 func FuzzFormatIdempotency(f *testing.F) {
@@ -37,6 +45,16 @@ func FuzzFormatIdempotency(f *testing.F) {
 	f.Add("//\r ")
 	f.Add("// loop \r\t\nMOVQ AX, BX\n")
 	f.Add("TEXT ·f(SB), NOSPLIT, $0 // tail\r\n\tMOVQ AX, BX\r\n\tRET\r\n")
+	// A block comment ahead of code on one line: the comment must not swallow
+	// the statement that follows it.
+	f.Add("/* head */ MOVQ AX, BX\n")
+	f.Add("/* head */ DATA d<>+0(SB)/8, $1\n")
+	// The exotic operand shapes the corpus taught the parser: register ranges,
+	// PC-relative jumps with a negative displacement and the U+2215 package
+	// path.
+	f.Add("TEXT ·f(SB), $0\n\tV4FMADDPS [Z0-Z3], Z2, Z1\n\tRET\n")
+	f.Add("TEXT ·f(SB), $0\n\tJMP -3(PC)\n\tRET\n")
+	f.Add("TEXT ·f(SB), $0\n\tCALL internal∕runtime∕atomic·Xchg(SB)\n\tRET\n")
 
 	f.Fuzz(func(t *testing.T, src string) {
 		once := Source(src)
@@ -44,10 +62,188 @@ func FuzzFormatIdempotency(f *testing.F) {
 		if once != twice {
 			t.Fatalf("formatting is not idempotent:\nfirst:  %q\nsecond: %q", once, twice)
 		}
-		if _, errs := parser.Parse("in.s", src); len(errs) == 0 {
-			if _, errs := parser.Parse("out.s", once); len(errs) > 0 {
+		in, out := tokenView(src), tokenView(once)
+		if !sameView(in, out) {
+			t.Fatalf("formatting changed the token stream:\ninput:  %v\noutput: %v\n%s", viewString(in), viewString(out), once)
+		}
+		file, errs := parser.Parse("in.s", src)
+		if len(errs) > 0 || hasStrayIllegal(src) {
+			// A file the parser already reports on, or one whose stray
+			// characters the formatter documents dropping, has no tree to
+			// preserve; the token view above already pinned its tokens.
+			return
+		}
+		reFile, errs := parser.Parse("out.s", once)
+		if len(errs) > 0 {
 			t.Fatalf("formatted output of clean input does not parse: %v\n%s", errs[0], once)
 		}
+		if got := scrub(reFile); !reflect.DeepEqual(got, scrub(file)) {
+			t.Fatalf("formatting changed the tree:\ninput:  %s\noutput: %s\n%s", dump(scrub(file)), dump(got), once)
 		}
 	})
 }
+
+// hasStrayIllegal reports whether src lexes to an Illegal token other than
+// the operand brackets, which the formatter drops by contract.
+func hasStrayIllegal(src string) bool {
+	for _, tok := range lexer.Tokenize(src) {
+		if tok.Kind == token.Illegal && tok.Text != "[" && tok.Text != "]" {
+			return true
+		}
+	}
+	return false
+}
+
+// tokenView is the meaning of a source file as the assembler reads it: the
+// token kinds and texts in order, ignoring line structure.  Stray Illegal
+// runes are dropped because the formatter documents dropping them; comment
+// texts are compared trailing-whitespace-trimmed because line comments lose
+// exactly that on the way through the lexer, and an unterminated block
+// comment runs to the end of the file, so the formatter's mandatory final
+// newline (and any line ending it lands after) is layout, not content the
+// formatter destroyed.
+type viewToken struct {
+	kind token.Kind
+	text string
+}
+
+func tokenView(src string) []viewToken {
+	var out []viewToken
+	for _, tok := range lexer.Tokenize(src) {
+		switch tok.Kind {
+		case token.EOF, token.Newline:
+			continue
+		case token.Illegal:
+			if tok.Text != "[" && tok.Text != "]" {
+				continue
+			}
+		case token.Comment:
+			text := strings.TrimRight(tok.Text, " \t\r")
+			if strings.HasPrefix(text, "/*") && !strings.HasSuffix(text, "*/") {
+				text = strings.TrimRight(text, " \t\r\n")
+			}
+			out = append(out, viewToken{tok.Kind, text})
+			continue
+		}
+		out = append(out, viewToken{tok.Kind, tok.Text})
+	}
+	return out
+}
+
+func sameView(a, b []viewToken) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if a[i] != b[i] {
+			return false
+		}
+	}
+	return true
+}
+
+func viewString(v []viewToken) string {
+	var b strings.Builder
+	for i, t := range v {
+		if i > 0 {
+			b.WriteByte(' ')
+		}
+		b.WriteString(t.kind.String() + "(" + t.text + ")")
+	}
+	return b.String()
+}
+
+// scrub reduces a parsed file to what formatting must preserve: every field
+// except source positions, the file path, and a preprocessor line's Raw text.
+// Positions move with layout, the path is an input of the call, and Raw is a
+// single-space join of the directive's tokens, whose input spelling the
+// canonical operand spacing may legitimately re-glue ("#define A (x)" formats
+// to "#define A(x)").  The token view already pins those tokens.
+func scrub(f *ast.File) *ast.File {
+	v := reflect.ValueOf(f).Elem()
+	scrubValue(v)
+	v.FieldByName("Path").SetString("")
+	return f
+}
+
+// scrubValue walks v and zeroes every token.Position it reaches, so two trees
+// taken from the same text at different layouts compare equal.
+func scrubValue(v reflect.Value) {
+	switch v.Kind() {
+	case reflect.Pointer, reflect.Interface:
+		if !v.IsNil() {
+			scrubValue(v.Elem())
+		}
+	case reflect.Struct:
+		switch v.Type() {
+		case reflect.TypeFor[token.Position]():
+			v.Set(reflect.Zero(v.Type()))
+			return
+		case reflect.TypeFor[ast.Preproc]():
+			v.FieldByName("Raw").SetString("")
+		}
+		for _, field := range v.Fields() {
+			scrubValue(field)
+		}
+	case reflect.Slice, reflect.Array:
+		for i := 0; i < v.Len(); i++ {
+			scrubValue(v.Index(i))
+		}
+	case reflect.Map:
+		for _, k := range v.MapKeys() {
+			scrubValue(v.MapIndex(k))
+		}
+	}
+}
+
+// dump renders a scrubbed tree as text, following pointers, because the fmt
+// verbs stop at the first address inside a slice of interfaces.
+func dump(v any) string {
+	var b strings.Builder
+	dumpValue(reflect.ValueOf(v), &b)
+	return b.String()
+}
+
+func dumpValue(v reflect.Value, b *strings.Builder) {
+	switch v.Kind() {
+	case reflect.Pointer, reflect.Interface:
+		if v.IsNil() {
+			b.WriteString("nil")
+			return
+		}
+		dumpValue(v.Elem(), b)
+	case reflect.Struct:
+		b.WriteString(v.Type().Name() + "{")
+		for i := 0; i < v.NumField(); i++ {
+			if i > 0 {
+				b.WriteString(", ")
+			}
+			b.WriteString(v.Type().Field(i).Name + ":")
+			dumpValue(v.Field(i), b)
+		}
+		b.WriteString("}")
+	case reflect.Slice:
+		if v.IsNil() {
+			b.WriteString("nil")
+			return
+		}
+		b.WriteString("[")
+		for i := 0; i < v.Len(); i++ {
+			if i > 0 {
+				b.WriteString(", ")
+			}
+			dumpValue(v.Index(i), b)
+		}
+		b.WriteString("]")
+	case reflect.Map:
+		b.WriteString("map{")
+		for _, k := range v.MapKeys() {
+			dumpValue(k, b)
+			b.WriteString(":")
+			dumpValue(v.MapIndex(k), b)
+		}
+		b.WriteString("}")
+	default:
+		b.WriteString(fmt.Sprintf("%v", v))
+	}
+}

format/testdata/fuzz/FuzzFormatIdempotency/182930100b51847b

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("#include\"00000000\"0000")

format/testdata/fuzz/FuzzFormatIdempotency/51d239769677847e

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("/*\r")