diff --git a/AGENTS.md b/AGENTS.md index 5be4cd4..25e4fc5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -60,13 +60,22 @@ testdata/corpus/ — real-world .pgsql procedures used as the safety/idempoten 4. **Graceful degradation**: any span the parser cannot handle is passed through verbatim rather than corrupted. 5. **Runtime safety gate**: every frontend (CLI `fmt`, LSP `textDocument/formatting` and - `rangeFormatting`) calls `format.SemanticallyEqual(src, out)` before writing or returning - formatted output. It re-lexes both sides and compares non-trivia token streams - (case-insensitive for identifiers/keywords, exact otherwise, recursing into dollar-quoted - bodies). If it ever returns false, the formatter has a bug — the caller must refuse to - write/emit the result and keep the original source, never guess or best-effort it. This is - not just a test assertion (`pkg/format/format_test.go`); it is enforced at runtime so a - formatter bug can never silently drop or alter code. + `rangeFormatting`) calls `format.VerifySafe(src, out, style)` before writing or returning + formatted output. If it returns a non-nil error the formatter has a bug — the caller must + refuse to write/emit the result and keep the original source, never guess or best-effort + it. This is enforced at runtime, not just in `pkg/format/*_test.go`, so a formatter bug can + never silently drop or alter code. `VerifySafe` runs four checks: + - **Semantic equivalence** (`SemanticallyEqual`): re-lex both sides, compare the non-trivia + token streams — case-insensitive for identifiers/keywords, exact otherwise, recursing + into dollar-quoted bodies. Comments and whitespace are trivia and are ignored here. + - **Comment preservation** (`CommentsPreserved`): every `--` and `/* */` comment in `src` + reappears in `out`, in order, with the same content (line endings and indentation are + normalised away; dropping, merging, splitting, reordering, or rewording a comment is + not). Descends into dollar-quoted bodies. + - **Structural balance** (`StructurallyBalanced`): the `( ) [ ]` and BEGIN/CASE/IF/LOOP…END + nesting profile of `out` matches `src`, counting only real code tokens (comment and + string/dollar-quote contents are skipped). + - **Idempotence**: re-formatting `out` yields `out` unchanged. **Line endings**: the formatter re-emits all layout with `st.Newline` (default `\n`), so a CRLF input file is normalised to LF on write. This includes `\r\n` that sits *inside* a diff --git a/cmd/pgtidy/fmt.go b/cmd/pgtidy/fmt.go index 73533a6..36ea73a 100644 --- a/cmd/pgtidy/fmt.go +++ b/cmd/pgtidy/fmt.go @@ -63,8 +63,8 @@ func cmdFmt(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return 2 } out := format.File(parser.Parse(string(src)), st) - if !format.SemanticallyEqual(string(src), out) { - _, _ = fmt.Fprintln(stderr, "pgtidy: refusing to format stdin: formatter safety check failed (output would change code content)") + if err := format.VerifySafe(string(src), out, st); err != nil { + _, _ = fmt.Fprintf(stderr, "pgtidy: refusing to format stdin: formatter safety check failed: %v\n", err) return 2 } switch { @@ -90,8 +90,8 @@ func cmdFmt(args []string, stdin io.Reader, stdout, stderr io.Writer) int { continue } out := format.File(parser.Parse(string(src)), st) - if !format.SemanticallyEqual(string(src), out) { - _, _ = fmt.Fprintf(stderr, "pgtidy: refusing to format %s: formatter safety check failed (output would change code content)\n", path) + if err := format.VerifySafe(string(src), out, st); err != nil { + _, _ = fmt.Fprintf(stderr, "pgtidy: refusing to format %s: formatter safety check failed: %v\n", path, err) exit = 2 continue } diff --git a/docs/todo.md b/docs/todo.md index 2d27afd..1da3c82 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -93,6 +93,14 @@ Legend: ✅ done · 🚧 in progress · ⬜ not started - `fmt_test.go`: stdin, --check (un/formatted), -w idempotence, unknown-command. - Safety invariants #2 (semantic equivalence), #3 (idempotence), #4 (graceful degradation) are tested in `pkg/format` over the corpus. +- **Runtime safety gate** (`format.VerifySafe`, `pkg/format/safety.go`): every frontend runs + it before emitting. Bundles `SemanticallyEqual` (code token stream) + `CommentsPreserved` + (no comment dropped/merged/split/reworded, recursing into bodies) + `StructurallyBalanced` + (`()[]` / BEGIN·CASE·IF·LOOP…END profile, ignoring comment & string contents) + an + idempotence re-format. On failure the CLI prints the reason and keeps the original. + Caught two real bugs: multi-line `/* */` bodies were reindented as code, and col-0 `--` + lines were glued onto the previous line (merging consecutive comments) — both fixed in + `formatBodyStatements`. --- diff --git a/pkg/format/body.go b/pkg/format/body.go index bc83119..78e1c1b 100644 --- a/pkg/format/body.go +++ b/pkg/format/body.go @@ -398,6 +398,22 @@ func formatBodyStatements(text string, st config.Style) string { normalised := strings.ReplaceAll(text, "\r\n", "\n") rawLines := strings.Split(normalised, "\n") + // Mark the continuation lines of every multi-line /* … */ block comment. + // Those lines are comment content, not code: they must be carried verbatim + // with the comment's opening line, never split off and reindented as if + // they were statements of their own. + inBlockComment := make([]bool, len(rawLines)) + for _, t := range lexer.Lex(normalised) { + if t.Kind != lexer.BlockComment { + continue + } + n := strings.Count(t.Text, "\n") + start := t.Line - 1 // lexer Line is 1-based within normalised + for k := 1; k <= n && start+k < len(inBlockComment); k++ { + inBlockComment[start+k] = true + } + } + maxBlanks := st.PlpgsqlMaxBlankLines if maxBlanks < 0 { maxBlanks = 0 @@ -488,8 +504,21 @@ func formatBodyStatements(text string, st config.Style) string { stmt = nil } - for _, rawLine := range rawLines { + for j, rawLine := range rawLines { line := strings.TrimRight(rawLine, "\r") + + if inBlockComment[j] { + // Verbatim continuation of a multi-line block comment: glue it to the + // bline holding the comment's opening line. + if len(stmt) > 0 { + last := &stmt[len(stmt)-1] + last.text += "\n" + line + } else { + stmt = append(stmt, bline{text: line}) + } + continue + } + indent := leadingWhitespace(line) stripped := line[len(indent):] @@ -502,6 +531,12 @@ func formatBodyStatements(text string, st config.Style) string { fw := lowerASCII(firstBodyKeyword(stripped)) isColZero := indent == "" joinToPrev := isColZero && parenDepth == 0 && len(stmt) > 0 && !sqlClauseKw[fw] + // A col-0 comment-only line is its own thing: never glue it onto the + // previous line — doing so buries a code line's trailing text in a + // comment and collapses consecutive -- comment lines into one. + if joinToPrev && len(significantBodyTokens(stripped)) == 0 { + joinToPrev = false + } // Don't join a col-0 continuation to a comment-only preceding bline: // the comment has no structural keyword so `continue ;` at col-0 would // disappear into the comment text and be invisible to the lexer. diff --git a/pkg/format/format_test.go b/pkg/format/format_test.go index a1ebedb..5aa089c 100644 --- a/pkg/format/format_test.go +++ b/pkg/format/format_test.go @@ -184,18 +184,16 @@ func TestCorpusIdempotentAndSafe(t *testing.T) { } src := string(data) once := format(src) - twice := format(once) - if once != twice { - t.Errorf("%s: not idempotent", e.Name()) - } - if !semanticallyEqual(src, once) { - t.Errorf("%s: formatting changed semantics", e.Name()) + // VerifySafe bundles every runtime gate: semantic equivalence, comment + // preservation, structural balance, and idempotence. + if err := VerifySafe(src, once, config.Default()); err != nil { + t.Errorf("%s: %v", e.Name(), err) } } if seen == 0 { t.Skip("no corpus files") } - t.Logf("formatted %d corpus files (idempotent + semantically equal)", seen) + t.Logf("verified %d corpus files (semantic + comments + structure + idempotence)", seen) } // semanticallyEqual is a test-local alias for the exported safety check. diff --git a/pkg/format/safety.go b/pkg/format/safety.go index f598411..b27e00c 100644 --- a/pkg/format/safety.go +++ b/pkg/format/safety.go @@ -1,11 +1,52 @@ package format import ( + "fmt" "strings" + "git.warky.dev/wdevs/pgtidy/pkg/config" "git.warky.dev/wdevs/pgtidy/pkg/lexer" + "git.warky.dev/wdevs/pgtidy/pkg/parser" ) +// VerifySafe runs every safety invariant against a formatting result before it +// is written to disk or returned to an editor. src is the original input, out +// the formatted output, and st the style out was produced with. It returns nil +// when out is safe to emit, otherwise an error naming the invariant that failed. +// +// The checks, in order of cost: +// +// 1. Semantic equivalence — the non-trivia (code) token stream is unchanged: +// identifiers/keywords compare case-insensitively, everything else exactly, +// recursing into dollar-quoted bodies. Comments and whitespace are trivia +// and are deliberately ignored here. +// 2. Comment preservation — every -- and /* */ comment in src reappears in out, +// in the same order, with the same content (ignoring only trailing +// whitespace and CRLF/LF). The formatter may move or re-indent a comment but +// must never drop, merge, split, or reword one. +// 3. Structural balance — the ( ) [ ] and BEGIN/CASE/IF/LOOP…END nesting +// profile of out matches src's, counting only real code tokens (anything +// inside a comment or a string/dollar-quoted literal is ignored). +// 4. Idempotence — formatting out again yields out unchanged. +// +// Any failure means the formatter has a bug: the caller must keep the original +// source and never emit out. +func VerifySafe(src, out string, st config.Style) error { + if !SemanticallyEqual(src, out) { + return fmt.Errorf("code token stream changed") + } + if err := CommentsPreserved(src, out); err != nil { + return err + } + if err := StructurallyBalanced(src, out); err != nil { + return err + } + if reformatted := File(parser.Parse(out), st); reformatted != out { + return fmt.Errorf("output is not idempotent (a second format pass would change it)") + } + return nil +} + // SemanticallyEqual reports whether a and b have the same non-trivia token // stream, i.e. formatting may only ever change whitespace/comment trivia and // layout — it must never add, remove, or alter a token of actual code. @@ -14,9 +55,9 @@ import ( // must match exactly. Dollar-quoted body tokens are compared recursively so // that independent body reformatting doesn't trigger a false failure. // -// The CLI and LSP must call this before ever writing or emitting formatted -// output: if it returns false, the formatter has a bug and the original -// source must be kept, never the (corrupting) formatted output. +// The CLI and LSP must call this (via VerifySafe) before ever writing or +// emitting formatted output: if it returns false, the formatter has a bug and +// the original source must be kept, never the (corrupting) formatted output. func SemanticallyEqual(a, b string) bool { ta := significantTokens(a) tb := significantTokens(b) @@ -55,6 +96,151 @@ func SemanticallyEqual(a, b string) bool { return true } +// CommentsPreserved reports whether every comment in a survives into b with its +// text intact. Comments are compared in document order; each is reduced to its +// sequence of non-blank text lines (line endings normalised, every line trimmed +// of surrounding whitespace, blank lines dropped) so that the formatter is free +// to move or re-indent a comment but can never drop, merge, split, reorder, or +// reword one. Comments inside dollar-quoted bodies are included (the bodies are +// lexed recursively). A non-nil error describes the first divergence. +func CommentsPreserved(a, b string) error { + ca := comments(a) + cb := comments(b) + if len(ca) != len(cb) { + return fmt.Errorf("comment count changed: input has %d, output has %d", len(ca), len(cb)) + } + for i := range ca { + if ca[i] != cb[i] { + return fmt.Errorf("comment %d/%d changed:\n input: %q\n output: %q", i+1, len(ca), ca[i], cb[i]) + } + } + return nil +} + +// comments returns the normalised text of every -- and /* */ comment in src, in +// order, descending into dollar-quoted bodies. +func comments(src string) []string { + var out []string + for _, t := range lexer.Lex(src) { + switch t.Kind { + case lexer.LineComment, lexer.BlockComment: + out = append(out, normComment(t.Text)) + case lexer.DollarString: + if _, inner, _, ok := splitDollarQuote(t.Text); ok { + out = append(out, comments(inner)...) + } + } + } + return out +} + +// normComment canonicalises a comment token to its content — the ordered list of +// non-blank text lines, each stripped of surrounding whitespace, joined with LF. +// Line endings and indentation are layout, not content, so they are discarded; +// dropping or rewording an actual line of comment text still shows up. +func normComment(s string) string { + s = strings.ReplaceAll(s, "\r\n", "\n") + s = strings.ReplaceAll(s, "\r", "\n") + var lines []string + for _, ln := range strings.Split(s, "\n") { + if ln = strings.TrimSpace(ln); ln != "" { + lines = append(lines, ln) + } + } + return strings.Join(lines, "\n") +} + +// StructurallyBalanced reports whether a and b have the same delimiter and block +// nesting profile: identical counts of ( ) [ ] and of the PL/pgSQL block +// keywords BEGIN / CASE / IF / LOOP / END (and the compound END IF / END LOOP / +// END CASE), plus an identical running paren/bracket depth trace. Only real code +// tokens are counted — anything inside a -- or /* */ comment is trivia and is +// skipped, and string / dollar-quoted literals are opaque single tokens whose +// contents never register (dollar-quoted bodies are recursed into separately). +// +// Given SemanticallyEqual, this is defence in depth: an independent re-count +// with different code that catches a structural token slipping through a bug in +// the token-stream comparison (e.g. its dollar-quote or CRLF handling), and it +// pins down *where* the structure broke. +func StructurallyBalanced(a, b string) error { + pa := structureProfile(a) + pb := structureProfile(b) + if pa.parenDepthTrace != pb.parenDepthTrace { + return fmt.Errorf("parenthesis/bracket nesting changed") + } + for _, k := range structureKeys { + if pa.counts[k] != pb.counts[k] { + return fmt.Errorf("structural token %q count changed: input %d, output %d", k, pa.counts[k], pb.counts[k]) + } + } + return nil +} + +var structureKeys = []string{"(", ")", "[", "]", "begin", "case", "if", "loop", "end", "end if", "end loop", "end case"} + +type structProfile struct { + counts map[string]int + // parenDepthTrace is the sequence of running ( ) [ ] depths after each + // bracket token, joined with commas — a compact fingerprint of the nesting + // shape that diverges as soon as an open/close is added, dropped, or moved. + parenDepthTrace string +} + +func structureProfile(src string) structProfile { + p := structProfile{counts: map[string]int{}} + var trace strings.Builder + depth := 0 + toks := significantTokens(src) // trivia (comments/whitespace) already excluded + for i := 0; i < len(toks); i++ { + t := toks[i] + switch t.Kind { + case lexer.LParen: + p.counts["("]++ + depth++ + fmt.Fprintf(&trace, "%d,", depth) + case lexer.RParen: + p.counts[")"]++ + depth-- + fmt.Fprintf(&trace, "%d,", depth) + case lexer.LBracket: + p.counts["["]++ + depth++ + fmt.Fprintf(&trace, "%d,", depth) + case lexer.RBracket: + p.counts["]"]++ + depth-- + fmt.Fprintf(&trace, "%d,", depth) + case lexer.Ident: + switch lowerASCII(t.Text) { + case "begin", "case", "if", "loop": + p.counts[lowerASCII(t.Text)]++ + case "end": + p.counts["end"]++ + if i+1 < len(toks) && toks[i+1].Kind == lexer.Ident { + switch lowerASCII(toks[i+1].Text) { + case "if": + p.counts["end if"]++ + case "loop": + p.counts["end loop"]++ + case "case": + p.counts["end case"]++ + } + } + } + case lexer.DollarString: + if _, inner, _, ok := splitDollarQuote(t.Text); ok { + sub := structureProfile(inner) + for _, k := range structureKeys { + p.counts[k] += sub.counts[k] + } + trace.WriteString("[" + sub.parenDepthTrace + "]") + } + } + } + p.parenDepthTrace = trace.String() + return p +} + // normNL collapses CRLF to LF so string literals compare independent of the // source file's line-ending convention. func normNL(s string) string { return strings.ReplaceAll(s, "\r\n", "\n") } diff --git a/pkg/format/safety_test.go b/pkg/format/safety_test.go new file mode 100644 index 0000000..02480f2 --- /dev/null +++ b/pkg/format/safety_test.go @@ -0,0 +1,91 @@ +package format + +import ( + "strings" + "testing" + + "git.warky.dev/wdevs/pgtidy/pkg/config" +) + +func TestCommentsPreserved(t *testing.T) { + cases := []struct { + name string + a, b string + wantErr bool + }{ + {"identical", "select 1; -- note", "select 1;\n-- note", false}, + {"reindented block comment", "/* a\n b */ select 1", " /* a\nb */\nselect 1", false}, + {"crlf line comment", "-- note\r\nselect 1", "-- note\nselect 1", false}, + {"dropped comment", "select 1; -- keep me\nselect 2;", "select 1;\nselect 2;", true}, + {"merged comments", "-- one\n-- two\nselect 1", "-- one -- two\nselect 1", true}, + {"reworded comment", "-- alpha\nselect 1", "-- beta\nselect 1", true}, + {"comment inside body preserved", // -- inside a dollar-quoted body + "do $$ begin\n-- inner\nperform 1;\nend $$;", + "DO\n$$\nbegin\n -- inner\n perform 1;\nend\n$$;", false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + err := CommentsPreserved(c.a, c.b) + if (err != nil) != c.wantErr { + t.Fatalf("CommentsPreserved(%q, %q) err = %v, wantErr %v", c.a, c.b, err, c.wantErr) + } + }) + } +} + +func TestCommentsPreservedIgnoresCodeText(t *testing.T) { + // A ( ; keyword etc. inside a comment must not be read as code by the check. + a := "select 1; -- ( begin case end ) ;\nselect 2;" + b := "select 1;\nselect 2;\n-- ( begin case end ) ;" + if err := CommentsPreserved(a, b); err != nil { + t.Fatalf("comment content that looks like code tripped the check: %v", err) + } +} + +func TestStructurallyBalanced(t *testing.T) { + if err := StructurallyBalanced("select f((a+b)*c) from t", "select f( ( a + b ) * c )\nfrom t"); err != nil { + t.Errorf("whitespace-only reformat flagged: %v", err) + } + // Delimiters that live inside a comment or a string must not count. + if err := StructurallyBalanced("select ')(' as x -- ((((\nfrom t", "select ')(' as x\n-- ((((\nfrom t"); err != nil { + t.Errorf("comment/string delimiters counted: %v", err) + } + if err := StructurallyBalanced("select (a) from t", "select (a from t"); err == nil { + t.Errorf("dropped ')' not detected") + } +} + +func TestVerifySafeCatchesNonIdempotent(t *testing.T) { + src := "create function f() returns void language sql as $$ select 1 $$;" + out := format(src) + if err := VerifySafe(src, out, config.Default()); err != nil { + t.Fatalf("clean format rejected: %v", err) + } + // A hand-mangled "output" that differs from what the formatter would produce + // must be rejected (idempotence gate). + if err := VerifySafe(src, out+"\n\n\n", config.Default()); err == nil { + t.Errorf("non-idempotent output accepted") + } +} + +func TestVerifySafeBlockCommentInBody(t *testing.T) { + // Regression: a multi-line /* */ comment inside a PL/pgSQL body was being + // re-split and reindented as if its lines were statements. + src := "CREATE FUNCTION f() RETURNS void LANGUAGE plpgsql AS $$\n" + + "BEGIN\n" + + " /*\n" + + " update t u\n" + + " set x = 1\n" + + " where u.id = 2\n" + + " and u.y = 3;\n" + + " */\n" + + " perform 1;\n" + + "END $$;\n" + out := format(src) + if err := VerifySafe(src, out, config.Default()); err != nil { + t.Fatalf("block comment in body mangled: %v", err) + } + if !strings.Contains(out, "where u.id = 2") { + t.Errorf("block comment interior lost a line:\n%s", out) + } +} diff --git a/pkg/lsp/server.go b/pkg/lsp/server.go index becbb4f..a6adc15 100644 --- a/pkg/lsp/server.go +++ b/pkg/lsp/server.go @@ -124,7 +124,7 @@ func (s *server) handle(raw []byte) bool { s.reply(req.ID, []textEdit{}) return false } - if !format.SemanticallyEqual(text, formatted) { + if err := format.VerifySafe(text, formatted, s.cfg); err != nil { s.reply(req.ID, []textEdit{}) return false } @@ -228,7 +228,7 @@ func (s *server) rangeFormat(text string, r lspRange) []textEdit { if formatted == text { return nil } - if !format.SemanticallyEqual(text, formatted) { + if err := format.VerifySafe(text, formatted, s.cfg); err != nil { return nil } diff --git a/testdata/corpus/test_mm_proc.pgsql b/testdata/corpus/test_mm_proc.pgsql index 5d7d8cb..bc2cfb8 100644 --- a/testdata/corpus/test_mm_proc.pgsql +++ b/testdata/corpus/test_mm_proc.pgsql @@ -138,7 +138,9 @@ BEGIN select ctl.rid_parent as rid_parent , ctl.tableprefix as parent_prefix - from tasklist ctl --inner join t_adproclient cli on cli.rid_adproclient = ctl.rid_parent -- and ctl.tableprefix = 'tcli' + from tasklist ctl +--inner join t_adproclient cli on cli.rid_adproclient = ctl.rid_parent +-- and ctl.tableprefix = 'tcli' where ctl.rid_tasklist = p_data_rid into m_data_rid, m_data_prefix; elsif p_data_prefix::citext = 'ehr' @@ -545,13 +547,14 @@ BEGIN /* ---Use this code to test - select pl_writefile('/mnt/t/temp/t.docx',r.p_doc) ,r.* - from mm_proc('docx','allfieldvalues', ( +select pl_writefile('/mnt/t/temp/t.docx',r.p_doc) +,r.* +from mm_proc('docx','allfieldvalues', ( select tat.maindoc from templateattachment tat --cross join view_blob(tat.rid_templateattachment,'templateattachment','maindoc','rid_templateattachment','utf8') v where tat.description ilike 'Form 16 (%' - ), 'TCLI',1000016) r +), 'TCLI',1000016) r */ /* @@ -1163,7 +1166,7 @@ BEGIN m_exec_orderstr = r_lp_t.parent_order_string; --raise notice 'Applying order % by for % %.', r_lp_t.parent_order_string, r_lp_t.parent_table_name,r_lp_t.field_name; /* - select s.parent_order_string + select s.parent_order_string from tmp_merge_init_fields f inner join tmp_merge_init_src s on s.parent_rid = r_lp_t.rid and s.merge_type = G_MTYPE_TBLROOT @@ -1171,7 +1174,7 @@ BEGIN where f.source = r_lp_t.source limit 1 into m_exec_orderstr; - */ + */ end if; @@ -1330,7 +1333,9 @@ BEGIN inner join tmp_merge_init_fields d on d . mergetag = c . mergetag and d . tblparent = r_lp_t . tblid - where f . grand_rid = r_lp_t . parent_rid and f . merge_type = G_MTYPE_TBLROOT --and f.parent_rid = any(a_tblroot) --and d.table_level > 0 + where f . grand_rid = r_lp_t . parent_rid and f . merge_type = G_MTYPE_TBLROOT +--and f.parent_rid = any(a_tblroot) +--and d.table_level > 0 group by f . rid ) loop raise notice 'Inner Loop: %', r_lp_c . qry;