From b3842dce526d1d1db284bd112459779bbd8f6e06 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 18 Jul 2026 18:07:51 +0100 Subject: [PATCH 1/2] refactor(walk): make the boundary-crossing forms the primitives The three transformers that must cross the subquery scope boundary (CTE hoisting, INTO removal, ILIKE -> LIKE) each hand-listed the five subquery-bearing SqlExpression variants. That compiles clean -- and silently stops crossing -- the day a sixth is added, which is exactly the R3 failure mode: PR #33 fixed two live bugs of that shape. Invert it. map_children_crossing / visit_children_crossing take a second closure for the nested statement and are now the primitives; map_children is that with an identity handler, visit_children with a no-op one. The variant list exists once, in walk.rs, so adding one is a compile error in a single place. Two things the signature is forced into: - Both closures take an explicit ctx rather than capturing. CTEHoister's recursion is &mut self, and two closures each capturing self mutably will not borrow-check; the same applies to &mut deps on the visit path. ctx gives one mutable borrow that the helper splits across the calls. - f_stmt takes and returns the Box, not the statement. By value, the opaque path -- which is most transformers -- would unbox, free and realloc a large struct at every subquery. Passing the Box keeps the identity handler a true passthrough. Behaviour is unchanged; the three transformers already crossed. Tests pin the property the split exists to provide: the variants map_children treats as opaque must be reachable via the crossing forms, and the tuple forms must yield both their same-scope operands and their statement. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/ENGINE_REFACTORING.md | 16 +- src/query_plan/cte_hoister.rs | 91 ++----- src/query_plan/ilike_to_like_transformer.rs | 52 +--- src/query_plan/into_clause_remover.rs | 48 +--- src/sql/parser/walk.rs | 287 +++++++++++++++----- 5 files changed, 270 insertions(+), 224 deletions(-) diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index c757b145..44def674 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -98,12 +98,21 @@ feature work**, and so we can tell the difference between "this is awkward" and expression form multiplies across ~40 files. - **Done so far:** `src/sql/parser/walk.rs` — `map_children` (owned rewrite) and `visit_children` / `visit_all` (borrowing collector), exhaustive, no catch-all. - Two design rules worth preserving: + Three design rules worth preserving: - **Direct children only, callers drive recursion** — this is what collapses a transformer to its one real rule plus a delegate. - **Scope boundary = walker boundary** — subquery statements are not descended into. Auto-descending would break the alias expanders, which get outer-alias scoping right today only *by omission*. + - **The crossing forms are the primitives** — `map_children_crossing` / + `visit_children_crossing` take a second closure for the nested statement, + and the opaque forms are those with an identity/no-op handler. This keeps + the set of subquery-bearing variants written down *once*. Transformers that + must cross (CTE hoisting, INTO removal, `ILIKE`) previously hand-listed + those five variants each, which compiles clean — and silently stops + crossing — the day an `Exists` is added. Both crossing closures take an + explicit `ctx` parameter: a `&mut self` transformer cannot hand out two + closures that each capture `self` mutably. - **Next:** Migrate the ~10 `query_plan` transformers, one commit each. Treat `having_alias_transformer` as a behaviour **fix**, not a refactor (see R3). Note `WindowSpec::order_by` is now descended into, so each migration needs a @@ -120,8 +129,8 @@ feature work**, and so we can tell the difference between "this is awkward" and src/sql/parser/formatter.rs:139 src/sql/parser/formatter.rs:668 src/sql/recursive_parser.rs:569 - src/sql/parser/walk.rs:63 (added by R2) - src/sql/parser/walk.rs:214 (added by R2) + src/sql/parser/walk.rs:108 (added by R2 — map_children_crossing) + src/sql/parser/walk.rs:280 (added by R2 — visit_children_crossing) ``` Every other consumer compiled clean and would have silently ignored it. @@ -247,3 +256,4 @@ R5 dead code ─────── opportunistic | 2026-07-18 | R2 partial: `walk.rs` traversal helpers landed (additive) | #31 | | 2026-07-18 | R2 group 1: the three boundary-crossing transformers migrated; two silent bugs fixed | #33 | | 2026-07-18 | R3 evidence: P9–P12 filed after probing the engine; corpus gains tier 07 (grouping) | — | +| 2026-07-18 | R2: `*_crossing` helpers become the primitives; the three crossing transformers stop hand-listing subquery variants | — | diff --git a/src/query_plan/cte_hoister.rs b/src/query_plan/cte_hoister.rs index d0038e0b..4612f09f 100644 --- a/src/query_plan/cte_hoister.rs +++ b/src/query_plan/cte_hoister.rs @@ -125,47 +125,21 @@ impl CTEHoister { /// Hoist CTEs from an expression /// - /// The only real rule is the subquery arms: recurse into the nested - /// statement so its CTEs get pulled up to the top level. They stay - /// explicit because [`walk::map_children`] treats a subquery statement as - /// a scope boundary -- crossing it is precisely this transformer's job. + /// The only real rule is the nested statements: recurse into them so their + /// CTEs get pulled up to the top level. `map_children` treats a subquery + /// statement as a scope boundary, and crossing it is precisely this + /// transformer's job, so this uses the `crossing` form -- which keeps the + /// list of subquery-bearing variants in `walk` rather than here. + /// + /// `self` is threaded through as the walk context because both closures + /// need it mutably; capturing it twice would not borrow-check. fn hoist_from_expression(&mut self, expr: SqlExpression) -> SqlExpression { - match expr { - SqlExpression::ScalarSubquery { query } => SqlExpression::ScalarSubquery { - query: Box::new(self.hoist_from_statement(*query)), - }, - SqlExpression::InSubquery { expr, subquery } => SqlExpression::InSubquery { - expr: Box::new(self.hoist_from_expression(*expr)), - subquery: Box::new(self.hoist_from_statement(*subquery)), - }, - SqlExpression::NotInSubquery { expr, subquery } => SqlExpression::NotInSubquery { - expr: Box::new(self.hoist_from_expression(*expr)), - subquery: Box::new(self.hoist_from_statement(*subquery)), - }, - // Defensive, not a demonstrable fix: the tuple forms fell into the - // old catch-all, but the parser currently rejects WITH anywhere in - // expression position ("Tuple IN requires a subquery on the right"), - // so no input reaches these arms today. Kept for symmetry with the - // other subquery arms, which are equally unreachable for the same - // reason. - SqlExpression::InSubqueryTuple { exprs, subquery } => SqlExpression::InSubqueryTuple { - exprs: exprs - .into_iter() - .map(|e| self.hoist_from_expression(e)) - .collect(), - subquery: Box::new(self.hoist_from_statement(*subquery)), - }, - SqlExpression::NotInSubqueryTuple { exprs, subquery } => { - SqlExpression::NotInSubqueryTuple { - exprs: exprs - .into_iter() - .map(|e| self.hoist_from_expression(e)) - .collect(), - subquery: Box::new(self.hoist_from_statement(*subquery)), - } - } - other => walk::map_children(other, |e| self.hoist_from_expression(e)), - } + walk::map_children_crossing( + expr, + self, + |h, e| h.hoist_from_expression(e), + |h, stmt| Box::new(h.hoist_from_statement(*stmt)), + ) } /// Recursively hoist from a WHERE clause @@ -237,34 +211,17 @@ impl CTEHoister { /// Find CTE references in an expression /// - /// The only real rule is the subquery arms: descend into the nested - /// statement and look for CTE references there. Those must stay explicit - /// because [`walk::visit_children`] treats a subquery statement as a scope - /// boundary and will not enter it. Everything else is plain traversal. + /// The only real rule is the nested statements: descend into them and look + /// for CTE references there. `visit_children` treats a subquery statement + /// as a scope boundary and will not enter it, so this uses the `crossing` + /// form. `deps` is the walk context -- both closures need it mutably. fn find_cte_refs_in_expression(&self, expr: &SqlExpression, deps: &mut HashSet) { - match expr { - SqlExpression::ScalarSubquery { query } => { - self.find_cte_references(query, deps); - } - SqlExpression::InSubquery { expr, subquery } - | SqlExpression::NotInSubquery { expr, subquery } => { - self.find_cte_refs_in_expression(expr, deps); - self.find_cte_references(subquery, deps); - } - // Tuple forms were missing from the old catch-all. Unlike the - // hoisting path these ARE reachable: the subquery need not contain - // a WITH, only a reference to an already-hoisted CTE. - SqlExpression::InSubqueryTuple { exprs, subquery } - | SqlExpression::NotInSubqueryTuple { exprs, subquery } => { - for e in exprs { - self.find_cte_refs_in_expression(e, deps); - } - self.find_cte_references(subquery, deps); - } - other => { - walk::visit_children(other, |child| self.find_cte_refs_in_expression(child, deps)) - } - } + walk::visit_children_crossing( + expr, + deps, + |deps, child| self.find_cte_refs_in_expression(child, deps), + |deps, stmt| self.find_cte_references(stmt, deps), + ) } /// Get CTEs in dependency order diff --git a/src/query_plan/ilike_to_like_transformer.rs b/src/query_plan/ilike_to_like_transformer.rs index 33262768..8d1f5a01 100644 --- a/src/query_plan/ilike_to_like_transformer.rs +++ b/src/query_plan/ilike_to_like_transformer.rs @@ -68,48 +68,16 @@ impl ILikeToLikeTransformer { } } - // Subqueries are a scope boundary for `walk::map_children`, which - // will not descend into a nested statement. ILIKE -> LIKE is - // scope-independent, so this transformer deliberately crosses that - // boundary and these arms stay explicit. Delegating them would - // silently stop ILIKE being rewritten inside subqueries. - SqlExpression::ScalarSubquery { query } => SqlExpression::ScalarSubquery { - query: Box::new(self.transform_statement(*query)), - }, - - SqlExpression::InSubquery { expr, subquery } => SqlExpression::InSubquery { - expr: Box::new(self.transform_expression(*expr)), - subquery: Box::new(self.transform_statement(*subquery)), - }, - - SqlExpression::NotInSubquery { expr, subquery } => SqlExpression::NotInSubquery { - expr: Box::new(self.transform_expression(*expr)), - subquery: Box::new(self.transform_statement(*subquery)), - }, - - // Previously missing: the tuple forms fell into the hand-rolled - // catch-all, so neither the LHS operands nor the subquery were - // transformed at all. - SqlExpression::InSubqueryTuple { exprs, subquery } => SqlExpression::InSubqueryTuple { - exprs: exprs - .into_iter() - .map(|e| self.transform_expression(e)) - .collect(), - subquery: Box::new(self.transform_statement(*subquery)), - }, - - SqlExpression::NotInSubqueryTuple { exprs, subquery } => { - SqlExpression::NotInSubqueryTuple { - exprs: exprs - .into_iter() - .map(|e| self.transform_expression(e)) - .collect(), - subquery: Box::new(self.transform_statement(*subquery)), - } - } - - // Everything else is plain traversal. - other => walk::map_children(other, |e| self.transform_expression(e)), + // Everything else is plain traversal. ILIKE -> LIKE is + // scope-independent, so we cross the subquery boundary: the + // `crossing` form rewrites nested statements too, without this + // file having to name the subquery-bearing variants itself. + other => walk::map_children_crossing( + other, + &mut (), + |_, e| self.transform_expression(e), + |_, stmt| Box::new(self.transform_statement(*stmt)), + ), } } diff --git a/src/query_plan/into_clause_remover.rs b/src/query_plan/into_clause_remover.rs index 7ddeeb48..39f188bc 100644 --- a/src/query_plan/into_clause_remover.rs +++ b/src/query_plan/into_clause_remover.rs @@ -110,46 +110,18 @@ impl IntoClauseRemover { /// /// The only real rule here is about subqueries: every nested /// `SelectStatement` needs its `into_table` cleared. Everything else is - /// plain traversal, delegated to [`walk::map_children`]. + /// plain traversal, delegated to [`walk::map_children_crossing`]. /// - /// The subquery arms must stay explicit. `map_children` treats a subquery - /// statement as a **scope boundary** and deliberately does not descend into - /// it — correct for the alias expanders, but exactly what this transformer - /// has to do. Delegating them would silently stop INTO being removed from - /// nested queries. + /// `map_children` treats a subquery statement as a **scope boundary** and + /// deliberately does not descend into it — correct for the alias expanders, + /// but exactly what this transformer has to do, hence the `crossing` form. fn remove_from_expression(expr: SqlExpression) -> SqlExpression { - match expr { - SqlExpression::ScalarSubquery { query } => SqlExpression::ScalarSubquery { - query: Box::new(Self::remove_from_statement(*query)), - }, - SqlExpression::InSubquery { expr, subquery } => SqlExpression::InSubquery { - expr: Box::new(Self::remove_from_expression(*expr)), - subquery: Box::new(Self::remove_from_statement(*subquery)), - }, - SqlExpression::NotInSubquery { expr, subquery } => SqlExpression::NotInSubquery { - expr: Box::new(Self::remove_from_expression(*expr)), - subquery: Box::new(Self::remove_from_statement(*subquery)), - }, - // Previously missing: the tuple forms fell into the catch-all, so - // `WHERE (a, b) IN (SELECT ... INTO #t ...)` kept its INTO clause. - SqlExpression::InSubqueryTuple { exprs, subquery } => SqlExpression::InSubqueryTuple { - exprs: exprs - .into_iter() - .map(Self::remove_from_expression) - .collect(), - subquery: Box::new(Self::remove_from_statement(*subquery)), - }, - SqlExpression::NotInSubqueryTuple { exprs, subquery } => { - SqlExpression::NotInSubqueryTuple { - exprs: exprs - .into_iter() - .map(Self::remove_from_expression) - .collect(), - subquery: Box::new(Self::remove_from_statement(*subquery)), - } - } - other => walk::map_children(other, Self::remove_from_expression), - } + walk::map_children_crossing( + expr, + &mut (), + |_, e| Self::remove_from_expression(e), + |_, stmt| Box::new(Self::remove_from_statement(*stmt)), + ) } } diff --git a/src/sql/parser/walk.rs b/src/sql/parser/walk.rs index cf9a1506..5bb3f810 100644 --- a/src/sql/parser/walk.rs +++ b/src/sql/parser/walk.rs @@ -32,33 +32,84 @@ //! //! # Scope boundaries //! -//! **Subqueries are opaque.** These helpers do not descend into the -//! `SelectStatement` inside `ScalarSubquery`, `InSubquery`, `NotInSubquery`, -//! `InSubqueryTuple` or `NotInSubqueryTuple`, because that statement is a -//! *different query scope*. Descending automatically would be wrong for the -//! alias expanders — a SELECT alias from the outer query must not be expanded -//! inside a subquery that has its own FROM. +//! **Subqueries are opaque by default.** [`map_children`] and +//! [`visit_children`] do not descend into the `SelectStatement` inside +//! `ScalarSubquery`, `InSubquery`, `NotInSubquery`, `InSubqueryTuple` or +//! `NotInSubqueryTuple`, because that statement is a *different query scope*. +//! Descending automatically would be wrong for the alias expanders — a SELECT +//! alias from the outer query must not be expanded inside a subquery that has +//! its own FROM. //! //! Same-scope operands of those variants *are* visited: `InSubquery`'s `expr` //! and `InSubqueryTuple`'s `exprs` belong to the enclosing query, only the //! `subquery` itself is skipped. //! -//! A caller that does want to cross the boundary (the CTE hoister, for one) -//! matches those variants explicitly and delegates the rest here. +//! # Crossing the boundary +//! +//! Some transformers legitimately need to cross it — `ILIKE` -> `LIKE` is +//! scope-independent, INTO removal and CTE hoisting have to reach nested +//! statements by definition. Those callers use +//! [`map_children_crossing`] / [`visit_children_crossing`], which take a second +//! closure for the nested statement. +//! +//! Both closures take an explicit `ctx` parameter rather than capturing what +//! they need. That is forced, not stylistic: a transformer whose recursion is +//! `&mut self` (the CTE hoister) cannot hand out two closures that each capture +//! `self` mutably. Threading the state through as `ctx` gives one mutable +//! borrow, split across the two calls by the helper. +//! +//! **The crossing forms are the primitives.** `map_children` is defined as +//! `map_children_crossing` with an identity statement handler, and +//! `visit_children` as `visit_children_crossing` with a no-op one. This is +//! deliberate: it means the set of subquery-bearing variants is written down +//! **exactly once in the codebase**, in this file. A caller that hand-listed +//! those variants itself would compile clean — and silently stop crossing — +//! the day a new one is added (`Exists`, for instance). Here, adding a variant +//! is a compile error in one place. //! //! Window specs, by contrast, *are* same-scope: `WindowSpec::order_by` holds //! real expressions and is descended into. (`partition_by` is `Vec`, //! so there is nothing to walk.) -use super::ast::{SimpleWhenBranch, SqlExpression, WhenBranch}; +use super::ast::{SelectStatement, SimpleWhenBranch, SqlExpression, WhenBranch}; /// Rebuild `expr`, replacing each direct child expression with `f(child)`. /// -/// Leaf nodes are returned unchanged. See the module docs for why subqueries -/// are not descended into. +/// Leaf nodes are returned unchanged. Subquery statements are a scope boundary +/// and are **not** descended into — see the module docs. Use +/// [`map_children_crossing`] when you need to rewrite them too. pub fn map_children( expr: SqlExpression, mut f: impl FnMut(SqlExpression) -> SqlExpression, +) -> SqlExpression { + // The closure is its own context; the identity statement handler is what + // makes subqueries opaque. It hands the `Box` straight back, so the opaque + // path -- which is most callers -- does no work at all for a subquery. + map_children_crossing(expr, &mut f, |f, e| f(e), |_, stmt| stmt) +} + +/// Rebuild `expr`, replacing each direct child expression with `f(ctx, child)` +/// **and** each directly nested subquery statement with `f_stmt(ctx, stmt)`. +/// +/// This is the primitive [`map_children`] is built on; it is the only +/// exhaustive match over `SqlExpression` in the rewrite path. Callers that must +/// reach into nested statements (CTE hoisting, INTO removal, scope-independent +/// operator rewrites) use this instead of hand-listing the subquery variants, +/// so a newly added subquery-bearing variant breaks the build here rather than +/// being silently skipped at each call site. +/// +/// `ctx` carries whatever mutable state the two closures share — typically the +/// transformer itself. See the module docs for why it is a parameter rather +/// than a capture. +/// +/// `f_stmt` takes and returns the `Box`, not the statement, so that the opaque +/// case ([`map_children`], whose handler is `|_, stmt| stmt`) is a passthrough +/// rather than an unbox/realloc of a large struct at every subquery. +pub fn map_children_crossing( + expr: SqlExpression, + ctx: &mut C, + mut f: impl FnMut(&mut C, SqlExpression) -> SqlExpression, + mut f_stmt: impl FnMut(&mut C, Box) -> Box, ) -> SqlExpression { match expr { // ---- Leaves: nothing to walk ---- @@ -70,8 +121,10 @@ pub fn map_children( | SqlExpression::DateTimeConstructor { .. } | SqlExpression::DateTimeToday { .. }) => e, - // ---- Scope boundary: the inner statement is deliberately untouched ---- - e @ SqlExpression::ScalarSubquery { .. } => e, + // ---- Scope boundary: only `f_stmt` may touch the inner statement ---- + SqlExpression::ScalarSubquery { query } => SqlExpression::ScalarSubquery { + query: f_stmt(ctx, query), + }, // ---- Same-scope children ---- SqlExpression::MethodCall { @@ -81,14 +134,14 @@ pub fn map_children( } => SqlExpression::MethodCall { object, method, - args: args.into_iter().map(&mut f).collect(), + args: args.into_iter().map(|e| f(&mut *ctx, e)).collect(), }, SqlExpression::ChainedMethodCall { base, method, args } => { SqlExpression::ChainedMethodCall { - base: Box::new(f(*base)), + base: Box::new(f(&mut *ctx, *base)), method, - args: args.into_iter().map(&mut f).collect(), + args: args.into_iter().map(|e| f(&mut *ctx, e)).collect(), } } @@ -98,7 +151,7 @@ pub fn map_children( distinct, } => SqlExpression::FunctionCall { name, - args: args.into_iter().map(&mut f).collect(), + args: args.into_iter().map(|e| f(&mut *ctx, e)).collect(), distinct, }, @@ -107,11 +160,11 @@ pub fn map_children( args, mut window_spec, } => { - let args = args.into_iter().map(&mut f).collect(); + let args = args.into_iter().map(|e| f(&mut *ctx, e)).collect(); // partition_by is Vec; only order_by carries expressions. for item in &mut window_spec.order_by { let taken = std::mem::replace(&mut item.expr, SqlExpression::Null); - item.expr = f(taken); + item.expr = f(&mut *ctx, taken); } SqlExpression::WindowFunction { name, @@ -121,29 +174,29 @@ pub fn map_children( } SqlExpression::BinaryOp { left, op, right } => SqlExpression::BinaryOp { - left: Box::new(f(*left)), + left: Box::new(f(&mut *ctx, *left)), op, - right: Box::new(f(*right)), + right: Box::new(f(&mut *ctx, *right)), }, SqlExpression::InList { expr, values } => SqlExpression::InList { - expr: Box::new(f(*expr)), - values: values.into_iter().map(&mut f).collect(), + expr: Box::new(f(&mut *ctx, *expr)), + values: values.into_iter().map(|e| f(&mut *ctx, e)).collect(), }, SqlExpression::NotInList { expr, values } => SqlExpression::NotInList { - expr: Box::new(f(*expr)), - values: values.into_iter().map(&mut f).collect(), + expr: Box::new(f(&mut *ctx, *expr)), + values: values.into_iter().map(|e| f(&mut *ctx, e)).collect(), }, SqlExpression::Between { expr, lower, upper } => SqlExpression::Between { - expr: Box::new(f(*expr)), - lower: Box::new(f(*lower)), - upper: Box::new(f(*upper)), + expr: Box::new(f(&mut *ctx, *expr)), + lower: Box::new(f(&mut *ctx, *lower)), + upper: Box::new(f(&mut *ctx, *upper)), }, SqlExpression::Not { expr } => SqlExpression::Not { - expr: Box::new(f(*expr)), + expr: Box::new(f(&mut *ctx, *expr)), }, SqlExpression::CaseExpression { @@ -153,11 +206,11 @@ pub fn map_children( when_branches: when_branches .into_iter() .map(|b| WhenBranch { - condition: Box::new(f(*b.condition)), - result: Box::new(f(*b.result)), + condition: Box::new(f(&mut *ctx, *b.condition)), + result: Box::new(f(&mut *ctx, *b.result)), }) .collect(), - else_branch: else_branch.map(|e| Box::new(f(*e))), + else_branch: else_branch.map(|e| Box::new(f(&mut *ctx, *e))), }, SqlExpression::SimpleCaseExpression { @@ -165,42 +218,42 @@ pub fn map_children( when_branches, else_branch, } => SqlExpression::SimpleCaseExpression { - expr: Box::new(f(*expr)), + expr: Box::new(f(&mut *ctx, *expr)), when_branches: when_branches .into_iter() .map(|b| SimpleWhenBranch { - value: Box::new(f(*b.value)), - result: Box::new(f(*b.result)), + value: Box::new(f(&mut *ctx, *b.value)), + result: Box::new(f(&mut *ctx, *b.result)), }) .collect(), - else_branch: else_branch.map(|e| Box::new(f(*e))), + else_branch: else_branch.map(|e| Box::new(f(&mut *ctx, *e))), }, SqlExpression::Unnest { column, delimiter } => SqlExpression::Unnest { - column: Box::new(f(*column)), + column: Box::new(f(&mut *ctx, *column)), delimiter, }, - // ---- Subquery variants: walk the same-scope operand, skip the statement ---- + // ---- Subquery variants: same-scope operand via `f`, statement via `f_stmt` ---- SqlExpression::InSubquery { expr, subquery } => SqlExpression::InSubquery { - expr: Box::new(f(*expr)), - subquery, + expr: Box::new(f(&mut *ctx, *expr)), + subquery: f_stmt(ctx, subquery), }, SqlExpression::NotInSubquery { expr, subquery } => SqlExpression::NotInSubquery { - expr: Box::new(f(*expr)), - subquery, + expr: Box::new(f(&mut *ctx, *expr)), + subquery: f_stmt(ctx, subquery), }, SqlExpression::InSubqueryTuple { exprs, subquery } => SqlExpression::InSubqueryTuple { - exprs: exprs.into_iter().map(&mut f).collect(), - subquery, + exprs: exprs.into_iter().map(|e| f(&mut *ctx, e)).collect(), + subquery: f_stmt(ctx, subquery), }, SqlExpression::NotInSubqueryTuple { exprs, subquery } => { SqlExpression::NotInSubqueryTuple { - exprs: exprs.into_iter().map(&mut f).collect(), - subquery, + exprs: exprs.into_iter().map(|e| f(&mut *ctx, e)).collect(), + subquery: f_stmt(ctx, subquery), } } } @@ -209,8 +262,27 @@ pub fn map_children( /// Call `f` on each direct child expression of `expr`. /// /// The borrowing counterpart to [`map_children`], for collectors that only read. -/// Same scope rules apply. +/// Same scope rules apply: subquery statements are not visited. Use +/// [`visit_children_crossing`] when you need to reach them. pub fn visit_children<'a>(expr: &'a SqlExpression, mut f: impl FnMut(&'a SqlExpression)) { + // The closure is its own context; the no-op statement handler is what makes + // subqueries opaque. + visit_children_crossing(expr, &mut f, |f, e| f(e), |_, _| {}); +} + +/// Call `f` on each direct child expression of `expr` **and** `f_stmt` on each +/// directly nested subquery statement. +/// +/// The borrowing counterpart to [`map_children_crossing`], and the primitive +/// [`visit_children`] is built on. As there, the subquery-bearing variants are +/// listed only here, so a new one is a compile error rather than a silent skip +/// at every call site, and `ctx` carries the state the two closures share. +pub fn visit_children_crossing<'a, C>( + expr: &'a SqlExpression, + ctx: &mut C, + mut f: impl FnMut(&mut C, &'a SqlExpression), + mut f_stmt: impl FnMut(&mut C, &'a SelectStatement), +) { match expr { // ---- Leaves: nothing to walk ---- SqlExpression::Column(_) @@ -221,55 +293,60 @@ pub fn visit_children<'a>(expr: &'a SqlExpression, mut f: impl FnMut(&'a SqlExpr | SqlExpression::DateTimeConstructor { .. } | SqlExpression::DateTimeToday { .. } => {} - // ---- Scope boundary ---- - SqlExpression::ScalarSubquery { .. } => {} + // ---- Scope boundary: only `f_stmt` may see the inner statement ---- + SqlExpression::ScalarSubquery { query } => f_stmt(ctx, query), // ---- Same-scope children ---- SqlExpression::MethodCall { args, .. } | SqlExpression::FunctionCall { args, .. } => { - args.iter().for_each(f); + args.iter().for_each(|e| f(&mut *ctx, e)); } SqlExpression::ChainedMethodCall { base, args, .. } => { - f(base); - args.iter().for_each(f); + f(&mut *ctx, base); + args.iter().for_each(|e| f(&mut *ctx, e)); } SqlExpression::WindowFunction { args, window_spec, .. } => { - args.iter().for_each(&mut f); + args.iter().for_each(|e| f(&mut *ctx, e)); // partition_by is Vec; only order_by carries expressions. - window_spec.order_by.iter().for_each(|item| f(&item.expr)); + window_spec + .order_by + .iter() + .for_each(|item| f(&mut *ctx, &item.expr)); } SqlExpression::BinaryOp { left, right, .. } => { - f(left); - f(right); + f(&mut *ctx, left); + f(&mut *ctx, right); } SqlExpression::InList { expr, values } | SqlExpression::NotInList { expr, values } => { - f(expr); - values.iter().for_each(f); + f(&mut *ctx, expr); + values.iter().for_each(|e| f(&mut *ctx, e)); } SqlExpression::Between { expr, lower, upper } => { - f(expr); - f(lower); - f(upper); + f(&mut *ctx, expr); + f(&mut *ctx, lower); + f(&mut *ctx, upper); } - SqlExpression::Not { expr } | SqlExpression::Unnest { column: expr, .. } => f(expr), + SqlExpression::Not { expr } | SqlExpression::Unnest { column: expr, .. } => { + f(&mut *ctx, expr) + } SqlExpression::CaseExpression { when_branches, else_branch, } => { for branch in when_branches { - f(&branch.condition); - f(&branch.result); + f(&mut *ctx, &branch.condition); + f(&mut *ctx, &branch.result); } if let Some(e) = else_branch { - f(e); + f(&mut *ctx, e); } } @@ -278,23 +355,28 @@ pub fn visit_children<'a>(expr: &'a SqlExpression, mut f: impl FnMut(&'a SqlExpr when_branches, else_branch, } => { - f(expr); + f(&mut *ctx, expr); for branch in when_branches { - f(&branch.value); - f(&branch.result); + f(&mut *ctx, &branch.value); + f(&mut *ctx, &branch.result); } if let Some(e) = else_branch { - f(e); + f(&mut *ctx, e); } } - // ---- Subquery variants: same-scope operand only ---- - SqlExpression::InSubquery { expr, .. } | SqlExpression::NotInSubquery { expr, .. } => { - f(expr) + // ---- Subquery variants: same-scope operand via `f`, statement via `f_stmt` ---- + SqlExpression::InSubquery { expr, subquery } + | SqlExpression::NotInSubquery { expr, subquery } => { + f(&mut *ctx, expr); + f_stmt(ctx, subquery); } - SqlExpression::InSubqueryTuple { exprs, .. } - | SqlExpression::NotInSubqueryTuple { exprs, .. } => exprs.iter().for_each(f), + SqlExpression::InSubqueryTuple { exprs, subquery } + | SqlExpression::NotInSubqueryTuple { exprs, subquery } => { + exprs.iter().for_each(|e| f(&mut *ctx, e)); + f_stmt(ctx, subquery); + } } } @@ -402,6 +484,63 @@ mod tests { assert_eq!(columns(cond), vec!["outer_col"]); } + /// The mirror of the test above, and the property the whole `crossing` + /// split exists to provide: the same variants that `map_children` treats as + /// opaque *must* be reachable through the crossing forms. If a new + /// subquery-bearing variant is ever added and wired only into the leaves, + /// this is what notices. + #[test] + fn crossing_walkers_do_reach_into_subqueries() { + // Visit side: the nested statement is handed to `f_stmt`. + let expr = expr_of("(SELECT MAX(inner_col) FROM other)"); + let mut statements_seen = 0; + visit_children_crossing(&expr, &mut statements_seen, |_, _| {}, |n, _| *n += 1); + assert_eq!( + statements_seen, 1, + "a scalar subquery's statement must be reachable when crossing" + ); + + // Map side: and it can be rewritten in place. + let rewritten = map_children_crossing( + expr, + &mut (), + |_, e| e, + |_, mut stmt| { + stmt.limit = Some(1); + stmt + }, + ); + match rewritten { + SqlExpression::ScalarSubquery { query } => assert_eq!(query.limit, Some(1)), + other => panic!("expected a scalar subquery, got {other:?}"), + } + } + + /// The tuple forms carry *both* same-scope operands and a nested statement; + /// crossing must reach both, not one or the other. + #[test] + fn crossing_reaches_tuple_subquery_operands_and_statement() { + let stmt = Parser::new("SELECT a FROM t WHERE (a, b) IN (SELECT x, y FROM u)") + .parse() + .expect("should parse"); + let cond = &stmt.where_clause.expect("where clause").conditions[0].expr; + + let mut ctx = (Vec::::new(), 0); + visit_children_crossing( + cond, + &mut ctx, + |ctx, e| { + if let SqlExpression::Column(c) = e { + ctx.0.push(c.name.clone()); + } + }, + |ctx, _| ctx.1 += 1, + ); + + assert_eq!(ctx.0, vec!["a", "b"], "same-scope operands must be visited"); + assert_eq!(ctx.1, 1, "the subquery statement must be visited too"); + } + #[test] fn map_children_rewrites_nested_expressions() { let expr = expr_of("CASE WHEN a > 1 THEN UPPER(b) ELSE c END"); From 163f49e830a86a835ca635a7d8ea2ed967c7d8b0 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 18 Jul 2026 18:08:41 +0100 Subject: [PATCH 2/2] docs(refactoring): record PR number for the R2 crossing entry Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/ENGINE_REFACTORING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index 44def674..38f36d31 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -256,4 +256,4 @@ R5 dead code ─────── opportunistic | 2026-07-18 | R2 partial: `walk.rs` traversal helpers landed (additive) | #31 | | 2026-07-18 | R2 group 1: the three boundary-crossing transformers migrated; two silent bugs fixed | #33 | | 2026-07-18 | R3 evidence: P9–P12 filed after probing the engine; corpus gains tier 07 (grouping) | — | -| 2026-07-18 | R2: `*_crossing` helpers become the primitives; the three crossing transformers stop hand-listing subquery variants | — | +| 2026-07-18 | R2: `*_crossing` helpers become the primitives; the three crossing transformers stop hand-listing subquery variants | #35 |