Skip to content

fix(expr): fix memory accounting gaps in coercion, attributes, and slices #418

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mwiebe merged 3 commits into from
Sep 30, 2026
Merged

fix(expr): fix memory accounting gaps in coercion, attributes, and slices #418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f9da3d6
fix(expr): fix memory accounting gaps in coercion, attributes, and sl…
mwiebe Sep 30, 2026
d57a5f1
fix(expr): keep the string slice stride non-zero on 32-bit targets
mwiebe Sep 30, 2026
7f0de06
fix(expr): build list slices into a BudgetedVec with no index vector
mwiebe Sep 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 72 additions & 46 deletions crates/openjd-expr/src/eval/evaluator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down
173 changes: 145 additions & 28 deletions crates/openjd-expr/src/functions/comparison.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,25 +171,43 @@ fn compute_slice_indices(len: i64, start: Option<i64>, stop: Option<i64>, step:
}
}

fn collect_indices(start: i64, stop: i64, step: i64) -> Vec<usize> {
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<Item = usize> {
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 {
Expand All @@ -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<ExprValue> = 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 {
Expand All @@ -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<char> = 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<char>`) 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))
}

Expand Down Expand Up @@ -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<Option<i64>> = 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<char>`, including multi-byte characters and
/// backward steps.
#[test]
fn slice_string_matches_char_indexing() {
let text = "aé漢😀bçdz";
let chars: Vec<char> = text.chars().collect();
let len = chars.len() as i64;
let bounds: Vec<Option<i64>> = 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<i64>| 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());
}
}
}
}
}
Loading
Loading