From 2f7f12cfa9f967bbbc965631474cf2ea2f40da1c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 21:21:21 +0000 Subject: [PATCH] fmt: preserve ') VALUES (' seams and !=/<> operator spellings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two formatting fixes reported from running sqlc fmt on a real project: The boundary before a bare VALUES list was decided by comparing the source lines of the neighbouring nodes (the last column and the first value), which always differ when both paren lists are multi-line — so an authored ') VALUES (' seam broke apart. The seam now gets its own group, and AttachComments reads the author's actual choice out of the source by scanning back from the first value across '(' and the VALUES keyword. The parsers normalized inequality spellings: PostgreSQL's grammar turns != into <>, and the MySQL converter mapped opcode.NE to != regardless of what was written. The format parsers now read the author's spelling back out of the source — PostgreSQL via a new NewFormatParser that rewrites A_Expr names at the operator's recorded location, MySQL via the same source-scan approach SQLite already used. The compiler's parsers keep the canonical names. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01T4UpYmwPdxzePtsRXoL1H8 --- internal/cmd/fmt.go | 4 +- .../endtoend/testdata/fmt/mysql/query.sql | 3 + .../endtoend/testdata/fmt/mysql/stdout.txt | 19 ++---- .../testdata/fmt/postgresql/query.sql | 18 ++++++ .../testdata/fmt/postgresql/stdout.txt | 11 ++-- internal/engine/dolphin/convert.go | 36 ++++++++++- internal/engine/dolphin/parse.go | 5 ++ internal/engine/postgresql/parse.go | 35 +++++++++++ internal/sql/ast/comment.go | 60 ++++++++++++++++++- internal/sql/ast/insert_stmt.go | 16 ++++- 10 files changed, 183 insertions(+), 24 deletions(-) diff --git a/internal/cmd/fmt.go b/internal/cmd/fmt.go index 54763d9420..1dc7f4bc55 100644 --- a/internal/cmd/fmt.go +++ b/internal/cmd/fmt.go @@ -45,7 +45,9 @@ type queryFormatter interface { func newQueryFormatter(engine config.Engine) queryFormatter { switch engine { case config.EnginePostgreSQL: - return postgresql.NewParser() + // The format parser preserves the author's operator spellings + // (`!=` vs `<>`) that the grammar would otherwise normalize. + return postgresql.NewFormatParser() case config.EngineSQLite: return sqlite.NewParser() case config.EngineMySQL: diff --git a/internal/endtoend/testdata/fmt/mysql/query.sql b/internal/endtoend/testdata/fmt/mysql/query.sql index c8afff5624..ead44f8de0 100644 --- a/internal/endtoend/testdata/fmt/mysql/query.sql +++ b/internal/endtoend/testdata/fmt/mysql/query.sql @@ -70,3 +70,6 @@ WHERE p.bio IS NOT NULL; SHOW WARNINGS; CREATE TABLE scores (points decimal(10, 5), views bigint unsigned NOT NULL); + +-- name: SpelledOperators :many +SELECT id FROM authors WHERE name != ? AND bio <> ?; diff --git a/internal/endtoend/testdata/fmt/mysql/stdout.txt b/internal/endtoend/testdata/fmt/mysql/stdout.txt index a015b7ada3..97f3d89606 100644 --- a/internal/endtoend/testdata/fmt/mysql/stdout.txt +++ b/internal/endtoend/testdata/fmt/mysql/stdout.txt @@ -31,24 +31,21 @@ FROM authors -- soft-deleted rows are filtered WHERE bio IS NOT NULL -@@ -21,8 +25,9 @@ +@@ -21,7 +25,8 @@ SELECT /* inline note */ id, name FROM authors ORDER BY name; -- name: CountSigils :one +SELECT count(*) +FROM authors -+WHERE id != ? AND name != @user_name; # session variable stays -SELECT count(*) FROM authors --WHERE id <> ? AND name <> @user_name; # session variable stays + WHERE id <> ? AND name <> @user_name; # session variable stays -- name: CastUnsigned :one - SELECT CAST(id AS UNSIGNED) FROM authors LIMIT 1; -@@ -34,14 +39,11 @@ +@@ -34,11 +39,7 @@ SELECT id FROM authors LIMIT 1; -- name: CreateAuthor :execresult -+INSERT INTO authors (name, bio) -+VALUES (?, ?); ++INSERT INTO authors (name, bio) VALUES (?, ?); -insert into authors ( - name, bio -) values ( @@ -56,12 +53,8 @@ -); -- name: CasePreserved :many -+SELECT ID, Name FROM Authors WHERE Name != '' ORDER BY Name; --SELECT ID, Name FROM Authors WHERE Name <> '' ORDER BY Name; - - -- name: LiteralsSurvive :one - SELECT true AS t, false AS f, 1.50 AS score, CASE WHEN name = '' THEN NULL ELSE name END AS n FROM authors LIMIT 1; -@@ -62,7 +64,8 @@ + SELECT ID, Name FROM Authors WHERE Name <> '' ORDER BY Name; +@@ -62,7 +63,8 @@ SELECT /*+ MAX_EXECUTION_TIME(1000) */ id FROM authors LIMIT 1; -- name: UpdateWithJoin :exec diff --git a/internal/endtoend/testdata/fmt/postgresql/query.sql b/internal/endtoend/testdata/fmt/postgresql/query.sql index 96e8ed6ef7..d01e05cf2d 100644 --- a/internal/endtoend/testdata/fmt/postgresql/query.sql +++ b/internal/endtoend/testdata/fmt/postgresql/query.sql @@ -29,5 +29,23 @@ SELECT id, name, bio, created_at FROM authors WHERE name LIKE $1 AND bio IS NOT -- name: AtParamsGlued :many SELECT name FROM authors WHERE name = @slug AND @filter::bool; +-- name: CreateAuthorLong :one +INSERT INTO authors ( + name, + bio +) VALUES ( + $1, + $2 +) +RETURNING *; + +-- name: CreateAuthorBrokenValues :one +INSERT INTO authors (name, bio) +VALUES ($1, $2) +RETURNING *; + +-- name: SpelledOperators :many +SELECT id FROM authors WHERE name != $1 AND bio <> $2; + -- name: DeleteAuthor :exec DELETE FROM authors WHERE id = @id \ No newline at end of file diff --git a/internal/endtoend/testdata/fmt/postgresql/stdout.txt b/internal/endtoend/testdata/fmt/postgresql/stdout.txt index e58072656d..1b0f70af83 100644 --- a/internal/endtoend/testdata/fmt/postgresql/stdout.txt +++ b/internal/endtoend/testdata/fmt/postgresql/stdout.txt @@ -20,18 +20,17 @@ ORDER BY name; -- name: CreateAuthor :one -@@ -13,8 +15,5 @@ +@@ -13,8 +15,4 @@ -INSERT INTO authors ( - name, bio -) VALUES ( - $1, $2 -) -+INSERT INTO authors (name, bio) -+VALUES ($1, $2) ++INSERT INTO authors (name, bio) VALUES ($1, $2) RETURNING *; -- name: PickyQuery :many -@@ -21,5 +20,6 @@ +@@ -21,5 +19,6 @@ -SELECT id, -- the primary key - name +SELECT @@ -40,8 +39,8 @@ FROM authors WHERE id > $1; -@@ -30,4 +30,4 @@ - SELECT name FROM authors WHERE name = @slug AND @filter::bool; +@@ -48,4 +47,4 @@ + SELECT id FROM authors WHERE name != $1 AND bio <> $2; -- name: DeleteAuthor :exec -DELETE FROM authors WHERE id = @id diff --git a/internal/engine/dolphin/convert.go b/internal/engine/dolphin/convert.go index c52fb7e2ef..50a3b75f83 100644 --- a/internal/engine/dolphin/convert.go +++ b/internal/engine/dolphin/convert.go @@ -21,6 +21,31 @@ type cc struct { // lowercasing them for case-insensitive catalog matching; the format // parser sets it (see NewFormatParser). preserveCase bool + // src is the statement's source text, set alongside preserveCase: the + // format parser reads spellings the tree does not keep (`!=` vs `<>`) + // back out of it. + src string +} + +// operatorSpelling reads the operator token that sits immediately before +// the operand at byte offset rpos, reporting which of the candidate +// spellings the author wrote. Source that does not end in a candidate right +// there (a comment against the operand, no recorded position) returns +// ok=false and the caller keeps the canonical name. +func (c *cc) operatorSpelling(rpos int, candidates ...string) (string, bool) { + if c.src == "" || rpos <= 0 || rpos > len(c.src) { + return "", false + } + end := rpos + for end > 0 && (c.src[end-1] == ' ' || c.src[end-1] == '\t' || c.src[end-1] == '\r' || c.src[end-1] == '\n') { + end-- + } + for _, cand := range candidates { + if end >= len(cand) && c.src[end-len(cand):end] == cand { + return cand, true + } + } + return "", false } func todo(n pcast.Node) *ast.TODO { @@ -224,11 +249,20 @@ func (c *cc) convertBinaryOperationExpr(n *pcast.BinaryOperationExpr) ast.Node { Location: n.OriginTextPosition(), } } else { + name := opToName(n.Op) + if n.Op == opcode.NE && c.src != "" { + // MySQL spells inequality two ways (!= and <>) and the tree + // keeps only the opcode, so the format parser reads the + // author's choice back out of the source. + if op, ok := c.operatorSpelling(n.R.OriginTextPosition(), "!=", "<>"); ok { + name = op + } + } return &ast.A_Expr{ // TODO: Set kind Name: &ast.List{ Items: []ast.Node{ - &ast.String{Str: opToName(n.Op)}, + &ast.String{Str: name}, }, }, Lexpr: c.convert(n.L), diff --git a/internal/engine/dolphin/parse.go b/internal/engine/dolphin/parse.go index 9f1f409593..c830b92df4 100644 --- a/internal/engine/dolphin/parse.go +++ b/internal/engine/dolphin/parse.go @@ -97,6 +97,11 @@ func (p *Parser) ParseFile(r io.Reader) (*ast.File, error) { searchFrom := 0 for i := range stmtNodes { converter := &cc{preserveCase: p.preserveCase} + if p.preserveCase { + // The format parser also preserves operator spellings the tree + // does not keep; the compiler's parser stays canonical. + converter.src = src + } // A statement sqlc has no node for converts to a TODO and stays in // the list: the formatter needs its extent to keep it as written, // and Parse filters it out for the compiler. diff --git a/internal/engine/postgresql/parse.go b/internal/engine/postgresql/parse.go index 0de54e6eb3..43fa73a335 100644 --- a/internal/engine/postgresql/parse.go +++ b/internal/engine/postgresql/parse.go @@ -11,6 +11,7 @@ import ( "github.com/sqlc-dev/sqlc/internal/source" "github.com/sqlc-dev/sqlc/internal/sql/ast" + "github.com/sqlc-dev/sqlc/internal/sql/astutils" "github.com/sqlc-dev/sqlc/internal/sql/sqlerr" ) @@ -144,7 +145,38 @@ func NewParser() *Parser { return &Parser{} } +// NewFormatParser returns the parser sqlc fmt uses. It differs from the +// compiler's parser in one way: where the grammar normalizes an operator the +// author may spell two ways (`!=` parses as `<>`), the spelling is read back +// out of the source so formatting preserves it. The compiler keeps the +// normalized name, which is the one the catalog knows. +func NewFormatParser() *Parser { + return &Parser{preserveSpelling: true} +} + type Parser struct { + preserveSpelling bool +} + +// restoreOperatorSpelling rewrites operators the grammar normalized back to +// the spelling the author wrote. The scanner turns `!=` into `<>` before the +// AST exists; an A_Expr's location points at the operator token, so the +// source says which spelling to print. +func restoreOperatorSpelling(n ast.Node, src string) { + astutils.Walk(astutils.VisitorFunc(func(node ast.Node) { + expr, ok := node.(*ast.A_Expr) + if !ok || expr.Name == nil || len(expr.Name.Items) != 1 { + return + } + s, ok := expr.Name.Items[0].(*ast.String) + if !ok || s.Str != "<>" { + return + } + loc := expr.Location + if loc >= 0 && loc+2 <= len(src) && src[loc:loc+2] == "!=" { + expr.Name = &ast.List{Items: []ast.Node{&ast.String{Str: "!="}}} + } + }), n) } var errSkip = errors.New("skip stmt") @@ -208,6 +240,9 @@ func (p *Parser) ParseFile(r io.Reader) (*ast.File, error) { if n == nil { return nil, fmt.Errorf("unexpected nil node") } + if p.preserveSpelling { + restoreOperatorSpelling(n, contents) + } stmts = append(stmts, ast.Statement{ Raw: &ast.RawStmt{ Stmt: n, diff --git a/internal/sql/ast/comment.go b/internal/sql/ast/comment.go index ea877328b0..d09178f306 100644 --- a/internal/sql/ast/comment.go +++ b/internal/sql/ast/comment.go @@ -2,6 +2,7 @@ package ast import ( "sort" + "strings" "github.com/sqlc-dev/sqlc/internal/sql/format" ) @@ -201,13 +202,70 @@ func AttachComments(raw *RawStmt, d format.Dialect, comments []Comment, src stri break } } - if prevPos >= 0 && nextPos >= 0 && lineOf(prevPos) != lineOf(nextPos) { + if prevPos < 0 || nextPos < 0 { + continue + } + // The boundary before a bare VALUES list is a seam between two paren + // lists, so the neighbouring nodes' lines cannot decide it: with both + // lists broken they always differ, even when the author glued + // `) VALUES (`. Read the author's choice at the keyword itself. + if sel, ok := a.node.(*SelectStmt); ok && items(sel.ValuesLists) { + if broken, ok := valuesSeamBroken(src, nextPos); ok { + if broken { + table.breaks[a.node] = true + } + continue + } + } + if lineOf(prevPos) != lineOf(nextPos) { table.breaks[a.node] = true } } return table } +// valuesSeamBroken reports whether the author broke the line before a bare +// VALUES keyword. nextPos is the position of the first printed node inside +// the list; scanning backward from it crosses `(` and then the keyword, and +// the whitespace in front of the keyword holds the answer. Source that does +// not scan that way (an extra paren, a comment against the keyword) returns +// ok=false and the caller falls back to the line heuristic. +func valuesSeamBroken(src string, nextPos int) (broken bool, ok bool) { + if nextPos <= 0 || nextPos > len(src) { + return false, false + } + isSpace := func(c byte) bool { + return c == ' ' || c == '\t' || c == '\r' || c == '\n' + } + i := nextPos - 1 + for i >= 0 && isSpace(src[i]) { + i-- + } + if i < 0 || src[i] != '(' { + return false, false + } + i-- + for i >= 0 && isSpace(src[i]) { + i-- + } + const keyword = "VALUES" + if i+1 < len(keyword) || !strings.EqualFold(src[i+1-len(keyword):i+1], keyword) { + return false, false + } + i -= len(keyword) + // A keyword, not the tail of an identifier. + if i >= 0 && !isSpace(src[i]) && src[i] != ')' { + return false, false + } + for i >= 0 && isSpace(src[i]) { + if src[i] == '\n' { + broken = true + } + i-- + } + return broken, true +} + type anchor struct { node Node pos int diff --git a/internal/sql/ast/insert_stmt.go b/internal/sql/ast/insert_stmt.go index e6d3d3a190..da0214d21e 100644 --- a/internal/sql/ast/insert_stmt.go +++ b/internal/sql/ast/insert_stmt.go @@ -61,8 +61,20 @@ func (n *InsertStmt) Format(buf *TrackedBuffer, d format.Dialect) { if n.DefaultValues { buf.WriteString(" DEFAULT VALUES") } else if set(n.SelectStmt) { - buf.beforeClause(n.SelectStmt, d) - buf.Line() + if sel, ok := n.SelectStmt.(*SelectStmt); ok && items(sel.ValuesLists) { + // The seam before a bare VALUES list keeps the author's own + // choice — `) VALUES (` glued or VALUES on its own line — so it + // gets a group of its own: a break inside either paren list must + // not decide it. AttachComments reads the choice out of the + // source (see valuesSeamBroken) and marks the boundary. + buf.Group() + buf.beforeClause(n.SelectStmt, d) + buf.Line() + buf.EndGroup() + } else { + buf.beforeClause(n.SelectStmt, d) + buf.Line() + } buf.astFormat(n.SelectStmt, d) }