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..d0f6aa9e 100644 --- a/crates/openjd-expr/src/functions/comparison.rs +++ b/crates/openjd-expr/src/functions/comparison.rs @@ -171,25 +171,43 @@ fn compute_slice_indices(len: i64, start: Option, stop: Option, step: } } -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 += step; + std::iter::from_fn(move || { + let in_range = if forward { idx < stop } else { idx > stop }; + if !in_range { + return None; } + let current = idx; + idx = idx.saturating_add(step); + Some(current) + }) + .filter(|&i| i >= 0) + .map(|i| i as usize) +} + +/// 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 +/// `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) } else { - while idx > stop { - if idx >= 0 { - indices.push(idx as usize); - } - idx += step; - } + start.saturating_sub(stop) + }; + if span <= 0 { + return 0; } - indices + // 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 { @@ -202,12 +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); - 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()) + // 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)?; + 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 { @@ -220,16 +244,42 @@ 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); + // `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 { + // 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,71 @@ mod tests { "__contains__ on a range_expr requires an int or float item" ); } + + /// `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 built from `slice_indices`. + #[test] + 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)) + .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), + slice_indices(s, e, step).count(), + "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 = slice_indices(s, e, step) + .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..0f229a74 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,430 @@ 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)); +} + +/// 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 +/// 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..9f422229 100644 --- a/crates/openjd-expr/tests/integration/test_slicing.rs +++ b/crates/openjd-expr/tests/integration/test_slicing.rs @@ -99,6 +99,44 @@ 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'`). 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!( + eval("'hello'[1::9223372036854775807]").to_display_string(), + "e" + ); + assert_eq!( + 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]" + ); + assert_eq!( + 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 === #[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..0858e245 100644 --- a/specs/expr/function-library.md +++ b/specs/expr/function-library.md @@ -183,6 +183,20 @@ 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 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, + 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