gfql/ir: migrate WhereClause.expr readers to expr_tree, then remove the field (#1200 slice 5)

[lmeyerov]

Context

#1200 has shipped:

• Slice 1 (feat(gfql/ir): expose Lark boolean-expression tree as WhereClause.expr_tree (#1200 slice 1/N) #1202, merged): WhereClause.expr_tree: Optional[BooleanExpr] exposed as additive field alongside the existing expr: Optional[ExpressionText].
• Slice 2 (feat(gfql/binder): walk expr_tree, emit one BoundPredicate per top-level AND (#1200 slice 2/N) #1207, merged): binder walks expr_tree when populated; falls back to expr.text when not.
• Slice 3a (PR fix(cypher): walk expr_tree to lift label-only WHERE predicates (#1194) #1209, in review): parser-side generic_where_clause walks tree; closes gfql/cypher: resolve Lark ambiguity between where_predicates and expr (grammar-level follow-up to #1125) #1194.

The WhereClause.expr field is still load-bearing across the rest of the codebase. Two examples from cypher/lowering.py:

# lowering.py:5919  — needs surface text
expr_text = query.where.expr.text.strip()

# lowering.py:8142-8144  — needs source span for error reporting
value=where.expr.text,
line=where.expr.span.line,
column=where.expr.span.column,

This issue tracks migrating every where.expr reader to consume expr_tree (or text/span derived from it), then removing the WhereClause.expr field. ExpressionText itself stays — other AST fields use it (WithItem.expression, OrderByItem.expression, CallArg.args, WithClause.where, MatchClause.where, page values).

Reader inventory (master @ 0b1fb0c, post-#1207)

Read sites

File	Line	Access pattern	What's needed	Migration approach
frontends/cypher/binder.py	1011	where.expr is not None → where.expr.text	text	drop the elif branch (expr_tree should always be populated when expr is, post-#1209)
cypher/lowering.py	5917	query.where.expr is None (gate)	gate	switch to expr_tree is None
cypher/lowering.py	5919	query.where.expr.text.strip()	text	reconstruct from expr_tree via _boolean_expr_to_text (see #1207's binder helper)
cypher/lowering.py	5957	.text for error value	text	same
cypher/lowering.py	6155-6157	.text	text	same
cypher/lowering.py	6163-6165	reentry_where.expr.text	text	same
cypher/lowering.py	6240-6242	.text	text	same
cypher/lowering.py	6322-6329	passes query.where.expr to row_where; assigns	full ExpressionText object	this site needs careful conversion since it forwards the value
cypher/lowering.py	7512-7515	where_clause.expr for rewrite_expr	full ExpressionText	the rewrite helper needs an expr_tree-aware variant
cypher/lowering.py	7530-7533	same for reentry_where	same	same
cypher/lowering.py	7997-7998	query.where.expr appended to pre_join_filters	full ExpressionText	downstream consumers of pre_join_filters need migration too
cypher/lowering.py	8129-8144	where.expr + .text + .span.line/.column	text + span	use expr_tree.span for span; _boolean_expr_to_text(expr_tree) for text
cypher/ast_normalizer.py	432-436	where.expr + passes to _rewrite_shortest_path_expr_text	full ExpressionText	rewrite helper needs to walk expr_tree instead of text
cypher/ast_normalizer.py	508-512	constructs new WhereClause(expr=..., expr_tree=...)	both	once expr is removed, drop the param

(Total: ~14 distinct read sites across 3 files; some sites have multiple references.)

Write sites

File	Line	Construction
cypher/parser.py	1170, 1206	structured-predicates path: WhereClause(predicates=..., expr=None, span=...) — already drops expr
cypher/parser.py	1209	generic path: WhereClause(predicates=(), expr=ExpressionText(...), expr_tree=expr_tree, span=span) — needs to drop expr arg
cypher/parser.py	other WhereClause(...) constructions	check each
cypher/ast_normalizer.py	511	constructs WhereClause(expr=..., expr_tree=...) — needs to drop expr arg

Pre-requisite: parser invariant

For this migration to be safe, the parser must guarantee that expr_tree is not None everywhere expr is not None was previously populated. Verify by:

1. Auditing _build_transformer for every WhereClause(...) construction.
2. Adding an invariant test: assert (where.expr_tree is None) == (where.expr is None) (or, after removal, assert where.expr_tree is None or has_meaning(where.expr_tree)).

This may surface edge cases — e.g. WHERE bodies that produce text but no BooleanExpr (single non-label atoms going through generic_where_clause post-#1209). If so, those cases need either a single-atom BooleanExpr synthesis or a different downstream signal.

Helper to lift to shared module

_boolean_expr_to_text currently lives in frontends/cypher/binder.py (introduced by #1207) for binder-internal use. To support lowering reading text from expr_tree, this helper should likely move to a shared location (cypher/ast.py as a BooleanExpr method, or a sibling cypher/_expr_text.py).

Done when

• WhereClause.expr field is removed from cypher/ast.py:197.
• All 14+ read sites migrated to expr_tree-derived text/span.
• All WhereClause(...) construction sites drop the expr= argument.
• _boolean_expr_to_text (or equivalent) is in a shared module so lowering can import it.
• Full GFQL test suite green (tests/compute/gfql/).
• ExpressionText class itself stays (other AST fields still use it).

Out of scope

• expr_split.py retirement (independent, blocked on Cypher/GFQL WHERE cannot mix generic row predicates with pattern predicates #1031).
• _split_conjuncts in predicate_pushdown (load-bearing for IR-direct, see closed gfql/passes: predicate_pushdown drops _split_conjuncts in favor of pre-split BoundPredicates from #1207 (#1200 slice 3b) #1210).
• Lowering's _CYPHER_BARE_LABEL_PREDICATE_RE (different operation than parser's fullmatch, see closed gfql/cypher: retire _CYPHER_BARE_LABEL_PREDICATE_RE second copy in lowering.py (#1200 slice 3d) #1211).
• The WhereClause constructions in mixed-pattern handlers (where_pattern_and_expr_clause, expr_and_where_pattern_clause at parser.py:1046, 1058) — these may reconstruct expr_text from sliced source rather than walking a tree; check whether they need their own migration approach.

Estimated cost

Multi-PR likely. Reasonable slicing:

• Sub-PR A: lift _boolean_expr_to_text to shared module; add invariant assertion that (expr_tree is None) == (expr is None).
• Sub-PR B: migrate the simple text-only readers (binder L1011, lowering L5919, 5957, 6157, 6165, 6242).
• Sub-PR C: migrate the ExpressionText-passthrough readers (lowering L6322, 7515, 7533, 7998 — these need rewrite helpers updated too).
• Sub-PR D: migrate ast_normalizer.py readers; drop expr= from constructions.
• Sub-PR E: drop the WhereClause.expr field.

Each PR keeps the invariant assertion green; the field disappears in the last one.

Related

• gfql/ir: expose Lark and_op/or_op tree in WhereClause so passes walk structure instead of re-parsing text #1200 — umbrella
• feat(gfql/ir): expose Lark boolean-expression tree as WhereClause.expr_tree (#1200 slice 1/N) #1202 — slice 1 (producer)
• feat(gfql/binder): walk expr_tree, emit one BoundPredicate per top-level AND (#1200 slice 2/N) #1207 — slice 2 (binder reader migrated)
• fix(cypher): walk expr_tree to lift label-only WHERE predicates (#1194) #1209 — slice 3a (parser-side cleanup; in review)
• Cypher/GFQL WHERE cannot mix generic row predicates with pattern predicates #1031 — independent grammar work for mixed pattern+expr WHERE

[lmeyerov]

Sub-PR A merged in #1214: lifted boolean_expr_to_text helper to shared module + established expr_tree invariant. Sub-PR B (text-only reader migration) is the natural next slice.

[lmeyerov]

Sub-PR B merged in #1216. Migrated 7 text-only readers in binder.py + lowering.py to consume boolean_expr_to_text(expr_tree) directly. Dropped the dead elif where.expr is not None binder fallback (unreachable per #1214 invariant) plus its now-redundant test fixture.

Sub-PR C in flight next from this session — handles ExpressionText-passthrough callers (rewrite_expr integration in lowering, pre_join_filters, row_where passthrough, _extract_relationship_type_where, plus the entangled span readers and _rewrite_shortest_path_expr_text in ast_normalizer). Going with Option B for rewrite_expr (text round-trip + single-atom resynthesis) per the call posted earlier; will also fix the latent text/tree staleness bug at lowering.py:7515/7533.

After C lands, ready for D + E (drop expr= from WhereClause(...) writes + remove the field) — owned by whoever did #1214.

[lmeyerov]

Sub-PR C merged in #1218. All ExpressionText-passthrough readers migrated; latent text/tree staleness bug at replace(where, expr=rewrite(where.expr, ...)) fixed as a side effect.

Reader-side migration is complete. Only remaining where.expr reference in the codebase is the constructor passthrough at ast_normalizer.py:520-526 (expr=query.where.expr inside a re-construction) — that's part of D's scope (drop expr= from WhereClause(...) writers).

D + E are unblocked. Per the split, they're owned by whoever did #1214. Suggested combined PR: drop expr= from the 5 parser construction sites (parser.py:1031, 1043, 1176, 1221) + the 1 ast_normalizer site (509-512), update or delete the invariant test (test_where_clause_expr_tree_invariant.py — becomes degenerate when the field disappears), and remove the field from cypher/ast.py:197.

After D + E merge, #1213 closes.