Skip to content

fix(workspace): analyse every statement of a SQL function body - #947

Merged
psteinroe merged 5 commits into
mainfrom
fix/multi-statement-sql-function-bodies
Oct 26, 2026
Merged

psteinroe merged 5 commits into
mainfrom
fix/multi-statement-sql-function-bodies

Conversation

@psteinroe

@psteinroe psteinroe commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #942.

The workspace treated the body of a LANGUAGE sql function as a single statement. A body with several statements doesn't parse as one, so it was reported as a syntax error, and none of its statements were linted or typechecked. An empty body got the same error:

create function add_user(p_name text) returns int language sql as $$
  insert into users (name) values (p_name);
  select count(*)::int from users;
$$;
-- before: syntax error "No root node found in parse result" over the whole body
-- after: no syntax error; both statements are analysed with `p_name` in scope

create function noop() returns void language sql as $$ $$;
-- before: the same syntax error
-- after: nothing

SqlFunctionBody::statements (in pgls_query_ext, next to sql_function_body) splits the body the way Postgres does (pg_parse_query), using the statement locations libpg_query returns. Each statement becomes a child of the CREATE FUNCTION, with its range in the document and its trailing semicolon, like top-level statements. Two cases stay as before:

  • A body with a single statement is still one child spanning the whole body, so hover, completions and the formatter see exactly what they saw before.
  • A body that doesn't parse is also still one child, so its syntax error is still reported for the body.

An empty body now has no children.

The formatter only rewrote bodies with exactly one child, and it now checks that explicitly. A body with several statements is left as written. Before this PR it was skipped because it didn't parse; without the check, the last statement would have replaced the whole body.

The regression harness and the rule specs use the same split, so they now analyse 357–373 body statements per version, up from 347–359. There are no new findings on accepted statements and no changes to rejected-statement findings, only the header counts.

Tests: unit tests for statements() (single statement, several with and without semicolons, empty, unparsable), document tests for a body with several statements (no syntax error, function context, document ranges) and an empty body, and a formatter test that keeps a body with several statements as written.

Not covered: BEGIN ATOMIC, RETURN, '' and E'...' bodies. #918 and #941 (on the #888 stack) extract those, and they conflict with this stack in the body extraction (document.rs, sql_function.rs). Whichever lands second should split string bodies with SqlFunctionBody::statements.

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
psteinroe changed the base branch from fix/polymorphic-function-params to main October 26, 2026 08:20
# Conflicts:
#	crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_15.snap
#	crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_16.snap
#	crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_17.snap
#	crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_18.snap
@psteinroe
psteinroe merged commit a4276b1 into main Oct 26, 2026
11 checks passed
@psteinroe
psteinroe deleted the fix/multi-statement-sql-function-bodies branch October 26, 2026 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant