Skip to content

fix(plpgsql-deparser): decide assignment targets by parse mode, not a ':=' substring test - #345

Merged
pyramation merged 1 commit into
mainfrom
fix/deparse-assign-named-args
Aug 7, 2026
Merged

fix(plpgsql-deparser): decide assignment targets by parse mode, not a ':=' substring test#345
pyramation merged 1 commit into
mainfrom
fix/deparse-assign-named-args

Conversation

@pyramation

Copy link
Copy Markdown
Collaborator

Summary

PlpgsqlDeparser.deparseAssign decided whether the stored query text already contained the
assignment target with expr.includes(':='). := is also PostgreSQL's named-argument
operator, so any assignment whose value is a call with named arguments looked self-contained
and its target was silently dropped:

{ PLpgSQL_stmt_assign: { varno: /* v_body */, expr: { PLpgSQL_expr: {
    query: "ast_helpers.create_function(v_schema_name := 'app_public', ...)" } } } }

- ast_helpers.create_function(v_schema_name := 'app_public', ...);   // value never assigned
+ v_body := ast_helpers.create_function(v_schema_name := 'app_public', ...);

The AST already answers the question, so no string inspection is needed. PostgreSQL parses an
assignment's right-hand side in a dedicated raw parse mode per target shape and libpg-query
preserves it on PLpgSQL_expr.parseMode; in those modes the query text is the whole
assignment (cnt := cnt + 1), and in every lower mode it is only the value.

const LOWEST_ASSIGN_PARSE_MODE = ParseMode.RAW_PARSE_PLPGSQL_ASSIGN1;

exprCarriesAssignmentTarget(parseMode?: number) {
  return (parseMode ?? ParseMode.RAW_PARSE_DEFAULT) >= LOWEST_ASSIGN_PARSE_MODE;
}

This mirrors the same fix already proven in constructive-db's SQL deparser
(deparser_plpgsql.stmt_assign, parse_mode >= plpgsql_assign_parse_mode()).

The existing un-parenthesizing of subscripted/field targets carried in the text
((a)[2] := 5a[2] := 5) is unchanged, now gated on the parse mode instead.

Confirmed parse-mode values

RawParseMode values were confirmed empirically against the libpg-query 18.1.4 this package
builds on (and match src/include/nodes/parsenodes.h): RAW_PARSE_DEFAULT 0,
RAW_PARSE_TYPE_NAME 1, RAW_PARSE_PLPGSQL_EXPR 2, RAW_PARSE_PLPGSQL_ASSIGN1 3,
ASSIGN2 4, ASSIGN3 5. Parsing cnt := cnt + 1 / a[2] := 5 yields parseMode: 3,
r.f := 7 yields 4, and PERFORM 1 yields 0. The constant reuses the existing
ParseMode enum in hydrate-types.ts rather than a bare literal.

Back-compat decision: a missing parseMode means "not an assignment mode"

Chosen deliberately (option (a) of the two considered):

  • Real parser output always carries parseMode, so parser-produced ASTs are correct in every
    case, with the mode — not a string scan — deciding.
  • The bug that actually bites is hand-built nodes: generators (e.g. constructive-db's
    ast-plpgsql) put the value in expr and the target in varno, which is the natural
    shape given the node's fields. Falling back to the substring test when the mode is absent
    would leave exactly the reported bug unfixed for them, which is the whole point of this PR.
  • It matches the reference implementation, which reads the mode defensively and defaults to
    0
    — i.e. treats absence as a non-assignment mode.
  • A hand-built node that does embed its target in the query text must now say so with
    parseMode: 3 (pinned by a test); previously it relied on the text containing :=. That is
    the accepted cost — such a node was already ambiguous and, with named arguments in the
    value, already wrong.

Tests

New cases in packages/plpgsql-deparser/__tests__/deparser-fixes.test.ts (two also added as
round-trip fixtures in __fixtures__/plpgsql/plpgsql_deparser_fixes.sql). The two hand-built
cases fail before the fix and pass after; the parser round-trip cases guard the behaviour:

  • value with named arguments, both parsed (parse → deparse → reparse, AST compared) and
    hand-built without parseMode — target survives;
  • sum := sum + n still deparses once, not twice;
  • a[2] := ... / r.f := ... targets still emitted correctly;
  • absent parseMode pinned as value-only, and parseMode: 3 pinned as whole-assignment;
  • (v_x)[2] := 5 with ASSIGN3 still un-parenthesized.

Findings, deliberately left alone

  • Snapshot/fixture churn: __fixtures__/plpgsql-generated/generated.json re-numbers the
    plpgsql_call-* entries because test_proc11 (a OUT int, VARIADIC b int[]) now parses and
    is no longer skipped, shifting later keys by one. This drift is pre-existing: running
    pnpm fixtures on unmodified main produces the same 19 changed lines, so the committed
    file was stale relative to the current libpg-query. No existing snapshot changed; the only
    snapshot added is for the new named-argument test. All 252 fixtures round-trip.
  • packages/plpgsql-parse/src/deparse.ts derives an assignment's leading keyword with
    query.trim().split(/[\s:=]+/)[0], which is wrong for value-only assignment text (it takes
    the first token of the value rather than the target). It only affects statement-to-line
    mapping in the pretty printer, so it is out of scope here.

Verification

pnpm build, pnpm lint (no new warnings), and the full suite pass: 3170 tests across every
package that has tests. pgsql-types and @pgsql/cli fail pnpm test on main with "No
tests found" — unrelated to this change and unchanged by it.

Link to Devin session: https://app.devin.ai/sessions/20e40cc5892e4971bdd50c5f8ab84d24
Requested by: @pyramation

@pyramation pyramation self-assigned this Aug 7, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@pyramation
pyramation merged commit 9d88ad9 into main Aug 7, 2026
15 checks passed
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