[noupstream] Refactor EvalState & other types to account for DetNix' thread-safety

[Naxdy]

Since DetNix supports parallel eval, and after discussing with @edolstra , we can represent our thread-safety in Rust, by implementing Send and Sync for most types, as well as refactor EvalState to take in immutable references to itself for most calls.

This allows us to safely use e.g. an Arc<EvalState> across multiple threads for parallel eval.

Summary by CodeRabbit

• Refactor

  • API methods now accept immutable references instead of requiring mutable references, improving ergonomics.
  • One method deprecated in favor of an alternative.

• Improvements

  • Enhanced thread-safety across evaluator and store types, enabling safer multi-threaded usage.

[coderabbitai]

📝 Walkthrough

Walkthrough

Context management refactoring across multiple crates removes persistent context fields from EvalState and Store, replacing them with per-call Context::new() allocation. API signatures update from &mut self to &self. Thread-safety trait implementations (Send/Sync) are added to multiple public types.

Changes

Cohort / File(s)	Summary
EvalState API redesign nix-bindings-expr/src/eval_state.rs	Removed context field from struct; shifted all methods from &mut self to &self; updated value_type_unforced to delegate to value_type and mark as deprecated; added unsafe impl Send and unsafe impl Sync.
Store context removal nix-bindings-store/src/store.rs	Removed persistent context: Context field; all operations now create fresh local Context::new() per method call instead of reusing stored context; updated StoreWeak::upgrade and Clone impl accordingly.
PrimOp FFI integration nix-bindings-expr/src/primop.rs	Updated context handling to construct local Context::new() instances in PrimOp::new and PrimOp::new_value instead of using stored eval_state.context.
Thread-safety trait implementations nix-bindings-expr/src/value.rs, nix-bindings-util/src/context.rs, nix-bindings-store/src/path/mod.rs, nix-bindings-flake/src/lib.rs	Added unsafe impl Send and unsafe impl Sync for Value, Context, StorePath, FlakeSettings, and LockedFlake; updated test to remove unnecessary mut qualifier on eval_state.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 With context now created fresh for each call,
The structs shed their baggage, lighter than all.
Send and Sync march proudly across threads they roam,
Once tangled bindings now find a new home!
✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: refactoring EvalState and other types to support DetNix thread-safety via Send/Sync traits and immutable references.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/multithread-evalstate

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

nix-bindings-expr/src/primop.rs (1)

50-54: ⚠️ Potential issue | 🔴 Critical

Leaked PrimOpContext creates aliased mutable references to EvalState.

PrimOpContext is intentionally leaked (line 68, with TODO comment) and stores a &'a mut EvalState reference. That same reference also lives in the returned PrimOp struct (line 90). If Nix invokes the callback concurrently across threads—especially after thread-safety changes to EvalState—multiple &mut references to the same object violate Rust's aliasing rules and constitute undefined behavior.

The fix requires refactoring the callback state to use a shared, immutable reference (&EvalState) paired with interior mutability, or a shared handle like Arc<EvalState>, so that concurrent access does not produce aliased mutable references.

Also applies to: 68-72, 121-125, 143-143

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nix-bindings-expr/src/primop.rs` around lines 50 - 54, The current
PrimOp::new leaks a PrimOpContext holding a &'a mut EvalState and also stores
that same mutable reference in the returned PrimOp, which can create aliased
&mut EvalState references when callbacks run concurrently; change the callback
state to hold a shared reference instead (e.g., &EvalState with interior
mutability types like RefCell/Mutex/RwLock or use Arc<EvalState> for
cross-thread sharing) and update PrimOpContext, the leaked allocation site (the
TODO leak), and the PrimOp struct fields to use the shared/cloneable handle so
the boxed callback f and any stored context no longer contain &'a mut EvalState
but a shared, thread-safe reference/handle to EvalState.

🧹 Nitpick comments (1)

nix-bindings-util/src/context.rs (1)

8-15: Update the Context docs to match the new allocation model.

The type docs still say EvalState and Store keep a private reusable Context, but this PR moved those call sites to per-operation Context::new(). Leaving the old description here makes the new threading story misleading.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nix-bindings-util/src/context.rs` around lines 8 - 15, Update the Context
documentation to reflect the new per-operation allocation model: replace
references that claim EvalState and Store keep a private reusable Context with a
note that callers now create a fresh Context per operation via Context::new(),
mention that Context is cheap to allocate and safe to reuse across threads
(unsafe impl Send remains), and remove or update the example about storing a
private context in EvalState or Store and the mention of Clone::clone; keep the
guidance about using check_call! for proper usage semantics.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@nix-bindings-expr/src/eval_state.rs`:
- Around line 507-514: The code currently panics by calling unwrap() on the
result of check_call!(raw::get_type(...)) in value_type; instead propagate
backend errors and convert the optional raw type into a Result. Change
value_type (and the call site using check_call! on raw::get_type) to use the ?
operator to propagate check_call! errors, then handle the Option from
ValueType::from_raw without unwrap (e.g., map the None case to an Err with a
meaningful error using ValueType::from_raw(...).ok_or_else(...)). Keep the
initial force(value)? call and return Ok(ValueType) on success.
- Around line 371-382: Gate the unconditional thread-safety impls behind the
DetNix feature: wrap the unsafe impls for EvalStateRef and EvalState
(specifically the blocks declaring "unsafe impl Send for EvalStateRef {}",
"unsafe impl Send for EvalState {}", and "unsafe impl Sync for EvalState {}")
with a cfg feature attribute (e.g. #[cfg(feature = "DetNix")] or your crate's
DetNix feature name) so the Send/Sync guarantees are only compiled when DetNix
is enabled; ensure no other code paths expose these impls unconditionally.

---

Outside diff comments:
In `@nix-bindings-expr/src/primop.rs`:
- Around line 50-54: The current PrimOp::new leaks a PrimOpContext holding a &'a
mut EvalState and also stores that same mutable reference in the returned
PrimOp, which can create aliased &mut EvalState references when callbacks run
concurrently; change the callback state to hold a shared reference instead
(e.g., &EvalState with interior mutability types like RefCell/Mutex/RwLock or
use Arc<EvalState> for cross-thread sharing) and update PrimOpContext, the
leaked allocation site (the TODO leak), and the PrimOp struct fields to use the
shared/cloneable handle so the boxed callback f and any stored context no longer
contain &'a mut EvalState but a shared, thread-safe reference/handle to
EvalState.

---

Nitpick comments:
In `@nix-bindings-util/src/context.rs`:
- Around line 8-15: Update the Context documentation to reflect the new
per-operation allocation model: replace references that claim EvalState and
Store keep a private reusable Context with a note that callers now create a
fresh Context per operation via Context::new(), mention that Context is cheap to
allocate and safe to reuse across threads (unsafe impl Send remains), and remove
or update the example about storing a private context in EvalState or Store and
the mention of Clone::clone; keep the guidance about using check_call! for
proper usage semantics.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8c9e24d8-e1d7-494f-898d-60b433bcdbab

📥 Commits

Reviewing files that changed from the base of the PR and between ce3142c and 15b159b.

📒 Files selected for processing (7)

• nix-bindings-expr/src/eval_state.rs
• nix-bindings-expr/src/primop.rs
• nix-bindings-expr/src/value.rs
• nix-bindings-flake/src/lib.rs
• nix-bindings-store/src/path/mod.rs
• nix-bindings-store/src/store.rs
• nix-bindings-util/src/context.rs