Skip to content

fix(workspace): one walk for lint, hover, completions and inlay hints - #1002

Open
psteinroe wants to merge 2 commits into
feat/function-body-statementsfrom
fix/one-statement-walk
Open

psteinroe wants to merge 2 commits into
feat/function-body-statementsfrom
fix/one-statement-walk

Conversation

@psteinroe

@psteinroe psteinroe commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #997. Part of the rebuild described in #997 (point 3: one walk). Replaces #974 and #965.

The analyser and the workspace each had a loop that follows the catalog through the statements of a file: Analyser::analyse for lint and typecheck, and WorkspaceServer::walk_statements for hover, completions, inlay hints, semantic tokens and the expand-* action. They handled migration sections, statements that don't parse and identifier parameters each in their own way, and they differed.

What was broken

The workspace's loop only switched to a new migration section when it reached a statement in it. Completions walk up to the start of the statement being completed, so in the first statement after a down section they saw the down section's changes, while lint didn't:

-- migrate:up
create table items (id int);
-- migrate:down
alter table items add column down_only int;
-- migrate:up
select | from items;                -- completions offered down_only
select down_only from items;        -- lint: unknownColumn

Statements with identifier parameters were handled differently too: the analyser applied create table :name (...) to the catalog with the placeholder name, so the file then had a table name; the workspace tainted the catalog instead.

What changed

  • pgls_analyser::walk_statements(WalkParams, visit) in walk.rs is the only walk. It holds the section handling (each section has its own session and transaction mode, a down section's changes are dropped afterwards, and the implicit transaction blocks of feat(analyser): report statements that can't run in a transaction block #940), unparsable statements tainting the catalog, function bodies not being applied, and identifier parameters. visit gets each statement with its AnalysedFileContext. WalkParams::end stops the walk, and the walk returns the catalog that a statement at end sees: the section that contains end decides which one, not the last statement walked. That fixes the completions case above. best_effort turns on Catalog::best_effort for the editor features (feat(catalog): keep editor features working after DO, CALL and unparsable statements; resolve CALL #914).
  • Analyser::analyse runs the rules in visit. WorkspaceServer::walk_statements collects the statements that start before end and calls the same function; its visitors skip statements with identifier parameters, like before.
  • Identifier parameters: a statement on placeholder names that isn't a plain query taints the catalog instead of being applied to it, but only if it runs (not in a function body). It is still applied to the session: ALTER TABLE authors ADD COLUMN :"column" TEXT takes its ACCESS EXCLUSIVE lock, and CREATE TABLE t (:"c" text) creates t in the transaction, for the lock rules. The analyser still turns typecheck off for the rest of the file after one, since a tainted catalog can still have wrong column types. Hover, completions and inlay hints keep working on the tainted catalog.
  • may_change_catalog and is_plain_query moved into walk.rs and are private to the crate now.
  • The statements share their parse trees with the workspace's cache instead of copying them (perf(analyser): share statement trees with the workspace instead of copying them #965): AnalysableStatement::root is an Arc<NodeEnum> (AnalysableStatement::new still takes an owned tree), AnalysedFileContext::stmts is &[&NodeEnum], the workspace caches Result<Arc<NodeEnum>, SyntaxDiagnostic>, and the formatter copies a tree with Arc::make_mut only when it puts a formatted body into it. Before, every analysis copied each tree twice, and every hover or inlay hint request copied every statement before the cursor.

Tests

  • Specs for runningStatementWhileHoldingAccessExclusive, avoidWideLockWindow and lockTimeoutWarning after statements with a psql variable as a column name: the lock is held, and a table created with such a column doesn't need a lock timeout. The rules spec harness now converts psql variables like the workspace does, otherwise these statements don't parse. They fail if the walk skips the session for such statements, which fix(workspace): build the catalog the same way for lint, hover, completions and inlay hints #974 did.
  • lint_hover_and_completions_see_the_same_catalog (workspace) checks typecheck diagnostics, completions and hover in the same file for three cases: the first statement after a down section, a statement after one that doesn't parse, and a statement after create table :tbl.
  • pgls_analyser (rules specs, Postgres regression; the snapshots don't change), pgls_workspace and pgls_lsp pass, and clippy on the whole workspace.

Cost

The same walk as before for each request: O(statements before the end of the request). The workspace now collects the statements before walking them, which costs a Vec of them. Caching the walk follows later in the stack.

pgls_analyser::walk_statements is the only loop that follows the catalog
and the session through a file: sections, unparsable statements, function
bodies and identifier parameters are handled once. The analyser runs its
rules in it, and the workspace resolves hover, completions, inlay hints,
semantic tokens and code actions in it. The catalog that a statement at
the end of the walk sees is the one of its section, so completions in the
first statement after a down section no longer see its changes.

Statements share their parse trees with the workspace's cache instead of
copying them.
… parameters

A statement whose names are psql variables still takes its locks and
changes the transaction, so the walk applies it to the session and only
taints the catalog. The rules spec harness converts psql variables like
the workspace does, and three specs check that the lock rules see such
statements.

This branch has not been deployed

No deployments
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