Repository navigation
test(pretty_print): fuzz the option matrix in CI - #912
Merged
Merged
Conversation
…functions break sensibly
…print-grant-admin-option
…int-long-identifiers
This was referenced Oct 24, 2026
psteinroe
changed the base branch from
fix/pretty-print-long-identifiers
to
main
October 26, 2026 08:54
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
Stacked on #912. Fixes formatter options and `format` check output that were hard to use, found in the release review. **Option errors list the valid values** ```sh $ postgres-language-server format --clause-body-style=foo a.sql # before Error: couldn't parse `foo`: Value not supported for clause body style # after Error: couldn't parse `foo`: expected 'break' or 'compact' $ postgres-language-server format --values-row-layout=foo a.sql # before Error: couldn't parse `foo`: Value not supported for Layout. Use 'fit' or 'expanded'. # after Error: couldn't parse `foo`: expected 'fit' or 'expanded' ``` All formatter enums now say what they expect, without naming the Rust type. `Layout` is shared by `--layout` and `--values-row-layout`, and the user never sees that name. **A zero line width or indent size is rejected** `--line-width=0` and `--indent-size=0`, and `"lineWidth": 0` / `"indentSize": 0` in the config file, were accepted. `lineWidth` and `indentSize` are now `NonZeroU16` / `NonZeroU8`, as `files.maxSize` already is. The CLI says `couldn't parse '0': number would be zero for non-zero type`, the config file `invalid value: integer '0', expected a nonzero u16`. The JSON schema now has `minimum: 1` for both. **Check output names the line** Without `--write`, `format` printed one diagnostic per statement that would change. When a statement's diff fits on one line, it is printed without line numbers, and the header only names the file. In a large file there was no way to find the statement. The message now carries the line: ``` a.sql format ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ × Statement at line 4 could be formatted using `--write` - SELECT··x·FROM·y·WHERE·z=1; + select·x·from·y·where·z·=·1; ``` I put the line in the message rather than giving the diagnostic a span. With a span, the printer adds a code frame of the statement above the diff. When only the blank lines between statements change, the whole-file diff now says "File could be formatted" instead of "Statement". **Help shows the values** Every format flag was shown as `=ARG` in `--help` and in the CLI reference. They now show `NUMBER`, `<spaces|tabs>`, `<fit|expanded>` and so on, like the other options. **Docs** The `layout` option was described as "always breaks between clauses". Expanded layout also puts every select list item and every AND/OR operand on its own line and breaks before every CASE branch. A select-list-only subquery and a CHECK condition follow the line width instead. The option's description (and so the schema, bindings and CLI reference) and `docs/features/formatting.md` now say this, and the docs have an example. Together with the CASE and tabs docs in #894, this covers the docs gaps from the review. **Tests** - CLI tests for the two option errors and for rejecting zero, with the two check-output snapshots updated for the new messages. - A configuration test for rejecting zero in the config file. - Schema, TypeScript bindings and CLI reference regenerated with `just gen-lint`. **Risks** A config file with `lineWidth` or `indentSize` set to 0 used to load and now fails to load with the error above. With either set to 0 the formatter output was unusable anyway: every group broke, or nothing was indented.
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
…ace (#934) Stacked on #917. Comments written before a keyword that has no AST node of its own (`ELSE`, `THEN`, `END`, GRANT's `ON`/`TO`/`FROM`) moved past the keyword, sometimes into another branch or clause. The output was stable on reformat, so nothing flagged it, but the comment no longer sat where it was written. Finding P17 of the release review. ```sql select case -- why when a then 'x' /* inline */ else 'y' end from t; -- before select case when a then 'x' else /* inline */ 'y' -- why end from t; -- after select case -- why when a then 'x' /* inline */ else 'y' end from t; select case when a /* cond */ then 1 else 2 /* last */ end from t; -- before select case when a then /* cond */ 1 else 2 end from /* last */ t; -- after select case when a /* cond */ then 1 else 2 /* last */ end from t; grant select -- c on t to u; -- before grant select on table -- c t to u; -- after grant select -- c on table t to u; ``` **Why they moved** Comments are attached to AST nodes by source position, and a comment that leads goes to the next node: a block comment, an own-line comment, or a comment after a clause keyword. For `'x' /* inline */ else 'y'` the next node is `'y'`, so the comment came out after `else`. A comment right after `case` didn't count as following a clause keyword, so it trailed the whole CASE and came out after the last branch. A comment before `end` led the next node after the CASE, in the FROM clause. **What changed** - When the token after a comment is a keyword that no node starts at, the comment remembers that keyword (`before_keyword`). A parent that writes such a keyword before a child can take the comments written before it, with `emit_comments_before_keyword`, and emit them before the keyword. They are emitted like the trailing comments of the previous node, so an own-line comment keeps its own line. Only comments recorded for that exact keyword are taken. A parent that doesn't call it keeps today's placement, so nothing else moves. - CASE takes them before `ELSE` and `THEN`. GRANT/REVOKE takes them before `ON` and `TO`/`FROM`, and GRANT role before `TO`/`FROM`. - A comment before `END` stays with the last branch: it trails the previous node instead of leading the next one, because nothing inside the CASE follows `END`. - `CASE` counts as a clause keyword, so a line comment right after it goes before the first `WHEN`. - The renderer collapses two spaces in a row inside a group that stays on one line, as it already did for broken groups. The comment emission otherwise gave `a /* cond */ then`. **Tests** - New tests in `comment_layout.rs` cover a comment before `ELSE`, `THEN` and `END`, after `CASE`, and before GRANT's `ON` and `TO`, in fit and expanded layout, each formatted twice to check idempotence. - Two unit tests in `comments.rs` cover the attachment. - No snapshot changes: the snapshot harness renders without comments. - The option matrix from #912 passes over the fixtures and the regression suite. **Not covered** Other keyword-only positions (`ON` in a join, `AS` before an alias, `FROM` in a SELECT, ...) keep the old placement. They can opt in the same way when needed.
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
Stacked on #934. In expanded layout, a CASE nested in the THEN of another CASE moved right by `then ` plus two indent levels per level of nesting. #884 aligns each CASE's branches with the column it starts at, and a nested CASE starts after `then `. With the default indent, twenty levels reached 184 columns at width 100. Finding P16 of the release review. **What changed** A CASE written as the result of a `THEN` or `ELSE` no longer aligns with its own column. It is indented from the line of its keyword: its branches one level in, and `END` under `THEN` or `ELSE`. Each level now costs two indents, the minimum for a WHEN and its THEN: ```sql -- layout: expanded, indentSize: 4 -- before case status when 'A' then case when enabled then 'active' else 'paused' end else 'closed' end -- after case status when 'A' then case when enabled then 'active' else 'paused' end else 'closed' end ``` `else case when ... else case ...` chains read the same way. A CASE anywhere else, after `select` or a leading comma for instance, still aligns with the column it starts at, which is what #884 was about. Fit layout doesn't align, so it is unchanged. This changes the nested layout that #884's `expanded_nested_case_keeps_each_branch_at_its_own_indent` test asserted, and the test now expects the new layout. I preferred this over breaking after `then` for every nested CASE: that would add a line and a level for every nested CASE, also the shallow ones that fit today. The docs section on expanded CASE describes the nested layout. **Tests and snapshot changes** - `case_layout.rs`: the updated nested test, a CASE nested in ELSE, and twenty levels nested in THEN staying within width 100. - The only snapshot change is the nested CASE in `expanded_case_alignment` (added in #912), whose inner branches move left to the new indent. - The option matrix passes over the fixtures and the regression suite.
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
Stacked on #946. #912 listed this as not covered. The snapshot width check (`assert_line_lengths` in `tests.rs`) didn't catch lines past the width in two common cases: - It only counted the characters between string literals. A call like `json_value(cast('{"a": ...}' as jsonb), '$.a.b'` of 95 columns passed at width 80, because most of it was inside quotes. - It excused a line whose longest identifier plus 20 columns didn't fit. That is how the `json_value` layout fixed in #898 got through. **The new check** `common::width_problems` replaces it. It uses the Postgres lexer and measures the whole line, with tabs counted as `indentSize` columns. A line is only excused when its indentation, its longest token (string literals included) and 12 more columns don't fit. That covers a long literal, a long quoted identifier, a function body, or code nested so deeply that there is no room left. A `--` comment ending the line is written as in the source and is not counted. The snapshot tests and the option matrix from #912 both use it. The matrix checks width only at 80 and 120 with `castStyle: cast`: at 40 some statements can't fit, and a `::` cast can't break either (`select '...'::int8` with a compact select list). **Formatter changes it needed** The stricter check found these, and they are fixed here: - **Text after a multi-line literal.** A group that contained a multi-line literal or function body stayed on one line as long as the literal's first line fit. Whatever followed its last line was never measured, e.g. a chain of `#-` operators running to 206 columns in `explain`. The renderer now checks that the text after the last line of such a token fits too. - **`is [not] distinct from`, `like`, `ilike`, `similar to`.** These now wrap their right-hand side like a comparison operator does, instead of always printing a space. - **SQL-syntax calls** (`extract`, `position`, `overlay`, `substring`, `xmlexists`). Their arguments are a group inside the call's parentheses, so `second from cast(...)` stays together on the indented line when it fits there, and `from` only breaks when it doesn't. **Snapshot changes** All reviewed: - text after a multi-line literal moves to the next line, or the call around the literal breaks (`explain`, `jsonb_jsonpath`, `misc_functions`, `tsearch`); - `is [not] distinct from` / `like` wrap before the right-hand side (`xid`, `txid`, `opr_sanity`, `strings`); - `extract(field from source)` breaks after `from` only when the arguments don't fit the indented line (`timetz`, `interval`). New tests in `width_layout.rs` cover the check itself, `is not distinct from`, `similar to`, and text after a multi-line literal. All fixtures pass the stricter check, and the option matrix passes with it.
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 #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.rsformats two corpora under 32 configurations: every combination oflayout,clauseBodyStyle,logicalOperatorPlacement,commaStyleandvaluesRowLayout. 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: operatorwith tabs, andisolateSemicolonwith width 40. The two corpora:Each output must:
format_statementdoesn't refuse it as changing the AST;(or before)and,, no double space, no trailing whitespace, no blank line;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 --workspacewould slow the Test job on all three platforms. The two tests are therefore#[ignore]d. A newFormatter Option Matrixjob inpull_request.ymlruns them once, in release on Ubuntu, whenpgls_pretty_print, its codegen,pgls_query, the regression fixtures,Cargo.lockor 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, andfilter+over.expanded_case_alignment.sql: expanded CASE withindentSize: 4afterselectand afteron, a broken WHEN condition, and a CASE nested in THEN.Not covered
Follow-ups: The snapshot width check is tightened in #950.