From f9da3d65b03d200b40fb34f1d9e7cd9d9335d08b Mon Sep 17 00:00:00 2001 From: Mark <399551+mwiebe@users.noreply.github.com> Date: Tue, 29 Sep 2026 20:08:43 -0700 Subject: [PATCH 1/3] fix(expr): fix memory accounting gaps in coercion, attributes, and slices The evaluator tracks memory by adding each value's size when it is created and subtracting it when it is consumed. Several places got this wrong, and two places could turn a memory or operation limit error into an ordinary error that the evaluator then ignored. - eval_node coerced a value to the target type after the value had been tracked, so it was tracked at the old size and later released at the new size. When coercion grows a value (int to string, range_expr to list[int]) the release subtracted too much, and a range_expr coerced to a list was never checked against the memory limit at all. eval_node now releases the original and tracks the coerced value. - eval_attribute replaces errors from its base evaluation and property dispatch with friendlier ones (undefined variable, property not available). Limit errors went through the same replacement and became ordinary errors, which an if/else with an unresolved test or an and/or past an unresolved operand then absorbed. eval_call does the same for a failed method call whose name is also a property. Both now leave limit errors unchanged. - Omitted slice bounds were passed to dispatch as Null values that were never tracked, and dispatch subtracted 64 bytes for each. They are now tracked. The slice's early exits for an unresolved receiver or bound also left the receiver and bounds tracked after discarding them; they are now released. - slice_string collected the input into a Vec (4 bytes per character) and an index Vec (8 per selected element), neither tracked nor checked against the limit, then built the result from an iterator whose capacity grew past the actual length. It now computes the number of selected characters arithmetically, checks min(input bytes, 4 x count) before allocating, copies the characters directly from the input, and shrinks the buffer so the tracked size is exact. slice_list gets the same check before allocating. While testing this, collect_indices was found to overflow on a step near i64::MAX ('hello'[1::9223372036854775807] panicked in debug builds); it now saturates. One existing pinned figure moves by the two placeholders in [::-1] (128000160 -> 128000288). New tests: nine in test_memory.rs for the evaluator fixes, one with a custom library for the eval_call case, two for slice budgeting, two unit tests in comparison.rs for slice_len and the character copy, and extreme-step and multi-byte cases in test_slicing.rs. specs/expr/evaluator.md and specs/expr/function-library.md are updated to match. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> --- crates/openjd-expr/src/eval/evaluator.rs | 118 +++--- .../openjd-expr/src/functions/comparison.rs | 138 ++++++- .../tests/integration/test_memory.rs | 364 +++++++++++++++++- .../tests/integration/test_slicing.rs | 30 ++ specs/expr/evaluator.md | 40 +- specs/expr/function-library.md | 12 + 6 files changed, 643 insertions(+), 59 deletions(-) diff --git a/crates/openjd-expr/src/eval/evaluator.rs b/crates/openjd-expr/src/eval/evaluator.rs index 4ec74a28..8e5d9637 100644 --- a/crates/openjd-expr/src/eval/evaluator.rs +++ b/crates/openjd-expr/src/eval/evaluator.rs @@ -40,13 +40,12 @@ fn append_sub_error(msg: &mut String, err: &ExpressionError, is_last: bool) { } /// Whether an error is (or contains, through compound sub-errors) a -/// budget exceedance. `eval_ifexp` uses this to decide whether a -/// failing branch under an unresolved test may be absorbed into an -/// `Unresolved` result: a *value* error may vanish at run time when the -/// resolved test selects the other branch, but the budgets are global -/// to the evaluation — the memory and operations were spent in this -/// evaluation no matter which branch run time takes — so exhaustion -/// must propagate. +/// budget exceedance. `eval_speculative` uses this to decide whether an +/// error may be absorbed, and `eval_attribute` and `eval_call` use it to +/// decide whether an error may be rewritten. A value error may not +/// matter at run time (the resolved test may select the other branch), +/// but the memory and operations were spent in this evaluation either +/// way, so a budget error must always propagate. /// /// Deliberately coarse: when a *compound* error contains a budget /// exceedance among value sub-errors, the whole compound propagates @@ -274,7 +273,8 @@ impl<'a> Evaluator<'a> { /// This is the per-node target-type propagation primitive defined by /// RFC 0005 §"Target Type Propagation Rules". The target applies to /// **this node's result only**: after `evaluate_inner` returns, the - /// value is coerced toward `target` via [`ExprValue::coerce`]. + /// value is coerced toward `target` via [`ExprValue::coerce`], and the + /// memory tracking is updated to the coerced value's size. /// /// Children are evaluated with `target = None` (unconstrained) by /// default — a caller's `target_type=string` request must not leak @@ -327,13 +327,27 @@ impl<'a> Evaluator<'a> { } Ok(val) => { if let Some(tt) = target { - val.coerce(tt, self.path_format).map_err(|msg| { + // Coercion can change the value's size (int -> string, + // list[int] -> list[string]), and later releases use + // the coerced value's size. Release the value at the + // size it was tracked at, then track the coerced one. + // If coercion fails the value is gone, so it stays + // released. + self.release(&val); + let coerced = val.coerce(tt, self.path_format).map_err(|msg| { let e = ExpressionError::new(msg); if let Some(src) = &self.expr_source { e.with_node(src, node) } else { e } + })?; + self.track(coerced).map_err(|e| { + if let Some(src) = &self.expr_source { + e.with_node(src, node) + } else { + e + } }) } else { Ok(val) @@ -658,8 +672,15 @@ impl<'a> Evaluator<'a> { // Fall back: evaluate the value, then access the attribute via library. // If the base evaluation fails (e.g., "Param" is a subtable not a value), // and we had a dotted path, report the dotted path as undefined with suggestions. + // + // Both fallback arms below replace the original error with a + // friendlier one. A budget error is never replaced: the rewritten + // error would be a value error that an enclosing + // `eval_speculative` could absorb, and the memory and operations + // were spent regardless. let value = match self.eval_node(&a.value, None) { Ok(v) => v, + Err(e) if contains_budget_error(&e) => return Err(e), Err(_) if dotted_path.is_some() => { let path = dotted_path.as_ref().unwrap(); let available = self.collect_symbol_names(); @@ -684,6 +705,7 @@ impl<'a> Evaluator<'a> { let attr_node = ast::Expr::Attribute(a.clone()); match self.dispatch_with_node(&prop_name, vec![value.clone()], Some(&attr_node)) { Ok(v) => Ok(v), + Err(e) if contains_budget_error(&e) => Err(e), Err(_) => { let src = self.expr_source.unwrap_or(""); let val_type = value.expr_type(); @@ -1187,7 +1209,11 @@ impl<'a> Evaluator<'a> { let result = result.map_err(|e| { let src = self.expr_source.unwrap_or(""); let call_node = ast::Expr::Call(c.clone()); + // Like `eval_attribute`'s fallbacks: never replace a + // budget error with a value error that an enclosing + // `eval_speculative` could absorb. if is_method_call + && !contains_budget_error(&e) && !lib .get_signatures(&format!("__property_{name}__")) .is_empty() @@ -1342,56 +1368,56 @@ impl<'a> Evaluator<'a> { // Handle slice syntax: value[start:stop:step] if let ast::Expr::Slice(sl) = &*s.slice { - let start = match sl - .lower - .as_ref() - .map(|e| self.eval_node(e, None)) - .transpose()? - { - Some(v) => v, - None => ExprValue::Null, + // An omitted bound is passed to dispatch as a `Null` + // placeholder. Dispatch releases every operand, so the + // placeholder must be tracked like an evaluated bound; + // otherwise the release would subtract bytes that were never + // added. + let start = match &sl.lower { + Some(e) => self.eval_node(e, None)?, + None => self.track(ExprValue::Null)?, }; - let stop = match sl - .upper - .as_ref() - .map(|e| self.eval_node(e, None)) - .transpose()? - { - Some(v) => v, - None => ExprValue::Null, + let stop = match &sl.upper { + Some(e) => self.eval_node(e, None)?, + None => self.track(ExprValue::Null)?, }; - let step = match sl - .step - .as_ref() - .map(|e| self.eval_node(e, None)) - .transpose()? - { - Some(v) => v, - None => ExprValue::Null, + let step = match &sl.step { + Some(e) => self.eval_node(e, None)?, + None => self.track(ExprValue::Null)?, }; if let ExprValue::Int(0) = &step { return Err(ExpressionError::new("Slice step cannot be zero")); } - if value.is_unresolved() { + // When the result is a type-only `Unresolved`, the operands + // are discarded. Release them before tracking the result; + // this is a success path, so nothing else would reset the + // memory tracking. + let unresolved_result = if value.is_unresolved() { let inner = unwrap_unresolved(&value.expr_type()); - if let Some(elem) = inner.list_element_type() { - return self.track(ExprValue::unresolved(ExprType::list(elem.clone()))); - } - return self.track(ExprValue::unresolved(inner)); - } - - // If any slice bound is unresolved, propagate unresolved - let any_bound_unresolved = - start.is_unresolved() || stop.is_unresolved() || step.is_unresolved(); - if any_bound_unresolved { + Some(match inner.list_element_type() { + Some(elem) => ExprValue::unresolved(ExprType::list(elem.clone())), + None => ExprValue::unresolved(inner), + }) + } else if start.is_unresolved() || stop.is_unresolved() || step.is_unresolved() { + // Any unresolved bound makes the result unresolved. if value.is_list() { let elem_type = value.list_elem_type().unwrap(); - return self.track(ExprValue::unresolved(ExprType::list(elem_type.clone()))); + Some(ExprValue::unresolved(ExprType::list(elem_type.clone()))) } else if matches!(&value, ExprValue::String(_)) { - return self.track(ExprValue::unresolved(ExprType::STRING)); + Some(ExprValue::unresolved(ExprType::STRING)) + } else { + None + } + } else { + None + }; + if let Some(result) = unresolved_result { + for operand in [&value, &start, &stop, &step] { + self.release(operand); } + return self.track(result); } // Dispatch 4-arg __getitem__ through the library diff --git a/crates/openjd-expr/src/functions/comparison.rs b/crates/openjd-expr/src/functions/comparison.rs index b9ca1eb3..24d7d64b 100644 --- a/crates/openjd-expr/src/functions/comparison.rs +++ b/crates/openjd-expr/src/functions/comparison.rs @@ -171,6 +171,10 @@ fn compute_slice_indices(len: i64, start: Option, stop: Option, step: } } +/// Indices a slice visits, in order. The step is added with saturation so +/// a step near `i64::MAX` or `i64::MIN` cannot overflow; a saturated index +/// is past every bound `compute_slice_indices` can return, so the loop +/// ends. fn collect_indices(start: i64, stop: i64, step: i64) -> Vec { let mut indices = Vec::new(); let mut idx = start; @@ -179,19 +183,38 @@ fn collect_indices(start: i64, stop: i64, step: i64) -> Vec { if idx >= 0 { indices.push(idx as usize); } - idx += step; + idx = idx.saturating_add(step); } } else { while idx > stop { if idx >= 0 { indices.push(idx as usize); } - idx += step; + idx = idx.saturating_add(step); } } indices } +/// Number of indices [`collect_indices`] yields for the same arguments. +/// Computed arithmetically so callers can check the memory budget before +/// building the result. Expects `(start, stop)` from +/// [`compute_slice_indices`]: for a forward step `start >= 0`, and for a +/// backward step `stop >= -1`, so a negative `start` yields nothing (as +/// `collect_indices`'s `idx >= 0` filter would). +fn slice_len(start: i64, stop: i64, step: i64) -> usize { + let span = if step > 0 { + stop.saturating_sub(start) + } else { + start.saturating_sub(stop) + }; + if span <= 0 { + return 0; + } + // ceil(span / |step|), in u64 so `|i64::MIN|` cannot overflow. + ((span as u64 - 1) / step.unsigned_abs() + 1) as usize +} + pub fn slice_list(ctx: Ctx, a: &[ExprValue]) -> R { let step = extract_int_or_none(&a[3]).unwrap_or(1); if step == 0 { @@ -202,11 +225,16 @@ pub fn slice_list(ctx: Ctx, a: &[ExprValue]) -> R { let start = extract_int_or_none(&a[1]); let stop = extract_int_or_none(&a[2]); let (s, e) = compute_slice_indices(len, start, stop, step); + // Check the result's slot count before allocating the index vector + // or the element vector. `make_list_checked` checks again with the + // elements' heap sizes once they are known. + let count = slice_len(s, e, step); + ctx.count_ops(count)?; + ctx.check_memory(count.saturating_mul(std::mem::size_of::()))?; let result: Vec = collect_indices(s, e, step) .into_iter() .filter_map(|i| a[0].list_get(i as i64)) .collect(); - ctx.count_ops(result.len())?; ExprValue::make_list_checked(ctx, result, elem_type.clone()) } @@ -220,16 +248,38 @@ pub fn slice_string(ctx: Ctx, a: &[ExprValue]) -> R { if step == 0 { return Err(ExpressionError::new("Slice step cannot be zero")); } - let chars: Vec = s.chars().collect(); - let len = chars.len() as i64; + let len = s.chars().count() as i64; let start = extract_int_or_none(&a[1]); let stop = extract_int_or_none(&a[2]); let (sv, ev) = compute_slice_indices(len, start, stop, step); - let result: String = collect_indices(sv, ev, step) - .into_iter() - .filter(|&i| i < chars.len()) - .map(|i| chars[i]) - .collect(); + let count = slice_len(sv, ev, step); + if count == 0 { + return Ok(ExprValue::String(String::new())); + } + // A non-zero step never visits an index twice, so each selected + // character is a distinct character of `s`. The result is therefore + // at most `s.len()` bytes and at most 4 bytes per selected character. + // Check that bound before allocating, then copy the characters + // directly from `s` (no index vector, no `Vec`) and shrink the + // buffer so the tracked size equals the actual size. + let max_bytes = s.len().min(count.saturating_mul(4)); + ctx.check_memory(max_bytes)?; + let mut result = String::with_capacity(max_bytes); + let stride = step.unsigned_abs() as usize; + if step > 0 { + result.extend(s.chars().skip(sv as usize).step_by(stride).take(count)); + } else { + // Walking from the end, the k-th character has index `len - 1 - k`. + // `count > 0` guarantees `0 <= sv < len`. + result.extend( + s.chars() + .rev() + .skip((len - 1 - sv) as usize) + .step_by(stride) + .take(count), + ); + } + result.shrink_to_fit(); Ok(ExprValue::String(result)) } @@ -361,4 +411,72 @@ mod tests { "__contains__ on a range_expr requires an int or float item" ); } + + /// `slice_len` must agree with `collect_indices` for every argument + /// combination `compute_slice_indices` can produce, since it is used + /// to check the budget for the result `collect_indices` then builds. + #[test] + fn slice_len_matches_collect_indices() { + let bounds: Vec> = std::iter::once(None) + .chain((-9..=9).map(Some)) + .chain([i64::MIN, i64::MAX, -1_000_000, 1_000_000].map(Some)) + .collect(); + let steps = [1, 2, 3, 7, -1, -2, -3, -7, i64::MAX, i64::MIN]; + for len in 0..=7 { + for &start in &bounds { + for &stop in &bounds { + for &step in &steps { + let (s, e) = compute_slice_indices(len, start, stop, step); + assert_eq!( + slice_len(s, e, step), + collect_indices(s, e, step).len(), + "len={len} start={start:?} stop={stop:?} step={step}" + ); + } + } + } + } + } + + /// `slice_string` must select the same characters as indexing into a + /// collected `Vec`, including multi-byte characters and + /// backward steps. + #[test] + fn slice_string_matches_char_indexing() { + let text = "aé漢😀bçdz"; + let chars: Vec = text.chars().collect(); + let len = chars.len() as i64; + let bounds: Vec> = std::iter::once(None) + .chain((-(len + 2)..=(len + 2)).map(Some)) + .collect(); + for &start in &bounds { + for &stop in &bounds { + for step in [1, 2, 3, -1, -2, -3] { + let (s, e) = compute_slice_indices(len, start, stop, step); + let expected: String = collect_indices(s, e, step) + .into_iter() + .filter(|&i| i < chars.len()) + .map(|i| chars[i]) + .collect(); + let to_val = |b: Option| b.map_or(ExprValue::Null, ExprValue::Int); + let got = slice_string( + &mut TestContext, + &[ + ExprValue::String(text.to_string()), + to_val(start), + to_val(stop), + ExprValue::Int(step), + ], + ) + .unwrap(); + let ExprValue::String(got) = got else { + panic!("slice_string returned a non-string"); + }; + assert_eq!(got, expected, "start={start:?} stop={stop:?} step={step}"); + // The buffer is shrunk, so capacity equals length. + assert_eq!(got.capacity(), got.len()); + } + } + } + } } diff --git a/crates/openjd-expr/tests/integration/test_memory.rs b/crates/openjd-expr/tests/integration/test_memory.rs index 7778d93a..57a3e483 100644 --- a/crates/openjd-expr/tests/integration/test_memory.rs +++ b/crates/openjd-expr/tests/integration/test_memory.rs @@ -604,11 +604,13 @@ fn reverse_range_slice_memory_checked_before_walk() { .unwrap_err() .to_string(); // 128,000,000 = 2,000,000 × size_of::() (64); the - // remainder is the tracked RangeExpr and slice-argument values. + // remainder is the tracked RangeExpr and the three slice operands: + // the `-1` step and the two `Null` placeholders for the omitted + // start and stop. assert_eq!( e, [ - "Expression memory usage (128000160 bytes) exceeded limit (10000 bytes)\n", + "Expression memory usage (128000288 bytes) exceeded limit (10000 bytes)\n", " range_expr('1-2000000')[::-1]\n", " ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~", ] @@ -1492,3 +1494,361 @@ fn chained_comparison_carries_middle_operand_once() { .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) .expect("the chain must leave no footprint"); } + +// ══════════════════════════════════════════════════════════════ +// A coerced value is tracked at its coerced size +// ══════════════════════════════════════════════════════════════ + +/// Target-type coercion runs after a node's value is tracked and can +/// change its size. The evaluator releases the original and tracks the +/// coerced value, so the memory limit applies to what the expression +/// actually produces. A `range_expr` is a few dozen bytes; coerced to +/// `list[int]` it becomes 100,000 ints (800 KB), which exceeds a 100 KB +/// limit. Previously only the small `range_expr` was ever tracked. +#[test] +fn root_target_coercion_is_charged_at_coerced_size() { + let st = SymbolTable::new(); + let target = openjd_expr::ExprType::list(openjd_expr::ExprType::INT); + let e = ParsedExpression::new("range_expr('1-100000')") + .and_then(|p| { + p.with_memory_limit(100_000) + .with_target_type(&target) + .evaluate(&[&st]) + }) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + // 800064 = the coerced list[int] alone (64 + 100,000 × 8). The + // range_expr it replaced was released first. + "Expression memory usage (800064 bytes) exceeded limit (100000 bytes)\n", + " range_expr('1-100000')\n", + " ^~~~~~~~~~~~~~~~~~~~~~", + ] + .concat() + ); +} + +/// `eval_call` releases its arguments at their coerced size. If they had +/// been tracked at the pre-coercion size, the release would subtract more +/// than was added, and part of some other live value would go uncounted. +/// Under a `list[string]` target each element is evaluated toward +/// `string`, so `join` receives argument targets and `range(2000)` (16 KB +/// as `list[int]`) is coerced to `list[string]` (about 55 KB). A 1 MB +/// string is live at the same time; a further 500 KB then totals 1.51 MB +/// and fails a 1.5 MB limit. With the old under-count, 39 KB of the 1 MB +/// string went uncounted and the expression fit. +#[test] +fn call_argument_coercion_releases_what_was_charged() { + let st = SymbolTable::new(); + let target = openjd_expr::ExprType::list(openjd_expr::ExprType::STRING); + let e = ParsedExpression::new("['A' * 1000000, join(range(2000), ','), 'B' * 500000]") + .and_then(|p| { + p.with_memory_limit(1_500_000) + .with_target_type(&target) + .evaluate(&[&st]) + }) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + // 1509153 = the 1 MB string (1000064), the joined string + // (8953: 6890 digits and 1999 commas), the 'B' operand (72), + // the `500000` operand (64), and the 500,000 bytes the + // multiplication checks for its result before building it. + // The coerced argument has been released. + "Expression memory usage (1509153 bytes) exceeded limit (1500000 bytes)\n", + " ['A' * 1000000, join(range(2000), ','), 'B' * 500000]\n", + " ~~~~^~~~~~~~", + ] + .concat() + ); +} + +// ══════════════════════════════════════════════════════════════ +// Attribute access never rewrites a budget error +// ══════════════════════════════════════════════════════════════ + +/// Symbol table with an unresolved `Session.Flag`, a `Big` path whose +/// string is 1 MB long (tracking it exceeds any limit below 1 MB), and a +/// `Deep` path of 200,000 components (`.parts` builds a list far past +/// the limits used here). +fn attribute_symtab() -> SymbolTable { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + st.set( + "Big", + ExprValue::new_path( + format!("/{}", "a".repeat(1_000_000)), + openjd_expr::PathFormat::Posix, + ), + ) + .unwrap(); + st.set( + "Deep", + ExprValue::new_path("/a".repeat(200_000), openjd_expr::PathFormat::Posix), + ) + .unwrap(); + st +} + +fn eval_attribute_expr(expr: &str, mem: usize) -> Result { + let st = attribute_symtab(); + ParsedExpression::new(expr).and_then(|p| { + p.with_memory_limit(mem) + .with_path_format(openjd_expr::PathFormat::Posix) + .evaluate(&[&st]) + }) +} + +/// `Big.name` is not a symbol, so the evaluator falls back to evaluating +/// `Big` and dispatching the property. Tracking the 1 MB path exceeds +/// the limit inside that base evaluation. The fallback that would +/// otherwise report "Undefined variable 'Big.name'" must let the budget +/// error through. +#[test] +fn attribute_base_lookup_propagates_memory_error() { + let e = eval_attribute_expr("Big.name", 500_000) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + "Expression memory usage (1000065 bytes) exceeded limit (500000 bytes)\n", + " Big.name\n", + " ^~~", + ] + .concat() + ); +} + +/// The same base-lookup failure inside a construct that absorbs value +/// errors. If the budget error were rewritten as an undefined-variable +/// error, the `or` would absorb it and the expression would succeed +/// with an `Unresolved` result. +#[test] +fn attribute_base_lookup_memory_error_is_not_absorbed() { + let e = eval_attribute_expr("Session.Flag or Big.name == 'x'", 500_000) + .unwrap_err() + .to_string(); + assert!( + e.starts_with("Expression memory usage (1000065 bytes) exceeded limit (500000 bytes)"), + "expected the memory error to propagate, got:\n{e}" + ); +} + +/// `Deep.parts` fails inside the property dispatch: the 200,001-element +/// result list fails the memory check before it is built. The fallback +/// that would otherwise report "'parts' property is not available for +/// path" must let the budget error through. +#[test] +fn attribute_property_dispatch_propagates_memory_error() { + let e = eval_attribute_expr("Deep.parts", 2_000_000) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + // 13400129 = the 400 KB path (400064) plus the estimate for + // the 200,001 one-character parts (the root "/" and 200,000 + // "a"s): 200,001 × 64 slots plus 200,001 bytes of heap. + "Expression memory usage (13400129 bytes) exceeded limit (2000000 bytes)\n", + " Deep.parts\n", + " ~~~~~^~~~~", + ] + .concat() + ); +} + +/// The same property-dispatch failure inside an absorbing construct. +#[test] +fn attribute_property_dispatch_memory_error_is_not_absorbed() { + let e = eval_attribute_expr("Session.Flag or len(Deep.parts) == 0", 2_000_000) + .unwrap_err() + .to_string(); + assert!( + e.starts_with("Expression memory usage ("), + "expected the memory error to propagate, got:\n{e}" + ); +} + +/// Control: a real value error in the property dispatch is still +/// rewritten to the friendlier message, and can still be absorbed. +#[test] +fn attribute_value_error_is_still_rewritten_and_absorbable() { + let e = eval_attribute_expr("'abc'.name", 1_000_000) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + "'name' property is not available for string. Available for: path\n", + " 'abc'.name\n", + " ~~~~~~^~~~", + ] + .concat() + ); + let v = eval_attribute_expr("Session.Flag or 'abc'.name == 'x'", 1_000_000).unwrap(); + assert!(v.is_unresolved()); +} + +/// `eval_call` has its own rewrite: a failed method call whose name is +/// also a property becomes "'name' is a property, not a method". No +/// method in the default library shares a name with a property, so this +/// needs a custom library. Here the default library is extended with a +/// `stem(path, int)` method that exhausts the operation budget. The +/// budget error must not be rewritten, so it must not be absorbable. +#[test] +fn call_property_method_rewrite_exempts_budget_errors() { + use openjd_expr::function_library::{EvalContext, FunctionLibrary}; + fn exhausting_stem( + ctx: &mut dyn EvalContext, + _args: &[ExprValue], + ) -> Result { + ctx.count_ops(usize::MAX / 2)?; + Ok(ExprValue::String(String::new())) + } + let mut lib: FunctionLibrary = + (*FunctionLibrary::for_profile(&openjd_expr::ExprProfile::current())).clone(); + lib.register_sig("stem", "(path, int) -> string", exhausting_stem) + .unwrap(); + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + st.set( + "P", + ExprValue::new_path("/a/b.txt", openjd_expr::PathFormat::Posix), + ) + .unwrap(); + let run = |expr: &str| { + ParsedExpression::new(expr).and_then(|p| { + p.with_library(&lib) + .with_path_format(openjd_expr::PathFormat::Posix) + .evaluate(&[&st]) + }) + }; + + // Top level: the budget error is reported as itself. + let e = run("P.stem(1)").unwrap_err().to_string(); + assert!( + e.starts_with("Expression operation count ("), + "expected the operation-limit error to propagate, got:\n{e}" + ); + // Inside a construct that absorbs value errors: not absorbed. + let e = run("Session.Flag or P.stem(1) == 'x'") + .unwrap_err() + .to_string(); + assert!( + e.starts_with("Expression operation count ("), + "expected the operation-limit error to propagate, got:\n{e}" + ); + // Control: a real value error still gets the rewrite. + let e = run("P.stem()").unwrap_err().to_string(); + assert_eq!( + e, + [ + "'stem' is a property, not a method. Use .stem instead of .stem()\n", + " P.stem()\n", + " ~~^~~~~~", + ] + .concat() + ); +} + +// ══════════════════════════════════════════════════════════════ +// Slice operands are tracked and released symmetrically +// ══════════════════════════════════════════════════════════════ + +/// An omitted slice bound is passed to dispatch as a `Null` placeholder, +/// and dispatch releases every operand. The placeholders are tracked when +/// created so the release matches. This is only visible when something +/// else is live: with a 300 KB string live, `[:]` on a 500 KB string, and +/// then a 700 KB string, the reported figure is exact. Untracked +/// placeholders would have subtracted 192 bytes (three omitted bounds) +/// that were never added. +#[test] +fn slice_placeholders_are_charged_before_dispatch_releases_them() { + let st = SymbolTable::new(); + let e = ParsedExpression::new("['C' * 300000, ('A' * 500000)[:], 'B' * 700000]") + .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + // 1500264 = the 300 KB string (300064), the sliced 500 KB + // result (500064; the slice shrinks its buffer, so capacity + // equals length), the 'B' operand (72), the `700000` operand + // (64), and the 700,000 bytes the multiplication checks for + // its result before building it. + "Expression memory usage (1500264 bytes) exceeded limit (1500000 bytes)\n", + " ['C' * 300000, ('A' * 500000)[:], 'B' * 700000]\n", + " ~~~~^~~~~~~~", + ] + .concat() + ); +} + +/// A string slice checks the memory budget for its result before +/// allocating anything. The input is still tracked at that point, so the +/// input and the projected result count together. A 1 MB string sliced +/// whole under a 1.5 MB limit fails at the slice, with the input, the +/// three placeholders, and the projected result in the figure. +/// Previously the slice built an untracked `Vec` (4 MB) and index +/// vector (8 MB), then a result whose capacity had grown to 1048576, and +/// the expression passed because the input was released before the +/// result was tracked. +#[test] +fn string_slice_budgets_result_before_allocating() { + let st = SymbolTable::new(); + let e = ParsedExpression::new("('A' * 1000000)[:]") + .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + // 2000256 = the 1 MB input (1000064), three `Null` placeholders + // (192) and the projected 1,000,000-byte result. + "Expression memory usage (2000256 bytes) exceeded limit (1500000 bytes)\n", + " ('A' * 1000000)[:]\n", + " ~~~~~~~~~~~~~~^~~~", + ] + .concat() + ); + // The bound also depends on the number of selected characters: a + // step-8 slice selects 125,000 characters and projects 500,000 bytes + // (four per character), which fits. + let v = ParsedExpression::new("len(('A' * 1000000)[::8])") + .and_then(|p| p.with_memory_limit(1_600_000).evaluate(&[&st])) + .expect("an eighth-size slice must fit where the whole does not"); + assert_eq!(v, ExprValue::Int(125000)); +} + +/// When a slice bound is unresolved the result is a type-only +/// `Unresolved`, and the sliced value is discarded. It must be released +/// on that exit. This is a success path, so nothing else would reset the +/// memory tracking, and the 1 MB string would otherwise stay counted and +/// the following 600 KB would exceed a 1.5 MB limit. +#[test] +fn slice_with_unresolved_bound_releases_sliced_value() { + let mut st = SymbolTable::new(); + st.set( + "Session.Start", + ExprValue::unresolved(openjd_expr::ExprType::INT), + ) + .unwrap(); + let expr = "len(('A' * 1000000)[Session.Start:]) + len('B' * 600000)"; + ParsedExpression::new(expr) + .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) + .unwrap_or_else(|e| panic!("{expr}: the discarded sliced value must be released: {e}")); +} diff --git a/crates/openjd-expr/tests/integration/test_slicing.rs b/crates/openjd-expr/tests/integration/test_slicing.rs index 1a655965..2bfd994f 100644 --- a/crates/openjd-expr/tests/integration/test_slicing.rs +++ b/crates/openjd-expr/tests/integration/test_slicing.rs @@ -99,6 +99,36 @@ fn str_slice_reverse() { fn str_slice_step() { assert_eq!(eval("'hello'[::2]").to_display_string(), "hlo"); } +#[test] +fn str_slice_multibyte_step_and_reverse() { + assert_eq!(eval("'aé漢😀b'[1:4]").to_display_string(), "é漢😀"); + assert_eq!(eval("'aé漢😀b'[::2]").to_display_string(), "a漢b"); + assert_eq!(eval("'aé漢😀b'[::-1]").to_display_string(), "b😀漢éa"); + assert_eq!(eval("'aé漢😀b'[3:0:-1]").to_display_string(), "😀漢é"); +} + +/// A step near `i64::MAX` / `i64::MIN` selects at most one element; the +/// index walk must not overflow past it (Python: `'hello'[1::2**63-1]` +/// is `'e'`). +#[test] +fn slice_with_extreme_step_selects_one_element() { + assert_eq!( + eval("'hello'[1::9223372036854775807]").to_display_string(), + "e" + ); + assert_eq!( + eval("'hello'[3::-9223372036854775808]").to_display_string(), + "l" + ); + assert_eq!( + eval("[1, 2, 3, 4, 5][1::9223372036854775807]").to_display_string(), + "[2]" + ); + assert_eq!( + eval("[1, 2, 3, 4, 5][3::-9223372036854775808]").to_display_string(), + "[4]" + ); +} // === TestRangeExprSlicing === #[test] diff --git a/specs/expr/evaluator.md b/specs/expr/evaluator.md index cb11d5f8..a6bc26da 100644 --- a/specs/expr/evaluator.md +++ b/specs/expr/evaluator.md @@ -175,6 +175,16 @@ evaluator is consumed once, at the root `evaluate` entry point. A node kind added later is therefore unconstrained-by-default rather than inheriting the caller's target by accident. +Coercion can change a value's size (`int` → `string`, `list[int]` → +`list[string]`, `range_expr` → `list[int]`), and later releases of the +value use the coerced size. So `eval_node` releases the value at the size +the child tracked it at, coerces, and tracks the coerced value. This is +also the only memory check a materialized `range_expr` gets: `coerce` +runs outside the budget (it is a post-evaluation hook and a public API), +so the `list[int]` is allocated first, capped by `coerce` itself at the +default operation limit's worth of elements, and then tracked, which is +where it is checked against the memory limit. + **Symbolic parameter types unconstrain their position.** `sorted: (list[T1]) -> list[T1]` constrains its argument to any list type, which is not a type `ExprValue::coerce` can coerce toward, so the position gets @@ -217,6 +227,15 @@ Handles dotted access like `Param.Frame` or `path.name`. Resolution order: This dual-purpose resolution is why `Param.Frame` works as a variable lookup and `my_path.name` works as a property access, using the same syntax. +Steps 2 and 3 replace the original error with a friendlier one: a failed base +evaluation with a dotted path becomes "Undefined variable 'Param.Frame'" with +a suggestion, and a failed property dispatch becomes "'name' property is not +available for string" (or "is a method, not a property"). Budget errors are +not replaced; they propagate unchanged. A replaced budget error would be a +value error that an enclosing construct may absorb (see +[Speculative evaluation](#speculative-evaluation)), and the memory and +operations were spent regardless. + ### Operator dispatch table (`eval/op_table.rs`) The mapping from Python AST operators to dunder function names — and the @@ -389,6 +408,12 @@ applies two rules: 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. +The first rule requires that every construct between the budget failure and +the absorbing one passes the error through unchanged. Two places replace +errors with friendlier ones: [`eval_attribute`](#attribute-eval_attribute)'s +fallbacks and [`eval_call`](#call-eval_call)'s "is a property, not a method" +error for a failed method call. Both leave budget errors as they are. + ### IfExp (`eval_ifexp`) Ternary: `x if condition else y`. Evaluates the condition unconstrained (see [Target Type Propagation](#target-type-propagation)) and asserts it @@ -417,7 +442,13 @@ are evaluated unconstrained; coercion is signature-driven inside Rejects: - Direct dunder calls (`__add__(1, 2)` — use `1 + 2` instead) -- Calling properties as methods (`path.name()` — use `path.name` instead) +- Calling properties as methods (`path.name()` — use `path.name` instead). + When a method call fails and a property of the same name exists, the + original error is replaced with "is a property, not a method". Budget + errors are not replaced (see + [Speculative evaluation](#speculative-evaluation)). No method in the + default library shares a name with a property, so this only applies to + custom libraries. ### List (`eval_list`) Evaluates list literals. Validates max 2 nesting levels. Coerces elements when mixed @@ -493,6 +524,13 @@ Handles indexing (`x[0]`) and slicing (`x[1:3]`, `x[::2]`). target describes the subscript's *result*, so `[10, 20, 30][0]` with `target_type=int` must not coerce the `list[int]` receiver or the index (issue #291, case A) +- A slice's four operands (receiver, start, stop, step) are tracked and + released symmetrically. An omitted bound is passed to `__getitem__` as + a `Null` placeholder, which is tracked when created so that dispatch's + release of every operand matches. When the result is a type-only + `Unresolved` (the receiver or any bound is unresolved), the operands + are released before the result is tracked. This is a success path, so + nothing else would reset the memory tracking. ## Dispatch Flow diff --git a/specs/expr/function-library.md b/specs/expr/function-library.md index cd2117d2..011260b1 100644 --- a/specs/expr/function-library.md +++ b/specs/expr/function-library.md @@ -183,6 +183,18 @@ The following function families use this pattern: output from a worst-case non-overlapping replacement count or from element/separator lengths, then reserve the bound before constructing the output. +- **Slicing** (`__getitem__` with a slice on a `string` or `list`) computes the + number of selected elements arithmetically (`slice_len`, tested to agree + with the index walk `collect_indices`) before building anything. A list + slice checks `count × size_of::()` before allocating the index + vector and the element vector; `make_list_checked` then checks again with + the elements' heap sizes. A string slice checks + `min(input bytes, 4 × count)`: every selected character is a distinct + character of the input, so both are upper bounds. It then copies the + characters directly from the input, with no `Vec` or index vector, + and shrinks the result buffer so the tracked size equals the actual size. + The input is still tracked during the call (dispatch releases it + afterwards), so the input and the projected result count together. ## Registration From d57a5f198f9aa3e098f29fc53fc9a952b19e8fdf Mon Sep 17 00:00:00 2001 From: Mark <399551+mwiebe@users.noreply.github.com> Date: Wed, 30 Sep 2026 09:00:14 -0700 Subject: [PATCH 2/3] fix(expr): keep the string slice stride non-zero on 32-bit targets step.unsigned_abs() as usize truncates to zero on 32-bit targets such as wasm32 when the step is a multiple of 2^32 (including i64::MIN), and step_by(0) panics. Reproduced on i686-pc-windows-msvc with the existing extreme-step test. Clamp with usize::try_from(..).unwrap_or(usize::MAX): when more than one character is selected the step is below the string length and fits; when one is selected the stride only needs to be non-zero. Adds 2^32 steps to the slicing test. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> --- crates/openjd-expr/src/functions/comparison.rs | 6 +++++- crates/openjd-expr/tests/integration/test_slicing.rs | 10 +++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/crates/openjd-expr/src/functions/comparison.rs b/crates/openjd-expr/src/functions/comparison.rs index 24d7d64b..1419789b 100644 --- a/crates/openjd-expr/src/functions/comparison.rs +++ b/crates/openjd-expr/src/functions/comparison.rs @@ -265,7 +265,11 @@ pub fn slice_string(ctx: Ctx, a: &[ExprValue]) -> R { let max_bytes = s.len().min(count.saturating_mul(4)); ctx.check_memory(max_bytes)?; let mut result = String::with_capacity(max_bytes); - let stride = step.unsigned_abs() as usize; + // `count > 1` implies `|step| < len`, which fits in `usize`. When + // `count == 1` the stride is irrelevant (`take(1)`), but it must not + // be zero: `as usize` would truncate `2^32` or `i64::MIN` to zero on + // 32-bit targets such as wasm32, and `step_by(0)` panics. + let stride = usize::try_from(step.unsigned_abs()).unwrap_or(usize::MAX); if step > 0 { result.extend(s.chars().skip(sv as usize).step_by(stride).take(count)); } else { diff --git a/crates/openjd-expr/tests/integration/test_slicing.rs b/crates/openjd-expr/tests/integration/test_slicing.rs index 2bfd994f..9f422229 100644 --- a/crates/openjd-expr/tests/integration/test_slicing.rs +++ b/crates/openjd-expr/tests/integration/test_slicing.rs @@ -109,7 +109,9 @@ fn str_slice_multibyte_step_and_reverse() { /// A step near `i64::MAX` / `i64::MIN` selects at most one element; the /// index walk must not overflow past it (Python: `'hello'[1::2**63-1]` -/// is `'e'`). +/// is `'e'`). Steps that are multiples of 2^32 (including `i64::MIN`) +/// truncate to zero if cast to a 32-bit `usize`, so they also guard the +/// string slice on 32-bit targets such as wasm32. #[test] fn slice_with_extreme_step_selects_one_element() { assert_eq!( @@ -120,6 +122,8 @@ fn slice_with_extreme_step_selects_one_element() { eval("'hello'[3::-9223372036854775808]").to_display_string(), "l" ); + assert_eq!(eval("'hello'[::4294967296]").to_display_string(), "h"); + assert_eq!(eval("'hello'[::-4294967296]").to_display_string(), "o"); assert_eq!( eval("[1, 2, 3, 4, 5][1::9223372036854775807]").to_display_string(), "[2]" @@ -128,6 +132,10 @@ fn slice_with_extreme_step_selects_one_element() { eval("[1, 2, 3, 4, 5][3::-9223372036854775808]").to_display_string(), "[4]" ); + assert_eq!( + eval("[1, 2, 3, 4, 5][::4294967296]").to_display_string(), + "[1]" + ); } // === TestRangeExprSlicing === From 7f0de06534cdd20a16ff5f3c3cd7a604853abaa1 Mon Sep 17 00:00:00 2001 From: Mark <399551+mwiebe@users.noreply.github.com> Date: Wed, 30 Sep 2026 09:17:24 -0700 Subject: [PATCH 3/3] fix(expr): build list slices into a BudgetedVec with no index vector slice_list checked count x 64 bytes up front but then allocated an index Vec by repeated push and collected the result through a filter_map, whose size hint is zero, so the result Vec grew by doubling. Peak allocation was about 2.25x the checked amount, and the elements were all cloned before any per-element check. The indices are now produced lazily by an iterator (slice_indices, replacing collect_indices), and the result is built into a BudgetedVec::with_capacity that reserves the exact count once and charges each element as it is pushed, the same way slice_range builds its reverse walk. A new test slices 100 strings of 100 KB and fails at the 20th push (12009056 bytes) where the previous code built all 100 first (20009056 bytes). Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> --- .../openjd-expr/src/functions/comparison.rs | 71 +++++++++---------- .../tests/integration/test_memory.rs | 69 ++++++++++++++++++ specs/expr/function-library.md | 10 +-- 3 files changed, 108 insertions(+), 42 deletions(-) diff --git a/crates/openjd-expr/src/functions/comparison.rs b/crates/openjd-expr/src/functions/comparison.rs index 1419789b..d0f6aa9e 100644 --- a/crates/openjd-expr/src/functions/comparison.rs +++ b/crates/openjd-expr/src/functions/comparison.rs @@ -171,37 +171,32 @@ fn compute_slice_indices(len: i64, start: Option, stop: Option, step: } } -/// Indices a slice visits, in order. The step is added with saturation so -/// a step near `i64::MAX` or `i64::MIN` cannot overflow; a saturated index -/// is past every bound `compute_slice_indices` can return, so the loop -/// ends. -fn collect_indices(start: i64, stop: i64, step: i64) -> Vec { - let mut indices = Vec::new(); +/// Indices a slice visits, in order, produced lazily so no index vector +/// is allocated. The step is added with saturation so a step near +/// `i64::MAX` or `i64::MIN` cannot overflow; a saturated index is past +/// every bound `compute_slice_indices` can return, so the walk ends. +fn slice_indices(start: i64, stop: i64, step: i64) -> impl Iterator { + let forward = step > 0; let mut idx = start; - if step > 0 { - while idx < stop { - if idx >= 0 { - indices.push(idx as usize); - } - idx = idx.saturating_add(step); + std::iter::from_fn(move || { + let in_range = if forward { idx < stop } else { idx > stop }; + if !in_range { + return None; } - } else { - while idx > stop { - if idx >= 0 { - indices.push(idx as usize); - } - idx = idx.saturating_add(step); - } - } - indices + let current = idx; + idx = idx.saturating_add(step); + Some(current) + }) + .filter(|&i| i >= 0) + .map(|i| i as usize) } -/// Number of indices [`collect_indices`] yields for the same arguments. +/// Number of indices [`slice_indices`] yields for the same arguments. /// Computed arithmetically so callers can check the memory budget before /// building the result. Expects `(start, stop)` from /// [`compute_slice_indices`]: for a forward step `start >= 0`, and for a /// backward step `stop >= -1`, so a negative `start` yields nothing (as -/// `collect_indices`'s `idx >= 0` filter would). +/// `slice_indices`'s `i >= 0` filter would). fn slice_len(start: i64, stop: i64, step: i64) -> usize { let span = if step > 0 { stop.saturating_sub(start) @@ -225,17 +220,18 @@ pub fn slice_list(ctx: Ctx, a: &[ExprValue]) -> R { let start = extract_int_or_none(&a[1]); let stop = extract_int_or_none(&a[2]); let (s, e) = compute_slice_indices(len, start, stop, step); - // Check the result's slot count before allocating the index vector - // or the element vector. `make_list_checked` checks again with the - // elements' heap sizes once they are known. + // Reserve the result's exact capacity once (checked against the + // budget), then charge each element as it is pushed. Nothing else is + // allocated: the indices are produced lazily. let count = slice_len(s, e, step); ctx.count_ops(count)?; - ctx.check_memory(count.saturating_mul(std::mem::size_of::()))?; - let result: Vec = collect_indices(s, e, step) - .into_iter() - .filter_map(|i| a[0].list_get(i as i64)) - .collect(); - ExprValue::make_list_checked(ctx, result, elem_type.clone()) + let mut result = BudgetedVec::with_capacity(ctx, count)?; + for i in slice_indices(s, e, step) { + if let Some(v) = a[0].list_get(i as i64) { + result.push(ctx, v)?; + } + } + ExprValue::make_list_checked(ctx, result.into_vec(), elem_type.clone()) } pub fn slice_string(ctx: Ctx, a: &[ExprValue]) -> R { @@ -416,11 +412,11 @@ mod tests { ); } - /// `slice_len` must agree with `collect_indices` for every argument + /// `slice_len` must agree with `slice_indices` for every argument /// combination `compute_slice_indices` can produce, since it is used - /// to check the budget for the result `collect_indices` then builds. + /// to check the budget for the result built from `slice_indices`. #[test] - fn slice_len_matches_collect_indices() { + fn slice_len_matches_slice_indices() { let bounds: Vec> = std::iter::once(None) .chain((-9..=9).map(Some)) .chain([i64::MIN, i64::MAX, -1_000_000, 1_000_000].map(Some)) @@ -433,7 +429,7 @@ mod tests { let (s, e) = compute_slice_indices(len, start, stop, step); assert_eq!( slice_len(s, e, step), - collect_indices(s, e, step).len(), + slice_indices(s, e, step).count(), "len={len} start={start:?} stop={stop:?} step={step}" ); } @@ -457,8 +453,7 @@ mod tests { for &stop in &bounds { for step in [1, 2, 3, -1, -2, -3] { let (s, e) = compute_slice_indices(len, start, stop, step); - let expected: String = collect_indices(s, e, step) - .into_iter() + let expected: String = slice_indices(s, e, step) .filter(|&i| i < chars.len()) .map(|i| chars[i]) .collect(); diff --git a/crates/openjd-expr/tests/integration/test_memory.rs b/crates/openjd-expr/tests/integration/test_memory.rs index 57a3e483..0f229a74 100644 --- a/crates/openjd-expr/tests/integration/test_memory.rs +++ b/crates/openjd-expr/tests/integration/test_memory.rs @@ -1834,6 +1834,75 @@ fn string_slice_budgets_result_before_allocating() { assert_eq!(v, ExprValue::Int(125000)); } +/// A list slice reserves its result's slots once, checked against the +/// budget, and charges each element as it is pushed. With the input list +/// still tracked (it is released by dispatch afterwards), a whole-list +/// slice of 50,000 ints under a limit that fits the input but not two +/// copies fails at the slice, and the figure is the input plus the +/// reservation. Previously the result grew by doubling with no check +/// until it was complete, and an index vector was allocated alongside. +#[test] +fn list_slice_reserves_result_before_building() { + let mut st = SymbolTable::new(); + st.set("L", ExprValue::ListInt((0..50_000).collect())) + .unwrap(); + let e = ParsedExpression::new("L[:]") + .and_then(|p| p.with_memory_limit(3_500_000).evaluate(&[&st])) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + // 3600256 = the 50,000-int input list (64 + 50,000 × 8 = + // 400064), three `Null` placeholders (192), and the reserved + // 50,000 result slots (50,000 × 64 = 3,200,000; the result + // holds ExprValues until make_list_checked packs them). The + // reservation alone is under the limit; the input still being + // tracked pushes the total over. + "Expression memory usage (3600256 bytes) exceeded limit (3500000 bytes)\n", + " L[:]\n", + " ~^~~", + ] + .concat() + ); + // Elements are charged as they are pushed, so a slice of large + // strings fails partway through, at the push that crosses the limit, + // rather than after every element has been cloned. 100 strings of + // 100 KB under a 12 MB limit: the input (about 10 MB) and the 100 + // reserved slots fit; the 20th clone does not. + let mut st = SymbolTable::new(); + st.set( + "S", + ExprValue::make_list( + (0..100) + .map(|_| ExprValue::String("x".repeat(100_000))) + .collect(), + openjd_expr::ExprType::STRING, + ) + .unwrap(), + ) + .unwrap(); + let e = ParsedExpression::new("S[:]") + .and_then(|p| p.with_memory_limit(12_000_000).evaluate(&[&st])) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + // 12009056 = the input list (64 + 100 × 24 for the String + // headers + 100 × 100,000 = 10,002,464), three placeholders + // (192), and 20 pushed clones (20 × 100,064 = 2,001,280) with + // the remaining 80 reserved slots (80 × 64 = 5,120) still + // counted. Before per-push charging, all 100 clones were built + // first and the figure was the input plus the whole result. + "Expression memory usage (12009056 bytes) exceeded limit (12000000 bytes)\n", + " S[:]\n", + " ~^~~", + ] + .concat() + ); +} + /// When a slice bound is unresolved the result is a type-only /// `Unresolved`, and the sliced value is discarded. It must be released /// on that exit. This is a success path, so nothing else would reset the diff --git a/specs/expr/function-library.md b/specs/expr/function-library.md index 011260b1..0858e245 100644 --- a/specs/expr/function-library.md +++ b/specs/expr/function-library.md @@ -185,10 +185,12 @@ The following function families use this pattern: output. - **Slicing** (`__getitem__` with a slice on a `string` or `list`) computes the number of selected elements arithmetically (`slice_len`, tested to agree - with the index walk `collect_indices`) before building anything. A list - slice checks `count × size_of::()` before allocating the index - vector and the element vector; `make_list_checked` then checks again with - the elements' heap sizes. A string slice checks + with the lazy index iterator `slice_indices`) before building anything. A + list slice reserves the result's exact capacity once through + `BudgetedVec::with_capacity`, which checks `count × size_of::()`, + then charges each element as it is pushed; `make_list_checked` then checks + again with the elements' heap sizes. No index vector is allocated. A + string slice checks `min(input bytes, 4 × count)`: every selected character is a distinct character of the input, so both are upper bounds. It then copies the characters directly from the input, with no `Vec` or index vector,