fix: keep backticks on names containing $ in fmt - #6322
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
Two points, neither in the regex change itself.
No changelog entry. development.md asks for a CHANGELOG.md line on user-facing changes, and this is one — the sibling fmt fix #6311 has an entry in the same [unreleased] block. Pushing one to this branch.
The description's SQL claim only holds with --no-format. It says `a$b` "still compiles to SELECT a$b FROM t, which is correct for the dialects that allow it". The default output path splits the $:
$ printf 'from t\nfilter `a$b` == 1\n' | prqlc compile --hide-signature-comment
SELECT
*
FROM
t
WHERE
a $b = 1a $b is a different identifier from the one that was written, and it isn't valid in the dialects that accept a$b either. This is sqlformat splitting the token rather than anything in this diff — same class as #3188 — but the description asserts the opposite of what a user of the default path sees, so I'll correct it there.
Worth flagging separately, since it's adjacent and currently untracked: the parameter half of this was fixed ($sd renders intact now, via #3077), but an identifier containing $ still splits, and #3286 was closed as a duplicate of #3077 so nothing open covers it. Closing it on the PRQL side would mean dropping $ from utils::valid_ident() as well, so these names get dialect-quoted as "a$b" — that changes generated SQL for anyone using $ in identifiers on Postgres today, which is a maintainer call rather than a follow-on to this fix.
prqlc fmtdrops the backticks around any name containing$, and the result no longer lexes as the same name —$starts a$paramtoken in PRQL, so the formatter turns one identifier into an identifier followed by a parameter.This is the same round-trip break as #6200 and #6214, at the one remaining hole in the rule those two consolidated.
valid_prql_ident()accepts$in both leading and subsequent positions because its regex was copied from the SQL-sideutils::valid_ident(), where$genuinely is a Postgres identifier character. The PRQL lexer'sident_part()never accepts it. Dropping$from the codegen regex is the whole fix; params are written separately asformat!("${id}")and don't go through this path.This change doesn't move the SQL:
`a$b`compiles toSELECT a$b FROM tbefore and after it. Worth knowing when reading that claim, though — the default output path renders it asSELECT a $b FROM t, becausesqlformatsplits the$the same way it does in #3188, so the SQL a user actually sees names a different identifier and isn't valid in the dialects that accepta$b. That's a pre-existing bug independent offmt, and it currently has no open tracker: #3286 was closed as a duplicate of #3077, whose fix covered$parambut not an identifier containing$.Failure modes and verification
Three shapes, all now formatted with backticks intact:
select {`a$b`}select {a$b}aapplied to$bselect {x = t.`$foo`}select {x = t.$foo}$foowhere an ident part is expectedlet `$foo` = 5let $foo = 5$foowhere a declaration name is expectedThe regression test is
codegen::ast::test::test_quoted_dollar_in_name, covering all three.assert_is_formattedround-trips through the parser, so each case asserts the formatted output re-parses to the same source. Restoring$to the character class while keeping the test fails it:No snapshot moved — nothing in the repo's test corpus uses a
$in an identifier.task prqlc:pull-requestruns 763 tests; 762 pass. The one failure isqueries::results::read_csv, which needs to download DuckDB'sjsonextension and can't reachextensions.duckdb.orgfrom this sandbox — unrelated to this change.cargo fmt --checkandcargo clippy -p prqlc --all-targets --no-default-features --features=default,lsp -- -D warningsare both clean.One related divergence I did not change:
display_ident_part()inprqlc-parserhas the same$-at-start hole, as the other surviving copy of the "needs backticks" rule. Since #6214 routedExprKind::Identthroughwrite_ident_part, that copy only backsIdent'sDisplayimpl — error messages andDebugoutput, which nothing re-parses — so there's no round-trip to break there. Worth folding intowrite_ident_parteventually, but that's a separate change with its own snapshot churn.