Skip to content

feat(linter): check SQL function result column counts - #819

Closed
psteinroe wants to merge 19 commits into
feat/next-linter-versionfrom
feat/function-return-type-check
Closed

psteinroe wants to merge 19 commits into
feat/next-linter-versionfrom
feat/function-return-type-check

Conversation

@psteinroe

@psteinroe psteinroe commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Add the functionReturnTypeMismatch typecheck rule. It reports LANGUAGE sql functions whose final statement doesn't return what the function declares. Postgres rejects these functions with 42P13 invalid_function_definition.

create table users_hidden.users (id uuid, first_name text, last_name text, email text);
-- Return type mismatch in function declared to return users_hidden.users.
-- Final statement returns too few columns: expected 4, found 2.
create function f(user_id uuid) returns users_hidden.users language sql as $$
  select u.id, u.first_name from users_hidden.users u where u.id = user_id;
$$;

What is checked

The rule compares column counts, not column types:

  • A scalar result needs exactly one column.
  • A composite result (a table row type, a composite type, OUT parameters, RETURNS TABLE) needs one column per attribute, or a single whole-row column.
  • Unless the function returns void, the final statement must be a SELECT, or DML with RETURNING.

Bodies come from the AS string or from BEGIN ATOMIC. Composite types created earlier in the file are taken into account.

When it stays silent

It reports nothing when the result can't be known: record, polymorphic and %TYPE results, bodies that reference unknown objects or don't parse, plpgsql functions, and procedures. It is also silent while check_function_bodies is off, which pg_dump output sets. The session now tracks that setting with the same SET LOCAL and transaction semantics as search_path.

Every resolver test case and spec was checked against Postgres 15. Comparing the column types is left for a follow-up.

Refs #431

@psteinroe
psteinroe added this pull request to stack #820 October 1, 2026 10:41
@psteinroe psteinroe changed the title feat(linter): functionReturnTypeMismatch checks SQL function result column counts feat(linter): check SQL function result column counts Oct 1, 2026
@psteinroe
psteinroe force-pushed the feat/function-return-type-check branch from f2925b9 to 17eccba Compare October 1, 2026 19:34
Adds the catalog overlay, the session state, and a conservative name
resolver that reports a finding only when Postgres would certainly fail.
The schema cache now records which tables are partitions or inheritance
children.
Rules are identified by name (category lint/<rule>); groups are metadata.
Rules are regrouped into correctness, safety, destructive, style, and
typecheck, and migration-only rules are skipped outside the migrations
directory. preferBigintOverInt and preferBigintOverSmallint are merged
into preferBigInt, and concurrentRefreshMatviewLock is removed.

New typecheck rules check names against the catalog: unknownRelation,
unknownColumn, unknownSchema, ambiguousColumn, unknownFunction,
insertColumnMismatch, unknownType, and missingFromClauseEntry. New
correctness rule invalidDropTypeSignature.
linter.rules.<rule> and linter.groups.<group> replace group-nested rule
configuration. linter.rules.safety.<rule> is still accepted and reported
as deprecated.
Typecheck rules run when a schema is loaded and typecheck.enabled is set.
EXPLAIN runs only for statements that reference unchanged database
objects. linter.enabled now switches the lint rules.
The schema cache becomes the catalog's database snapshot
(pgls_catalog::Snapshot), loaded with the db feature or from JSON. The
JSON format, the generated SchemaCache TS type, and the
invalidateSchemaCache command are unchanged.

Removes the unused pgls_type_resolver crate.
Catalog::snapshot() turns the overlay back into a Snapshot. Completions and
hover use it with the statements before the cursor applied, so they show
objects the file created and hide the ones it dropped. The analyser now
takes the snapshot from the catalog base.
Mirrors the layout of pgls_pretty_print: resolve/nodes/<node>.rs with
resolve_<node>(r, n) functions and a single resolve_node_enum dispatcher.
The names in scope move into the Resolver as a stack of query levels and
CTEs.
Legacy specifiers keep working and report their flat replacement. `lint/safety` keeps its former meaning of every lint rule. The deprecated `linter.rules.safety` is marked deprecated in the JSON schema and keeps its former type name `Safety`.
Drop the empty `security` group and the stale `lint/performance` and `lint/safety` categories. The deprecated `linter.rules.safety` accepts only the rules it had. Document the two typecheck switches and that `migrationsDir` limits migration-only rules.
…ion by owner

Rename view.rs to lookup.rs, move column naming into resolve/, make the search path expansion a Snapshot method, and split Session into settings (search path, role) and locks (lock, timeout, and constraint state). ROLLBACK now also forgets a SET LOCAL ROLE.
@psteinroe
psteinroe force-pushed the feat/next-linter-version branch from 1acba80 to 6b28e2d Compare October 2, 2026 10:56
@psteinroe
psteinroe force-pushed the feat/function-return-type-check branch from 17eccba to bdb3e82 Compare October 2, 2026 10:56
@psteinroe
psteinroe force-pushed the feat/next-linter-version branch from 6b28e2d to 93f3b66 Compare October 4, 2026 17:05
@psteinroe
psteinroe removed this pull request from stack #820 October 4, 2026 17:05
@psteinroe

Copy link
Copy Markdown
Collaborator Author

Combined into #815, which now contains the whole former stack (#815, #819, #823).

psteinroe added a commit that referenced this pull request Oct 4, 2026
Restructures the linter around flat rule IDs and replaces the EXPLAIN-based typecheck with static type checking backed by an in-memory catalog. The linter infers the type of every expression and reports what Postgres would reject, without running the statement. EXPLAIN stays as a fallback. Existing configs and suppression comments keep working and print deprecation warnings. Completions and hover now see the tables and columns created earlier in the file.

This combines the former stack of #815, #819 and #823 into one PR.

**Flat rule IDs and groups**

Rules are identified by name: `lint/banDropColumn`, `linter.rules.banDropColumn`, `pgls-ignore banDropColumn`. Groups are metadata, configured in bulk under `linter.groups`, like oxlint's categories. Moving a rule to another group no longer breaks configs or suppressions. The rules were regrouped into `correctness`, `safety`, `destructive`, `style`, `typecheck`, and `nursery`.

```jsonc
// before
{ "linter": { "rules": { "safety": { "banDropColumn": "off" } } } }
// after
{ "linter": { "rules": { "banDropColumn": "off" }, "groups": { "style": "off" } } }
```

Precedence is rule > deprecated `linter.rules.safety.<rule>` > group > `recommended`/`all` preset. `preferBigintOverInt` and `preferBigintOverSmallint` are merged into `preferBigInt` with the options `checkInt` and `checkSmallint`. `concurrentRefreshMatviewLock` is removed; `requireConcurrentRefreshMatview` covers it, and the old name maps to it.

**Backward compatibility and warnings**

Everything users write keeps working:

- **Config:** `linter.rules.safety.<rule>`, `safety.recommended`, and `safety.all` still apply, including the removed rule names, which map to `preferBigInt`. The CLI prints one `Warning:` line per deprecated setting before it runs. The LSP shows a single warning message per session that lists them. The JSON schema marks `linter.rules.safety` as `deprecated`, so editors flag it in the config file.
- **Suppressions:** `lint/safety/<rule>`, `lint/<group>`, and the removed rule names still suppress; a removed rule stands for the rule that replaced it. Each such comment gets a warning diagnostic that names the flat form, like ``Use `banDropColumn` instead``. `lint/safety` keeps its former meaning of every lint rule. `safety` is today's group.
- **Rule selectors:** the `only` and `skip` lists of the workspace API accept the old IDs and the removed rule names. `safety` and `lint/safety` still select every lint rule.
- **API:** The generated TypeScript types keep their names, including `Safety` and `SchemaCache`. So do the JSON formats, the `schema export` command, and the `invalidateSchemaCache` command.

What changes with an unchanged setup:

- **Diagnostic categories:** `lint/safety/<rule>` becomes `lint/<rule>`. Scripts that match on the LSP diagnostic code or the JSON, GitHub, GitLab, or JUnit reporter output need updating.
- **`linter.enabled: false`** now disables the linter. It used to be ignored.
- **`migrations.migrationsDir`:** when set, migration-only rules (most of `safety` and `destructive`) are skipped for files outside it.

**Catalog and name resolution**

The new crate `pgls_catalog` merges `pgls_schema_cache` and adds:

- a catalog that applies the file's DDL to the database snapshot: temp tables, CTAS and view columns, inheritance, renames, drops, and `ROLLBACK` / savepoints;
- the session state: search path, role, transactions, locks, `check_function_bodies`;
- a conservative name resolver.

No DDL runs against the database. Anything the catalog can't be sure about stays silent, including statements that don't parse and may have changed the catalog. EXPLAIN remains as a fallback for statements that only reference unchanged database objects. `typecheck.enabled` switches both off. The unused `pgls_type_resolver` crate is deleted.

**Typecheck rules**

All are recommended and report errors.

| Rule | Reports | Postgres error |
| --- | --- | --- |
| `unknownRelation`, `unknownColumn`, `unknownSchema`, `unknownFunction`, `unknownType` | references to objects that don't exist | 42P01, 42703, 3F000, 42883, 42704 |
| `ambiguousColumn` | a column of several FROM items | 42702 |
| `insertColumnMismatch` | more values than target columns, or the other way round | 42601 |
| `missingFromClauseEntry` | `t.col` without `t` in FROM | 42P01 |
| `operatorTypeMismatch` | `1 + now()`, `'1' + '1'` | 42883, 42725 |
| `functionArgumentMismatch` | `length(1)`, ambiguous overloads | 42883, 42725 |
| `invalidCast` | `now()::int` | 42846 |
| `assignmentTypeMismatch` | `insert into t (qty) values (now())`, also `UPDATE` and `ON CONFLICT` | 42804 |
| `functionReturnTypeMismatch` | `LANGUAGE sql` functions whose final statement returns the wrong number or types of columns | 42P13 |

**How typing works**

The snapshot now loads casts, operators, and typing metadata for types and functions. `pgls_catalog::typing` ports the algorithms of the latest supported Postgres (18, pinned to `REL_18_6`) for coercion, common types, polymorphic types, and function and operator overload selection. Each port links the Postgres function it follows at that tag. Postgres 15 to 17 resolve types the same way; where 18 accepts more (`old`/`new` in `RETURNING`), it is gated on the server version. The resolver types every clause Postgres types. Anything it doesn't model is unknown, and unknown never reports. `AGENTS.md` describes how to port and test the type rules.

**No false positives**

Two tests compare the analysis with Postgres:

- `resolve/differential_tests.rs` checks a corpus of statements against a live Postgres on every run. Statements Postgres accepts must give no findings, inferred column types must match what Postgres describes, and expected errors must be found.
- `pgls_analyser/tests/postgres_regress.rs` runs Postgres' own regression SQL for 15, 16, 17 and 18 (the fixtures from #824, about 170,000 statements) through the linter, against the catalog of a fresh database on each version. It is a normal `cargo test` without network or database and takes about 11 seconds. Any finding on a statement Postgres accepted fails it. The findings on statements Postgres rejected are snapshotted per version, so changes in detection show up in review. `just record-regress <major> --catalog-only` re-records a catalog.

It found 644 false positives on Postgres 15 and 426 more on 16 to 18; all are fixed. One of them affected the editor beyond typing: statements that don't parse were dropped before linting, so a `CREATE TABLE` with newer syntax made every later use of the table report a missing relation. Such statements now make missing objects unknown for the rest of the file, unless they can't change the catalog (queries, `COPY`, maintenance, ...).

**Schema cache**

The `SchemaCache` JSON gains casts, operators, and typing fields on types and functions. All of them are optional, so older exports still load.

Review focus: false positives in `pgls_catalog/src/resolve`, `catalog/ddl` and `typing`. The known gaps are listed in the module docs of `catalog/ddl`, `resolve` and `typing`. Statements with psql identifier parameters (`:schema.table`) are not typechecked.

Fixes #624
Fixes #692
Fixes #369
Fixes #431
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant