Repository navigation
feat(analyser): port new rules from Squawk - #885
Merged
Merged
Conversation
Ports the Squawk rules that have no counterpart in our analyser yet: - correctness: banDuplicateColumnAssignments, banUncommittedTransaction, identifierTooLong - destructive: banAlterIdentity, banDropConstraint, banDropDefault, banDropDomain, banDropExtension, banDropFunction, banDropGeneratedExpression, banDropSequence, banDropType, banDropView, banSetSchema, renamingObject - safety: banAlterDomainWithAddConstraint, banCreateDomainWithConstraint - style: requireEnumValueOrdering, requireTableSchema The rule context now exposes the statement text, since the parser truncates long identifiers before identifierTooLong could see them, and pgls_catalog exports its FigureColname port for view column names.
psteinroe
added a commit
that referenced
this pull request
Oct 22, 2026
Makes the repo easier for coding agents to work with, based on the friction from porting the Squawk rules in #885. ## AGENTS.md - **Running commands as an agent:** builds fail with `Unable to find libclang` outside the Nix shell, and in a non-interactive shell `nix develop` alone just opens a subshell. The new section shows how to wrap commands with `nix develop -c ...`, warns that the first run takes minutes, says to run `bun install` before `just format`/`just gen-lint` (otherwise `oxfmt` is missing and the generated TS stays unformatted), and notes that `sqlx::test` tests need the docker database. - **Lint rules:** scaffolding several rules without running the whole of `gen-lint` for each one, `version: "next"`, how spec expectations are counted, the one-diagnostic rule for doc examples, the Postgres 17 libpg_query bindings, identifiers truncated to 63 bytes in the AST, `ctx.stmt().iter()` for nested nodes, and how to dump an AST. - **Snapshot testing:** `INSTA_UPDATE=always` to accept snapshots without the interactive review. ## CONTRIBUTING.md The spec example still used `-- expect_lint/<group>/<ruleName>`, which no longer matches since the flat rule ids (#815). It now uses `-- expect_lint/<ruleName>` and explains that only the number of diagnostics per rule is checked.
This was referenced Oct 23, 2026
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
Three follow-ups to `identifierTooLong` from #885 (recommended, so it runs on every statement). **Cost** The rule ran `pgls_query::scan` on every statement, because the parser has already truncated the names in the AST. Scanning costs about as much as parsing, and the rule was about 84% of the cost of all rules ported in #885. It now skips the scan unless the statement could contain a long identifier: a run of more than 63 bytes that can be part of an unquoted identifier (`[A-Za-z0-9_$]` or a non-ASCII byte), or more than 63 bytes between two quotes. A name is never longer than its text (downcasing keeps the length, `""` and Unicode escapes are longer than what they stand for), so the check can't miss one. Quotes in strings or comments can't hide a quoted identifier either: inside one, quotes come in pairs, so only an odd run of quotes counts as a boundary. Measured on a debug build as on-CPU thread time (minimum of 5 runs; the machine was busy, so wall-clock is meaningless): | | before | after | |---|---|---| | scan step over the PG 15–18 regression corpus (172,586 statements) | 2.36 s | 0.28 s (1,850 statements, 1.1%, still get scanned) | | rule over the PG 18 corpus, minus the no-rule baseline (46,158 statements) | +0.93 s | +0.19 s or less, within noise | | rule on a synthetic 10k-statement migration, minus the baseline | +279 ms | ~0 ms | For comparison, 6.3% of the corpus statements contain a `"`, so "skip unless there's a quote" would have scanned six times as many. **`U&"..."` identifiers** Only `IDENT` tokens were checked, so this was silent although Postgres prints a truncation notice: ```sql create table U&"aaaa…(64 bytes)…" (id int); create table U&"x!0061!0061…(80 escapes)" uescape '!' (id int); ``` `UIDENT` tokens are now decoded like `str_udeescape` does (4- and 6-digit escapes, surrogate pairs, doubled escape characters, and the `UESCAPE` character as `check_uescapechar` allows it). Identifiers whose escapes Postgres rejects are skipped. Each case in the spec was checked against Postgres for the NOTICE. **One diagnostic per name** A long table name used three times in a statement gave three identical diagnostics. Each distinct name is now reported once per statement, at its first occurrence. Unquoted names are compared, and shown, downcased like Postgres does (ASCII only), so `Long_Name…` and `long_name…` are the same name. **Tests** Spec files `edge_cases.sql` (multi-byte names, doubled quotes, strings/comments/dollar-quoted bodies, a quote in a string next to a long quoted identifier with spaces, the repeated name) and `unicode_escapes.sql`; unit tests for the pre-filter (including the cases that must pass it) and the `U&` decoding. **Not covered** libpg_query (PG 17) fails to parse a comment between `U&"..."` and `UESCAPE`, which Postgres accepts, so such a statement is reported as a syntax error and never reaches the rule.
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
…like Postgres (#891) The column names we give unnamed select list entries (our port of Postgres' `FigureColname`) were wrong in three places, and both `banDuplicateColumnAssignments` (new in #885, recommended) and `unknownColumn` trust them: - An unaliased scalar subquery was named `?column?`. Postgres transforms the subquery first and takes the name of its column, so `(select max(a) from t)` is `max`. - `(t.c).arr[1]` was named after `c`, because only the last indirection item was looked at. Postgres takes the last field name anywhere in the list: `arr`. - Every `CASE` was named `case`. Postgres uses the name of the ELSE result if that is a good name, so `case when ... then 1 else a end` is `a`. So valid migrations failed the linter: ```sql create view max_and_min as select (select max(a) from t), (select min(a) from t); -- before: × Column ?column? is specified more than once. -- after: nothing (Postgres names the columns max and min) create view v as select (t.c).arr[1], c from t; -- before: × Column c is specified more than once. create view va as select (select max(a) from t); select max from va; -- before: × Column max does not exist. ``` The casts of subqueries follow from this: `(select max(a) from t)::text` keeps `max`, and `(select 1)::text` stays `?column?` (the subquery's name is a strong name, so the cast doesn't replace it with `text`). **What changed** - `EXPR_SUBLINK` takes the name of the first column of the subquery: the alias of the first entry of its left-most select, otherwise the name figured for its value, `column1` for VALUES, and unknown for a `*` (it expands to a column of the relation). The name is strong, as in Postgres. - `ColumnRef` and `A_Indirection` take the last field name in the list, ignoring `*` and subscripts, and an indirection without one falls back to its argument. - `CASE` ports the ELSE handling. - `MULTIEXPR_SUBLINK` has no name, as in Postgres (it only appears in `UPDATE ... SET (a, b) = (select ...)`). - `figure_column_name` returns `None` for a top-level `*`, instead of `?column?` for `*`/`t.*`. The resolver already expanded those itself; `output_name` (ORDER BY / GROUP BY references) no longer gets a bogus `?column?` for them. Every name was checked against Postgres 15 (`create view` + `pg_attribute`), and the three duplicates in the new spec are rejected by Postgres with 42701. `banDuplicateColumnAssignments` now also runs in `postgres_regress.rs`, so any finding on a statement Postgres accepted fails CI on every version. The corpus is clean; its three findings are on rejected statements (`update.sql` 42601, `create_table.sql` and `typed_table.sql` 42701) and show up in the per-version snapshots. The rule docs said `insert into t (a, a)` fails with `42601`. Postgres raises `42701 duplicate_column` (`column "a" specified more than once`); only duplicate SET assignments are `42601`. **Tests** Spec files `banDuplicateColumnAssignments/unnamed_columns.sql` (accepted views, `CREATE TABLE AS`, `SELECT INTO` with subqueries, set operations, VALUES, casts, field subscripts, CASE), `unnamed_duplicates.sql` (the cases Postgres does reject), and `unknownColumn/unnamed_view_columns.sql`. Regress snapshots for 15–18 gain the three true positives. **Not covered** The subquery's first column is unknown if it is a `*` (we'd need the relation's first column), so that case reports nothing.
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
…quawk (#905) Stacked on #891. Follow-ups to the rules ported from Squawk in #885: where the diagnostics point, and what they cover. Every new error case was checked against Postgres 15 in a rolled-back transaction. **banDuplicateColumnAssignments** - Diagnostics pointed at the whole statement. They now point at the second occurrence: the column name in SET, INSERT and column definitions, and the whole select list entry (e.g. `coalesce(a, b) as x`) in views, `CREATE TABLE AS` and `SELECT INTO`. View column lists and COPY column lists have no locations and still point at the statement. - A `*` anywhere in a view's select list turned the check off. It now only hides the names it may expand to: ```sql create view v as select t.*, 1 as x, 2 as x from t; -- before: nothing -- after: × Column x is specified more than once. (Postgres: 42701) ``` If the view's explicit column names reach into the `*`, we still can't tell what the later entries are called, so `create view v (a, c, d) as select t.*, 2 as x, 3 as x` stays silent (Postgres accepts it when `t` has two columns). - New statements, all rejected by Postgres with 42701: `alter table users add column email text, add column email text` (also after `ADD COLUMN IF NOT EXISTS email`; a later `IF NOT EXISTS` is skipped by Postgres and not reported), `create type pair as (a int, a int)`, `copy users (id, id)`, and `create foreign table ... (id int, id int)`. ADD COLUMN and COPY have their own detail text. **requireTableSchema** (opt-in) - Points at the table name, and the note uses it (`e.g. public.posts` instead of `public.my_table`). `DROP TABLE a, b` points at each name. - Temporary tables created earlier in the file (`create temp table`, `create table pg_temp.x`, `select ... into temp x`) are no longer reported on later `ALTER`/`DROP`/`CREATE INDEX`, until they are dropped. An unqualified name refers to them since `pg_temp` comes first in the search path. Squawk reports these too. - Scope: statements that create, change or drop a table, and the tables they reference by name. Newly covered: `CREATE FOREIGN TABLE`, `ALTER/DROP FOREIGN TABLE`, `ALTER TABLE ... RENAME` (table, column, constraint), `SET SCHEMA`, `SELECT INTO`, `CREATE INDEX ON`, and inside those statements `REFERENCES`, `PARTITION OF`, `INHERITS`, `LIKE`, `ATTACH PARTITION`. Queries (the body of a view, `CREATE TABLE AS ... SELECT ... FROM x`, DML) stay out of scope, otherwise the rule would fire on every query. This is wider than Squawk, which only checks CREATE/ALTER/DROP TABLE; the rule is opt-in, and a user who enables it wants every table name in DDL qualified. **banDropConstraint, banDropDefault** (opt-in) One diagnostic per statement without a name became one per command, with the name, pointing at its `DROP CONSTRAINT` / `DROP DEFAULT`: ```sql alter table users drop constraint users_email_key, drop constraint if exists users_name_check; -- before: ! Dropping a constraint may break existing clients. -- after: ! Dropping constraint users_email_key may break existing clients. -- ! Dropping constraint users_name_check may break existing clients. ``` **renamingObject** (opt-in) Most of its findings on the regression corpus are `ALTER INDEX ... RENAME` and `RENAME CONSTRAINT`, which Squawk's `renaming-object` reports as well, so they stay. We also report collations, text search configurations, FDWs and other objects Squawk doesn't. I kept those: clients do reference collations and text search configurations by name, and the rule is opt-in. The one change is `ALTER TYPE ... RENAME ATTRIBUTE`, which moves to `renamingColumn` like in Squawk (an attribute is a column of a composite type). **requireEnumValueOrdering** now has `applies_to: Migration`, like the other DDL rules from #885. **Tests** New spec files for the new duplicate cases (and the accepted ones next to them), the new requireTableSchema positions and temporary tables, multiple commands for banDropConstraint/banDropDefault, and RENAME ATTRIBUTE. The spec snapshots don't show spans, so there are unit tests per rule asserting the text each diagnostic points at, using a small test helper `spanned_texts` in `lib.rs`. `StatementTokens` gets a `keyword_pairs` helper for the `DROP CONSTRAINT` / `DROP DEFAULT` spans. **Not covered** Duplicates that need the catalog (`ADD COLUMN` of an existing column, `LIKE t` plus a column of `t`) and view column lists / COPY column lists still point at the statement.
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
…ction and banDuplicateColumnAssignments (#909) Stacked on #905, and merges #897 (both change `identifierTooLong`), so until #897 lands the diff here includes it; the change of this PR is the last commit. Three of the rules ported from Squawk in #885 get fixes, using the fix API from #859. Squawk has fixes for these; `requireEnumValueOrdering`'s Squawk fix inserts a placeholder that doesn't run, so it isn't ported. **identifierTooLong: safe** The fix spells out the name Postgres already uses: the identifier truncated to 63 bytes at a character boundary, the way Postgres truncates it. Since #897 reports each name once per statement, the fix rewrites every identifier of the statement that stands for the name: ```sql select aaaa…(64).id from aaaa…(64) where AAAA…(64).id = 1; -- fix: select aaaa…(63).id from aaaa…(63) where AAAA…(63).id = 1; ``` Unquoted identifiers keep their case as written, quoted ones are re-quoted from the truncated name (so a `""` cut in half can't break the quoting). `U&"..."` identifiers are left alone, because rewriting them would leave a `UESCAPE` clause behind. The report proposed making it unsafe for quoted names. I made it safe for all: the statement does exactly the same afterwards, quoted or not, which is what `FixKind::Safe` means. Other statements that use the long name get their own diagnostic and fix, and leaving them as they are is still equivalent. The fix doesn't pick a good short name, that's still up to the user (the note says so). **banUncommittedTransaction: unsafe** Adds `commit;` on a new line after the last statement of the file, or of the migration section the `BEGIN` is in: ```sql -- section: up transaction:false begin; create table users (id bigint); commit; -- added -- section: down transaction:false ``` Unsafe because it changes what the migration does (its changes are now applied), and whether `COMMIT` belongs at the end, or a `ROLLBACK` was meant, is a guess. To know where the file or section ends, `AnalysedFileContext` now knows where its statements start and where the last one ends, including statements that don't parse, via `end_offset()`, relative to the current statement like other fix edits. **banDuplicateColumnAssignments: unsafe** Keeps the last assignment of a SET clause (UPDATE, ON CONFLICT DO UPDATE, MERGE) and removes the earlier ones with their commas. Only if every assignment to the column is a plain `col = value`: no fix for INSERT column lists (the values would have to move too), `(a, b) = ...` assignments, or subscripts and fields. Unsafe because we can't tell which value was meant. **Tests** Every spec snapshot of the three rules now shows the fixed SQL, and the test harness checks it still parses. New specs: `banDuplicateColumnAssignments/fixes.sql` (comments between assignments, ON CONFLICT, and the cases without a fix), `banUncommittedTransaction/fix_in_section.sql`. **Risks** The fixes are plain token edits, independent of the fix infrastructure changes in flight. One interaction to watch: #906 checks each fix by applying it to the statement text and re-parsing. The COMMIT fix's edit lies past the end of the `BEGIN` statement (at the end of the file or section), so `RuleFix::apply` on the statement text would slice out of range. Whichever of the two lands second has to handle edits outside the statement, e.g. skip the re-parse check for them or apply the fix to the file text. **Follow-ups:** #906 keeps fixes that edit past the end of their statement without re-parsing them, so the `commit;` fix works 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.
Ports the Squawk rules added since our last sync (#508) that have no counterpart in our analyser. Squawk rules we already cover under another name (
ban-drop-schema,ban-drop-trigger,require-concurrent-reindex,require-concurrent-partition-detach,require-lock-timeout,require-statement-timeout,ban-disable-trigger,prefer-repack) are left alone, and existing rules are unchanged.New rules
banDuplicateColumnAssignmentsUPDATE,INSERT,ON CONFLICT,MERGE, including CTEs) or declared twice (CREATE TABLE, views,CREATE TABLE AS,SELECT INTO). Postgres rejects all of these. View column names come from ourFigureColnameport.banUncommittedTransactionBEGIN(orCOMMIT AND CHAIN) that is never committed or rolled back. Silent in sections the migration tool runs in a transaction.identifierTooLongbanDropConstraintDROP CONSTRAINTon tables and domainsbanDropDefaultDROP DEFAULTon columns, view columns and domainsbanDropDomain/banDropExtension/banDropSequenceDROPbanDropFunctionDROP FUNCTION/PROCEDURE/ROUTINE/AGGREGATEbanDropTypeDROP TYPE/CAST/OPERATOR/OPERATOR CLASS/OPERATOR FAMILYbanDropViewDROP VIEWandDROP MATERIALIZED VIEWbanDropGeneratedExpressionALTER COLUMN ... DROP EXPRESSIONbanAlterIdentityADD GENERATED ALWAYS,DROP IDENTITY,SET GENERATED ALWAYS. Unlike Squawk,BY DEFAULTis not reported because it keeps both kinds of inserts working.banSetSchemaALTER ... SET SCHEMAfor any objectrenamingObjectrenamingTable/renamingColumncover those), plusRENAME VALUEon enumsbanCreateDomainWithConstraintCREATE DOMAINwith a constraint (DEFAULT/NULLare not counted)banAlterDomainWithAddConstraintALTER DOMAIN ... ADD CONSTRAINTrequireEnumValueOrderingADD VALUEwithoutBEFORE/AFTERrequireTableSchemaCREATE TABLE,CREATE TABLE AS,ALTER TABLE,DROP TABLE(temp tables are fine)Squawk enables most of the destructive rules by default. Here they're opt-in, like
renamingTableandchangingColumnType, so upgrading doesn't suddenly flag everyDROP FUNCTION. Only the three correctness rules are recommended, since each one flags a statement that fails or does something you didn't intend.Supporting changes
LinterRuleContext::sql()exposes the statement text. The parser has already truncated long identifiers in the AST, soidentifierTooLongreads the scanner tokens instead.pgls_catalog::resolve::figure_column_nameis now public.agentic/port_squawk_rules.mdis updated.Not ported
ban-add-column,ban-revoke,ban-drop-index,ban-set-default,ban-create-policy,ban-alter-*-options, …). They flag almost every schema change and seem too noisy to be worth it.NOT ENFORCEDinban-drop-constraint: our libpg_query bindings don't have Postgres 18's enforceability yet.Testing
cargo test -p pgls_analyser --test rules_testspasses (213 tests).pgls_analyser,pgls_configurationandpgls_workspacetests pass.cargo clippy --all-targets --all-featuresis clean,just gen-lintandrules_checkpass, and formatting checks pass.