fix(tiers): evaluate :or destructuring defaults eagerly in every tier

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

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));
 

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<Value>` arguments rather than

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(())
 }

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.

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<GlobalEnv>, 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(), "<test>".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",
+    );
+}

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<String, Outcome> {
             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"],