refactor(walk): make the boundary-crossing forms the traversal primitives

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 | #35 |

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<String>) {
-        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

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)),
+            ),
         }
     }
 

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)),
+        )
     }
 }