fix: quote reserved words in fmt - #6214
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
fmt still drops backticks on true / false / null. It's the same round-trip break as import/enum, but a worse failure mode — the output parses, as a different query — and test_every_keyword_is_quoted can't catch it, since those three lex as Literals rather than TokenKind::Keyword and so aren't in lexer::KEYWORDS. Details inline, plus a suggestion on the doc comment.
The choice(KEYWORDS.map(just)) rewrite preserves the original alternation order, and cargo test -p prqlc -p prqlc-parser passes locally on the merged tree.
`true` / `false` / `null` are reserved by the lexer (`boolean()` and `null()` both end with `end_expr()`) but produce `TokenKind::Literal` rather than `TokenKind::Keyword`, so they were absent from `lexer::KEYWORDS` and `write_ident_part` left them unquoted. Adds a sibling `lexer::RESERVED_LITERALS` and rejects both lists. Separately, `ExprKind::Ident` wrote idents via `pr::Ident`'s `Display` impl, which carries its own copy of the quoting rule and knows nothing about reserved words — so expression-position names lost their backticks even for keywords already in `KEYWORDS`. Routes it through `write_ident_part` like every other codegen path.
fmtfmt
prql-bot
left a comment
There was a problem hiding this comment.
cargo doc fails on this branch. The RESERVED_LITERALS doc comment links three private items — boolean, null, end_expr — from a pub const, which rustdoc::private_intra_doc_links denies under the workflow-level RUSTDOCFLAGS: -Dwarnings. Reproduced on the merged tree:
$ RUSTDOCFLAGS="-Dwarnings" cargo doc -p prqlc-parser --no-deps
error: public documentation for `RESERVED_LITERALS` links to private item `boolean`
--> prqlc/prqlc-parser/src/lexer/mod.rs:206:36
|
206 | /// [`TokenKind::Keyword`] — see [`boolean`] and [`null`]. They're reserved just
| ^^^^^^^ this item is private
...
error: could not document `prqlc-parser`This PR's own CI won't catch it: the 🗒️ Doc step in test-rust.yaml is gated on inputs.nightly == 'true', which rules resolved to false here — so test-rust is green while cargo doc is broken, and it would surface on the nightly run after merge instead. The suggestion inline drops the link brackets; with it, RUSTDOCFLAGS="-Dwarnings" cargo doc -p prqlc-parser --no-deps is clean. (KEYWORDS's own doc comment above is fine — TokenKind::Keyword and RESERVED_LITERALS are both public.)
Nothing else outstanding. cargo test -p prqlc-parser --lib, cargo test -p prqlc --lib -- codegen and cargo test -p prqlc --test integration -- fmt all pass on the merged tree, and the two tracks.* snapshot changes are covered by the idempotence assertion in the fmt harness — pl_to_prql output is re-parsed and re-formatted — so the unquoted wildcard is verified to round-trip rather than just asserted to.
`boolean`, `null` and `end_expr` are private fns, so linking them from a `pub const`'s docs trips `rustdoc::private_intra_doc_links`, which is an error under the workflow's `RUSTDOCFLAGS: -Dwarnings`. Plain code spans keep the pointer without breaking `cargo doc`.
prql-bot
left a comment
There was a problem hiding this comment.
b932806 applies the suggestion verbatim, and RUSTDOCFLAGS="-Dwarnings" cargo doc -p prqlc-parser --no-deps now exits clean on the merged tree — the private_intra_doc_links error from my previous review is gone. Nothing else outstanding from either review; I can't approve my own PR, so this comment is the close-out.
fmtemitted source that no longer round-trips for names that collide with reserved words. Two distinct causes, both now fixed: the codegen kept its own hand-written copy of the keyword list (two entries behind the lexer's, missingimportandenum), and expression-position identifiers bypassed the quoting logic entirely by going throughpr::Ident'sDisplayimpl — a fifth copy of the "needs backticks" rule that knows nothing about reserved words.#6200 fixed this class of bug for names that aren't valid bare idents; these slipped through because the lists and code paths
write_ident_partsits behind were already inconsistent. #6210 gave thegrammars/files and the playground a "keep in sync with the lexer" comment for the same reason — this removes the need for one on the codegen side by deleting the duplicate.Details
Three bugs, in ascending order of nastiness:
Same for
enum. Cause:write_ident_partconsulted a localkeywords()copy of the list that had drifted.Reserved literals —
true/false/nullare reserved just as firmly (boolean()andnull()both end withend_expr()), but lex asTokenKind::Literalrather thanTokenKind::Keyword, so they were absent from the keyword list entirely.Expression position — silent change of meaning, since the output still parses:
SELECT "true", "null"before the round-trip,SELECT true, NULLafter. This one affected every reserved word, includingletand the others already in the list —ExprKind::Identwrote viaident.to_string(), andpr::Ident'sDisplayhas its own quoting rule that only checks character validity.Changes:
prqlc/prqlc-parser/src/lexer/mod.rs— extractpub const KEYWORDS: [&str; 10], add the siblingpub const RESERVED_LITERALS: [&str; 3];keyword()now builds its parser from the former viachoice(KEYWORDS.map(just)).prqlc/prqlc/src/codegen/ast.rs— delete the localkeywords()OnceLock<HashSet>, read both lexer lists directly. A linear scan of 13&strbeats hashing, so this drops theHashSetimport along with the duplication.ExprKind::Identnow goes through a sharedwrite_identhelper instead ofDisplay.web/book/src/reference/syntax/keywords.md— the documented list had drifted the same two entries; addsenumandimport. (It already listedtrue/false/nullcorrectly — the docs were ahead of the code there.)Tests:
test_every_reserved_word_is_quotediteratesKEYWORDS.iter().chain(RESERVED_LITERALS.iter())and checks both declaration and expression position, so a word added to either lexer list in future is covered without touching the test.test_reserved_literals_lex_as_literalsin the parser pinsRESERVED_LITERALSto actual lexer behaviour, since it's hand-written next toboolean()/null()rather than derived.Snapshot changes: two
fmtintegration snapshots change fromtracks.+ backtick-star totracks.*. That's the sameDisplay-vs-write_ident_partdivergence in the other direction —Displaywas over-quoting*, whichvalid_prql_ident()explicitly allows. Both forms compile to identical SQL, and the new output matches the.prqlsource.Verification:
cargo test -p prqlc -p prqlc-parserpasses;cargo clippy --all-targetsandcargo fmt --checkare clean.task prqlc:pull-requestcouldn't run locally —cargo-instaisn't on the sandbox's PATH, which is what #6144 addresses — so CI is the first full-matrix run.