From 71ff4eacf6def847b373cca4cc0e2496159133cb Mon Sep 17 00:00:00 2001 From: Mark <399551+mwiebe@users.noreply.github.com> Date: Mon, 28 Sep 2026 17:35:35 -0700 Subject: [PATCH] fix(expr): keep the live memory footprint accurate when errors are absorbed The evaluator tracks two kinds of resource counters: the live footprint (current_memory, which each new allocation is checked against) and the cumulative spend (peak_memory and operation_count, which are never refunded). Several code paths left the live footprint wrong after a value was discarded or a sub-expression failed. When the error propagated to the top this did not matter, because the counters were never read again. But two constructs absorb value errors and keep evaluating: an if/else whose test is unresolved evaluates both branches, and an and/or chain evaluates operands after an unresolved one. Each continued on an evaluator whose footprint no longer matched what was held. This fixes those paths and adds one helper that every absorbing construct uses. Under-counting (footprint too low, so an expression could exceed the memory limit): - eval_compare passed clones of its operands to dispatch, which releases what it is given, and then released the originals too. Every comparison subtracted its operands twice, and the excess came out of whatever the enclosing expression still held. With a 1 MB string held and a comparison of two 200 KB strings, the footprint dropped by 400 KB more than was freed, and a 1.7 MB expression passed a 1.5 MB limit. Each link now hands its operands to dispatch and, when the chain continues, clones the right operand first and tracks the clone as the carried value. Over-counting (footprint too high, so valid expressions failed): - dispatch_with_node and dispatch_with_span released a call's operands only on success. A failed operator's operands stayed charged. Both now share dispatch_released, which releases on either path. eval_call already did this for function arguments. - eval_ifexp never released the branch values when its test was unresolved. The result is an Unresolved carrying only a type, so every branch value is now released: the surviving branch when one fails, both when both succeed. - eval_boolop never released operands its result did not carry: the unresolved placeholder, a concrete operand replaced by the next, or a speculative operand that did not decide the result. All are released. - eval_list tracked each element and then tracked the list built from them without releasing the elements, so a literal was charged for the list and every element again, in its own construction check and for as long as it was held. One 600 KB element read as 1.2 MB. Elements are released as they are consumed into the list. New helper eval_speculative is the one place the two absorption rules live, used by eval_ifexp's branches and eval_boolop's post-unresolved operands. A budget error (directly or inside a compound error) propagates immediately; a value error is absorbed and the live footprint is reset to what it was before the attempt, while peak memory and the operation count keep what the attempt consumed. One behaviour change: a budget error in the if-branch now propagates before the else-branch is evaluated. Previously both branches always ran, and a budget failure paired with a value failure surfaced inside a "Both branches fail" compound error; now the budget error surfaces as itself. Two pinned diagnostics move by exactly the released element trackers of a three-element Int list literal (192 bytes): `[1, 2, 3] * 10000000` 1920000576 -> 1920000384, and a comprehension over `[1, 2, 3]` 1800768 -> 1800576. One test that asserted the compound error is rewritten to assert the fail-fast behaviour, and one byte-count expectation is updated for the released boolop placeholder. Nine new tests pin the releases and each fails on the previous code; a tenth guards the comparison chain against a leak. specs/expr/evaluator.md gains a Speculative evaluation section and updates BoolOp, IfExp, Compare, List and Dispatch Flow to match. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> --- crates/openjd-expr/src/eval/evaluator.rs | 188 +++++++++++----- .../tests/integration/test_memory.rs | 210 ++++++++++++++++-- specs/expr/evaluator.md | 87 ++++++-- 3 files changed, 397 insertions(+), 88 deletions(-) diff --git a/crates/openjd-expr/src/eval/evaluator.rs b/crates/openjd-expr/src/eval/evaluator.rs index 4f57f320..4ec74a28 100644 --- a/crates/openjd-expr/src/eval/evaluator.rs +++ b/crates/openjd-expr/src/eval/evaluator.rs @@ -60,6 +60,17 @@ fn contains_budget_error(err: &ExpressionError) -> bool { ) || err.sub_errors().iter().any(contains_budget_error) } +/// Outcome of evaluating a sub-expression whose failure an enclosing +/// construct may absorb (see [`Evaluator::eval_speculative`]). +enum Speculative { + /// Evaluated to a value; it is tracked. + Value(ExprValue), + /// Failed with a value error that the caller absorbs. The + /// evaluator's live footprint has been reset to what it was before + /// the attempt; the spend (peak memory, operation count) stands. + Absorbed(ExpressionError), +} + /// Default memory limit: 100 million bytes. pub const DEFAULT_MEMORY_LIMIT: usize = 100_000_000; // 100 million bytes per spec @@ -464,24 +475,43 @@ impl<'a> Evaluator<'a> { self.current_memory = self.current_memory.saturating_sub(size); } - fn dispatch_with_node( + /// Call a library function. The arguments are consumed by the call, + /// so they are released from the live footprint whether the call + /// succeeds or fails; a successful result is tracked. Releasing on + /// failure matters when an enclosing construct absorbs the error and + /// keeps evaluating (see `eval_speculative`): the operands are gone + /// and must not stay charged. `eval_call` releases its arguments + /// before dispatch for the same reason. + /// + /// The operation-count check before the call is the one exit that + /// does not release. It fails only with a budget error, which is + /// never absorbed, so the footprint is not read afterwards. + fn dispatch_released( &mut self, name: &str, args: Vec, - node: Option<&ast::Expr>, ) -> Result { self.count_op()?; let input_size: usize = args.iter().map(|a| a.memory_size()).sum(); let lib = self.library; - let result = lib.call(name, &args, self).map_err(|e| { + let result = lib.call(name, &args, self); + self.current_memory = self.current_memory.saturating_sub(input_size); + self.track(result?) + } + + fn dispatch_with_node( + &mut self, + name: &str, + args: Vec, + node: Option<&ast::Expr>, + ) -> Result { + self.dispatch_released(name, args).map_err(|e| { if let (Some(src), Some(n)) = (self.expr_source, node) { e.with_node(src, n) } else { e } - })?; - self.current_memory = self.current_memory.saturating_sub(input_size); - self.track(result) + }) } /// Like `dispatch_with_node` but uses a `TextRange` for error positioning, @@ -492,18 +522,13 @@ impl<'a> Evaluator<'a> { args: Vec, range: ruff_text_size::TextRange, ) -> Result { - self.count_op()?; - let input_size: usize = args.iter().map(|a| a.memory_size()).sum(); - let lib = self.library; - let result = lib.call(name, &args, self).map_err(|e| { + self.dispatch_released(name, args).map_err(|e| { if let Some(src) = self.expr_source { e.with_span(src, range.start().to_usize(), range.end().to_usize()) } else { e } - })?; - self.current_memory = self.current_memory.saturating_sub(input_size); - self.track(result) + }) } fn eval_number(&mut self, n: &ast::ExprNumberLiteral) -> Result { @@ -759,34 +784,31 @@ impl<'a> Evaluator<'a> { ast::BoolOp::Or => false, }); let mut seen_unresolved = false; - for node in &b.values { + let last_index = b.values.len().saturating_sub(1); + for (index, node) in b.values.iter().enumerate() { + let is_last = index == last_index; if seen_unresolved { - // After an unresolved operand, suppress errors in subsequent operands - // (the unresolved value might short-circuit at runtime). - // But if a subsequent operand determines the result, return it. - match self.eval_node(node, None) { - Ok(val) => match b.op { + // After an unresolved operand, later operands are still + // evaluated to catch type errors, but a value error is + // absorbed: the unresolved operand might short-circuit + // at runtime. A concrete operand that decides the result + // is still returned. + match self.eval_speculative(node, None)? { + Speculative::Value(val) => match b.op { ast::BoolOp::And => { if matches!(&val, ExprValue::Null | ExprValue::Bool(false)) { return Ok(val); } + self.release(&val); } ast::BoolOp::Or => { if !matches!(&val, ExprValue::Null | ExprValue::Bool(false)) { return Ok(val); } + self.release(&val); } }, - Err(e) => { - // Suppressed — unresolved might short-circuit. - // Except budget exceedances: the memory and - // operations were spent in this evaluation no - // matter what a runtime short-circuit skips - // (same rule as `eval_ifexp`). - if contains_budget_error(&e) { - return Err(e); - } - } + Speculative::Absorbed(_) => {} } continue; } @@ -798,6 +820,9 @@ impl<'a> Evaluator<'a> { // `int` target must not try to coerce the `true`). last = self.eval_node(node, None)?; if last.is_unresolved() { + // The result will be a fresh `Unresolved(BOOL)`, so this + // placeholder is not kept. + self.release(&last); seen_unresolved = true; continue; } @@ -814,6 +839,12 @@ impl<'a> Evaluator<'a> { } } } + // This operand did not decide the result and is replaced by + // the next one. The final operand is returned, so it stays + // tracked. + if !is_last { + self.release(&last); + } } if seen_unresolved { return self.track(ExprValue::unresolved(ExprType::BOOL)); @@ -838,16 +869,24 @@ impl<'a> Evaluator<'a> { // or `Param.Path < 1`) is refused at validation time rather than on // the worker. Only the value is deferred, not the type check. let mut seen_unresolved = false; - for (op, right_node) in c.ops.iter().zip(c.comparators.iter()) { + let last_index = c.ops.len().saturating_sub(1); + for (index, (op, right_node)) in c.ops.iter().zip(c.comparators.iter()).enumerate() { let right = self.eval_node(right_node, None)?; let dispatch = table.cmpop(*op)?; let op_name = dispatch.dunder; + // Dispatch consumes and releases both operands. When the + // chain continues, `right` is also the next link's `left`, + // so it is cloned first and the clone is tracked below as + // the value the chain carries. Each operand is therefore + // charged once and released by the link that consumes it. + let is_last = index == last_index; + let carried = if is_last { None } else { Some(right.clone()) }; // For 'in'/'not in', container is first arg (right), item is second (left) let args = if dispatch.container_first { - vec![right.clone(), left.clone()] + vec![right, left] } else { - vec![left.clone(), right.clone()] + vec![left, right] }; // Use the compare expression's range for error caret positioning @@ -865,14 +904,14 @@ impl<'a> Evaluator<'a> { }; self.release(&result_val); if !result { - self.release(&left); - self.release(&right); return self.track(ExprValue::Bool(false)); } - self.release(&left); - left = right; + match carried { + // The carried operand stays live for the next link. + Some(next_left) => left = self.track(next_left)?, + None => break, + } } - self.release(&left); if seen_unresolved { return self.track(ExprValue::unresolved(ExprType::BOOL)); } @@ -910,11 +949,15 @@ impl<'a> Evaluator<'a> { }); } self.release(&test); - // Try both branches, catching errors (e.g. fail() in one branch) - let body = self.eval_node(&i.body, target); - let orelse = self.eval_node(&i.orelse, target); + // Evaluate both branches speculatively. A budget error in + // either propagates immediately. A value error is absorbed, + // because a runtime with the test resolved may never take + // that branch. The result is an `Unresolved` carrying only a + // type, so every branch value is released. + let body = self.eval_speculative(&i.body, target)?; + let orelse = self.eval_speculative(&i.orelse, target)?; match (body, orelse) { - (Err(be), Err(oe)) => { + (Speculative::Absorbed(be), Speculative::Absorbed(oe)) => { let mut msg = format!( "Both branches fail in the if/else:\n if-branch: {}\n", be.message() @@ -931,25 +974,21 @@ impl<'a> Evaluator<'a> { } Err(err) } - (Ok(b), Err(oe)) => { - if contains_budget_error(&oe) { - self.release(&b); - return Err(oe); - } + (Speculative::Value(b), Speculative::Absorbed(_)) => { let t = unwrap_unresolved(&b.expr_type()); + self.release(&b); self.track(ExprValue::unresolved(t)) } - (Err(be), Ok(o)) => { - if contains_budget_error(&be) { - self.release(&o); - return Err(be); - } + (Speculative::Absorbed(_), Speculative::Value(o)) => { let t = unwrap_unresolved(&o.expr_type()); + self.release(&o); self.track(ExprValue::unresolved(t)) } - (Ok(b), Ok(o)) => { + (Speculative::Value(b), Speculative::Value(o)) => { let bt = unwrap_unresolved(&b.expr_type()); let ot = unwrap_unresolved(&o.expr_type()); + self.release(&b); + self.release(&o); if bt == ot { self.track(ExprValue::unresolved(bt)) } else { @@ -1203,6 +1242,19 @@ impl<'a> Evaluator<'a> { } } } + // The elements were tracked as they were evaluated. From here + // they are consumed into the list, which is charged as one value + // (or as a type-only placeholder when any element is + // unresolved), so release them first. Otherwise the literal would + // be charged for the list and for every element again, both in + // the construction pre-check and for as long as the list is + // held. The error returns above leave the elements charged like + // any failed sub-expression's operands; an enclosing construct + // that absorbs the error resets the footprint (see + // `eval_speculative`). + for e in &elements { + self.release(e); + } if elements.iter().any(|e| e.is_unresolved()) { let val = unresolved_list_from_elements(&elements)?; return self.track(val); @@ -1425,6 +1477,38 @@ impl<'a> Evaluator<'a> { self.current_memory = baseline; } + /// Evaluate `node` where the caller may absorb a failure: the + /// branches of an unresolved-test conditional, or the operands of a + /// boolop after an unresolved one. Both absorption rules live here so + /// that every absorbing construct applies both: + /// + /// - A budget error (`MemoryLimitExceeded` / `OperationLimitExceeded`, + /// directly or inside a compound error's sub-errors) is returned as + /// `Err`. The memory and operations were spent regardless of what + /// a runtime would skip. + /// - A value error is returned as `Absorbed`, and the live footprint + /// is reset to what it was before the attempt. The failed + /// sub-expression's values are gone. Peak memory and the operation + /// count keep what the attempt consumed. + /// + /// The caller decides what an absorbed failure means: an `Unresolved` + /// result, a short-circuit, or a compound error. + fn eval_speculative( + &mut self, + node: &ast::Expr, + target: Option<&crate::types::ExprType>, + ) -> Result { + let baseline = self.current_memory; + match self.eval_node(node, target) { + Ok(v) => Ok(Speculative::Value(v)), + Err(e) if contains_budget_error(&e) => Err(e), + Err(e) => { + self.current_memory = baseline; + Ok(Speculative::Absorbed(e)) + } + } + } + /// Conclude a list comprehension whose result cannot be computed at /// this stage — because the iterable is unresolved, or because the /// filter evaluated to an unresolved condition on a concrete diff --git a/crates/openjd-expr/tests/integration/test_memory.rs b/crates/openjd-expr/tests/integration/test_memory.rs index e7daf02c..7778d93a 100644 --- a/crates/openjd-expr/tests/integration/test_memory.rs +++ b/crates/openjd-expr/tests/integration/test_memory.rs @@ -71,13 +71,16 @@ fn list_mul_exceeds_limit() { // List multiplication checks the projected result size against the // memory limit *before* op counting, so an over-memory repetition // reports a memory error even when it would also blow the op limit. + // 1920000384 = the `[1, 2, 3]` list (charged once; its element + // literals are released when the list is built) plus the projected + // 30M-element result. let e = eval_bounded("[1, 2, 3] * 10000000", 10000) .unwrap_err() .to_string(); assert!( e.contains( &[ - "Expression memory usage (1920000576 bytes) exceeded limit (10000 bytes)\n", + "Expression memory usage (1920000384 bytes) exceeded limit (10000 bytes)\n", " [1, 2, 3] * 10000000\n", " ~~~~~~~~~~^~~~~~~~~~", ] @@ -884,9 +887,12 @@ fn memory_limit_exceeded_after_unresolved_boolop_operand_propagates() { .evaluate_with_metrics(&[&st]) }) .expect_err("the second operand's budget exceedance must propagate"); + // 10000136 = the 10 MB string plus per-value overhead. The + // unresolved first operand is released once the result is known to + // be a fresh `Unresolved(BOOL)`, so it is not in the figure. assert!( err.message() - .contains("Expression memory usage (10000200 bytes) exceeded limit (1048576 bytes)"), + .contains("Expression memory usage (10000136 bytes) exceeded limit (1048576 bytes)"), "Got: {}", err.message() ); @@ -989,16 +995,12 @@ fn boolop_does_not_reabsorb_budget_error_from_nested_conditional() { ); } -/// `contains_budget_error` recurses into compound sub-errors. When -/// *both* branches of an unresolved-test conditional fail — one with a -/// budget exceedance, one with a value error — the conditional -/// produces a compound "Both branches fail" error whose own kind is not -/// a budget kind; only the recursion can see the budget exceedance -/// inside it. Wrapped in a boolop suppression site, that compound must -/// still propagate. (Without the recursion this test fails: the -/// compound is swallowed as a plain value error.) +/// A budget error in the if-branch of an unresolved-test conditional +/// propagates before the else-branch is evaluated, and a boolop +/// suppression site does not absorb it. The else-branch's value error +/// must not appear in the message: it was never evaluated. #[test] -fn budget_error_inside_compound_both_branches_fail_error_propagates_through_boolop() { +fn budget_error_in_if_branch_propagates_before_else_branch_runs() { let mut st = SymbolTable::new(); st.set( "Session.Flag", @@ -1013,14 +1015,20 @@ fn budget_error_inside_compound_both_branches_fail_error_propagates_through_bool .with_operation_limit(DEFAULT_OPERATION_LIMIT) .evaluate_with_metrics(&[&st]) }) - .expect_err("a compound error carrying a budget exceedance must propagate"); + .expect_err("the if-branch's budget exceedance must propagate through the boolop"); let msg = err.message(); assert!( - msg.contains("Both branches fail in the if/else:"), + msg.starts_with("Expression memory usage (10000136 bytes) exceeded limit (1048576 bytes)"), "Got: {msg}" ); - assert!(msg.contains("exceeded limit (1048576 bytes)"), "Got: {msg}"); - assert!(msg.contains("Cannot convert 'nope' to int"), "Got: {msg}"); + assert!( + !msg.contains("Both branches fail"), + "the else-branch must not have run: {msg}" + ); + assert!( + !msg.contains("Cannot convert 'nope'"), + "the else-branch must not have run: {msg}" + ); } /// Control: a compound both-branches-fail error with *no* budget @@ -1235,7 +1243,10 @@ fn large_element_comprehension_exceedance_reports_true_usage() { assert_eq!( e, [ - "Expression memory usage (1800768 bytes) exceeded limit (1500000 bytes)\n", + // 1800576 = the `[1, 2, 3]` iterable (charged once) plus two + // held 600 KB elements, the third being pushed, and the Vec's + // projected slack. + "Expression memory usage (1800576 bytes) exceeded limit (1500000 bytes)\n", " ['A' * 600000 for x in [1, 2, 3]]\n", " ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~", ] @@ -1314,3 +1325,170 @@ fn abandoned_comprehension_cannot_iterate_drops_iterable_charge() { "len([x for x in 'C' * 600000] if Session.Flag else []) + len('B' * 600000)", ); } + +// ══════════════════════════════════════════════════════════════ +// Absorbed failures and discarded values leave no footprint +// ══════════════════════════════════════════════════════════════ + +/// Evaluate `expr` under a 1.5 MB limit. The expression absorbs a +/// failure or discards a value, then allocates 600 KB. If the discarded +/// value were still charged, the final allocation would exceed the +/// limit. +fn assert_fits_after_absorption(expr: &str) { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + ParsedExpression::new(expr) + .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) + .unwrap_or_else(|e| panic!("{expr}: a discarded value must not stay charged: {e}")); +} + +/// When one branch of an unresolved-test conditional fails, the other +/// branch's value is discarded (the result is an `Unresolved` carrying +/// only its type) and must be released. Both arms. +#[test] +fn ifexp_absorbing_if_branch_failure_releases_else_value() { + assert_fits_after_absorption( + "len(int('x') if Session.Flag else 'A' * 1000000) + len('B' * 600000)", + ); +} + +#[test] +fn ifexp_absorbing_else_branch_failure_releases_if_value() { + assert_fits_after_absorption( + "len('A' * 1000000 if Session.Flag else int('x')) + len('B' * 600000)", + ); +} + +/// When both branches succeed, both values are discarded for the union +/// type. Both are live together while the union is formed (2 MB peak), +/// which the limit allows; the trailing 1.5 MB allocation fits only if +/// both were released afterwards. +#[test] +fn ifexp_with_unresolved_test_releases_both_branch_values() { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + let expr = "len('A' * 1000000 if Session.Flag else 'C' * 1000000) + len('B' * 1500000)"; + ParsedExpression::new(expr) + .and_then(|p| p.with_memory_limit(2_500_000).evaluate(&[&st])) + .unwrap_or_else(|e| panic!("{expr}: discarded branch values must not stay charged: {e}")); +} + +/// `'A' * 1000000 - 1` fails inside the binop while the 1 MB string is a +/// live operand. Absorbed by a boolop after an unresolved operand, the +/// string must be released. +#[test] +fn boolop_past_unresolved_absorbing_failed_binop_releases_its_operands() { + assert_fits_after_absorption( + "[Session.Flag or 'A' * 1000000 - 1 == 'x', len('B' * 600000) > 0][1]", + ); +} + +/// The same failed binop, absorbed by a conditional instead of a boolop. +#[test] +fn ifexp_absorbing_failed_binop_releases_its_operands() { + assert_fits_after_absorption( + "len(('A' * 1000000 - 1) if Session.Flag else '') + len('B' * 600000)", + ); +} + +/// A concrete boolop operand that does not decide the result is +/// replaced by the next one and must be released, both before and after +/// the unresolved operand. The operands are the large strings themselves +/// (a non-empty string is truthy, so `and` moves past it). +#[test] +fn boolop_releases_non_deciding_concrete_operands() { + assert_fits_after_absorption( + "[('A' * 1000000 and Session.Flag and 'C' * 1000000), len('B' * 600000) > 0][1]", + ); +} + +// ══════════════════════════════════════════════════════════════ +// A list literal is charged once +// ══════════════════════════════════════════════════════════════ + +/// A list literal's elements are released when they are consumed into +/// the list, so the literal is charged for the list only, not for the +/// list and every element again. One 600 KB element plus a further +/// 300 KB fits under a 1 MB limit. +#[test] +fn list_literal_elements_are_charged_once() { + let st = SymbolTable::new(); + ParsedExpression::new("len(['C' * 600000]) + len('B' * 300000)") + .and_then(|p| p.with_memory_limit(1_000_000).evaluate(&[&st])) + .expect("a 600 KB single-element literal plus 300 KB must fit under 1 MB"); + // With a second large literal following, the limit is exceeded while + // the second element is being produced, before the second list exists + // and before `+` runs (hence the caret on `'B' * 600000`). The figure + // is one list charge plus one element in flight. + let e = ParsedExpression::new("['C' * 600000] + ['B' * 600000]") + .and_then(|p| p.with_memory_limit(1_000_000).evaluate(&[&st])) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + // 1200224 = the 600 KB single-element list (600088) plus the + // second 600 KB string as it is produced (600064), its 'B' + // operand (72) and the operator's fixed overhead. + "Expression memory usage (1200224 bytes) exceeded limit (1000000 bytes)\n", + " ['C' * 600000] + ['B' * 600000]\n", + " ~~~~^~~~~~~~", + ] + .concat() + ); +} + +// ══════════════════════════════════════════════════════════════ +// A comparison releases its operands exactly once +// ══════════════════════════════════════════════════════════════ + +/// A comparison's operands are released once, by the dispatch that +/// consumes them. With a 1 MB string held, a comparison of two 200 KB +/// strings, and a further 700 KB allocation, the live footprint is +/// 1.7 MB and must exceed a 1.5 MB limit. If the comparison released +/// its operands twice, 400 KB of the held string would be uncharged +/// and the expression would fit. +#[test] +fn comparison_releases_operands_once() { + let st = SymbolTable::new(); + let e = ParsedExpression::new( + "['A' * 1000000, string('C' * 200000 == 'D' * 200000), 'B' * 700000]", + ) + .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + // 1700272 = the held 1 MB string (1000064), the "false" result + // string (72), the 700 KB string being produced (700064) and + // its 'B' operand (72). The comparison's operands are gone. + "Expression memory usage (1700272 bytes) exceeded limit (1500000 bytes)\n", + " ['A' * 1000000, string('C' * 200000 == 'D' * 200000), 'B' * 700000]\n", + " ~~~~^~~~~~~~", + ] + .concat() + ); +} + +/// A chained comparison carries its middle operand from one link to the +/// next. The carried value is charged once across the chain and released +/// by the link that consumes it. Three 300 KB operands, two links, then +/// a 1.45 MB allocation under a 1.5 MB limit: it fits only if the chain +/// left nothing charged. This guards against a leak; the test above +/// guards against a double release. +#[test] +fn chained_comparison_carries_middle_operand_once() { + let st = SymbolTable::new(); + ParsedExpression::new("[string('A' * 300000 < 'B' * 300000 < 'C' * 300000), 'D' * 1450000]") + .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) + .expect("the chain must leave no footprint"); +} diff --git a/specs/expr/evaluator.md b/specs/expr/evaluator.md index 2739ddba..cb11d5f8 100644 --- a/specs/expr/evaluator.md +++ b/specs/expr/evaluator.md @@ -296,6 +296,12 @@ evaluates `b < c`. The intermediate value `b` is reused. All comparison operands are evaluated unconstrained (see [Target Type Propagation](#target-type-propagation)). +Memory accounting: each link hands its two operands to dispatch, which releases +them (see [Dispatch Flow](#dispatch-flow)). When the chain continues, the right +operand is cloned first and the clone is tracked as the value carried into the +next link, so each operand is charged once and released by the link that +consumes it. + Comparison stays in the evaluator (not fully delegated to the library) because the chaining logic requires control flow that doesn't fit the simple dispatch model. @@ -353,26 +359,48 @@ would fail on the discarded ones (issue #291, case B). When an earlier operand is unresolved, subsequent operands are still evaluated (to catch type errors in them), but the final result is `Unresolved(BOOL)` unless a subsequent concrete operand proves the result by short-circuiting (e.g., -`Unresolved and false` returns `false`). Errors in operands past the unresolved one -are suppressed, since a runtime short-circuit could make them unreachable — except -budget exceedances (`MemoryLimitExceeded` / `OperationLimitExceeded`), which always -propagate: the memory/operations were spent in this evaluation no matter what a -runtime short-circuit skips (the same rule as IfExp below). +`Unresolved and false` returns `false`). Operands after the unresolved one are +evaluated speculatively (see [Speculative evaluation](#speculative-evaluation)): a +value error is absorbed, since a runtime short-circuit could make the operand +unreachable; a budget error propagates. Every operand value the result does not +carry is released: the unresolved placeholder (the result is a fresh +`Unresolved(BOOL)`), a concrete operand replaced by the next, and a speculative +operand that did not decide the result. + +### Speculative evaluation + +Two constructs evaluate sub-expressions whose failure they may absorb rather than +propagate: the branches of an unresolved-test conditional, and the operands of a +boolop after an unresolved one. Both use one helper, `eval_speculative`, which +applies two rules: + +- A budget error (`MemoryLimitExceeded` / `OperationLimitExceeded`, directly or + inside a compound error's sub-errors) is never absorbed. The memory and + operations were spent regardless of what a runtime would skip. It propagates + immediately: a conditional whose if-branch exceeds the budget does not evaluate + the else-branch. +- A value error is absorbed and the live footprint (`current_memory`) is reset to + what it was before the attempt, because the failed sub-expression's values are + gone. Peak memory and the operation count keep what the attempt consumed. + Dispatch also releases a failed call's operands (see + [Dispatch Flow](#dispatch-flow)); the reset applies regardless of how the + sub-expression failed. + +The caller decides what an absorbed failure means: an `Unresolved` of the other +branch's type, a runtime short-circuit, or a compound "both branches fail" error. ### IfExp (`eval_ifexp`) Ternary: `x if condition else y`. Evaluates the condition unconstrained (see [Target Type Propagation](#target-type-propagation)) and asserts it is bool-compatible, then evaluates only the selected branch with the parent target type. When the condition is unresolved, both branches are -evaluated and the result type is the union. If exactly one branch fails -with a *value* error, the error is absorbed and the result is -`Unresolved` of the healthy branch's type (a runtime with the condition -resolved may never take the failing branch); if both fail, a compound -error carries both as sub-errors. Budget exceedances -(`MemoryLimitExceeded` / `OperationLimitExceeded`) are exempt from the -single-branch absorption and always propagate: the memory/operations -were spent in this evaluation no matter which branch a runtime would -take. +evaluated speculatively (see above) and the result is an `Unresolved` +carrying only a type: the union of both branch types when both succeed; +the other branch's type when exactly one fails with a value error (a +runtime with the condition resolved may never take the failing branch); +a compound error carrying both as sub-errors when both fail. A budget +error in either branch propagates immediately. Because the result +carries no value, every branch value is released. ### Call (`eval_call`) Handles both function calls (`len(x)`) and method calls (`x.upper()`). @@ -396,6 +424,15 @@ Evaluates list literals. Validates max 2 nesting levels. Coerces elements when m types are present (int→float, path→string). Empty lists use the target type context to determine element type. +Memory accounting: each element is tracked as it is evaluated, then released as the +elements are consumed into the list, which is charged as one value (or as a +type-only `Unresolved` placeholder when any element is unresolved). A literal's +footprint is the list, not the list plus every element again. The error returns +before that point (a `null` element, excessive nesting) leave the elements +charged like any failed sub-expression's operands; an enclosing construct that +absorbs the error resets the footprint (see +[Speculative evaluation](#speculative-evaluation)). + ### ListComp (`eval_listcomp`) Evaluates list comprehensions: `[expr for var in iterable if condition]`. @@ -463,23 +500,33 @@ The `dispatch` method is the centralized point for calling library functions: ``` dispatch(name, args, ast_node) - │ - ├── Release input values from memory tracking - │ - ├── Check if any arg is unresolved - │ └── Yes → infer return type from signature, return Unresolved(return_type) │ ├── Call library.call(name, args, eval_context) │ │ + │ ├── Any arg unresolved → infer return type from signature, + │ │ return Unresolved(return_type) │ ├── Phase 1: Exact non-generic match │ ├── Phase 2: Non-generic with coercion (skip receiver coercion for methods) │ └── Phase 3: Generic match with type variable binding │ - ├── Track output value in memory + ├── Release input values from memory tracking (on success and on error) + │ + ├── On success: track output value in memory │ └── On error: attach AST node context for caret formatting ``` +The arguments are consumed by the call, so they are released from the +live footprint whether the call succeeds or fails. Releasing on failure +matters because the error may be absorbed by an enclosing construct (see +[Speculative evaluation](#speculative-evaluation)) that keeps evaluating +on the same evaluator: the operands are gone and must not stay charged. +`eval_call` releases its arguments before dispatch for the same reason; +the operator paths (`dispatch_with_node` / `dispatch_with_span`, sharing +`dispatch_released`) do the same. The operation-count check before the +call is the one exit that does not release; it fails only with a budget +error, which is never absorbed. + ## Fast Path: Simple Name Lookup `ParsedExpression::as_name_lookup()` detects expressions that are just a dotted name