refactor(sql-ast): replace instanceof with kind discriminants#253
Merged
refactor(sql-ast): replace instanceof with kind discriminants#253
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced runtime Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
@prisma-next/runtime-executor
@prisma-next/sql-runtime
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/contract-authoring
@prisma-next/contract-ts
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/cli
@prisma-next/emitter
@prisma-next/eslint-plugin
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/family-sql
@prisma-next/sql-kysely-lane
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-lane-sql-builder-new
@prisma-next/sql-lane
@prisma-next/target-postgres
@prisma-next/adapter-postgres
@prisma-next/driver-postgres
@prisma-next/core-control-plane
@prisma-next/core-execution-plane
@prisma-next/config
@prisma-next/contract
@prisma-next/operations
@prisma-next/plan
@prisma-next/utils
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/3-extensions/sql-orm-client/src/collection.ts (1)
3-3: Split the value and type imports.Keep
isWhereExpron its own import and moveToWhereExpr/WhereArgto a separateimport typeline to match the repo’s source-file import style.As per coding guidelines, "Prohibit inline type imports in source files; use separate import lines."♻️ Proposed cleanup
-import { isWhereExpr, type ToWhereExpr, type WhereArg } from '@prisma-next/sql-relational-core/ast'; +import { isWhereExpr } from '@prisma-next/sql-relational-core/ast'; +import type { ToWhereExpr, WhereArg } from '@prisma-next/sql-relational-core/ast';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/collection.ts` at line 3, The current import mixes value and type imports; split them so the runtime symbol is imported normally and the types are imported with an import type. Replace the single line importing isWhereExpr, ToWhereExpr, WhereArg with two imports: one importing isWhereExpr (value) and a separate "import type" line importing ToWhereExpr and WhereArg, referencing the same module and keeping the symbol names unchanged.packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts (1)
37-51: Default case may silently accept unexpected comparable kinds.The
defaultbranch returnsvaluewithout validation. If a newSqlComparablekind is added in the future (e.g., a subquery expression), it would pass through without being checked forParamRefelements.Consider making this exhaustive or explicitly listing expected kinds:
♻️ Suggested approach
case 'list-literal': if (value.values.some((entry) => entry.kind === 'param-ref')) { throw new Error('ParamRef is not supported in grouped having expressions'); } return value; - default: - return value; + case 'column-ref': + return value; + default: { + const _exhaustive: never = value; + throw new Error(`Unsupported comparable kind in grouped having: ${(_exhaustive as SqlComparable).kind}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts` around lines 37 - 51, The switch in validateGroupedComparable currently returns the input in the default branch which may allow new SqlComparable kinds (e.g., subquery or other complex nodes) to pass without checking for 'param-ref'; update validateGroupedComparable to handle all expected SqlComparable kinds explicitly (add cases for any other known kinds) and perform the same ParamRef validation for compound kinds, and make the default branch throw a descriptive Error like "Unsupported SqlComparable kind in grouped having expressions" (or use an exhaustive check/helper to ensure the switch is exhaustive) so unknown/new kinds fail-fast instead of silently passing.packages/3-extensions/sql-orm-client/src/where-binding.ts (1)
87-93: Nested ternary reduces readability.The condition chain is logically correct but difficult to follow. Consider restructuring as early returns or a switch statement.
♻️ Optional refactor for clarity
- if (comparable.kind === 'param-ref' || bindingColumn === undefined) { - return comparable.kind === 'param-ref' - ? comparable - : comparable.kind === 'literal' || comparable.kind === 'list-literal' - ? comparable - : bindExpression(contract, comparable, state); - } + if (comparable.kind === 'param-ref') { + return comparable; + } + + if (bindingColumn === undefined) { + if (comparable.kind === 'literal' || comparable.kind === 'list-literal') { + return comparable; + } + return bindExpression(contract, comparable, state); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/where-binding.ts` around lines 87 - 93, The nested ternary inside the if block (checking comparable.kind and bindingColumn) reduces readability: replace it with explicit early returns or a switch on comparable.kind to make intent clear. Specifically, in where-binding.ts around the conditional that references comparable, bindingColumn, and calls bindExpression(contract, comparable, state), restructure to first early-return comparable when comparable.kind === 'param-ref' or when bindingColumn is undefined and comparable.kind is 'literal' or 'list-literal', otherwise call and return bindExpression(contract, comparable, state); use a switch or if/else ladder on comparable.kind to avoid the nested ternary.packages/3-extensions/sql-orm-client/test/rich-collection.test.ts (1)
38-41: Consider checking kind before casting for clarity.The current pattern
(plan?.ast as SelectAst | undefined)?.kindcasts before checking. A cleaner approach would checkplan?.ast?.kind === 'select'first, then cast for property access.♻️ Optional refactor
- expect((plan?.ast as SelectAst | undefined)?.kind).toBe('select'); - expect((plan?.ast as SelectAst).limit).toBe(1); + expect(plan?.ast?.kind).toBe('select'); + const ast = plan?.ast as SelectAst; + expect(ast.limit).toBe(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/rich-collection.test.ts` around lines 38 - 41, The assertion casts plan?.ast to SelectAst before verifying its kind; change to first assert that plan?.ast?.kind === 'select' (or expect(plan?.ast?.kind).toBe('select')), then safely cast plan?.ast to SelectAst when asserting .limit and .meta.annotations (e.g., use a typed const like const selectAst = plan?.ast as SelectAst after the kind check) so the subsequent expects for SelectAst.limit and plan?.meta.annotations are clear and type-safe.packages/2-sql/4-lanes/relational-core/test/ast/builders.test.ts (1)
51-52: Consider verifying the conflict action type in addition to columns.The assertions now only verify
onConflict.columnsbut no longer distinguish betweendoNothing()anddoUpdateSet()actions. If theInsertOnConflictActionclasses now have akinddiscriminant, consider adding assertions like:expect(ast.onConflict?.action?.kind).toBe('do-update-set'); // line 51 expect(conflictAst.onConflict?.action?.kind).toBe('do-nothing'); // line 70This would maintain the same level of test coverage as the previous
instanceofchecks while using the new discriminant pattern.Also applies to: 70-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/test/ast/builders.test.ts` around lines 51 - 52, The test currently only asserts onConflict.columns but not the action type; update the assertions for the Insert AST(s) to also check the discriminant on the conflict action (e.g. assert ast.onConflict?.action?.kind === 'do-update-set' for the upsert case and conflictAst.onConflict?.action?.kind === 'do-nothing' for the do-nothing case) so the test verifies the InsertOnConflictAction kind instead of using instanceof; locate the Insert AST variables (ast and conflictAst) and add these kind assertions alongside the existing columns/returning expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/4-lanes/relational-core/test/ast/insert.test.ts`:
- Line 80: The test "stores on-conflict do-nothing actions" currently only
asserts that insertAst.onConflict?.action is defined; change it to assert the
discriminant by checking insertAst.onConflict?.action?.kind === 'do-nothing' so
the test verifies the action is the expected do-nothing variant (use the same
pattern as upsert.test.ts).
In `@packages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.ts`:
- Around line 57-78: The tests use toBeDefined() which is tautological; instead
assert the AST discriminant properties to ensure the correct node types and
conflict actions are produced — e.g. check select.kind, insert.kind,
update.kind, del.kind, column.kind, SubqueryExpr.of(select).kind,
lowerEmail(...).kind, AggregateExpr.sum(column).kind,
JsonObjectExpr.fromEntries(...).kind, JsonArrayAggExpr.of(column).kind,
binary.kind, AndExpr.of(...).kind, OrExpr.of(...).kind,
ExistsExpr.exists(select).kind, NullCheckExpr.isNull(column).kind,
EqColJoinOn.of(...).kind, JoinAst.left(...).kind, ProjectionItem.of(...).kind,
OrderByItem.asc(...).kind, and importantly assert
InsertOnConflict.on([column]).action.kind and
InsertOnConflict.on([column]).doUpdateSet({id: param}).action.kind equal their
expected discriminant values (and DefaultValueExpr().kind if present); replace
the generic toBeDefined() assertions with explicit equality checks against the
expected kind/action enum/string for each builder.
In `@packages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.ts`:
- Around line 95-96: Restore a fail-fast structural guard before constructing a
BinaryExpr so malformed where.left values are rejected: in the branch that
checks leftExpr.kind === 'column-ref' (and the analogous right-side checks),
validate that leftExpr (and rightExpr where applicable) actually conforms to the
ColumnRef shape and if not call errorFailedToBuildWhereClause() instead of
falling through; ensure this guard is present before the code that deconstructs
{ table, column } and before calling new BinaryExpr(...) so corrupted internal
builder state is rejected early.
In `@packages/3-extensions/sql-orm-client/src/collection.ts`:
- Around line 107-109: The helper isWhereDirectInput is too permissive because
isWhereExpr(value) only checks the discriminant and lets plain objects like {
kind: 'binary' } bypass shorthandToWhereExpr; tighten the check by ensuring an
AST node actually has the AST methods before treating it as such: update
isWhereDirectInput (or isWhereExpr) to require both the discriminant and that
value has an accept (or rewrite/fold/not) function (e.g. typeof (value as
any).accept === 'function') so shorthand objects go through
shorthandToWhereExpr(...) instead of being misclassified as prebuilt ASTs; keep
references to isWhereDirectInput, isWhereExpr, isToWhereExprInput and
shorthandToWhereExpr in your change.
In `@packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts`:
- Around line 58-64: The test helpers expectSubqueryExpr and
expectDerivedTableSource access expr.kind without guarding for undefined/null;
change each helper to first assert the input is defined (e.g.,
expect(expr).toBeDefined() or throw/assert when expr == null) before reading
.kind, then perform the existing kind equality check to 'subquery' and
'derived-table-source' respectively so TypeError is avoided and failures are
assertion-based; update both functions (expectSubqueryExpr,
expectDerivedTableSource) accordingly.
---
Nitpick comments:
In `@packages/2-sql/4-lanes/relational-core/test/ast/builders.test.ts`:
- Around line 51-52: The test currently only asserts onConflict.columns but not
the action type; update the assertions for the Insert AST(s) to also check the
discriminant on the conflict action (e.g. assert ast.onConflict?.action?.kind
=== 'do-update-set' for the upsert case and conflictAst.onConflict?.action?.kind
=== 'do-nothing' for the do-nothing case) so the test verifies the
InsertOnConflictAction kind instead of using instanceof; locate the Insert AST
variables (ast and conflictAst) and add these kind assertions alongside the
existing columns/returning expectations.
In `@packages/3-extensions/sql-orm-client/src/collection.ts`:
- Line 3: The current import mixes value and type imports; split them so the
runtime symbol is imported normally and the types are imported with an import
type. Replace the single line importing isWhereExpr, ToWhereExpr, WhereArg with
two imports: one importing isWhereExpr (value) and a separate "import type" line
importing ToWhereExpr and WhereArg, referencing the same module and keeping the
symbol names unchanged.
In `@packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts`:
- Around line 37-51: The switch in validateGroupedComparable currently returns
the input in the default branch which may allow new SqlComparable kinds (e.g.,
subquery or other complex nodes) to pass without checking for 'param-ref';
update validateGroupedComparable to handle all expected SqlComparable kinds
explicitly (add cases for any other known kinds) and perform the same ParamRef
validation for compound kinds, and make the default branch throw a descriptive
Error like "Unsupported SqlComparable kind in grouped having expressions" (or
use an exhaustive check/helper to ensure the switch is exhaustive) so
unknown/new kinds fail-fast instead of silently passing.
In `@packages/3-extensions/sql-orm-client/src/where-binding.ts`:
- Around line 87-93: The nested ternary inside the if block (checking
comparable.kind and bindingColumn) reduces readability: replace it with explicit
early returns or a switch on comparable.kind to make intent clear. Specifically,
in where-binding.ts around the conditional that references comparable,
bindingColumn, and calls bindExpression(contract, comparable, state),
restructure to first early-return comparable when comparable.kind ===
'param-ref' or when bindingColumn is undefined and comparable.kind is 'literal'
or 'list-literal', otherwise call and return bindExpression(contract,
comparable, state); use a switch or if/else ladder on comparable.kind to avoid
the nested ternary.
In `@packages/3-extensions/sql-orm-client/test/rich-collection.test.ts`:
- Around line 38-41: The assertion casts plan?.ast to SelectAst before verifying
its kind; change to first assert that plan?.ast?.kind === 'select' (or
expect(plan?.ast?.kind).toBe('select')), then safely cast plan?.ast to SelectAst
when asserting .limit and .meta.annotations (e.g., use a typed const like const
selectAst = plan?.ast as SelectAst after the kind check) so the subsequent
expects for SelectAst.limit and plan?.meta.annotations are clear and type-safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 888ed0ad-3892-4d55-82cd-13ed6a809bb7
⛔ Files ignored due to path filters (2)
projects/orm-client/plans/ast-kind-discriminants-plan.mdis excluded by!projects/**projects/orm-client/specs/ast-kind-discriminants.spec.mdis excluded by!projects/**
📒 Files selected for processing (52)
packages/2-sql/4-lanes/kysely-lane/src/transform/transform-dml.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.tspackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/utils/guards.tspackages/2-sql/4-lanes/relational-core/test/ast/builders.test.tspackages/2-sql/4-lanes/relational-core/test/ast/common.test.tspackages/2-sql/4-lanes/relational-core/test/ast/insert.test.tspackages/2-sql/4-lanes/relational-core/test/ast/kind-discriminants.test.tspackages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.tspackages/2-sql/4-lanes/relational-core/test/ast/select.test.tspackages/2-sql/4-lanes/relational-core/test/column-builder-operations.test.tspackages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.tspackages/2-sql/4-lanes/sql-lane/test/include-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-mutation.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-operators-and-projection.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.tspackages/2-sql/4-lanes/sql-lane/test/select-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-comparison-operators.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml-returning.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-includes.ast.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-joins.ast.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-joins.builder.test.tspackages/2-sql/4-lanes/sql-lane/test/sql.test.tspackages/2-sql/5-runtime/src/plugins/budgets.tspackages/2-sql/5-runtime/src/plugins/lints.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/query-plan-aggregate.tspackages/3-extensions/sql-orm-client/src/query-plan-meta.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/grouped-collection.test.tspackages/3-extensions/sql-orm-client/test/helpers.tspackages/3-extensions/sql-orm-client/test/integration/create.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/upsert.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-mutations.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/rich-collection.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.tspackages/3-extensions/sql-orm-client/test/rich-query-plans.test.tspackages/3-extensions/sql-orm-client/test/where-binding.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/test/adapter.test.ts
💤 Files with no reviewable changes (2)
- packages/2-sql/4-lanes/relational-core/test/ast/common.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-joins.ast.test.ts
wmadden
force-pushed
the
tml-2096-avoid-instanceof-in-sql-query-ast-methods
branch
from
02b2bd1 to
b2025c7
Compare
Base automatically changed from
tml-2099-move-budgets-plugin-from-framework-domain-to-sql-domain-next-to-lints
to
main
March 26, 2026 15:05
Replace instanceof-based dispatch on SQL query AST nodes with structural kind tags. Covers ~112 instanceof sites across 13 files in 6 packages, adds exhaustiveness checking, and eliminates silent failures from duplicate package instances.
3 milestones, 37 tasks: foundation (add kind tags + union types), migrate ~112 instanceof dispatch sites across 13 files, then test migration and verification.
Every concrete AST class gets a readonly kind string literal tag, abstract bases declare abstract readonly kind: string, and discriminated union types (AnyQueryAst, AnyFromSource, AnyExpression, AnyWhereExpr, AnyInsertOnConflictAction) are exported. Purely additive — existing instanceof dispatch continues to work. Includes foundation tests asserting correct kind values and tag uniqueness, plus a structural dispatch test simulating cross-module-boundary scenarios.
Replace all 25 instanceof checks in types.ts (rewriteComparable, foldComparable, JsonObjectExpr, ListLiteralExpr, JoinAst, SelectAst, InsertAst, UpdateAst) and the 1 instanceof check in guards.ts (getColumnInfo) with kind-based structural dispatch.
Replace all ~39 instanceof checks in adapter.ts with kind-based structural dispatch using switch statements with exhaustive never defaults. Imports converted to type-only where classes were only used for instanceof checks.
Replace 6 instanceof checks in lints.ts: isSqlQueryAst uses a kind-in-set check, getFromSourceTableDetail and evaluateAstLints use exhaustive switch on kind. Imports converted to type-only.
Replace 19 instanceof checks across where-binding.ts, query-plan-aggregate.ts, collection.ts, and query-plan-meta.ts with kind-based structural dispatch.
Replace 17 instanceof checks across transform.ts, transform-expr.ts, transform-dml.ts, and where-expr.ts with kind-based structural dispatch.
Replace 5 instanceof checks in predicate-builder.ts with kind-based structural dispatch. Fix test assertions that used plain objects instead of AST constructors for toEqual comparisons.
Replace all remaining instanceof checks on AST classes in test files with kind-based structural dispatch. Remove the obsolete instanceof warning comment from AstNode. Zero instanceof on AST classes remain in both production and test code.
Replace 2 instanceof checks (SelectAst, AggregateExpr) with kind-based structural dispatch, consistent with the rest of the AST migration.
Add narrowed abstract kind union types to QueryAst, FromSource, Expression, WhereExpr, and InsertOnConflictAction. This ensures new subclasses must declare a kind value from the parent union, preventing silent dispatch gaps. Remove the expressionKinds Set in predicate-builder.ts (now redundant since Expression.kind is type-narrowed). Type the whereExprKinds Set against WhereExpr[kind] in collection.ts. Simplify the duck-typing guard in getColumnInfo.
Both branches of the ternary called the same method. This redundancy predates the instanceof migration but was preserved by it.
Replace 117 toBeInstanceOf assertions on AST classes across 31 test files with kind-based assertions. This ensures tests exercise the structural dispatch mechanism rather than the class hierarchy. Unused class imports converted to type-only or removed.
These assertions were mechanically converted from toBeInstanceOf but just verify that a static readonly field equals itself. The dedicated kind-discriminants.test.ts already covers tag correctness systematically.
Move queryAstKinds and whereExprKinds to the AST package so consumers import the canonical set instead of re-declaring it locally. Both sets are typed against the narrowed abstract kind union for compile-time validation of their members.
Move type guard functions to the AST package alongside their kind sets so consumers import canonical guards instead of re-implementing them.
TypeScript cannot narrow abstract class instances through kind discriminant checks — only plain discriminated union types narrow. Add explicit casts after kind guards, use AnyQueryAst union where narrowing is needed, add override to abstract kind declarations, and restore type imports removed by biome.
…ined Replace tautological toBeDefined() checks with kind discriminant assertions so tests actually verify AST node types, not just that objects are non-null.
Add accept-method check alongside kind discriminant in isWhereDirectInput
so plain objects with a matching kind value (e.g. { kind: 'binary' }) go
through shorthandToWhereExpr instead of being treated as prebuilt ASTs.
…d code, shouldBlock - Fix DriverWithExplain to use single-object `explain(request)` matching the actual postgres driver signature (was silently failing via catch block) - Replace shared `observedRows` closure variable with per-plan WeakMap tracking to prevent cross-request row count interference in concurrent executions - Remove dead `void ctx.now()` call whose return value was discarded - Align latency shouldBlock to use OR logic (like row budgets) so strict mode consistently blocks for all budget types
The latency budget should block (throw) in strict mode and warn in permissive mode, regardless of severity. The previous OR logic caused permissive mode with error severity to throw, breaking the integration test expectation that permissive mode always warns.
wmadden
force-pushed
the
tml-2096-avoid-instanceof-in-sql-query-ast-methods
branch
from
b2025c7 to
ed06563
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/2-sql/5-runtime/src/plugins/budgets.ts (1)
239-247:⚠️ Potential issue | 🟠 Major
severities.latencyno longer affects behavior.On Line 240,
shouldBlockignoresoptions.severities.latency, so a caller that explicitly configureslatency: 'error'in permissive mode now only gets a warning. That makes a public option inert and changes behavior outside this refactor. This also needs thelatencySeveritylocal restored next torowSeverity.⏱️ Suggested fix
- const shouldBlock = ctx.mode === 'strict'; + const shouldBlock = latencySeverity === 'error' || ctx.mode === 'strict';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 239 - 247, Restore and use a latencySeverity variable (like the existing rowSeverity) derived from options.severities.latency instead of always basing blocking on ctx.mode; compute latencySeverity from options.severities.latency, determine shouldBlock for the latency branch by checking if latencySeverity === 'error' (or equivalent) or combining it with ctx.mode as your policy requires, and pass that shouldBlock into emitBudgetViolation called with budgetError('BUDGET.TIME_EXCEEDED', ...) so the configured severities.latency controls behavior; update the latency branch where shouldBlock is currently set and ensure latencySeverity is declared alongside rowSeverity.
🧹 Nitpick comments (3)
packages/2-sql/4-lanes/relational-core/src/ast/types.ts (1)
1230-1232: Consider extracting a helper to reduce cast repetition.After the
kindcheck, TypeScript doesn't automatically narrowFromSourcetoDerivedTableSource. The explicit casts work but are repeated in multiple places.♻️ Optional: Extract a narrowing helper
function asDerivedTableSource(source: FromSource): DerivedTableSource | undefined { return source.kind === 'derived-table-source' ? source : undefined; } // Usage: const derived = asDerivedTableSource(this.from); if (derived) { pushRefs(derived.query.collectColumnRefs()); }Also applies to: 1256-1258, 1275-1277, 1301-1303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/src/ast/types.ts` around lines 1230 - 1232, Create a small type-narrowing helper (e.g., function asDerivedTableSource(source: FromSource): DerivedTableSource | undefined) and replace repeated casts like (this.from as DerivedTableSource) with a call to this helper; after checking source.kind === 'derived-table-source' use the returned value (e.g., const derived = asDerivedTableSource(this.from); if (derived) { pushRefs(derived.query.collectColumnRefs()); }) and apply the same pattern for the other occurrences that currently cast this.from.packages/2-sql/5-runtime/src/plugins/budgets.ts (1)
3-3: Split the type import off this runtime import.Keep
SelectAston its ownimport typeline; mixed inline type imports are disallowed in source files here.As per coding guidelines, "Prohibit inline type imports in source files; use separate import lines."♻️ Suggested change
-import { isQueryAst, type SelectAst } from '@prisma-next/sql-relational-core/ast'; +import { isQueryAst } from '@prisma-next/sql-relational-core/ast'; +import type { SelectAst } from '@prisma-next/sql-relational-core/ast';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` at line 3, The import currently mixes a runtime value and a type: change the combined line importing isQueryAst and SelectAst into two imports so the type is separate; e.g. keep the module specifier but use one import for the runtime symbol (import { isQueryAst } from '...') and a separate import type for the type (import type { SelectAst } from '...'), ensuring you preserve the original module path and exported names.packages/3-targets/6-adapters/postgres/src/core/adapter.ts (1)
1-35: Split the type-only symbols into animport typeblock.This import mixes runtime values with inline
typespecifiers. Please move the type-only names to a separateimport typeso the file matches the repo's source-file import convention.As per coding guidelines, "Prohibit inline type imports in source files; use separate import lines."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/6-adapters/postgres/src/core/adapter.ts` around lines 1 - 35, The mixed import should be split so runtime values (createCodecRegistry, LiteralExpr) stay in the normal import and all other symbols become a separate "import type" statement; update the current import that lists Adapter, AdapterProfile, AggregateExpr, AnyExpression, AnyFromSource, AnyInsertOnConflictAction, AnyQueryAst, BinaryExpr, CodecParamsDescriptor, ColumnRef, DeleteAst, Expression, FromSource, InsertAst, InsertValue, JoinAst, JoinOnExpr, JsonArrayAggExpr, JsonObjectExpr, ListLiteralExpr, LowererContext, NullCheckExpr, OperationExpr, OrderByItem, ParamRef, ProjectionItem, QueryAst, SelectAst, SubqueryExpr, UpdateAst, WhereExpr into a single import type line, keep createCodecRegistry and LiteralExpr in the non-type import, and remove inline "type" specifiers from the runtime import so the file follows the project's import convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/3-extensions/sql-orm-client/test/grouped-collection.test.ts`:
- Around line 226-231: The current branch in grouped-collection.test.ts quietly
returns undefined when an AggregateExpr loses its operand; update the block that
inspects having (and casts to BinaryExpr/AggregateExpr) to explicitly validate
AggregateExpr.expr exists and is a ColumnRef and throw or assert (with a clear
message referencing the aggregate kind and location) when it is missing or of
the wrong kind instead of returning undefined; this ensures the aggregate
operand is validated for functions that expect a column (inspect symbols:
having, BinaryExpr, AggregateExpr, ColumnRef) so broken sum/avg/min/max nodes
fail fast during the test.
---
Outside diff comments:
In `@packages/2-sql/5-runtime/src/plugins/budgets.ts`:
- Around line 239-247: Restore and use a latencySeverity variable (like the
existing rowSeverity) derived from options.severities.latency instead of always
basing blocking on ctx.mode; compute latencySeverity from
options.severities.latency, determine shouldBlock for the latency branch by
checking if latencySeverity === 'error' (or equivalent) or combining it with
ctx.mode as your policy requires, and pass that shouldBlock into
emitBudgetViolation called with budgetError('BUDGET.TIME_EXCEEDED', ...) so the
configured severities.latency controls behavior; update the latency branch where
shouldBlock is currently set and ensure latencySeverity is declared alongside
rowSeverity.
---
Nitpick comments:
In `@packages/2-sql/4-lanes/relational-core/src/ast/types.ts`:
- Around line 1230-1232: Create a small type-narrowing helper (e.g., function
asDerivedTableSource(source: FromSource): DerivedTableSource | undefined) and
replace repeated casts like (this.from as DerivedTableSource) with a call to
this helper; after checking source.kind === 'derived-table-source' use the
returned value (e.g., const derived = asDerivedTableSource(this.from); if
(derived) { pushRefs(derived.query.collectColumnRefs()); }) and apply the same
pattern for the other occurrences that currently cast this.from.
In `@packages/2-sql/5-runtime/src/plugins/budgets.ts`:
- Line 3: The import currently mixes a runtime value and a type: change the
combined line importing isQueryAst and SelectAst into two imports so the type is
separate; e.g. keep the module specifier but use one import for the runtime
symbol (import { isQueryAst } from '...') and a separate import type for the
type (import type { SelectAst } from '...'), ensuring you preserve the original
module path and exported names.
In `@packages/3-targets/6-adapters/postgres/src/core/adapter.ts`:
- Around line 1-35: The mixed import should be split so runtime values
(createCodecRegistry, LiteralExpr) stay in the normal import and all other
symbols become a separate "import type" statement; update the current import
that lists Adapter, AdapterProfile, AggregateExpr, AnyExpression, AnyFromSource,
AnyInsertOnConflictAction, AnyQueryAst, BinaryExpr, CodecParamsDescriptor,
ColumnRef, DeleteAst, Expression, FromSource, InsertAst, InsertValue, JoinAst,
JoinOnExpr, JsonArrayAggExpr, JsonObjectExpr, ListLiteralExpr, LowererContext,
NullCheckExpr, OperationExpr, OrderByItem, ParamRef, ProjectionItem, QueryAst,
SelectAst, SubqueryExpr, UpdateAst, WhereExpr into a single import type line,
keep createCodecRegistry and LiteralExpr in the non-type import, and remove
inline "type" specifiers from the runtime import so the file follows the
project's import convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c10066eb-4f4c-4f27-97b3-1af3ff4a6002
⛔ Files ignored due to path filters (2)
projects/orm-client/plans/ast-kind-discriminants-plan.mdis excluded by!projects/**projects/orm-client/specs/ast-kind-discriminants.spec.mdis excluded by!projects/**
📒 Files selected for processing (53)
packages/2-sql/4-lanes/kysely-lane/src/transform/transform-dml.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.tspackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/utils/guards.tspackages/2-sql/4-lanes/relational-core/test/ast/builders.test.tspackages/2-sql/4-lanes/relational-core/test/ast/common.test.tspackages/2-sql/4-lanes/relational-core/test/ast/insert.test.tspackages/2-sql/4-lanes/relational-core/test/ast/kind-discriminants.test.tspackages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.tspackages/2-sql/4-lanes/relational-core/test/ast/select.test.tspackages/2-sql/4-lanes/relational-core/test/column-builder-operations.test.tspackages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.tspackages/2-sql/4-lanes/sql-lane/test/include-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-mutation.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-operators-and-projection.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.tspackages/2-sql/4-lanes/sql-lane/test/select-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-comparison-operators.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml-returning.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-includes.ast.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-joins.ast.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-joins.builder.test.tspackages/2-sql/4-lanes/sql-lane/test/sql.test.tspackages/2-sql/5-runtime/src/plugins/budgets.tspackages/2-sql/5-runtime/src/plugins/lints.tspackages/2-sql/5-runtime/test/budgets.test.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/query-plan-aggregate.tspackages/3-extensions/sql-orm-client/src/query-plan-meta.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/grouped-collection.test.tspackages/3-extensions/sql-orm-client/test/helpers.tspackages/3-extensions/sql-orm-client/test/integration/create.test.tspackages/3-extensions/sql-orm-client/test/integration/include.test.tspackages/3-extensions/sql-orm-client/test/integration/upsert.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-mutations.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/rich-collection.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.tspackages/3-extensions/sql-orm-client/test/rich-query-plans.test.tspackages/3-extensions/sql-orm-client/test/where-binding.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/test/adapter.test.ts
💤 Files with no reviewable changes (2)
- packages/2-sql/4-lanes/relational-core/test/ast/common.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-joins.ast.test.ts
✅ Files skipped from review due to trivial changes (14)
- packages/3-targets/6-adapters/postgres/test/adapter.test.ts
- packages/2-sql/4-lanes/sql-lane/test/rich-mutation.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.ts
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
- packages/3-extensions/sql-orm-client/test/integration/create.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.ts
- packages/2-sql/4-lanes/sql-lane/test/rich-operators-and-projection.test.ts
- packages/3-extensions/sql-orm-client/src/collection.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-joins.builder.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-includes.ast.test.ts
- packages/3-extensions/sql-orm-client/test/integration/include.test.ts
- packages/3-extensions/sql-orm-client/test/where-binding.test.ts
- packages/3-extensions/sql-orm-client/test/integration/upsert.test.ts
- packages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/2-sql/4-lanes/sql-lane/test/select-builder.test.ts
- packages/2-sql/4-lanes/relational-core/test/ast/builders.test.ts
- packages/2-sql/4-lanes/relational-core/test/column-builder-operations.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-comparison-operators.test.ts
- packages/3-extensions/sql-orm-client/test/filters.test.ts
- packages/2-sql/4-lanes/relational-core/src/utils/guards.ts
- packages/3-extensions/sql-orm-client/test/helpers.ts
- packages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-dml.test.ts
- packages/3-extensions/sql-orm-client/test/rich-query-plans.test.ts
- packages/3-extensions/sql-orm-client/src/query-plan-meta.ts
- packages/3-extensions/sql-orm-client/test/rich-collection.test.ts
- packages/3-extensions/sql-orm-client/test/model-accessor.test.ts
- packages/3-extensions/sql-orm-client/src/where-binding.ts
- packages/2-sql/5-runtime/src/plugins/lints.ts
- packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts
- packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/where-expr.ts
- packages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
Comment on lines
+226
to
+231
| if (having?.kind !== 'binary' || (having as BinaryExpr).left.kind !== 'aggregate') { | ||
| return undefined; | ||
| } | ||
| return having.left.expr instanceof ColumnRef | ||
| ? `${having.left.expr.table}:${having.left.expr.column}` | ||
| const aggExpr = (having as BinaryExpr).left as AggregateExpr; | ||
| return aggExpr.expr?.kind === 'column-ref' | ||
| ? `${(aggExpr.expr as ColumnRef).table}:${(aggExpr.expr as ColumnRef).column}` |
There was a problem hiding this comment.
Fail fast when an aggregate loses its operand.
AggregateExpr.expr is optional. On Line 230, this branch quietly turns a broken sum/avg/min/max node into undefined, and the later containment assertion can still pass as long as one execution is correct. Make this branch throw/assert when the operand is missing or not a 'column-ref' so each metric builder is actually validated.
🧪 Suggested tightening
const aggExpr = (having as BinaryExpr).left as AggregateExpr;
- return aggExpr.expr?.kind === 'column-ref'
- ? `${(aggExpr.expr as ColumnRef).table}:${(aggExpr.expr as ColumnRef).column}`
- : undefined;
+ if (aggExpr.expr?.kind !== 'column-ref') {
+ throw new Error('Expected aggregate operand to be a column reference');
+ }
+ const columnExpr = aggExpr.expr as ColumnRef;
+ return `${columnExpr.table}:${columnExpr.column}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/3-extensions/sql-orm-client/test/grouped-collection.test.ts` around
lines 226 - 231, The current branch in grouped-collection.test.ts quietly
returns undefined when an AggregateExpr loses its operand; update the block that
inspects having (and casts to BinaryExpr/AggregateExpr) to explicitly validate
AggregateExpr.expr exists and is a ColumnRef and throw or assert (with a clear
message referencing the aggregate kind and location) when it is missing or of
the wrong kind instead of returning undefined; this ensures the aggregate
operand is validated for functions that expect a column (inspect symbols:
having, BinaryExpr, AggregateExpr, ColumnRef) so broken sum/avg/min/max nodes
fail fast during the test.
aqrln
reviewed
Mar 27, 2026
| } | ||
|
|
||
| const indexes = [...new Set(collectParamIndexes(plan.ast.where))].sort((a, b) => a - b); | ||
| const indexes = [...new Set(collectParamIndexes(selectAst.where))].sort((a, b) => a - b); |
Member
There was a problem hiding this comment.
plan.ast instead of selectAst here and below after applying the suggestion above
aqrln
reviewed
Mar 27, 2026
Comment on lines
121
to
124
| default: { | ||
| const _exhaustive: never = value; | ||
| throw new Error(`Unsupported comparable kind: ${(_exhaustive as { kind: string }).kind}`); | ||
| } |
Member
There was a problem hiding this comment.
I would just write this inline as
Suggested change
| default: { | |
| const _exhaustive: never = value; | |
| throw new Error(`Unsupported comparable kind: ${(_exhaustive as { kind: string }).kind}`); | |
| } | |
| default: | |
| throw new Error( | |
| `Unsupported comparable kind: ${(value satisfies never as { kind: string }).kind}`, | |
| ); |
but no strong preference either way
aqrln
reviewed
Mar 27, 2026
| // v8 ignore next 4 | ||
| default: { | ||
| const _exhaustive: never = value; | ||
| throw new Error(`Unsupported comparable kind: ${(_exhaustive as { kind: string }).kind}`); |
aqrln
reviewed
Mar 27, 2026
Comment on lines
265
to
269
| toExpr(): AnyExpression { | ||
| // Safe: every concrete Expression subclass is a member of AnyExpression. | ||
| // The double cast is needed because TS can't narrow `this` (abstract) to the union. | ||
| return this as unknown as AnyExpression; | ||
| } |
Member
There was a problem hiding this comment.
Hmm, how about we make this
abstract toExpr(): AnyExpression;instead?
Then every subclass is forced to have a boilerplate
override toExpr(): AnyExpression {
return this;
}implementation which will statically check that every subclass of Expression is also present in the AnyExpression union.
aqrln
reviewed
Mar 27, 2026
| if (this.from instanceof DerivedTableSource) { | ||
| pushRefs(this.from.query.collectColumnRefs()); | ||
| if (this.from.kind === 'derived-table-source') { | ||
| pushRefs((this.from as DerivedTableSource).query.collectColumnRefs()); |
Member
There was a problem hiding this comment.
as DerivedTableSource casts are no longer necessary here and below
Suggested change
| pushRefs((this.from as DerivedTableSource).query.collectColumnRefs()); | |
| pushRefs(this.from.query.collectColumnRefs()); |
aqrln
reviewed
Mar 27, 2026
| if (expr instanceof OperationExpr) { | ||
| const baseCol = expr.baseColumnRef(); | ||
| if (expr.kind === 'operation') { | ||
| const baseCol = (expr as OperationExpr).baseColumnRef(); |
Member
There was a problem hiding this comment.
Suggested change
| const baseCol = (expr as OperationExpr).baseColumnRef(); | |
| const baseCol = expr.baseColumnRef(); |
aqrln
reviewed
Mar 27, 2026
Member
There was a problem hiding this comment.
I'm not sure if this is a useful test
aqrln
reviewed
Mar 27, 2026
| if (leftExpr instanceof ColumnRef) { | ||
| const { table, column } = leftExpr; | ||
| if (leftExpr.kind === 'column-ref') { | ||
| const { table, column } = leftExpr as ColumnRef; |
Member
There was a problem hiding this comment.
Suggested change
| const { table, column } = leftExpr as ColumnRef; | |
| const { table, column } = leftExpr; |
aqrln
reviewed
Mar 27, 2026
| if (rightExpr instanceof ColumnRef) { | ||
| const { table, column } = rightExpr; | ||
| if (rightExpr.kind === 'column-ref') { | ||
| const { table, column } = rightExpr as ColumnRef; |
Member
There was a problem hiding this comment.
Suggested change
| const { table, column } = rightExpr as ColumnRef; | |
| const { table, column } = rightExpr; |
aqrln
reviewed
Mar 27, 2026
| join.source instanceof DerivedTableSource && | ||
| join.source.alias === 'post_lateral', | ||
| join.source.kind === 'derived-table-source' && | ||
| (join.source as DerivedTableSource).alias === 'post_lateral', |
Member
There was a problem hiding this comment.
Suggested change
| (join.source as DerivedTableSource).alias === 'post_lateral', | |
| join.source.alias === 'post_lateral', |
aqrln
reviewed
Mar 27, 2026
| join.source instanceof DerivedTableSource && | ||
| join.source.alias === 'posts_lateral', | ||
| join.source.kind === 'derived-table-source' && | ||
| (join.source as DerivedTableSource).alias === 'posts_lateral', |
Member
There was a problem hiding this comment.
Suggested change
| (join.source as DerivedTableSource).alias === 'posts_lateral', | |
| join.source.alias === 'posts_lateral', |
- Make the existing `ExpressionSource` implementation for `Expression` type safe. - Introduce a similar pattern to other abstract classes as a way to statically guarantee that all subclasses are included in the corresponding union type.
aqrln
approved these changes
Mar 27, 2026
This was referenced Mar 27, 2026
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
2 participants
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.
closes TML-2096
Before / After
Intent
Replace all
instanceof-based dispatch on SQL query AST node classes with structuralkinddiscriminant tags, then make discriminated union types the public API surface for all polymorphic AST references. Abstract base classes become internal implementation details.instanceofchecks fail silently when duplicate copies of a package exist innode_modules(a common pnpm workspace scenario). Structuralkinddispatch eliminates this failure mode and enables TypeScript exhaustiveswitchchecking at every dispatch site. Using union types for fields and parameters meansswitch (node.kind)narrows without casting.Change map
Phase 1: Kind discriminants
kindtags on 28 concrete classes, narrowedkindunions on 5 abstract classes, 5 discriminated union typesgetColumnInfotoBeInstanceOfto.kindassertionsPhase 2: Union-typed fields
ExpressionRewriter/AstRewriterinterfaces, removeSqlComparablealias, addAnySqlComparable/AnyOperationArgunion types, makerewriteComparable/foldComparableexhaustiveWhereExpr→AnyWhereExpr,ColumnRefcasts removedExpression→AnyExpression,WhereExpr→AnyWhereExpr,SqlComparable→AnySqlComparableQueryAst→AnyQueryAst,FromSource→AnyFromSource, exhaustive switches addedQueryAst→AnyQueryAst,Expression→AnyExpression,FromSource→AnyFromSource,WhereExpr→AnyWhereExpr, casts removed, error messages aligned tokind-based patternclass UnsupportedAst extends QueryAstreplaced with plain-object cast (abstract class no longer importable)Expression | ParamRef | LiteralExpr→AnyOperationArgWhereExprtoAnyWhereExprtype annotationsThe story
Phase 1: Kind discriminants
Add structural
kindtags to every AST node. Each of the 28 concrete AST classes getsreadonly kind = '<tag>' as const. The rootAstNodedeclaresabstract readonly kind: string, and the 5 intermediate abstract classes (QueryAst,FromSource,Expression,WhereExpr,InsertOnConflictAction) narrow it to their valid tag unions — so adding a new subclass with the wrongkindis a compile error.Export discriminated union types.
AnyQueryAst,AnyFromSource,AnyExpression,AnyWhereExpr, andAnyInsertOnConflictActionenableswitch (node.kind)with exhaustiveneverdefaults in dispatch sites that need full type narrowing.Migrate all ~112 production
instanceofchecks tokind-based dispatch. Multi-branch chains become exhaustiveswitchstatements. Single-branch guards becomeif (node.kind === 'tag'). Classes imported solely forinstanceofare converted to type-only imports, reducing runtime coupling.Migrate all ~117 test
toBeInstanceOfassertions. Test assertions now validate thekinddiscriminant rather than class identity, ensuring tests exercise the structural dispatch mechanism.Clean up follow-on sites. The
expressionKindsSet inpredicate-builder.ts(redundant now thatExpression.kindis type-narrowed) is removed. ThewhereExprKindsSet incollection.tsis typed againstWhereExpr['kind']. A dead branch inJoinAst.rewrite(predating this migration) is removed.Phase 2: Union-typed fields
Replace abstract class types with discriminated unions everywhere. All ~15 field declarations, constructor parameters, composite type aliases (
ProjectionExpr,JoinOnExpr,WhereArg),ExpressionRewriter/AstRewriterinterface return types, and abstract method return types (rewrite(),not(),toExpr()) are updated to reference union types instead of abstract base classes.SqlComparableis removed in favor ofAnySqlComparable.Propagate union types to all downstream packages. 24 files across
kysely-lane,sql-lane,sql-runtime,sql-orm-client, andpostgresare updated. All imports of abstract base classes for type annotations are replaced with union type imports. Casts that existed solely forkind-narrowing (e.g.,const node = ast as AnyQueryAst) are removed.Un-export all 6 abstract base classes.
AstNode,QueryAst,FromSource,Expression,WhereExpr, andInsertOnConflictActionbecome module-private. They remain in the file for method inheritance but are no longer part of the public API surface.Behavior changes & evidence
Every concrete AST class exposes a
readonly kinddiscriminant with a unique kebab-case literal type. Dispatch sites can structurally identify nodes without relying on class identity.instanceoffails silently when multiple copies of a package coexist innode_modules. Structuralkindtags work regardless of module resolution.Intermediate abstract classes narrow
kindto their valid tag union. Adding a newExpressionsubclass withkind = 'invalid'is a compile error.All production dispatch uses
kind-based switching. Zeroinstanceofon AST classes remain in production code.All polymorphic AST fields, parameters, and return types use discriminated union types.
switch (node.kind)narrows directly without casting.kindchecks — consumers had to cast to the union first. Union types eliminate this friction.Abstract base classes are no longer exported. They serve only as internal implementation details for method inheritance and immutability (
freeze()).UnsupportedAstclass replaced with plain-object castNo runtime behavior change: the class hierarchy is preserved. Both phases are pure dispatch-mechanism and type-system migrations.
Compatibility / migration / risk
kindproperty is additive; fields gain more precise types (narrower unions instead of broad abstract class).QueryAst,Expression, etc. will fail to compile. This is intentional.toEqualassertions against plain objects break if the class now has akindproperty that the plain object lacks. All affected tests were updated to use AST constructors (e.g.ProjectionItem.of(...)instead of{ alias, expr }).Follow-ups / open questions
toBeInstanceOfcalls on non-AST classes (e.g.Collection,PostCollection) remain untouched — those are outside the scope of this migration.Non-goals / intentionally out of scope
freeze(), and factory methods).instanceofon non-AST types (e.g.Error,Date,Collection).