Repository navigation
fix(workspace): do not typecheck SQL function bodies with EXPLAIN - #863
Merged
Merged
Conversation
This was referenced Oct 24, 2026
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
…harness (#939) Since #863, statements in SQL function bodies are only checked by the static typecheck rules, but the Postgres regression harness never analysed them, so a false positive in a body could slip through. This PR makes the harness (and the rule spec tests) analyse function bodies the way the workspace does. **Harness** For every `CREATE FUNCTION ... LANGUAGE sql` with a string body (`AS '...'` or `AS $$...$$`), the body is analysed right after the function, with the function's parameters in scope, by name and as `$n`. A finding in the body of a function Postgres accepted fails the test, like any other finding on an accepted statement. Findings in bodies are marked `(body)` in the per-version snapshots, and the header now counts the analysed bodies (347 on 15, 359 on 18). To keep the harness and the workspace in sync, the extraction moved out of `pgls_workspace`: - `pgls_query_ext::utils::sql_function_body` finds the body text and its range (moved from `get_sql_fn_body`, unchanged). - `pgls_catalog::resolve::FunctionContext::from_create_function` builds the parameters the body sees. It replaces the workspace's `get_sql_fn_signature` → `function_context` conversion and the copy in the `functionReturnTypeMismatch` check. `OUT` / `RETURNS TABLE` columns are still left out. The legacy `SQLFunctionSignature` is gone: `TypecheckDiagnosticsMapper` still returned it, but nothing has read it since #863. **False positives found** Across 15–18, one statement: ```sql -- polymorphism.sql create function formarray(anyelement, variadic anyarray) returns anyarray as $$ select array_prepend($1, $2); $$ language sql immutable strict; -- functionArgumentMismatch: Function array_prepend(anyelement, anyarray) does not exist. ``` It is listed in `KNOWN_FALSE_POSITIVES` with its root cause, and shows up under "Known false positives" in the snapshots. The test fails if an entry stops matching, so fixes have to remove their entries. The fix is stacked on this PR. The new body findings on statements Postgres rejected are all correct: `select_into.sql` (a table the function creates), `window.sql` (`tenk1`, which a fresh database doesn't have) and `compression.sql` (`fipshash`, from `test_setup`). **Not covered** Only bodies the workspace extracts today are analysed: - `BEGIN ATOMIC` and `RETURN` bodies (14–18 accepted functions per version) aren't extracted at all. - Bodies with more than one statement (3–5 per version) don't parse as a single statement, and the workspace reports them as a syntax error ("No root node found in parse result"). Fixed in #947. - `'...'` bodies with doubled quotes (4 per version) can't be found in the source text. #918 and #941 (on the #888 stack) extract `BEGIN ATOMIC`, `RETURN`, `''` and `E'...'` bodies. They rewrite the same `get_sql_fn_body`, which this PR moves to `pgls_query_ext`, so whichever lands second has to move their extraction behind the shared helper. The harness and the specs then pick up those bodies as well. Tests: `postgres_regress` passes, the rule specs (`rules_tests`) also analyse bodies now and none of their snapshots changed, there's a new unit test for `FunctionContext::from_create_function` (moved from the workspace's signature tests), and a workspace test checks that a body statement gets its function's parameters.
psteinroe
added a commit
that referenced
this pull request
Oct 26, 2026
… plpgsql leftovers (#924) Stacked on #899. A few section marker mistakes went unreported, and some leftovers from #862/#863 remained. With section markers configured, both of these passed without a diagnostic: ```sql create table users (id bigint); -- before the first marker: dbmate and goose reject the file -- migrate:up ... -- Migrate:Down -- dbmate's markers are case-sensitive: this is a plain comment ``` **What changed** - A new `migrations/sectionMarkers` diagnostic (warning) for: - statements before the first marker. dbmate fails with "dbmate does not support statements preceding the '-- migrate:up' block" ([migration.go](https://github.com/amacneil/dbmate/blob/v2.37.0/pkg/dbmate/migration.go#L75)), and goose with "must start with '-- +goose Up' annotation"; - lines that are markers once trimmed or lowercased, but not as written: dbmate's `-- Migrate:Up` or an indented ` -- migrate:up`, and goose's `-- +Goose Down`. The tools read these as plain comments, so the file silently has fewer sections. goose matches the annotation itself case-insensitively, but not `+goose`. An indented `-- +goose` annotation is different: goose [rejects it](https://github.com/pressly/goose/blob/v3.29.0/internal/sqlparser/parser.go#L321), and the message says so. Both are reported only when `sectionMarkers` is set. The statement ranges come from a `Document::statement_ranges` accessor, the same one #910 adds, so whichever merges second has an identical hunk. A marker inside a dollar-quoted body is still not reported: dbmate splits there too, so our sections match what the tool runs. - plpgsql_check: a query position that isn't a number no longer panics (`expect` on `parse`). The diagnostic falls back to the statement's span like other unlocatable issues. - `TypecheckDiagnosticsMapper` no longer computes the SQL function signature, which #863 left in place unused. - Docs (F15): the rules index marks rules with a fix with 🔧. The linting docs' example of a safe fix now uses `requireConcurrentReindex`, which is in the recommended preset, instead of `CREATE INDEX`, which needs a rule that isn't. They also say how to enable the other fixable rules and that 🛠 rules only run in `migrationsDir`. The migrations guide mentions the new diagnostics. **Tests** Unit tests for the new diagnostics: a statement before the first marker for dbmate and goose, wrong-case and indented dbmate markers, goose `+Goose`, and no diagnostics without markers. A workspace test checks that they're reported through `pull_file_diagnostics`. A plpgsql_check unit test covers a non-numeric query position. The mapper tests are updated. **Not covered** - goose `ENVSUB` (F19). - Single-quoted SQL function bodies with `''` still can't be located (F20). That needs a different way to find the body in the source than `find`. **Follow-ups:** goose `ENVSUB` is handled by #949. Single-quoted bodies with `''` are located by scanner token in #918 and #941 (on the #888 stack).
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.
The errors in #856 came from the legacy EXPLAIN-based type check, not the
typechecklint rules. To EXPLAIN a statement from aLANGUAGE sqlfunction body, that check swaps each function parameter for a placeholder value, and it gets two cases wrong:'{0}', sounnest(p_arr)is ambiguous (42725).extract(epoch from p_interval)is never replaced, so Postgres reads it as a missing column (42703).The static
typecheckrules already resolve function parameters themselves. This PR stops running EXPLAIN on statements inside SQL function bodies, so only those rules check them. Top-level statements still go through EXPLAIN as before. With the parameter placeholders gone,database_diagnosticsno longer builds theTypedIdentifierlist and passes an empty one.Genuine errors in function bodies are still reported. The new test in
server.tests.rs(DB-backed) checks two things:lint/unknownColumn.The
pgls_workspace,pgls_lspandpgls_clitests pass, and running the CLI on the issue'sminimal.sqlnow exits cleanly.docs/features/type_checking.mdnow says that the legacy check only covers top-level statements.Notes for reviewers:
TypecheckDiagnosticsMapperstill computes the function signature, butdatabase_diagnosticsnow ignores it. I left it in place to keep this diff small, because chore!: remove deprecated names and the EXPLAIN-based type check #860 removes the EXPLAIN check entirely.database_diagnostics.Fixes #856