Parse returns the tree without the protobuf round trip

CLAUDE.md

@@ -27,7 +27,9 @@ the main module; the only module allowed to link pg_query_go (cgo) is
   written by `cmd/generate`.
 - The public API mirrors pg_query_go exactly; consumers must migrate by
   changing one import path. Never rename, add, or remove exported symbols
-  without checking pg_query_go v6.2.2.
+  without checking pg_query_go v6.2.2. There is one deliberate addition,
+  `parser.ParseToTree` (PLAN.md § 1): the tree without the protobuf round
+  trip the cgo boundary forces upstream.
 - Location fields must byte-match the reference; when in doubt, check which
   `@N` the `gram.y` action uses.
 

PLAN.md

@@ -132,6 +132,27 @@ The `parser` subpackage's byte-level entry points (`ParseToProtobuf`,
 thin `proto.Marshal`/`Unmarshal` wrappers — they exist upstream only because
 cgo speaks bytes; here they are conveniences.
 
+Being conveniences, they are not what the root package builds on. Upstream's
+`Parse` decodes `ParseToProtobuf`'s bytes because the tree is built in C and
+protobuf is how it crosses the cgo boundary; encoding a tree the Go parser
+already holds only to decode it back costs about three quarters of `Parse`'s
+running time and doubles its allocations, so `Parse` calls
+**`parser.ParseToTree`** — oliphant's one deliberate addition to pg_query_go's
+surface, and the exception to § *The API does not change*. Two consequences,
+both pinned by `parser/tree_test.go`:
+
+- proto3 string fields must be valid UTF-8, so a tree carrying a raw invalid
+  byte fails to marshal and upstream reports that as an error from `Parse`.
+  Such a byte can only come from the input verbatim — the scanner rejects
+  escapes that would synthesize one, and identifier truncation is
+  `pg_mbcliplen`-equivalent — so `ParseToTree` falls back to the round trip
+  when `utf8.ValidString(input)` is false, and the behavior is unchanged.
+- The tree may share subtrees where the round trip deep-copied them: a
+  multi-column `UPDATE ... SET (a, b, c) = (...)` points every `ResTarget` at
+  one `MultiAssignRef` source, exactly as the C tree does, and protobuf has no
+  pointers. Callers that only read the tree — all pg_query_go's API admits —
+  cannot tell.
+
 ### 2. The AST is the generated protobuf types — one dependency, not zero
 
 The siblings' "zero dependencies, ever" rule bends once: pg_query_go's API

oliphant.go

@@ -33,15 +33,13 @@ func ParseToJSON(input string) (result string, err error) {
 }
 
 // Parse the given SQL statement into a parse tree (Go struct format)
+//
+// Upstream this decodes the protobuf the C parser returns across the cgo
+// boundary; here the parser is Go and hands back the tree itself, so
+// parser.ParseToTree skips the encode/decode. Behavior is unchanged — see
+// its comment for the UTF-8 fallback that keeps it that way.
 func Parse(input string) (tree *ParseResult, err error) {
-	protobufTree, err := parser.ParseToProtobuf(input)
-	if err != nil {
-		return
-	}
-
-	tree = &ParseResult{}
-	err = proto.Unmarshal(protobufTree, tree)
-	return
+	return parser.ParseToTree(input)
 }
 
 // Deparse - Deparses a given Go parse tree into a SQL statement

parser/parser.go

@@ -11,6 +11,7 @@ import (
 	"errors"
 	"fmt"
 	"strings"
+	"unicode/utf8"
 
 	"google.golang.org/protobuf/proto"
 
@@ -95,6 +96,53 @@ func ScanToProtobuf(input string) (result []byte, err error) {
 	return proto.Marshal(scan)
 }
 
+// ParseToTree parses the input and returns the tree the Go parser built,
+// without the protobuf round trip Parse would otherwise make.
+//
+// This is oliphant's one addition to pg_query_go's surface (see PLAN.md § 1).
+// Upstream's Parse decodes bytes because the parse tree is built in C and
+// crosses the cgo boundary as protobuf; here the tree is already a Go value,
+// so marshalling it only to unmarshal it back costs about three quarters of
+// Parse's time and doubles its allocations for no observable difference.
+//
+// "No observable difference" holds with one exception, which is why the
+// round trip survives as a fallback: proto3 string fields must be valid
+// UTF-8, so a tree carrying a raw invalid byte fails to marshal and upstream
+// reports that as an error from Parse. Such a byte can only have come from
+// the input verbatim — the scanner rejects escape sequences that would
+// synthesize one (E'\xff' raises "invalid byte sequence for encoding
+// UTF8"), and identifier truncation is pg_mbcliplen-equivalent, so it never
+// splits a character — which makes utf8.ValidString a sufficient guard.
+//
+// One difference remains, and is not observable through pg_query_go's own
+// API: the tree may share subtrees, where the round trip silently deep-copied
+// them (a multi-column UPDATE ... SET (a, b, c) = (...) points every
+// ResTarget at one MultiAssignRef source, exactly as the C tree does).
+// Callers that only read the tree cannot tell; callers that mutate one now
+// can. See TestParseToTreeSharesMultiAssignSource.
+func ParseToTree(input string) (*ast.ParseResult, error) {
+	if !utf8.ValidString(input) {
+		return parseViaProtobuf(input)
+	}
+	tree, perr := parse.Parse(input)
+	if perr != nil {
+		return nil, scanErr(perr)
+	}
+	return tree, nil
+}
+
+// parseViaProtobuf is upstream's Parse body: marshal the tree and decode it
+// again, so an input that cannot round-trip fails exactly as it does there.
+func parseViaProtobuf(input string) (tree *ast.ParseResult, err error) {
+	protobufTree, err := ParseToProtobuf(input)
+	if err != nil {
+		return
+	}
+	tree = &ast.ParseResult{}
+	err = proto.Unmarshal(protobufTree, tree)
+	return
+}
+
 func ParseToProtobuf(input string) ([]byte, error) {
 	tree, perr := parse.Parse(input)
 	if perr != nil {

parser/tree_test.go

@@ -0,0 +1,190 @@
+package parser_test
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"google.golang.org/protobuf/proto"
+
+	pg_query "github.com/sqlc-dev/oliphant"
+	"github.com/sqlc-dev/oliphant/ast"
+	"github.com/sqlc-dev/oliphant/internal/testfile"
+	"github.com/sqlc-dev/oliphant/parser"
+)
+
+// treeCorpusFiles lists every .test file of the parse suite — the corpus of
+// inputs whose trees are the thing under test here.
+func treeCorpusFiles(t *testing.T) []string {
+	t.Helper()
+	var files []string
+	err := filepath.WalkDir(filepath.Join("testdata", "parse"), func(path string, d os.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if !d.IsDir() && strings.HasSuffix(path, ".test") {
+			files = append(files, path)
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(files) == 0 {
+		t.Fatal("no corpus files found")
+	}
+	return files
+}
+
+// roundTrip is the body Parse had before ParseToTree: marshal the tree and
+// decode it again, which is what the cgo boundary forces upstream.
+func roundTrip(input string) (*ast.ParseResult, error) {
+	b, err := parser.ParseToProtobuf(input)
+	if err != nil {
+		return nil, err
+	}
+	tree := &ast.ParseResult{}
+	if err := proto.Unmarshal(b, tree); err != nil {
+		return nil, err
+	}
+	return tree, nil
+}
+
+// TestParseToTreeMatchesProtobuf pins the premise of skipping the round trip:
+// across the corpus, the tree the parser hands back directly and the tree
+// that survives a marshal/unmarshal are equal, and the two paths agree on
+// which inputs fail.
+func TestParseToTreeMatchesProtobuf(t *testing.T) {
+	var parsed int
+	for _, path := range treeCorpusFiles(t) {
+		cases, err := testfile.Read(path)
+		if err != nil {
+			t.Fatal(err)
+		}
+		rel, _ := filepath.Rel("testdata", path)
+		for _, c := range cases {
+			direct, directErr := parser.ParseToTree(c.Input)
+			round, roundErr := roundTrip(c.Input)
+			if (directErr == nil) != (roundErr == nil) {
+				t.Errorf("%s case %s: direct err=%v, round trip err=%v\ninput:\n%s",
+					rel, c.Name, directErr, roundErr, c.Input)
+				continue
+			}
+			if directErr != nil {
+				if directErr.Error() != roundErr.Error() {
+					t.Errorf("%s case %s: direct err %q, round trip err %q",
+						rel, c.Name, directErr, roundErr)
+				}
+				continue
+			}
+			parsed++
+			if !proto.Equal(direct, round) {
+				t.Errorf("%s case %s: tree differs after the protobuf round trip\ninput:\n%s",
+					rel, c.Name, c.Input)
+			}
+		}
+	}
+	t.Logf("compared %d parsed cases", parsed)
+}
+
+// TestParseToTreeInvalidUTF8 covers the one input class where the round trip
+// is load-bearing rather than redundant. proto3 string fields must be valid
+// UTF-8, so a raw invalid byte in the input makes upstream's Parse fail at
+// the decode; ParseToTree keeps that by falling back to the round trip
+// whenever the input is not valid UTF-8.
+func TestParseToTreeInvalidUTF8(t *testing.T) {
+	for _, input := range []string{
+		"SELECT '\xff'",
+		"SELECT * FROM \"tab\xffle\"",
+	} {
+		tree, err := parser.ParseToTree(input)
+		if err == nil {
+			t.Errorf("ParseToTree(%q) = %v, want an error", input, tree)
+			continue
+		}
+		if !strings.Contains(err.Error(), "invalid UTF-8") {
+			t.Errorf("ParseToTree(%q) error = %q, want the protobuf UTF-8 error", input, err)
+		}
+		if tree != nil {
+			t.Errorf("ParseToTree(%q) returned a tree alongside %v", input, err)
+		}
+	}
+
+	// A valid-UTF-8 input can never produce an invalid-UTF-8 string field:
+	// the scanner rejects escapes that would synthesize one, and identifier
+	// truncation is pg_mbcliplen-equivalent, so it never splits a character.
+	// These take the direct path and must still parse.
+	for _, input := range []string{
+		`SELECT E'\303\251'`,
+		`SELECT E'\xc3\xa9'`,
+		`SELECT U&'\00e9'`,
+		`SELECT "` + strings.Repeat("é", 40) + `"`,
+	} {
+		if _, err := parser.ParseToTree(input); err != nil {
+			t.Errorf("ParseToTree(%q) = %v, want no error", input, err)
+		}
+	}
+}
+
+// TestParseToTreeSharesMultiAssignSource documents the one way a tree from
+// ParseToTree differs from one that has been through protobuf: it can share
+// subtrees. A multi-column assignment points every ResTarget's
+// MultiAssignRef at one source node, exactly as the C tree does; protobuf has
+// no pointers, so encoding and decoding silently deep-copied it. Read-only
+// callers — which is what pg_query_go's API admits — cannot tell.
+func TestParseToTreeSharesMultiAssignSource(t *testing.T) {
+	const input = "UPDATE t SET (a, b, c) = (1, 2, 3)"
+
+	sources := func(tree *ast.ParseResult) []*ast.Node {
+		t.Helper()
+		var out []*ast.Node
+		for _, target := range tree.Stmts[0].Stmt.GetUpdateStmt().TargetList {
+			out = append(out, target.GetResTarget().Val.GetMultiAssignRef().Source)
+		}
+		if len(out) != 3 {
+			t.Fatalf("got %d targets, want 3", len(out))
+		}
+		return out
+	}
+
+	direct, err := parser.ParseToTree(input)
+	if err != nil {
+		t.Fatal(err)
+	}
+	got := sources(direct)
+	if got[0] != got[1] || got[1] != got[2] {
+		t.Errorf("ParseToTree: multi-assign sources are distinct nodes, want one shared node")
+	}
+
+	round, err := roundTrip(input)
+	if err != nil {
+		t.Fatal(err)
+	}
+	got = sources(round)
+	if got[0] == got[1] || got[1] == got[2] {
+		t.Errorf("round trip: multi-assign sources are shared, want the deep copy protobuf forces")
+	}
+
+	// Sharing or not, the trees are equal by value.
+	if !proto.Equal(direct, round) {
+		t.Errorf("trees differ by value")
+	}
+}
+
+// TestParseUsesTree checks the root package's Parse is the same call, so the
+// properties above describe what consumers actually get.
+func TestParseUsesTree(t *testing.T) {
+	const input = "SELECT 1"
+	viaRoot, err := pg_query.Parse(input)
+	if err != nil {
+		t.Fatal(err)
+	}
+	viaParser, err := parser.ParseToTree(input)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !proto.Equal(viaRoot, viaParser) {
+		t.Errorf("oliphant.Parse and parser.ParseToTree disagree")
+	}
+}