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) }