fmt: preserve ') VALUES (' seams and !=/<> operator spellings

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:

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 <> ?;

internal/endtoend/testdata/fmt/mysql/stdout.txt

@@ -31,37 +31,30 @@
  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 (
 -  ?, ?
 -);
 
  -- 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

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

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

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

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.

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,

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

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