Fix after updating nix#21
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughTests now return boxed ChangesRecoverable Primop Tests
Primop Error Type Disambiguation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 2981-2986: The test labeled "*_in_chain" is supposed to exercise a
chained recoverable error but the code currently returns
Err(Box::new(NixBindingsError::RecoverableError(String::from("transient
failure")))) directly; update the code in eval_state.rs so the RecoverableError
is created with a wrapped source (e.g., use std::error::Error::source or create
a nested error and attach it as the source) instead of returning it directly,
ensuring the chain is exercised (or alternatively, if you prefer to change the
test intent, rename the test and update its comment to reflect that it no longer
checks chaining); locate the case that currently returns
Err(...RecoverableError(...)) near es.new_value_int(42)? and either wrap that
RecoverableError around another error instance or update the test name/comment
accordingly.
🪄 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: 5b22263b-dee0-4726-9321-af2ed3dd53c2
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
nix-bindings-expr/src/eval_state.rsnix-bindings-expr/src/primop.rs
| Err(Box::new(NixBindingsError::RecoverableError(String::from( | ||
| "transient failure", | ||
| )))) | ||
| } else { | ||
| es.new_value_int(42) | ||
| Ok(es.new_value_int(42)?) | ||
| } |
There was a problem hiding this comment.
*_in_chain test no longer exercises chained recoverable errors.
Line 2980 says this case wraps recoverable errors in a chain, but Lines 2981-2983 now return the recoverable error directly. This makes the test behavior effectively duplicate the previous recoverable test.
Consider either restoring an actual wrapped source error here, or renaming/updating the test intent and comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nix-bindings-expr/src/eval_state.rs` around lines 2981 - 2986, The test
labeled "*_in_chain" is supposed to exercise a chained recoverable error but the
code currently returns
Err(Box::new(NixBindingsError::RecoverableError(String::from("transient
failure")))) directly; update the code in eval_state.rs so the RecoverableError
is created with a wrapped source (e.g., use std::error::Error::source or create
a nested error and attach it as the source) instead of returning it directly,
ensuring the chain is exercised (or alternatively, if you prefer to change the
test intent, rename the test and update its comment to reflect that it no longer
checks chaining); locate the case that currently returns
Err(...RecoverableError(...)) near es.new_value_int(42)? and either wrap that
RecoverableError around another error instance or update the test name/comment
accordingly.
There was a problem hiding this comment.
Except we have to do Ok(...?) because of this:
error: Cannot build '/nix/store/h59dqnbiyq4nclkyjz3qa713qlapyiyi-nix-bindings-expr-0.2.1.drv'.
Reason: builder failed with exit code 101.
Output paths:
/nix/store/z42cczb2w72c9gvqf9x6kjnd0d6gddx3-nix-bindings-expr-0.2.1
Last 25 log lines:
> 2943 | ... es.new_value_int(42)
> | ^^^^^^^^^^^^^^^^^^^^ expected `Result<Value, Box<dyn Error>>`, found `Result<Value, EvalStateError>`
> |
> = note: expected enum `std::result::Result<_, Box<(dyn std::error::Error + 'static)>>`
> found enum `std::result::Result<_, eval_state::EvalStateError>`
> help: use `?` to coerce and return an appropriate `Err`, and wrap the resulting value in `Ok` so the expression remains of type `Result`
> |
> 2943 | Ok(es.new_value_int(42)?)
> | +++ ++
>
> error[E0308]: mismatched types
> --> nix-bindings-expr/src/eval_state.rs:2985:29
> |
> 2985 | ... es.new_value_int(42)
> | ^^^^^^^^^^^^^^^^^^^^ expected `Result<Value, Box<dyn Error>>`, found `Result<Value, EvalStateError>`
> |
> = note: expected enum `std::result::Result<_, Box<(dyn std::error::Error + 'static)>>`
> found enum `std::result::Result<_, eval_state::EvalStateError>`
> help: use `?` to coerce and return an appropriate `Err`, and wrap the resulting value in `Ok` so the expression remains of type `Result`
> |
> 2985 | Ok(es.new_value_int(42)?)
> | +++ ++
>
> For more information about this error, try `rustc --explain E0308`.
> error: could not compile `nix-bindings-expr` (lib test) due to 2 previous errors
For full logs, run:
nix log /nix/store/h59dqnbiyq4nclkyjz3qa713qlapyiyi-nix-bindings-expr-0.2.1.drv
There was a problem hiding this comment.
@RossComputerGuy, understood — the Ok(es.new_value_int(42)?) change is clearly a required fix for the type mismatch (Result<Value, EvalStateError> vs Result<Value, Box<dyn Error>>). That part is correct.
My concern about the _in_chain test is actually separate and still stands: the inline comment on Line ~2980 says "Wrap RecoverableError in .context(), pushing it down the chain", but neither the old code nor the new code actually wraps the error in a chain — both return a RecoverableError directly. This makes the _in_chain test functionally equivalent to eval_state_primop_recoverable_error, and the stale comment is misleading.
Would you be open to either:
- Updating just the comment to remove the chain-wrapping claim, or
- Actually wrapping the error so the chain is exercised, e.g.:
// count == 0 branch
Err(Box::new(NixBindingsError::RecoverableError(String::from(
"transient failure",
))) as Box<dyn std::error::Error>)
// or with a genuine wrapper:
{
#[derive(Debug)]
struct Wrapper(Box<dyn std::error::Error + Send + Sync>);
impl std::fmt::Display for Wrapper {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { self.0.fmt(f) }
}
impl std::error::Error for Wrapper {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { Some(&*self.0) }
}
Err(Box::new(Wrapper(Box::new(NixBindingsError::RecoverableError(String::from("transient failure"))))))
}If chaining is the actual intended behavior to test, option 2 ensures the source() traversal in the recoverable-error detection path is exercised. If the test is just about retry-on-recoverable, option 1 (comment fix + possible rename) would suffice.
🧠 Learnings used
Learnt from: RossComputerGuy
Repo: DeterminateSystems/nix-bindings-rust PR: 12
File: nix-bindings-store/src/store.rs:275-287
Timestamp: 2026-02-23T23:55:07.470Z
Learning: The nix-bindings-rust project's minimum supported Nix version is 2.26, particularly for the fetchers. While some functionality may work with earlier versions, they are not tested or officially supported.
There was a problem hiding this comment.
There's no .context() function because that was from upstream using anyhow, this fork does not use anyhow.
There was a problem hiding this comment.
@RossComputerGuy, thanks for the context — that clears it up. The stale comment is a leftover from upstream's anyhow-based chaining approach, which no longer applies here.
Since the _in_chain test can't actually exercise error-source chaining without anyhow (or a hand-rolled wrapper), the simplest clean-up would be to update the test name and remove the misleading comment, since it's effectively just another retry-on-recoverable test at this point. For example:
#[test]
#[cfg(nix_at_least = "2.34.0pre")]
fn eval_state_primop_recoverable_error_retry() { // renamed from _in_chain
gc_registering_current_thread(|| {
let store = Store::open(None, []).unwrap();
let mut es = EvalState::new(store, []).unwrap();
let call_count = std::cell::Cell::new(0u32);
let v = es
.new_value_thunk(
"recoverable_chain_test",
Box::new(move |es: &mut EvalState| {
let count = call_count.get();
call_count.set(count + 1);
if count == 0 {
Err(Box::new(NixBindingsError::RecoverableError(String::from(
"transient failure",
))))
} else {
Ok(es.new_value_int(42)?)
}
}),
)
.unwrap();
// ...
})
.unwrap();
}That said, if you'd prefer to leave the test as-is and just drop the misleading comment, that works too. Happy to defer to your preference.
✏️ Learnings added
Learnt from: RossComputerGuy
Repo: DeterminateSystems/nix-bindings-rust PR: 21
File: nix-bindings-expr/src/eval_state.rs:2981-2986
Timestamp: 2026-04-29T16:25:11.002Z
Learning: The nix-bindings-rust fork (DeterminateSystems/nix-bindings-rust) does not use anyhow. The `.context()` error chaining method is not available; it originated in upstream code that used anyhow. Error handling uses Box<dyn std::error::Error> and custom error types instead.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: RossComputerGuy
Repo: DeterminateSystems/nix-bindings-rust PR: 12
File: nix-bindings-store/src/store.rs:275-287
Timestamp: 2026-02-23T23:55:07.470Z
Learning: The nix-bindings-rust project's minimum supported Nix version is 2.26, particularly for the fetchers. While some functionality may work with earlier versions, they are not tested or officially supported.
2 participants
Summary by CodeRabbit
Bug Fixes
Tests