Sync with upstream #6
Conversation
This builds on the previous commit to make it compatible with the currently locked build configuration.
…y/fix/rust-types fix: rust types
…ict + tests The rename makes it so that we'll have the original name for the more sensible lazy behavior.
Bindings: require_list functions
…y/real-path feat: nix_store::store add real_path function
... and fix an error message
nix-expr: Improve docs
WalkthroughThe change set adds extensive public APIs for Nix expression evaluation in Rust, expands ValueType variants, introduces a Store::real_path method, standardizes FFI char pointer usage to c_char in multiple crates, and adds crate-level docs and lint suppressions for raw C bindings. No breaking changes to existing public signatures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as Caller
participant RS as EvalState (Rust)
participant RAW as raw::EvalState (C)
participant ST as Store
Note over User,RS: Evaluate a Nix expression from a string
User->>RS: eval_from_string(expr, path)
RS->>ST: store() (access underlying store)
RS->>RAW: parse/eval via FFI
RAW-->>RS: Value*
RS-->>User: Value
rect rgba(200,245,200,0.3)
Note right of RS: Optional operations
User->>RS: force(value) / value_type(value)
RS->>RAW: force/type via FFI
RAW-->>RS: WHNF/type
RS-->>User: Result
end
sequenceDiagram
autonumber
participant Caller
participant Store as Store (Rust)
participant RAW as nix_store_real_path (C)
Caller->>Store: real_path(&StorePath)
Store->>RAW: call with result-string callback
RAW-->>Store: real path bytes
Store-->>Caller: String
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/nix-expr/src/eval_state.rs (2)
283-316: Consume the builder in build(self) to prevent reuse and tighten lifetimes.Consuming self avoids accidental reuse and makes the lookup_path lifetime story clearer.
Apply:
- pub fn build(&self) -> Result<EvalState> { + pub fn build(self) -> Result<EvalState> { @@ - let mut lookup_path: Vec<*const c_char> = self + let mut lookup_path: Vec<*const c_char> = self .lookup_path .iter() .map(|s| s.as_ptr()) .chain(std::iter::once(null())) // signal the end of the array .collect();Call sites chaining .build()? continue to work.
801-813: Temporary CString used for PrimOp arg name can dangle. Use a static cstr!.CString::new(...).as_c_str() creates a short‑lived buffer; PrimOp::new immediately takes pointers and passes to FFI. Prefer a static C string here.
- args: [CString::new("internal_unused").unwrap().as_c_str()], + args: [cstr!("internal_unused")],This mirrors the static doc string already used.
🧹 Nitpick comments (4)
rust/nix-expr/src/eval_state.rs (4)
438-441: Avoid unwrap on FFI result; at least provide a diagnostic.get_type “shouldn’t fail”, but unwrap panics obscurely. Keep Option API, but improve panic.
- let r = unsafe { check_call!(raw::get_type(&mut self.context, value.raw_ptr())) }; - // .unwrap(): no reason for this to fail, as it does not evaluate - ValueType::from_raw(r.unwrap()) + let r = unsafe { check_call!(raw::get_type(&mut self.context, value.raw_ptr())) } + .unwrap_or_else(|e| panic!("get_type unexpectedly failed without evaluation: {e}")); + ValueType::from_raw(r)If you’re open to API changes, consider Result<Option> to propagate errors instead.
598-613: Use push() instead of insert(i, ...) for clarity.insert at the end is equivalent to push and avoids index math.
- attrs.insert(i as usize, s.to_owned()); + attrs.push(s.to_owned());
150-156: Prefer a named Context over &mut Context::new() temporaries.Passing &mut of a temporary into FFI is legal but obscures lifetime boundaries.
- check_call!(raw::libexpr_init(&mut Context::new()))?; + let mut ctx = Context::new(); + check_call!(raw::libexpr_init(&mut ctx))?;
915-927: API consistency: call takes owned Values while call_multi borrows.Consider standardizing on borrowing (&Value) for both to avoid unnecessary moves and match new_value_apply signature. Defer if this would ripple widely.
Also applies to: 972-986
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
rust/nix-c-raw/src/lib.rs(1 hunks)rust/nix-expr/src/eval_state.rs(34 hunks)rust/nix-expr/src/value.rs(3 hunks)rust/nix-flake/src/lib.rs(3 hunks)rust/nix-store/src/store.rs(2 hunks)rust/nix-util/src/context.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
rust/nix-store/src/store.rs (3)
rust/nix-util/src/context.rs (1)
ptr(30-32)rust/nix-store/src/path.rs (2)
as_ptr(63-65)name(17-27)rust/nix-util/src/string_return.rs (2)
callback_get_result_string(14-39)callback_get_result_string_data(41-43)
rust/nix-expr/src/eval_state.rs (5)
rust/nix-expr/src/value.rs (3)
new(79-83)raw_ptr(101-103)from_raw(46-64)rust/nix-util/src/context.rs (1)
new(15-25)rust/nix-expr/src/primop.rs (1)
new(38-75)rust/nix-store/src/store.rs (2)
raw_ptr(172-174)open(78-112)rust/nix-store/src/path.rs (1)
as_ptr(63-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (11)
rust/nix-expr/src/value.rs (3)
11-38: LGTM! Excellent documentation additions.The expanded
ValueTypeenum variants and their associated documentation links to the official Nix manual provide clear context for each type. This improves API usability significantly.
40-65: LGTM! Documentation improvements enhance clarity.The updated doc comments for
from_rawclearly explain the return semantics for thunks and unknown types.
67-96: LGTM! Value documentation improvements are clear.The updated documentation provides better context for the
Valuetype and its lifecycle management, including proper references to the Nix manual.rust/nix-flake/src/lib.rs (1)
1-1: LGTM! Consistent FFI type standardization.The changes align with the broader codebase effort to use
c_charfor C string pointers, improving portability and consistency.Also applies to: 90-90, 128-128
rust/nix-c-raw/src/lib.rs (2)
1-9: LGTM! Clear crate-level documentation.The documentation appropriately explains that this crate contains raw C bindings and directs users to the higher-level
nix-storeandnix-exprcrates for normal use.
10-20: LGTM! Well-documented lint suppressions.The lint suppressions are appropriate for generated C bindings and each is clearly documented with the specific reason it's needed. This helps future maintainers understand why these warnings are suppressed.
rust/nix-store/src/store.rs (2)
220-233: LGTM! Well-implemented method following established patterns.The
real_pathmethod correctly follows the same pattern as other store methods likeget_uriandget_storedir, using the result string callback mechanism and proper error handling viacheck_call!.
304-306: LGTM! Appropriate test coverage for new method.The test verifies that
real_pathreturns the expected canonical path string, which is a good basic sanity check for the new functionality.rust/nix-expr/src/eval_state.rs (3)
134-143: Overall: solid API surface, docs, and thorough tests.Great coverage on eval/force/type checks, list ops, attr sets, primops, and GC lifecycle. The structure and ergonomics look good.
Please verify the two FFI contracts noted above and consider the small safety/docs nits.
Also applies to: 170-178, 334-364, 1144-1191
1083-1107: Confirm ownership semantics of BindingsBuilder vs make_attrs. raw::make_attrs may take ownership ofbindings_builder.ptr—verify the C-side contract to avoid a double-free. If it consumes the builder, wrapbindings_builder.ptrinManuallyDropor add a consumed-into_attrs path.
289-303: Ensure C API copies or extend CString lifetimes
In rust/nix-expr/src/eval_state.rs where you call raw::eval_state_builder_set_lookup_path, confirm that the C function duplicates the supplied C-string pointers. If it merely stores the raw pointers, keep the CString storage alive for the full lifetime of the resulting EvalState (e.g. stash them in the builder) or adjust the API to transfer/clone ownership.
| /// Returns a raw pointer to the underlying eval state builder. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The caller must ensure that the pointer is not used beyond the lifetime of this builder. | ||
| // TODO: This function should be marked `unsafe`. | ||
| pub fn raw_ptr(&self) -> *mut raw::eval_state_builder { | ||
| self.eval_state_builder | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
raw_ptr should be unsafe (or non-public) to reflect requirements.
The docs already mention safety requirements. Marking unsafe prevents accidental misuse.
- pub fn raw_ptr(&self) -> *mut raw::eval_state_builder {
+ pub unsafe fn raw_ptr(&self) -> *mut raw::eval_state_builder {
self.eval_state_builder
}Alternatively, reduce visibility to pub(crate).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Returns a raw pointer to the underlying eval state builder. | |
| /// | |
| /// # Safety | |
| /// | |
| /// The caller must ensure that the pointer is not used beyond the lifetime of this builder. | |
| // TODO: This function should be marked `unsafe`. | |
| pub fn raw_ptr(&self) -> *mut raw::eval_state_builder { | |
| self.eval_state_builder | |
| } | |
| /// Returns a raw pointer to the underlying eval state builder. | |
| /// | |
| /// # Safety | |
| /// | |
| /// The caller must ensure that the pointer is not used beyond the lifetime of this builder. | |
| // TODO: This function should be marked `unsafe`. | |
| pub unsafe fn raw_ptr(&self) -> *mut raw::eval_state_builder { | |
| self.eval_state_builder | |
| } |
🤖 Prompt for AI Agents
In rust/nix-expr/src/eval_state.rs around lines 317 to 325, the public method
raw_ptr exposes a raw pointer but is documented as requiring caller safety;
update the signature to reflect that requirement by either making the function
unsafe (pub unsafe fn raw_ptr(&self) -> *mut raw::eval_state_builder) or
reducing visibility to pub(crate) (pub(crate) fn raw_ptr(&self) -> *mut
raw::eval_state_builder) so accidental misuse is prevented; pick the approach
that matches the crate API (prefer unsafe if callers outside the crate must use
it, otherwise reduce visibility).
| /// Creates a new [attribute set][`ValueType::Attrs`] Nix value from an iterator of name-value pairs. | ||
| /// | ||
| /// Accepts any iterator that yields `(String, Value)` pairs and has an exact size. | ||
| /// Common usage includes [`Vec`], [`HashMap`], and array literals. | ||
| /// |
There was a problem hiding this comment.
Doc link uses non-existent ValueType::Attrs; should be AttrSet.
Fix to avoid broken intra-doc links.
- /// Creates a new [attribute set][`ValueType::Attrs`] Nix value from an iterator of name-value pairs.
+ /// Creates a new [attribute set][`ValueType::AttrSet`] Nix value from an iterator of name-value pairs.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Creates a new [attribute set][`ValueType::Attrs`] Nix value from an iterator of name-value pairs. | |
| /// | |
| /// Accepts any iterator that yields `(String, Value)` pairs and has an exact size. | |
| /// Common usage includes [`Vec`], [`HashMap`], and array literals. | |
| /// | |
| /// Creates a new [attribute set][`ValueType::AttrSet`] Nix value from an iterator of name-value pairs. | |
| /// | |
| /// Accepts any iterator that yields `(String, Value)` pairs and has an exact size. | |
| /// Common usage includes [`Vec`], [`HashMap`], and array literals. | |
| /// |
🤖 Prompt for AI Agents
In rust/nix-expr/src/eval_state.rs around lines 1038 to 1042, the intra-doc
reference uses the non-existent variant `ValueType::Attrs`; update the doc link
to the correct enum variant `ValueType::AttrSet` so the generated documentation
link is valid (replace `ValueType::Attrs` with `ValueType::AttrSet` in the
comment).
| @@ -1,5 +1,6 @@ | |||
| use anyhow::{bail, Result}; | |||
| use nix_c_raw as raw; | |||
| use std::os::raw::c_char; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
LGTM! Good FFI type standardization.
Using c_char instead of i8 for C string pointers improves portability, as c_char correctly handles platform differences in char signedness.
Note: Line 147 in the test code still uses *const i8. Consider updating it for consistency:
Also applies to: 54-54
🏁 Script executed:
#!/bin/bash
# Find any remaining i8 casts for string literals that should use c_char
rg -n '\*const i8' rust/nix-util/src/context.rsLength of output: 67
Use c_char in test string cast
Line 147: update as *const i8 → as *const c_char.
🤖 Prompt for AI Agents
In rust/nix-util/src/context.rs around line 147, the test casts a string pointer
using as *const i8 but the module imports c_char; change the cast to as *const
c_char so the test uses the platform-correct char type. Update the pointer casts
in that test to use c_char consistently and ensure any matching pointer types or
signatures in the test use c_char as well.
2 participants
Syncs the integration branch with upstream.
Summary by CodeRabbit
New Features
Documentation
Chores