Repository navigation
fix(analyser): check fixes that edit after their statement on the file - #1044
Merged
Merged
Conversation
psteinroe
added a commit
that referenced
this pull request
Nov 2, 2026
…eters (#1037) Since #906, `Analyser::analyse` only keeps a fix if the fixed statement still parses. The check parsed the statement as written. The linter itself first converts placeholders to parameters (`pgls_lexer::convert_to_positional_params`, which the workspace runs before parsing each statement). So a statement with a psql variable never passed the check, and lost every fix: ```sql create index users_tenant_idx on :"schema".users (email) where tenant = :'tenant'; -- before: requireConcurrentIndexCreation is reported, but has no fix -- after: create index concurrently users_tenant_idx on :"schema".users (email) where tenant = :'tenant'; ``` `keeps_statement_valid` now parses the fixed statement with its placeholders converted, the same way the statement was parsed. The conversion keeps the text length, so the edits apply unchanged. The placeholders stay as written in the fixed file, because the fix is applied to the source, not to the converted text. `pgls_lexer` moves from a dev-dependency of `pgls_analyser` to a normal dependency. It depends only on the tokenizer and the text crates, so there's no cycle, and the WASM build already pulls it in through the workspace. The spec harness (`rules_tests.rs`) now parses statements the way the workspace does: placeholders converted, with `with_identifier_parameters` set. Its "a fix must keep the input valid" assertion uses the same conversion, so specs can contain psql variables. No existing snapshot changes. ## Tests - Unit `fixes_of_statements_with_placeholders_are_checked_with_parameters`: `:name`, `:'name'`, `$name` and `:"name"`. A valid fix is kept and a broken one is still dropped. - Specs `requireConcurrentIndexCreation/fix_psql_variables` (value placeholders in a `WHERE`) and `constraintMissingNotValid/fix_psql_variables` (an identifier placeholder as the schema). Both now show the fix. - Workspace `test_fix_file_fixes_statements_with_psql_variables`: `fix_file` adds `CONCURRENTLY` and keeps `:"schema"`, `:'tenant'` and `$plan` as written. - All three fail with the old check (verified by reverting the one-line change). - `cargo test -p pgls_analyser -p pgls_workspace` (including the Postgres regression test), `cargo clippy -p pgls_analyser -p pgls_workspace --all-targets -- -D warnings`. ## Not covered - goose `${...}` expansions only exist with #949, which isn't merged, so there's no test for them here. With #949, the document already replaces an expansion with a `$NAME` placeholder before parsing, so this change would let those statements get fixes again. #949 also drops fixes that edit inside an expansion, and its `test_goose_envsub_expansions_stay_as_written` expects no fixes at all. Whichever PR merges second needs to update that test and the line in the migrations guide. - Fixes with edits after the end of their statement are still not checked (L1). **Update:** merged main after #1044, which replaced `keeps_statement_valid` with `keeps_sql_valid`. Both of its paths now parse with placeholders converted, through a small `parses` helper: the fix applied to the statement, and for a fix that edits after its statement, the file text with and without the fix. Before, a file with a placeholder never passed the "parses without the fix" guard, so such fixes were kept unchecked. New tests: unit `fixes_after_statements_with_placeholders_are_checked_with_parameters`, and spec `banUncommittedTransaction/no_semicolon_at_the_end_psql_variables` (the invalid `commit;` fix is dropped; without the conversion in the guard, the harness fails with "produces invalid SQL"). This also covers the L1 point under "Not covered".
psteinroe
added a commit
that referenced
this pull request
Nov 2, 2026
…nsaction when the last statement has none (#1062) Stacked on #1044. #1044 drops `banUncommittedTransaction`'s fix when the file's last statement has no semicolon, since adding `\ncommit;` after it doesn't parse. The fix can add the semicolon instead: ```sql begin; create table users (id bigint) -- fix: create table users (id bigint); -- commit; ``` While writing the spec I found that the fix put the `COMMIT` into the body of a SQL function when the file ends with a `CREATE FUNCTION`: the analyser took the end of the last statement from the last entry of its statement list, which is the last statement of the function's body. The fix guard didn't catch it, since the function still parses with `commit;` in its body. ### What changed - The analyser takes the end of a file, or section, from its last top-level statement (parsed or not), and records whether that statement ends with a semicolon. - `AnalysedFileContext::append_statement(sql)` replaces `end_offset`: it returns where to insert what to add a statement after the last one, with a `;` first if the last statement has none. Without the text of the last statement, it assumes a semicolon, and #1044's guard still checks the fix on the file. ### Tests - `banUncommittedTransaction/no_semicolon_at_the_end.sql` now has the fix `...;\ncommit;` (the harness checks that it parses). - New spec `banUncommittedTransaction/function_at_the_end.sql`: the `COMMIT` goes after the `CREATE FUNCTION`. Without the change, it goes into the function body. - `cargo test -p pgls_analyser`, `cargo clippy --workspace --all-targets -- -D warnings`.
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.
Follow-up to #906. The fix guard in
Analyser::analyseapplies each fix to its statement and drops it if the statement no longer parses. A fix with an edit after the end of its statement couldn't be checked that way and was kept unchecked.banUncommittedTransaction's fix is one: it addscommit;after the last statement. If the file doesn't end with a semicolon, that fix turnsinto
create table users (id bigint)\ncommit;, which doesn't parse. The spec harness, which checks that every fix produces valid SQL, panics on this file.What changed
AnalyserParams::source: the text of the file. The workspace and the spec harness pass it.Tests
fixes_after_the_statement_are_checked_on_the_file: aCOMMITadded after a statement with and without its semicolon, after a last statement without a semicolon (;\ncommit;is valid,\ncommit;isn't), inside the next statement, and in text that doesn't parse.banUncommittedTransaction/no_semicolon_at_the_end.sql: the diagnostic has no fix now. Without the change, the harness fails with "the fixAdd COMMIT at the endproduces invalid SQL".cargo test -p pgls_analyser,cargo test -p pgls_workspace,cargo clippy --workspace --all-targets -- -D warnings.Not covered
banUncommittedTransactioncould add;\ncommit;when the last statement has no semicolon, so this file still gets a fix. That's a change of the rule; here the guard only drops the invalid fix.