diff --git a/CLAUDE.md b/CLAUDE.md index 37ad656..5e9ce7d 100644 --- a/CLAUDE.md +++ b/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. diff --git a/PLAN.md b/PLAN.md index 3af7617..a3d03d8 100644 --- a/PLAN.md +++ b/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 diff --git a/oliphant.go b/oliphant.go index 6c6284c..a383ce0 100644 --- a/oliphant.go +++ b/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 diff --git a/parser/parser.go b/parser/parser.go index 3e727cc..ef5304c 100644 --- a/parser/parser.go +++ b/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 { diff --git a/parser/tree_test.go b/parser/tree_test.go new file mode 100644 index 0000000..12e14af --- /dev/null +++ b/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") + } +}