From af3e0cfc04dcd327fe7adbf39dad607aef461463 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 18:34:23 +0000 Subject: [PATCH] fix(tiers): evaluate `:or` destructuring defaults eagerly in every tier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clojure's `destructure` expands a symbol carrying an `:or` entry to `(get m :k default)`. `get` is an ordinary function, so the default is evaluated whether or not the key is present. The tree-walker does that; the compiled tiers did not, and the two disagreed: - `lower_with_default` lowered the default into the `then` arm of an `IsNil` branch, so it only ran on a miss. Lower it into the current block instead — straight-line and unconditional — and keep the branch purely as a select over two already-computed values. - `execute_ir` bound the arity's destructuring patterns into the env via `bind_fn_params` on top of the prologue the lowerer emits, so with the eager lowering above each default would run twice per call. The ANF lowerer never emits `LoadLocal`, so those env bindings are never read: bind only the named params there (`bind_fn_params_positional`), and let the prologue own the destructuring. Because tier-up happens partway through a run, and lowering is driven by a background worker, the old split made a side-effecting or throwing default fire a nondeterministic number of times: the issue's reproduction reported 133/130/140/130/144 evaluations out of 200 calls across five runs of the same binary. It now reports 200 in every tier and every run. Pinned in two places: `execution_tier_parity` gains an `:or` case with a side-effect counter, so tree-walk, eager IR, JIT and AOT must agree on the count (it fails without either half of this fix), and a new `destructure_or_default_eager` test runs the discriminating programs through the tree-walker and the IR tier side by side. Fixes #363 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NLQqFN41sWfs1hF5uyfN8N --- crates/cljrs-ir/README.md | 14 ++ crates/cljrs-ir/src/lower/anf.rs | 14 +- crates/cljrs-runtime/README.md | 21 +++ crates/cljrs-runtime/src/interp/apply.rs | 40 ++++- crates/cljrs-runtime/src/tiered/apply.rs | 8 +- .../tests/destructure_or_default_eager.rs | 155 ++++++++++++++++++ crates/cljrs/tests/execution_tier_parity.rs | 20 ++- 7 files changed, 263 insertions(+), 9 deletions(-) create mode 100644 crates/cljrs-runtime/tests/destructure_or_default_eager.rs diff --git a/crates/cljrs-ir/README.md b/crates/cljrs-ir/README.md index dd49d6158..177f7e492 100644 --- a/crates/cljrs-ir/README.md +++ b/crates/cljrs-ir/README.md @@ -215,6 +215,20 @@ for `[a b c]` patterns — a short collection binds the missing positions to `nil` rather than throwing. Both compile to the `rt_nth` bridge (already nil-on-OOB); the IR interpreter appends the nil default for `NthLenient`. +### `:or` destructuring defaults are eager + +`destructure` expands a symbol carrying an `:or` entry to `(get m :k default)`, +and `get` is an ordinary function: its third argument is evaluated whether or +not the key is present. `lower_with_default` therefore lowers the default into +the *current* block — straight-line and unconditional — and uses the `IsNil` +check only to select between the default and the looked-up value (an `IsNil` +branch over two empty arms, joined by a `Phi`). Lowering the default into the +branch's `then` arm instead would fire a side-effecting or throwing default only +on a miss, diverging from the tree-walker (`cljrs-runtime`'s +`interp/destructure.rs`) — and, because a function tiers up partway through a +run, changing a program's behaviour mid-execution. See issue #363; both tiers +are pinned by `cljrs-runtime/tests/destructure_or_default_eager.rs`. + Some `KnownFn` variants exist purely for analysis precision — the codegen and IR interpreter dispatch them through the dynamic builtin lookup like a regular `Call`, but the analyzer can use them to tighten diff --git a/crates/cljrs-ir/src/lower/anf.rs b/crates/cljrs-ir/src/lower/anf.rs index 44f9d49bf..e0edeaf7d 100644 --- a/crates/cljrs-ir/src/lower/anf.rs +++ b/crates/cljrs-ir/src/lower/anf.rs @@ -1037,9 +1037,22 @@ fn apply_default_if_nil( } fn lower_with_default(ctx: &mut LowerCtx, got: VarId, default_form: &Form) -> R { + // `destructure` expands an `:or` entry to `(get m :k default)`, and `get` + // is an ordinary function: its third argument is evaluated whether or not + // the key is present. So the default is lowered into the *current* block — + // straight-line, unconditional — and the nil check only selects between the + // two already-computed values. Lowering it into the `then` block instead + // would make a side-effecting or throwing default fire only on a miss, + // which is what the tree-walker does not do (see + // `cljrs-runtime/src/interp/destructure.rs`); with tier-up mid-program that + // divergence is observable as a behaviour change partway through a run. + let default_var = lower_form(ctx, default_form)?; + let nil_check = ctx.fresh_var(); ctx.emit(Inst::CallKnown(nil_check, KnownFn::IsNil, vec![got])); + // The select: both arms are empty, so no evaluation is conditional. The + // phi picks `default_var` when the looked-up value was nil, `got` otherwise. let then_block = ctx.fresh_block(); let else_block = ctx.fresh_block(); let merge_block = ctx.fresh_block(); @@ -1051,7 +1064,6 @@ fn lower_with_default(ctx: &mut LowerCtx, got: VarId, default_form: &Form) -> R }); ctx.start_block(then_block); - let default_var = lower_form(ctx, default_form)?; let then_exit = ctx.current_block_id(); ctx.finish_block(Terminator::Jump(merge_block)); diff --git a/crates/cljrs-runtime/README.md b/crates/cljrs-runtime/README.md index 12ad98a45..7da4e27e6 100644 --- a/crates/cljrs-runtime/README.md +++ b/crates/cljrs-runtime/README.md @@ -132,6 +132,10 @@ tests/ eager IR lowering: the lowerer must decline a `set!` on a local (own binary — it flips the process-wide eager-lowering switch) + destructure_or_default_eager.rs — `:or` destructuring defaults are evaluated + eagerly (`(get m :k default)`), identically + in the tree-walker and the IR tier (own + binary — forced eager lowering; issue #363) ``` --- @@ -953,6 +957,23 @@ Each handler evaluates its key expressions under the correct allocation context: | `handle_alter_var_root` | function return value | | `handle_intern` | value expression (3-arg form) | +### Parameter binding: `bind_fn_params` vs `bind_fn_params_positional` + +```rust +pub fn bind_fn_params(arity: &CljxFnArity, args: &[Value], env: &mut Env) -> EvalResult<()>; +pub fn bind_fn_params_positional(arity: &CljxFnArity, args: &[Value], env: &mut Env) -> EvalResult<()>; +``` + +Both bind an arity's positional params and its rest list into the current frame. +`bind_fn_params` additionally expands the arity's destructuring patterns, and is +what the tree-walker (and `cljrs-async`) calls. `bind_fn_params_positional` +stops short of that, and is what `tiered::apply::execute_ir` calls: the lowered +prologue already binds the destructured names as IR registers, and the ANF +lowerer never emits `LoadLocal`, so the env copy is never read. Producing it +anyway would evaluate each `:or` default twice per call — destructuring defaults +are eager, so a side-effecting one would fire once here and once in the prologue +(issue #363). + ### Value-level special form helpers (IR interpreter API) The IR interpreter receives already-evaluated `Vec` arguments rather than diff --git a/crates/cljrs-runtime/src/interp/apply.rs b/crates/cljrs-runtime/src/interp/apply.rs index ac167388b..716dcd2a9 100644 --- a/crates/cljrs-runtime/src/interp/apply.rs +++ b/crates/cljrs-runtime/src/interp/apply.rs @@ -652,8 +652,36 @@ pub fn call_cljrs_fn(f: &CljxFn, args: &[Value], caller_env: &mut Env) -> EvalRe } } -/// Bind function parameters in the current (top) frame. +/// Bind function parameters in the current (top) frame, expanding any +/// destructuring patterns the arity carries. pub fn bind_fn_params(arity: &CljxFnArity, args: &[Value], env: &mut Env) -> EvalResult<()> { + bind_fn_params_impl(arity, args, env, true) +} + +/// Bind only the *named* parameters — the positional slots and the rest list — +/// leaving the arity's destructuring patterns unexpanded. +/// +/// For the IR tier: the lowered prologue expands those same patterns into +/// explicit IR bindings (`lower_fn_body_destructured`), and the ANF lowerer +/// never emits `LoadLocal`, so a destructured name is only ever read as an IR +/// register — the env copy is unobservable. Producing it anyway would not be +/// free, though: an `:or` default is evaluated eagerly, exactly as +/// `(get m :k default)` evaluates its third argument, so a side-effecting +/// default would fire once here and once in the prologue — twice per call. +pub fn bind_fn_params_positional( + arity: &CljxFnArity, + args: &[Value], + env: &mut Env, +) -> EvalResult<()> { + bind_fn_params_impl(arity, args, env, false) +} + +fn bind_fn_params_impl( + arity: &CljxFnArity, + args: &[Value], + env: &mut Env, + destructure: bool, +) -> EvalResult<()> { let n = arity.params.len(); // Bind positional params. for (i, name) in arity.params.iter().enumerate() { @@ -670,7 +698,7 @@ pub fn bind_fn_params(arity: &CljxFnArity, args: &[Value], env: &mut Env) -> Eva }; env.bind(rest.clone(), rest_val.clone()); // Apply rest destructuring if present. - if let Some(ref pattern) = arity.destructure_rest { + if destructure && let Some(ref pattern) = arity.destructure_rest { // When the rest pattern is a map destructure (e.g. `& {:keys [bar]}`), // convert the rest args list into a map of alternating key-value pairs, // matching Clojure's keyword-arguments convention. @@ -684,9 +712,11 @@ pub fn bind_fn_params(arity: &CljxFnArity, args: &[Value], env: &mut Env) -> Eva } } // Apply positional destructuring patterns. - for (idx, pattern) in &arity.destructure_params { - let val = args.get(*idx).cloned().unwrap_or(Value::Nil); - crate::interp::destructure::bind_pattern(pattern, val, env)?; + if destructure { + for (idx, pattern) in &arity.destructure_params { + let val = args.get(*idx).cloned().unwrap_or(Value::Nil); + crate::interp::destructure::bind_pattern(pattern, val, env)?; + } } Ok(()) } diff --git a/crates/cljrs-runtime/src/tiered/apply.rs b/crates/cljrs-runtime/src/tiered/apply.rs index 1d6a89604..1416a1d3f 100644 --- a/crates/cljrs-runtime/src/tiered/apply.rs +++ b/crates/cljrs-runtime/src/tiered/apply.rs @@ -273,8 +273,12 @@ fn execute_ir( env.bind(name.clone(), self_val); } - // Bind params (including destructuring) into the env so LoadLocal can find them. - crate::interp::apply::bind_fn_params(arity, args, &mut env)?; + // Bind the named params into the env so LoadLocal can find them. The + // destructuring patterns are deliberately *not* expanded here: the lowered + // prologue binds those names as IR registers, and re-binding them in the + // env would evaluate every `:or` default a second time (destructuring + // defaults are eager, so a side-effecting one would fire twice per call). + crate::interp::apply::bind_fn_params_positional(arity, args, &mut env)?; // Push eval context so IR closures (which use with_eval_context) can // call back into the interpreter. diff --git a/crates/cljrs-runtime/tests/destructure_or_default_eager.rs b/crates/cljrs-runtime/tests/destructure_or_default_eager.rs new file mode 100644 index 000000000..e09b7d648 --- /dev/null +++ b/crates/cljrs-runtime/tests/destructure_or_default_eager.rs @@ -0,0 +1,155 @@ +//! Issue #363 — an `:or` destructuring default is evaluated *eagerly*, in both +//! tiers. +//! +//! `destructure` expands a symbol carrying an `:or` entry to +//! `(get m :k default)`. `get` is an ordinary function, so its third argument +//! is evaluated whether or not the key is present: on the JVM a side-effecting +//! default fires on every call, even one that supplies the key. +//! +//! The tree-walker does that (`interp/destructure.rs`); the IR lowerer used to +//! emit the default into the `then` arm of a nil-check branch, so it fired only +//! on a miss. Since a function tiers up partway through a run — at a point set +//! by a background worker — that made the *number* of times a default fired +//! nondeterministic. These tests run the same programs through both tiers and +//! demand the same answer, so the two cannot drift apart again. +//! +//! This file flips the process-wide eager-lowering switch, so it lives in its +//! own test binary; the tree-walk cases below are unaffected by it, since +//! `ExecutionMode::TreeWalk` never consults the IR path at all. + +use std::sync::Arc; + +use cljrs_reader::Parser; +use cljrs_runtime::env::env::{Env, GlobalEnv}; +use cljrs_value::Value; + +fn make_env(mode: cljrs_runtime::ExecutionMode) -> (Arc, Env) { + // Process-wide, and the reason this test is its own binary. + cljrs_runtime::tiered::force_eager_lowering(); + let globals = cljrs_runtime::Runtime::builder() + .execution_mode(mode) + .build() + .expect("runtime") + .into_globals(); + let env = Env::new(globals.clone(), "user"); + (globals, env) +} + +fn eval_pr(mode: cljrs_runtime::ExecutionMode, src: &str) -> String { + let (_globals, mut env) = make_env(mode); + let mut parser = Parser::new(src.to_string(), "".to_string()); + let forms = parser.parse_all().expect("parse error"); + let mut result = Value::Nil; + for form in forms { + result = cljrs_runtime::interp::eval::eval(&form, &mut env).expect("eval error"); + } + match result { + Value::Str(s) => s.get().as_str().to_string(), + // The type name, not `{:?}`: which type came back instead of a string + // is what the assertion is about, and a `Value` may hold a secret. + other => panic!("expected a string from pr-str, got a {}", other.type_name()), + } +} + +/// Run `src` through the tree-walker and through the IR tier and assert both +/// produce `expected` — the tier split in #363 is exactly a disagreement here. +fn assert_both_tiers(src: &str, expected: &str) { + assert_eq!( + eval_pr(cljrs_runtime::ExecutionMode::TreeWalk, src), + expected, + "tree-walking interpreter" + ); + assert_eq!( + eval_pr(cljrs_runtime::ExecutionMode::TieredNoJit, src), + expected, + "IR tier" + ); +} + +#[test] +fn a_present_key_still_evaluates_its_default() { + // The discriminating case: the key is supplied on every call, so a lazy + // lowering never runs the default and reports 0. + assert_both_tiers( + "(def counter (atom 0)) + (defn f [{:keys [x] :or {x (do (swap! counter inc) 1)}}] x) + (dotimes [_ 200] (f {:x 99})) + (pr-str @counter)", + "200", + ); +} + +#[test] +fn a_present_key_still_wins_over_its_default() { + // Eager evaluation must not change which value is *bound*. + assert_both_tiers( + "(def counter (atom 0)) + (defn f [{:keys [x] :or {x (do (swap! counter inc) 1)}}] x) + (pr-str (mapv f (repeat 200 {:x 99})))", + &format!("[{}]", vec!["99"; 200].join(" ")), + ); +} + +#[test] +fn a_missing_key_binds_the_default() { + assert_both_tiers( + "(defn f [{:keys [x] :or {x 7}}] x) + (pr-str (mapv f (repeat 200 {})))", + &format!("[{}]", vec!["7"; 200].join(" ")), + ); +} + +#[test] +fn a_default_evaluates_once_per_call_not_once_per_lowering() { + // A miss and a hit interleaved: 200 calls, 200 evaluations either way. + assert_both_tiers( + "(def counter (atom 0)) + (defn f [{:keys [x] :or {x (do (swap! counter inc) 1)}}] x) + (dotimes [i 200] (f (if (even? i) {:x 99} {}))) + (pr-str @counter)", + "200", + ); +} + +#[test] +fn strs_and_syms_defaults_are_eager_too() { + // `:strs` and `:syms` share `lower_with_default`; pin them alongside. + assert_both_tiers( + "(def counter (atom 0)) + (defn f [{:strs [x] :or {x (do (swap! counter inc) 1)}}] x) + (dotimes [_ 200] (f {\"x\" 99})) + (pr-str @counter)", + "200", + ); + assert_both_tiers( + "(def counter (atom 0)) + (defn f [{:syms [x] :or {x (do (swap! counter inc) 1)}}] x) + (dotimes [_ 200] (f {'x 99})) + (pr-str @counter)", + "200", + ); +} + +#[test] +fn an_explicit_binding_pairs_default_is_eager_too() { + // `{a :x}` rather than `:keys` — the other `apply_default_if_nil` site. + assert_both_tiers( + "(def counter (atom 0)) + (defn f [{a :x :or {a (do (swap! counter inc) 1)}}] a) + (dotimes [_ 200] (f {:x 99})) + (pr-str @counter)", + "200", + ); +} + +#[test] +fn a_let_destructuring_default_is_eager_too() { + // The same lowering serves `let*`, reached here through a hot function. + assert_both_tiers( + "(def counter (atom 0)) + (defn f [m] (let [{:keys [x] :or {x (do (swap! counter inc) 1)}} m] x)) + (dotimes [_ 200] (f {:x 99})) + (pr-str @counter)", + "200", + ); +} diff --git a/crates/cljrs/tests/execution_tier_parity.rs b/crates/cljrs/tests/execution_tier_parity.rs index 0f1e31899..6a39c02dd 100644 --- a/crates/cljrs/tests/execution_tier_parity.rs +++ b/crates/cljrs/tests/execution_tier_parity.rs @@ -25,6 +25,13 @@ const PROGRAM: &str = r#" (defn parity-add [a b] (+ a b)) (defn parity-sub [a b] (- a b)) (defn parity-mul [a b] (* a b)) +(def parity-or-default-count (atom 0)) +(defn parity-or-default + ;; An `:or` default is an ordinary argument of `(get m :k default)`, so it is + ;; evaluated on every call — including calls that supply the key. The atom + ;; counts how many times it actually ran (issue #363). + [{:keys [x] :or {x (do (swap! parity-or-default-count inc) 1)}}] + x) (def parity-input [1 2 3 4]) (def parity-literal-sink nil) (def parity-seq-sink nil) @@ -37,6 +44,8 @@ const PROGRAM: &str = r#" (parity-add 1 2) (parity-sub 2 1) (parity-mul 2 3) + ;; 50 calls that all supply `:x`; each still evaluates the default. + (parity-or-default {:x 7}) ;; Keeping allocation-heavy results reachable also prevents AOT's region ;; optimizer from turning the JIT warm-up into an allocation stress test. (def parity-literal-sink (parity-literals)) @@ -57,6 +66,10 @@ const PROGRAM: &str = r#" (pr-str (parity-sub 9223372036854775807 -1)))) (println (str "overflow-mul|value|" (pr-str (parity-mul 9223372036854775807 2)))) +;; Hit and miss, in that order, so the effect count below is 50 + 2 either way. +(println (str "or-default-hit|value|" (pr-str (parity-or-default {:x 7})))) +(println (str "or-default-miss|value|" (pr-str (parity-or-default {})))) +(println (str "or-default-effects|value|" (pr-str @parity-or-default-count))) (println (str "arithmetic-error|" (try @@ -228,7 +241,7 @@ fn parse_records(tier: Tier, output: &Output) -> BTreeMap { tier.name() ); } - assert_eq!(records.len(), 10, "{} stdout:\n{stdout}", tier.name()); + assert_eq!(records.len(), 13, "{} stdout:\n{stdout}", tier.name()); records } @@ -255,6 +268,11 @@ fn values_and_errors_match_across_all_execution_tiers() { Outcome::Value("[#\"[a-z]+\" 1/3 1.25M]".to_string()) ); assert_eq!(tree["recursive-aot"], Outcome::Value("55".to_string())); + // The key is present, so the default loses — but it still ran, once per + // call: 50 warm-up calls plus the hit and the miss below. + assert_eq!(tree["or-default-hit"], Outcome::Value("7".to_string())); + assert_eq!(tree["or-default-miss"], Outcome::Value("1".to_string())); + assert_eq!(tree["or-default-effects"], Outcome::Value("52".to_string())); assert_eq!(tree["seq-loop"], Outcome::Value("[1 2 3 4]".to_string())); assert_eq!( tree["overflow-add"],