diff --git a/crates/openjd-expr/src/eval/evaluator.rs b/crates/openjd-expr/src/eval/evaluator.rs index eefd6d09..4f57f320 100644 --- a/crates/openjd-expr/src/eval/evaluator.rs +++ b/crates/openjd-expr/src/eval/evaluator.rs @@ -1404,11 +1404,25 @@ impl<'a> Evaluator<'a> { } } - /// Propagate resource counters back from a child evaluator. - fn absorb_counters(&mut self, child: &Evaluator) { - self.current_memory = child.current_memory; + /// Absorb a child's *spend* — peak memory and operation count — + /// while resetting the *live footprint* to `baseline`. Every exit + /// from a comprehension iteration goes through this: the child's + /// tracked values never survive it. A child that spent memory and + /// operations has spent them whether or not it succeeded; if an + /// enclosing construct absorbs a failure (an unresolved-test + /// conditional or boolop) and continues, the parent's peak and op + /// count must include that spend or every absorbed failure evaluates + /// under-metered. But the child's *tracked* values — the body's + /// intermediates and result, the failed sub-expression's operands — + /// are dropped with it (a pushed element is charged separately, once, + /// by `BudgetedVec`), so `current_memory` must not carry them + /// forward: the enclosing construct keeps evaluating on this + /// evaluator, and a stale footprint would charge every later + /// allocation for memory that is not live. + fn absorb_spend_and_reset(&mut self, child: &Evaluator, baseline: usize) { self.peak_memory = child.peak_memory; self.operation_count = child.operation_count; + self.current_memory = baseline; } /// Conclude a list comprehension whose result cannot be computed at @@ -1427,6 +1441,10 @@ impl<'a> Evaluator<'a> { var_name: &str, elem_type: ExprType, ) -> Result { + // Nothing the child tracks survives this function: the loop + // variable is a placeholder and the filter/body values are + // evaluated only for their types. Every exit resets to here. + let memory_baseline = self.current_memory; let mut tmp = crate::symbol_table::SymbolTable::new(); tmp.set(var_name, ExprValue::unresolved(elem_type)) .map_err(|e| ExpressionError::new(e.to_string()))?; @@ -1435,7 +1453,14 @@ impl<'a> Evaluator<'a> { let mut child = self.child_evaluator(&combined); // Check filter clause type if present if let Some(if_clause) = if_clause { - let cond = child.evaluate(if_clause)?; + let cond = child.evaluate(if_clause); + let cond = match cond { + Ok(c) => c, + Err(e) => { + self.absorb_spend_and_reset(&child, memory_baseline); + return Err(e); + } + }; let cond_inner = unwrap_unresolved(&cond.expr_type()); let is_bool_compatible = cond_inner == ExprType::BOOL || cond_inner.code() == crate::types::TypeCode::Unresolved @@ -1443,6 +1468,7 @@ impl<'a> Evaluator<'a> { || (cond_inner.code() == crate::types::TypeCode::Union && cond_inner.params().contains(&ExprType::BOOL)); if !is_bool_compatible { + self.absorb_spend_and_reset(&child, memory_baseline); let err = ExpressionError::new(format!( "List comprehension filter must be a boolean, got {}", cond_inner @@ -1454,8 +1480,9 @@ impl<'a> Evaluator<'a> { }); } } - let body_val = child.evaluate(&lc.elt)?; - self.absorb_counters(&child); + let body_val = child.evaluate(&lc.elt); + self.absorb_spend_and_reset(&child, memory_baseline); + let body_val = body_val?; let body_type = unwrap_unresolved(&body_val.expr_type()); self.track(ExprValue::unresolved(ExprType::list(body_type))) } @@ -1503,6 +1530,14 @@ impl<'a> Evaluator<'a> { None } }); + // Footprint before the iterable is tracked. Every exit that + // abandons the comprehension — an error an enclosing construct + // may absorb — resets to *this*: the iterable is consumed by the + // comprehension and its charge must not outlive it. (The + // per-iteration baseline captured inside the loop still includes + // the iterable; the success path releases it explicitly after + // the loop.) + let comprehension_baseline = self.current_memory; let iterable = self.eval_node(&gen.iter, None)?; let var_name = match &gen.target { ast::Expr::Name(n) => n.id.to_string(), @@ -1510,10 +1545,12 @@ impl<'a> Evaluator<'a> { }; // Unresolved iterable: per-element evaluation is impossible, so - // the comprehension as a whole is unknown. + // the comprehension as a whole is unknown. The iterable is + // consumed here like everywhere else below. if iterable.is_unresolved() { let inner = unwrap_unresolved(&iterable.expr_type()); let elem_type = inner.list_element_type().cloned().unwrap_or(ExprType::INT); + self.current_memory = comprehension_baseline; return self.eval_listcomp_unresolved(lc, gen.ifs.first(), &var_name, elem_type); } @@ -1530,6 +1567,9 @@ impl<'a> Evaluator<'a> { } else if let ExprValue::RangeExpr(r) = &iterable { Box::new(r.iter().map(ExprValue::Int)) } else { + // A type error an enclosing construct may absorb — the + // iterable's charge must not outlive the comprehension. + self.current_memory = comprehension_baseline; return Err(ExpressionError::type_error(format!( "Cannot iterate over {}", iterable.expr_type() @@ -1561,7 +1601,19 @@ impl<'a> Evaluator<'a> { child.regex_cache = std::mem::take(&mut self.regex_cache); let mut include = true; if let Some(if_clause) = gen.ifs.first() { - let cond = child.evaluate(if_clause)?; + let cond = match child.evaluate(if_clause) { + Ok(c) => c, + Err(e) => { + // The child's spend counts even though it + // failed, but its tracked values are dropped + // with it (see `absorb_spend_and_reset`), and so + // is the iterable — the comprehension is + // abandoned. The regex cache comes back. + self.absorb_spend_and_reset(&child, comprehension_baseline); + self.regex_cache = child.regex_cache; + return Err(e); + } + }; if let ExprValue::Bool(b) = cond { include = b; } else if cond.is_unresolved() { @@ -1571,12 +1623,13 @@ impl<'a> Evaluator<'a> { // only pre-checks the budget; the finished list // would be tracked by make_list_checked, which is // never reached. - self.absorb_counters(&child); + self.absorb_spend_and_reset(&child, memory_baseline); self.regex_cache = child.regex_cache; - self.current_memory = memory_baseline; filter_unresolved = true; break; } else { + self.absorb_spend_and_reset(&child, comprehension_baseline); + self.regex_cache = child.regex_cache; let err = ExpressionError::new(format!( "List comprehension filter must be a boolean, got {}", cond.expr_type() @@ -1588,18 +1641,40 @@ impl<'a> Evaluator<'a> { }); } } - if include { - let elt = child.eval_node(&lc.elt, elem_target.as_ref())?; - result.push(self, elt)?; - } - self.absorb_counters(&child); + let elt = if include { + match child.eval_node(&lc.elt, elem_target.as_ref()) { + Ok(v) => Some(v), + Err(e) => { + self.absorb_spend_and_reset(&child, comprehension_baseline); + self.regex_cache = child.regex_cache; + return Err(e); + } + } + } else { + None + }; + // Hand the child's spend and the regex cache back before the + // push's budget pre-check, so an exit there leaves the + // parent's peak/op counters current. The footprint resets to + // the iteration baseline: the child's tracked values — the + // body's intermediates and its result (`eval_name` tracks + // the clone it returns for `x`; the loop variable's own slot + // in the temp symtab is never tracked) — do not survive the + // iteration, and the element about to be pushed is charged + // once, by BudgetedVec's pre-check, not again through the + // child's footprint. + self.absorb_spend_and_reset(&child, memory_baseline); self.regex_cache = child.regex_cache; - // Restore the iteration baseline: the child's tracked - // transients (the loop-variable clone and any intermediates) - // do not survive the iteration, and the result elements are - // accounted separately by BudgetedVec. Peak memory keeps - // the high-water mark absorbed above. - self.current_memory = memory_baseline; + if let Some(elt) = elt { + if let Err(e) = result.push(self, elt) { + // Only a budget exceedance can land here, which no + // enclosing construct absorbs — but the exit drops + // the iterable like every other, so account for it + // the same way. + self.current_memory = comprehension_baseline; + return Err(e); + } + } } // The iterable is consumed by the comprehension: release its // tracked memory now that iteration is done (the borrowing diff --git a/crates/openjd-expr/tests/integration/test_memory.rs b/crates/openjd-expr/tests/integration/test_memory.rs index 10b6ab5f..e7daf02c 100644 --- a/crates/openjd-expr/tests/integration/test_memory.rs +++ b/crates/openjd-expr/tests/integration/test_memory.rs @@ -577,9 +577,13 @@ fn comprehension_over_range_memory_bounded_incrementally() { assert_eq!( e, [ - // 16480 = logical result bytes plus the Vec's projected - // post-doubling capacity slack: the check accounts for the - // buffer the push is about to allocate, not just elements. + // 16480 = logical result bytes (including the element being + // pushed, charged once by BudgetedVec) plus the Vec's + // projected post-doubling capacity slack: the check accounts + // for the buffer the push is about to allocate, not just + // elements. The iteration's own transients are not in the + // figure — the footprint is reset to the pre-iteration + // baseline before the push. "Expression memory usage (16480 bytes) exceeded limit (10000 bytes) ", " [x for x in range_expr('1-2000000')] @@ -984,3 +988,329 @@ fn boolop_does_not_reabsorb_budget_error_from_nested_conditional() { err.message() ); } + +/// `contains_budget_error` recurses into compound sub-errors. When +/// *both* branches of an unresolved-test conditional fail — one with a +/// budget exceedance, one with a value error — the conditional +/// produces a compound "Both branches fail" error whose own kind is not +/// a budget kind; only the recursion can see the budget exceedance +/// inside it. Wrapped in a boolop suppression site, that compound must +/// still propagate. (Without the recursion this test fails: the +/// compound is swallowed as a plain value error.) +#[test] +fn budget_error_inside_compound_both_branches_fail_error_propagates_through_boolop() { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + let err = ParsedExpression::new( + "Session.Flag or ('A' * 10000000 if Session.Flag else int('nope')) == 'x'", + ) + .and_then(|p| { + p.with_memory_limit(1024 * 1024) + .with_operation_limit(DEFAULT_OPERATION_LIMIT) + .evaluate_with_metrics(&[&st]) + }) + .expect_err("a compound error carrying a budget exceedance must propagate"); + let msg = err.message(); + assert!( + msg.contains("Both branches fail in the if/else:"), + "Got: {msg}" + ); + assert!(msg.contains("exceeded limit (1048576 bytes)"), "Got: {msg}"); + assert!(msg.contains("Cannot convert 'nope' to int"), "Got: {msg}"); +} + +/// Control: a compound both-branches-fail error with *no* budget +/// exceedance inside is still suppressed by the boolop. +#[test] +fn compound_both_branches_fail_without_budget_error_is_suppressed_by_boolop() { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + let result = + ParsedExpression::new("Session.Flag or (int('a') if Session.Flag else int('b')) == 7") + .and_then(|p| { + p.with_memory_limit(1024 * 1024) + .with_operation_limit(DEFAULT_OPERATION_LIMIT) + .evaluate_with_metrics(&[&st]) + }) + .expect("a compound value error must be suppressed"); + assert!(result.value.is_unresolved()); +} + +// ══════════════════════════════════════════════════════════════ +// A failing comprehension's spend is absorbed into the parent +// ══════════════════════════════════════════════════════════════ + +/// A comprehension iteration that fails has still spent the memory and +/// operations its filter/body consumed before failing. When an +/// unresolved-test conditional absorbs the failure and continues, the +/// parent's counters must include that spend — otherwise every absorbed +/// comprehension failure evaluates under-metered. Observable through +/// `peak_memory`: the failing iteration builds a 1 MB string before +/// erroring, and that high-water mark must survive the failure. +#[test] +fn failing_comprehension_spend_is_absorbed_by_enclosing_conditional() { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + // The body builds a 1 MB string and then fails converting it to int; + // the conditional absorbs the value error (Session.Flag might select + // the else branch at run time). + let result = ParsedExpression::new("[int('A' * 1000000) for x in [1]] if Session.Flag else []") + .and_then(|p| p.evaluate_with_metrics(&[&st])) + .expect("the conditional absorbs the if-branch's value error"); + assert!(result.value.is_unresolved()); + assert!( + result.peak_memory >= 1_000_000, + "the failed iteration's 1 MB allocation must be reflected in peak memory; got {}", + result.peak_memory + ); +} + +/// Shared shape for the four other absorption sites: an enclosing +/// unresolved-test conditional swallows the comprehension's value +/// error, and the 1 MB the failing site allocated before erroring must +/// survive into the parent's high-water mark. +fn assert_absorbed_spend(expr: &str) { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + st.set( + "Session.List", + ExprValue::unresolved(openjd_expr::ExprType::list(openjd_expr::ExprType::INT)), + ) + .unwrap(); + let result = ParsedExpression::new(expr) + .and_then(|p| p.evaluate_with_metrics(&[&st])) + .unwrap_or_else(|e| panic!("the conditional must absorb the value error for {expr}: {e}")); + assert!(result.value.is_unresolved()); + assert!( + result.peak_memory >= 1_000_000, + "{expr}: the failed site's 1 MB allocation must be reflected in peak memory; got {}", + result.peak_memory + ); +} + +/// Concrete loop, filter clause errors. +#[test] +fn failing_comprehension_filter_spend_is_absorbed() { + assert_absorbed_spend("[x for x in [1] if int('A' * 1000000) > 0] if Session.Flag else []"); +} + +/// Concrete loop, filter evaluates to a non-boolean (the type-error +/// arm) after spending. +#[test] +fn nonbool_comprehension_filter_spend_is_absorbed() { + assert_absorbed_spend("[x for x in [1] if 'A' * 1000000] if Session.Flag else []"); +} + +/// Unresolved-iterable path, filter clause errors. +#[test] +fn failing_unresolved_comprehension_filter_spend_is_absorbed() { + assert_absorbed_spend( + "[x for x in Session.List if int('A' * 1000000) > 0] if Session.Flag else []", + ); +} + +/// Unresolved-iterable path, body errors. +#[test] +fn failing_unresolved_comprehension_body_spend_is_absorbed() { + assert_absorbed_spend("[int('A' * 1000000) for x in Session.List] if Session.Flag else []"); +} + +// ══════════════════════════════════════════════════════════════ +// An absorbed comprehension failure leaves no live footprint behind +// ══════════════════════════════════════════════════════════════ + +/// Absorbing a failing iteration's *spend* (peak, ops) must not also +/// carry its *live footprint* forward. The child's tracked values — the +/// loop-variable clone and the failed sub-expression's intermediates — +/// are dropped with it, and the enclosing construct that absorbs the +/// error keeps evaluating on this evaluator. `'A' * 1000000 - 1` fails +/// in the binop with the 1 MB string still tracked (dispatch releases +/// inputs only on success), so a stale footprint would charge the +/// later 600 KB allocation as 1.6 MB against a 1.5 MB limit. +fn assert_no_stale_footprint(expr: &str) { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + st.set( + "Session.List", + ExprValue::unresolved(openjd_expr::ExprType::list(openjd_expr::ExprType::INT)), + ) + .unwrap(); + let result = ParsedExpression::new(expr) + .and_then(|p| p.with_memory_limit(1_500_000).evaluate_with_metrics(&[&st])) + .unwrap_or_else(|e| panic!("{expr}: the dropped footprint must not be charged: {e}")); + // Peak still reflects the failed iteration's genuine 1 MB spend. + assert!( + result.peak_memory >= 1_000_000, + "{expr}: peak must include the failed iteration's spend; got {}", + result.peak_memory + ); +} + +#[test] +fn absorbed_comprehension_body_failure_leaves_no_stale_footprint() { + assert_no_stale_footprint( + "len(['A' * 1000000 - 1 for x in [1]] if Session.Flag else []) + len('B' * 600000)", + ); +} + +#[test] +fn absorbed_comprehension_filter_failure_leaves_no_stale_footprint() { + assert_no_stale_footprint( + "len([x for x in [1] if 'A' * 1000000 - 1] if Session.Flag else []) + len('B' * 600000)", + ); +} + +#[test] +fn absorbed_unresolved_comprehension_body_failure_leaves_no_stale_footprint() { + assert_no_stale_footprint( + "len(['A' * 1000000 - 1 for x in Session.List] if Session.Flag else []) + len('B' * 600000)", + ); +} + +#[test] +fn absorbed_unresolved_comprehension_filter_failure_leaves_no_stale_footprint() { + assert_no_stale_footprint( + "len([x for x in Session.List if 'A' * 1000000 - 1] if Session.Flag else []) + len('B' * 600000)", + ); +} + +// ══════════════════════════════════════════════════════════════ +// The push pre-check charges each element once +// ══════════════════════════════════════════════════════════════ + +/// The element a comprehension is about to push is accounted by +/// BudgetedVec's pre-check. It must not *also* sit in the parent's +/// live footprint through the child's tracked result — that double- +/// charge would shrink the effective memory limit by one element per +/// push. Five 100 KB elements plus Vec slack fit comfortably under +/// 560 KB; a double-charge needs ~600 KB and fails. +#[test] +fn comprehension_push_precheck_charges_each_element_once() { + let st = SymbolTable::new(); + let r = ParsedExpression::new("['A' * 100000 for x in range(5)]") + .and_then(|p| p.with_memory_limit(560_000).evaluate_with_metrics(&[&st])) + .expect("five 100 KB elements must fit under a 560 KB limit"); + assert_eq!( + r.value.expr_type(), + openjd_expr::ExprType::list(openjd_expr::ExprType::STRING) + ); +} + +/// The reported `used` for a large-element exceedance is the honest +/// figure: at the third push, two 600 KB elements are held plus the +/// third being pushed plus the Vec's projected slack — ~1.8 MB. A +/// double-charge of the pushed element would have reported the same +/// ~1.8 MB one push *earlier*, at the second element, when only ~1.2 MB +/// was live (pinned here indirectly: the two-element variant fits). +#[test] +fn large_element_comprehension_exceedance_reports_true_usage() { + let st = SymbolTable::new(); + ParsedExpression::new("['A' * 600000 for x in [1, 2]]") + .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) + .expect("two 600 KB elements must fit under a 1.5 MB limit"); + let e = ParsedExpression::new("['A' * 600000 for x in [1, 2, 3]]") + .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + "Expression memory usage (1800768 bytes) exceeded limit (1500000 bytes)\n", + " ['A' * 600000 for x in [1, 2, 3]]\n", + " ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~", + ] + .concat() + ); +} + +// ══════════════════════════════════════════════════════════════ +// An abandoned comprehension's iterable is not charged afterwards +// ══════════════════════════════════════════════════════════════ + +/// The iterable is tracked before the loop and, on the success path, +/// released after it ("consumed by the comprehension"). An error exit +/// that an enclosing construct absorbs must drop it too: the +/// comprehension is abandoned and nothing references the iterable, so +/// leaving its 600 KB in `current_memory` would charge every later +/// allocation for it — here the 600 KB `'B'` string, which then reads +/// as 1.2 MB against a 1 MB limit. The failures are chosen to be cheap +/// (`int()` releases its argument before dispatch; the type errors +/// allocate nothing) so the iterable's residue is what is observed, +/// not a body-time budget exceedance. The large list comes from the +/// symbol table: a list *literal* is charged for both its element and +/// itself while being built — a separate, pre-existing `eval_list` +/// accounting matter — which would trip the limit before the +/// comprehension runs. +fn assert_iterable_not_charged_after_abandonment(expr: &str) { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + st.set( + "Big", + ExprValue::make_list( + vec![ExprValue::String("C".repeat(600_000))], + openjd_expr::ExprType::STRING, + ) + .unwrap(), + ) + .unwrap(); + ParsedExpression::new(expr) + .and_then(|p| p.with_memory_limit(1_000_000).evaluate(&[&st])) + .unwrap_or_else(|e| panic!("{expr}: the abandoned iterable must not be charged: {e}")); +} + +/// Body error over a large iterable. +#[test] +fn abandoned_comprehension_body_error_drops_iterable_charge() { + assert_iterable_not_charged_after_abandonment( + "len([int('nope') for x in Big] if Session.Flag else []) + len('B' * 600000)", + ); +} + +/// Filter error over a large iterable. +#[test] +fn abandoned_comprehension_filter_error_drops_iterable_charge() { + assert_iterable_not_charged_after_abandonment( + "len([x for x in Big if int('nope') > 0] if Session.Flag else []) + len('B' * 600000)", + ); +} + +/// Non-boolean filter over a large iterable. +#[test] +fn abandoned_comprehension_nonbool_filter_drops_iterable_charge() { + assert_iterable_not_charged_after_abandonment( + "len([x for x in Big if 1] if Session.Flag else []) + len('B' * 600000)", + ); +} + +/// "Cannot iterate" type error: the iterable itself is the large value +/// (a string is tracked once, so a literal is fine here). +#[test] +fn abandoned_comprehension_cannot_iterate_drops_iterable_charge() { + assert_iterable_not_charged_after_abandonment( + "len([x for x in 'C' * 600000] if Session.Flag else []) + len('B' * 600000)", + ); +} diff --git a/crates/openjd-model/src/job/create_job/instantiate.rs b/crates/openjd-model/src/job/create_job/instantiate.rs index 311a2df9..8fdc246f 100644 --- a/crates/openjd-model/src/job/create_job/instantiate.rs +++ b/crates/openjd-model/src/job/create_job/instantiate.rs @@ -255,6 +255,58 @@ fn add_unresolved_session_symbols(symtab: &mut SymbolTable) -> Result<(), ModelE Ok(()) } +/// Evaluate a script's `let` bindings into a check symbol table, for +/// both check-symtab builders (step scripts and environments). +/// +/// Parses under the caller's host profile — the same profile pass 8 +/// parsed the bindings with — so syntax the profile does not enable is +/// refused here exactly as it was at template validation. (Parsing +/// with the latest profile instead would accept at job creation what +/// pass 8 refused, or vice versa after a crate upgrade.) Evaluates +/// under `PathFormat::Posix` like every other job-creation evaluation, +/// with the caller's budgets. A binding that fails to evaluate fails +/// job creation: its expression type-checked at pass 8 with everything +/// unresolved, so the failure comes from the real parameter values and +/// would deterministically recur in every session. +fn evaluate_check_let_bindings( + bindings: &[String], + check_symtab: &mut SymbolTable, + host_profile: &openjd_expr::ExprProfile, + budgets: super::EvalBudgets, +) -> Result<(), ModelError> { + let host_lib = openjd_expr::FunctionLibrary::for_profile(host_profile); + for binding in bindings { + let Some(eq_pos) = binding.find('=') else { + continue; + }; + let name = binding[..eq_pos].trim(); + let expr = binding[eq_pos + 1..].trim(); + if name.is_empty() || expr.is_empty() { + continue; + } + let parsed = openjd_expr::eval::ParsedExpression::with_profile(expr, host_profile) + .map_err(|e| { + ModelError::Expression(ExpressionError::new(format!( + "script let binding '{name}': {e}" + ))) + })?; + let val = budgeted( + parsed + .with_path_format(PathFormat::Posix) + .with_library(&host_lib), + budgets, + ) + .evaluate(&[check_symtab as &SymbolTable]) + .map_err(|e| { + ModelError::Expression(ExpressionError::new(format!( + "script let binding '{name}': {e}" + ))) + })?; + check_symtab.set(name, val)?; + } + Ok(()) +} + /// Build the check symbol table for a step script's carried-forward /// format strings (task scope): the step's symtab (concrete `Param.*` / /// `RawParam.*` / `Job.Name` / `Step.Name` / step-level `let` @@ -321,35 +373,7 @@ fn build_task_check_symtab( let host_profile = ctx .profile .to_expr_profile(openjd_expr::HostContext::Unresolved); - let host_lib = openjd_expr::FunctionLibrary::for_profile(&host_profile); - for binding in bindings { - if let Some(eq_pos) = binding.find('=') { - let name = binding[..eq_pos].trim(); - let expr = binding[eq_pos + 1..].trim(); - if !name.is_empty() && !expr.is_empty() { - let parsed = - openjd_expr::eval::ParsedExpression::with_profile(expr, &host_profile) - .map_err(|e| { - ModelError::Expression(ExpressionError::new(format!( - "script let binding '{name}': {e}" - ))) - })?; - let val = budgeted( - parsed - .with_path_format(PathFormat::Posix) - .with_library(&host_lib), - budgets, - ) - .evaluate(&[&check_symtab as &SymbolTable]) - .map_err(|e| { - ModelError::Expression(ExpressionError::new(format!( - "script let binding '{name}': {e}" - ))) - })?; - check_symtab.set(name, val)?; - } - } - } + evaluate_check_let_bindings(bindings, &mut check_symtab, &host_profile, budgets)?; } } @@ -394,15 +418,7 @@ pub(super) fn build_env_check_symtab( let host_profile = ctx .profile .to_expr_profile(openjd_expr::HostContext::Unresolved); - let host_lib = openjd_expr::FunctionLibrary::for_profile(&host_profile); - symtab = evaluate_let_bindings( - bindings, - &symtab, - Some(&host_lib), - PathFormat::Posix, - budgets.memory, - budgets.operations, - )?; + evaluate_check_let_bindings(bindings, &mut symtab, &host_profile, budgets)?; } } } diff --git a/crates/openjd-model/src/job/create_job/mod.rs b/crates/openjd-model/src/job/create_job/mod.rs index 7ba0ff73..6ced3282 100644 --- a/crates/openjd-model/src/job/create_job/mod.rs +++ b/crates/openjd-model/src/job/create_job/mod.rs @@ -112,11 +112,17 @@ pub fn create_job( ctx.profile.revision(), ))); } - let missing: Vec<&str> = crate::types::ModelExtension::ALL + // Iterate the template's own extension set rather than + // `ModelExtension::ALL`: exhaustive by construction, so a future + // variant omitted from `ALL` cannot escape the check. Sorted so the + // message is deterministic (the set is a HashSet). + let mut missing: Vec<&str> = template_profile + .extensions() .iter() - .filter(|e| template_profile.has_extension(**e) && !ctx.profile.has_extension(**e)) + .filter(|e| !ctx.profile.has_extension(**e)) .map(|e| e.as_str()) .collect(); + missing.sort_unstable(); if !missing.is_empty() { return Err(ModelError::Compatibility(format!( "create_job requires a context enabling every extension the template declares: \ diff --git a/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs b/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs index e6df2664..074f06ca 100644 --- a/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs +++ b/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs @@ -827,8 +827,9 @@ fn check_resolved_constraint( /// Every evaluation runs under `PathFormat::Posix`: template validation /// and job creation happen outside host context, where the model keeps /// all paths POSIX (only `openjd-sessions` evaluates under -/// `PathFormat::host()` — see the Path Parameters section of -/// `specs/model/job-creation.md`). This keeps the two stages consistent +/// `PathFormat::host()` — see `preprocess_job_parameters` in +/// `specs/model/job-creation.md`, which explains why paths stay POSIX +/// until template evaluation on the host). This keeps the two stages consistent /// with each other and with the POSIX-format values `create_job` seeds /// into its check symbol tables, and makes validation outcomes /// independent of the OS running them. diff --git a/crates/openjd-model/tests/integration/test_job_creation_resolved_values.rs b/crates/openjd-model/tests/integration/test_job_creation_resolved_values.rs index 53f9cc7d..4b110140 100644 --- a/crates/openjd-model/tests/integration/test_job_creation_resolved_values.rs +++ b/crates/openjd-model/tests/integration/test_job_creation_resolved_values.rs @@ -669,3 +669,75 @@ fn listcomp_with_task_param_filter_over_bound_param_passes_create_job() { create_default(template, &[("Files", "a,b,c"), ("X", "x"), ("N", "1")]) .expect("an unresolved comprehension filter must not fail job creation"); } + +// ══════════════════════════════════════════════════════════════ +// Step-script and environment `let` bindings share one check path +// ══════════════════════════════════════════════════════════════ + +/// Both check-symtab builders evaluate `let` bindings through the same +/// helper: parsed under the caller's host profile (as pass 8 parsed +/// them — never the latest profile, which would accept syntax pass 8 +/// refused or vice versa after a crate upgrade), evaluated under POSIX +/// with the caller's budgets, and reported with one message format. A +/// value-dependent failure in an environment `let` is therefore +/// reported exactly like the same failure in a step-script `let`. +fn let_failure_message(template: &str, params: &[(&str, &str)]) -> String { + let msg = create_default(template, params).expect_err("expected the let binding to fail"); + // Isolate the let-binding diagnostic (path/context prefixes differ + // between the two scopes by design; the binding message must not). + let start = msg + .find("script let binding") + .unwrap_or_else(|| panic!("no let-binding diagnostic in:\n{msg}")); + msg[start..].to_string() +} + +#[test] +fn env_let_and_script_let_failures_report_identically_at_create_job() { + let script_let = r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "Test", + "parameterDefinitions": [ + {"name": "X", "type": "STRING"}, + {"name": "N", "type": "INT"} + ], + "steps": [{"name": "S", "script": { + "let": ["q = 1 / int(Param.X)"], + "actions": {"onRun": {"command": "echo", "args": ["{{ q }}"]}} + }}] + }"#; + let env_let = r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "Test", + "parameterDefinitions": [ + {"name": "X", "type": "STRING"}, + {"name": "N", "type": "INT"} + ], + "jobEnvironments": [{"name": "Env", "script": { + "let": ["q = 1 / int(Param.X)"], + "actions": {"onEnter": {"command": "echo", "args": ["{{ q }}"]}} + }}], + "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "echo"}}}}] + }"#; + let params = [("X", "0"), ("N", "1")]; + let from_script = let_failure_message(script_let, ¶ms); + let from_env = let_failure_message(env_let, ¶ms); + assert_eq!( + from_script, from_env, + "the two scopes must report identically" + ); + assert!( + from_env.starts_with("script let binding 'q': Division by zero"), + "Got:\n{from_env}" + ); + assert!( + from_env.contains(" 1 / int(Param.X)\n"), + "Got:\n{from_env}" + ); + assert!(from_env.contains(" ~~^~~~~~~~~~~~~~"), "Got:\n{from_env}"); + + // Control: a non-zero divisor passes both scopes. + create_default(script_let, &[("X", "2"), ("N", "1")]).expect("script let must pass"); + create_default(env_let, &[("X", "2"), ("N", "1")]).expect("env let must pass"); +} diff --git a/crates/openjd-model/tests/integration/test_model_profile.rs b/crates/openjd-model/tests/integration/test_model_profile.rs index 8fb0dc01..08e46df2 100644 --- a/crates/openjd-model/tests/integration/test_model_profile.rs +++ b/crates/openjd-model/tests/integration/test_model_profile.rs @@ -143,6 +143,38 @@ fn context_missing_declared_extension_is_rejected() { ); } +#[test] +fn context_missing_several_declared_extensions_lists_them_sorted() { + // The missing set is derived from the template's own extension set + // (a HashSet) and sorted, so the message is deterministic and + // exhaustive regardless of declaration order. + let tpl = yaml_val( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "name": "RenderJob", + "extensions": ["FEATURE_BUNDLE_1", "EXPR"], + "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "run"}}}}] + }"#, + ); + let jt = decode_job_template( + tpl, + Some(&["EXPR", "FEATURE_BUNDLE_1"]), + &CallerLimits::default(), + ) + .unwrap(); + let params = preprocess_posix_defaults(&jt); + + let ctx = ValidationContext::from_profile(ModelProfile::current()); + let err = create_job(&jt, ¶ms, &ctx).unwrap_err(); + assert_eq!( + err.to_string(), + "Compatibility error: create_job requires a context enabling every extension the \ + template declares: missing EXPR, FEATURE_BUNDLE_1. An application that does not \ + support an extension should reject the template at decode via its \ + supported-extensions list." + ); +} + // ─── Test D: latest() has all ModelExtension::ALL variants ────────────────── #[test] diff --git a/crates/openjd-sessions/tests/integration/test_session_scenarios.rs b/crates/openjd-sessions/tests/integration/test_session_scenarios.rs index 8c8e9afc..106223ae 100644 --- a/crates/openjd-sessions/tests/integration/test_session_scenarios.rs +++ b/crates/openjd-sessions/tests/integration/test_session_scenarios.rs @@ -206,20 +206,7 @@ async fn run_scenario(scenario_path: &Path) { .unwrap_or_else(|e| panic!("Failed to preprocess params for '{}': {e}", scenario.name)); // Create job - let ctx = { - let mut exts = std::collections::HashSet::new(); - if let Some(ext_list) = &job_template.extensions { - exts.extend( - ext_list - .iter() - .filter_map(|e| e.as_str().parse::().ok()), - ); - } - openjd_model::ValidationContext::with_extensions( - openjd_model::SpecificationRevision::V2023_09, - exts, - ) - }; + let ctx = job_template.default_validation_context(); let job = create_job(&job_template, &job_params, &ctx) .unwrap_or_else(|e| panic!("Failed to create job for '{}': {e}", scenario.name)); diff --git a/specs/cli/summary.md b/specs/cli/summary.md index a87d025b..f5fbc2c0 100644 --- a/specs/cli/summary.md +++ b/specs/cli/summary.md @@ -56,6 +56,9 @@ execute(args) ├── Resolve job_template_dir and current_working_dir ├── preprocess_job_parameters() → param_values ├── create_job() → Job + │ context: job_template.default_validation_context() with + │ common::caller_limits() layered on — the same limits the decode + │ ran under (see run.md § Session Configuration / check.md § Caller-Limits Policy) │ └── Dispatch on --step ├── Some(step_name) → output_step_summary(&job, step_name, output_format) diff --git a/specs/expr/evaluator.md b/specs/expr/evaluator.md index ffbbd5e2..2739ddba 100644 --- a/specs/expr/evaluator.md +++ b/specs/expr/evaluator.md @@ -415,6 +415,33 @@ Evaluates list comprehensions: `[expr for var in iterable if condition]`. A filter whose *type* can never be a boolean is still an error on both paths. - Operation count: +1 per iteration +- Each iteration's child evaluator hands its *spend* — memory + high-water mark and operation count — back to the parent on **every** + exit, including when the filter or body errors. A failing iteration + has still spent its memory and operations in this evaluation; if an + enclosing construct absorbs the error (an unresolved-test conditional + or boolop) and continues, the parent's counters must include that + spend, or every absorbed comprehension failure evaluates + under-metered. The child's *live footprint* is never absorbed: on + every exit — success, failure, or abandonment, and on every exit of + the unresolved-iterable path — the parent's `current_memory` is reset + to its pre-iteration baseline. Nothing the child tracks survives the + iteration: the body's intermediates and its result are dropped (the + loop variable's own slot in the temp symbol table is never tracked; + what `eval_name` tracks is the clone it returns when the body reads + it), and the element being pushed is charged exactly once, by + `BudgetedVec`'s pre-check — not a second time through the child's + footprint, which would shrink the effective limit by one element per + push. The absorbing construct keeps evaluating on this evaluator, so + a stale footprint would charge every later allocation for memory + that is not live. +- The iterable is consumed by the comprehension. The success path + releases it after the loop; every exit that *abandons* the + comprehension — an unresolved iterable, a non-iterable value, a + failing filter or body, a push-time exceedance — resets + `current_memory` to the footprint captured *before* the iterable was + evaluated, so an enclosing construct that absorbs the error is not + charged for a list nothing references. - Iterates lists without copying and symbolic ranges lazily - Pre-checks the growing result vector's values and projected capacity against the memory limit diff --git a/specs/model/job-creation.md b/specs/model/job-creation.md index 0d2305da..50fd16c0 100644 --- a/specs/model/job-creation.md +++ b/specs/model/job-creation.md @@ -253,7 +253,7 @@ time, with everything only a session can know left `Unresolved`: here, deterministically, rather than in every session). - **Session scope** (job and step environments): as above minus `Task.*`, plus this environment's `Env.File.*` (`Unresolved`) and its - script-level `let` bindings evaluated in via `evaluate_let_bindings`. + script-level `let` bindings evaluated in. An environment `let` binding that fails to evaluate fails job creation, under the same error policy as every other evaluation this stage performs (see below): the bindings only evaluate when the @@ -262,6 +262,25 @@ time, with everything only a session can know left `Unresolved`: the real parameter values and would deterministically recur in every session that enters the environment. +Both scopes evaluate their script-level `let` bindings through one +shared path: each binding is **parsed under the context's host +profile** — the same profile pass 8 parsed it with, never the latest +profile, so syntax the profile does not enable is refused here exactly +as at template validation (and a crate upgrade cannot make job creation +accept what pass 8 refused, or vice versa) — then evaluated under +`PathFormat::Posix` with the caller's budgets. Failures from either +scope carry the same `script let binding '': ` diagnostic, +with the caret aligned to the bare expression (pass 8 and the run-time +path align it to the full `name = expr` binding string; the check path +reports the expression alone because its message already names the +binding). Structurally malformed bindings — no `=`, empty name, empty +expression — are skipped rather than reported: pass 8 rejects all three +at decode, so they cannot reach a template that came through +`decode_job_template`, and a hand-built `JobTemplate` that bypassed +decode still fails on them at run time. The public +`evaluate_let_bindings` (below) is the run-time entry point used by +`openjd-sessions` and `openjd-for-js`; the check symtabs do not use it. + Failures are `ModelError::ModelValidation` at the same field paths pass 8 uses, e.g. `steps[0] -> script -> actions -> onRun -> args[0]:` / diff --git a/specs/model/validation.md b/specs/model/validation.md index fdcf494f..a657b1b4 100644 --- a/specs/model/validation.md +++ b/specs/model/validation.md @@ -243,8 +243,10 @@ right `ExprProfile` from a model profile. Every evaluation this pass performs — format-string expressions and `let` bindings alike — runs under `PathFormat::Posix`. Template validation happens outside host context, where the model keeps all -paths POSIX (see the path-parameters discussion in -`specs/model/job-creation.md`); only `openjd-sessions` evaluates under +paths POSIX (see `preprocess_job_parameters` in +`specs/model/job-creation.md`, which explains why paths stay POSIX +until template evaluation on the host); only `openjd-sessions` +evaluates under `PathFormat::host()`. This keeps pass 8 consistent with job creation's re-checks (which read POSIX-format values out of their check symbol tables) and makes validation outcomes independent of the OS running