ParseFile returns the tree with the input's comments - #12
Merged
Conversation
A formatter cannot put comments back where they were written without being told where that was, and the grammar is the wrong place to ask: libpg_query's patch 04 has the scanner emit comments as tokens, and base_yylex drops them on the way to the parser. Scan exposes that channel, so Scan plus Parse already answers the question — by lexing the input twice and marshalling the entire token stream to protobuf to recover a handful of comments. On a six-statement query file that is 138us and 689 allocations against ParseFile's 70us and 444. parser.ParseFile keeps the comments from the pass the parse already makes and returns them beside the tree, as the ScanToken values Scan would have reported. It is oliphant's second deliberate addition to pg_query_go's surface, alongside ParseToTree, and like it lives in the parser subpackage so the root package stays a mirror function-for-function. The scanner collects at the one site every token is minted, rather than at the filter's drop site. A successful parse does route every comment through that drop site, but not every path there goes through Filter.Next — the UIDENT/UESCAPE resolution pulls tokens off to the side — and the mint site cannot be bypassed at all. Collection is opt-in via NewKeepingComments, and the parse path is unchanged when it is off: interleaved runs of the existing parse benchmarks show no difference. The corpus is what pins the result. Across all 46,756 cases that parse, ParseFile's tree is proto.Equal to ParseToTree's, and its comments are exactly the SQL_COMMENT and C_COMMENT tokens Scan reports for the same input — 11,880 cases carry at least one, so the oracle-derived scan goldens stand behind the comment spans too. Focused tests cover the positions a query file puts comments in (above a statement, trailing one on its terminator's line, inside one, between two, and after the last), nested block comments arriving as the single token PostgreSQL scans them as, and a failed parse returning no half-scanned list. Unlike Parse, ParseFile does not reject strings that are not valid UTF-8: that constraint is protobuf's, not the grammar's, and nothing here encodes the tree. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MB4HGvrQk93hmnpSPm21N8
ParseFile skipped the proto3 UTF-8 rejection Parse keeps, on the grounds that the constraint is the wire format's rather than the grammar's. Two entry points disagreeing about the same input is worse than the quirk, so it now answers as Parse does. The check cannot be on the input. An invalid byte fails only where it lands in a string field, and a comment is not one, so "SELECT 1 -- \xff" marshals and Parse returns its tree — and a comment is precisely what ParseFile must not reject one for. So it decides on the tree, marshalling it only when utf8.ValidString(input) is already false, which is the same guard Parse takes its own fallback on and costs nothing on valid input. TestParseFileInvalidUTF8 pins both sides against Parse: an invalid byte in a string field or an identifier fails with the same error from both, and one confined to a line or block comment parses for both, with the comment kept. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MB4HGvrQk93hmnpSPm21N8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A formatter cannot put comments back where they were written without being told where that was, and the grammar is the wrong place to ask: libpg_query's patch 04 has the scanner emit comments as tokens, and
base_yylexdrops them on the way to the parser.Scanexposes that channel, soScan+Parsealready answers the question — by lexing the input twice and marshalling the entire token stream to protobuf to recover a handful of comments:Scan+Parseparser.ParseFileParsealone (no comments)ParseFilekeeps the comments from the pass the parse already makes and returns them beside the tree, as theScanTokenvaluesScanwould have reported:This is oliphant's second deliberate addition to pg_query_go's surface, alongside
ParseToTreefrom #11, and like it lives in theparsersubpackage so the root package stays a mirror function-for-function. PLAN.md § 1 and CLAUDE.md's hard rule now record both.Where the comments are collected
At the one site every token is minted (
Scanner.token), not at the filter's drop site. A successful parse does route every comment through that drop site — I checked, including comments picked up asbase_yylexlookahead — but not every path there goes throughFilter.Next: the UIDENT/UESCAPE resolution pulls tokens off to the side. The mint site cannot be bypassed at all.Collection is opt-in through
NewKeepingComments, and the parse path is unchanged when it is off. Interleaved runs of the existing parse benchmarks (before/after binaries alternated, to cancel drift on a noisy box) show overlapping distributions —ParseSelect26993–8018 ns before, 7053–8298 ns after;ParseCreateTable17159–20246 ns before, 16932–21515 ns after.Agreeing with Parse on UTF-8
ParseFileaccepts and rejects exactly whatParsedoes, the proto3 UTF-8 rejection #11 kept included.That answer could not come from checking the input. An invalid byte fails only where it lands in a string field, and a comment is not one —
SELECT 1 -- \xffmarshals andParsereturns its tree — and a comment is precisely whatParseFilemust not reject one for. So it decides on the tree, marshalling it only whenutf8.ValidString(input)is already false, which is the same guardParsetakes its own fallback on and costs nothing on valid input.Testing
The corpus is what pins the result.
TestParseFileMatchesScanruns every case in the parse suite:ParseFile's tree isproto.EqualtoParseToTree's and the two agree on every error;SQL_COMMENT/C_COMMENTtokensScanreports for the same input — 11,880 cases carry at least one, so the oracle-derived scan goldens stand behind the comment spans too;Focused tests cover the positions a query file puts comments in (above a statement, trailing one on its terminator's line, inside one, between two, and after the last — that one is what a parse could plausibly miss, since nothing follows it but end of input), nested block comments arriving as the single token PostgreSQL scans them as rather than ending at the inner
*/, a failed parse returning no half-scanned list, and the UTF-8 agreement above in both directions.go test ./...is green. Checked against the real consumer as well: with areplacepointing here, sqlc builds and its wholeTestReplay/basecorpus passes — worth doing since this one touches the scanner.Next
The sqlc side:
ParseFileonpostgresql.Parserreturning*ast.File, a case innewQueryFormatter, and flippingtestdata/fmt/postgresqlfrom its skip notice to a real golden. That needs sqlc-dev/sqlc#4580 merged first.