Repository navigation
fix(pretty_print): keep long identifiers as written - #901
Merged
Merged
Conversation
…functions break sensibly
…print-grant-admin-option
…int-long-identifiers
psteinroe
changed the base branch from
fix/pretty-print-grant-admin-option
to
main
October 26, 2026 08:05
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
Stacked on #901. The snapshot tests render each fixture only with the options in its header, and the regression corpus test only runs the defaults. Bugs that need two options together got through both. Leading commas in fit layout (`select a , b`, fixed in #890) are one example. The release review found them by formatting everything under 288 option combinations. This PR lands a trimmed version of that harness, so the option matrix runs whenever the formatter changes. **What the test does** `tests/option_matrix.rs` formats two corpora under 32 configurations: every combination of `layout`, `clauseBodyStyle`, `logicalOperatorPlacement`, `commaStyle` and `valuesRowLayout`. Across those 32, indent (2 spaces, 4 spaces, tabs) and line width (40, 80, 120) cycle through all nine of their pairs. Upper-case keywords and constants come with width 120, `castStyle: operator` with tabs, and `isolateSemicolon` with width 40. The two corpora: - every statement of the snapshot fixtures, without their headers; - every statement Postgres 15–17 accepted in its regression suite. Each output must: - round-trip, i.e. `format_statement` doesn't refuse it as changing the AST; - format to itself again; - pass the whitespace check from #890: no space after `(` or before `)` and `,`, no double space, no trailing whitespace, no blank line; - not mix indentation: no tab when indenting with spaces, no space before a tab when indenting with tabs. Lines inside function bodies keep their source indentation and are skipped. Statements refused because a comment can't be placed (a comment after the terminator) are skipped; that refusal is by design. Line width isn't checked. At width 40 there are known statements the formatter can't fit, mostly node types listed as not covered in #898. The snapshot tests already check width at 80 and 100. **Where it runs** Both corpora take about 35 seconds each in release on 8 cores. In debug, the fixtures alone take about three minutes. Adding that to `cargo test --workspace` would slow the Test job on all three platforms. The two tests are therefore `#[ignore]`d. A new `Formatter Option Matrix` job in `pull_request.yml` runs them once, in release on Ubuntu, when `pgls_pretty_print`, its codegen, `pgls_query`, the regression fixtures, `Cargo.lock` or the workflow change. I picked a path-filtered PR job over a nightly run so a regression fails the PR that introduces it. PRs that don't touch the formatter don't pay for it. A third test, not ignored, checks that the matrix covers all 32 combinations and all nine indent/width pairs. On this stack, all 2.1 million formats pass. On `main`, the same test fails on the bugs fixed in #890–#901. **New fixtures** The review's test-gap list (P21) asked for these. VALUES with ORDER BY/LIMIT and short leading-comma lists came in #893 and #890: - `aggregate_filter.sql`: `count(*) filter`, two filtered aggregates in one expression, `within group` + `filter`, and `filter` + `over`. - `expanded_case_alignment.sql`: expanded CASE with `indentSize: 4` after `select` and after `on`, a broken WHEN condition, and a CASE nested in THEN. **Not covered** - The full 288-config matrix and the width check stay out. They are useful for a release review but too noisy and slow for CI. - The snapshot width check still excuses lines whose longest token plus 20 columns fits. Tightening it would need a pass over every fixture, so I left it for later. **Follow-ups:** The snapshot width check is tightened in #950.
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.
Stacked on #900.
Postgres keeps the first 63 bytes of an identifier (
NAMEDATALEN - 1), and so does the AST the formatter works from. A longer name was therefore rewritten to its truncated form:A quoted name could even end up cut in the middle, with a space before the closing quote. Postgres truncates both spellings the same way, so the meaning didn't change, but
docs/features/formatting.mdsays the formatter doesn't change what is written.What changed
format_statementnow scans the statement for identifier tokens longer than 63 bytes, and maps the name the parser cut each one down to back to the name as written: dequoted for a quoted identifier, lower-cased for an unquoted one. The truncation clips to whole characters liketruncate_identifier, so a multibyte character at the cut is handled. The identifier emitters (emit_identifier,emit_identifier_maybe_quoted,emit_type_identifier_maybe_quoted) look names up there and write the full name, quoted where needed as before. The AST check still passes, because the output parses back to the same truncated names.A name is only restored when that is unambiguous. If two spellings are cut to the same 63 bytes, or a name is written both in full and already truncated, the AST name is kept as is.
Tests
identifiers.rs: unquoted and quoted names, a multibyte character at the cut, and the two ambiguous cases.identifiers_longer_than_postgres_keeps_are_written_in_fullinsource_fidelity.rscovers a role, a quoted column and a table.No snapshot changes. The snapshot harness drives the emitter directly without the source, and no fixture has an identifier this long.
Not included: deeply nested statements (finding P22)
About 40 nested CASEs, 20 nested subqueries or 45 levels of AND/OR still fail with "recursion limit reached". This limit is prost's decode limit, a constant 100 in prost 0.13. The only way to change it is the
no-recursion-limitfeature, which removes the limit for every prost user in the build. Without a limit, hostile input would hit stack overflows, which abort the process, in the decoder and in the recursive AST walkers (formatter,iter, normalisation) instead of returning an error. Doing that safely needs a bounded decoder or parsing on a thread with a larger stack. That isn't a small change, so I left it out of this PR.