assorted: update nickel dep, set initial-env instead of writing files to disk - #218
Conversation
📝 WalkthroughWalkthroughThis PR updates the ChangesDependency and value construction APIs
Environment-based configuration injection
Program builder adoption in check modules
Sequence Diagram(s)sequenceDiagram
participant Loader
participant ProgramBuilder
participant NickelEnv
participant ConfigNCL
Loader->>Loader: build_injected_config(target, args)
activate Loader
Loader->>NickelEnv: create record with target.os/arch, args
deactivate Loader
Loader->>ProgramBuilder: ProgramBuilder::new()
Loader->>ProgramBuilder: extend_initial_env(__minimal_injected_config)
Loader->>ProgramBuilder: add_source(import "config.ncl")
ProgramBuilder->>NickelEnv: inject INJECTED_CONFIG_VAR=record
ConfigNCL->>NickelEnv: reference __minimal_injected_config directly
ConfigNCL->>ConfigNCL: use target.os, target.arch, args from environment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/decode/src/load.rs (1)
235-238: 💤 Low valuePath information is discarded in error mapping.
When mapping
BuilderError::Io { path, error }, thepathis ignored. This loses valuable context for debugging which file caused the IO error.♻️ Suggested improvement
Consider preserving the path in the error message or modifying
Error::IOto carry path context:.map_err(|e| match e { BuilderError::NoInputs => unreachable!(), - BuilderError::Io { path: _, error } => Error::IO(error), + BuilderError::Io { path, error } => { + tracing::debug!("IO error loading path: {:?}", path); + Error::IO(error) + } })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/decode/src/load.rs` around lines 235 - 238, The mapping of BuilderError::Io drops the path, losing context; update the error mapping in the closure that handles BuilderError (the match arm for BuilderError::Io { path, error }) to preserve the path by either populating a new or existing Error variant with the path (e.g., extend Error::IO to include path or create Error::IOWithPath) or by wrapping the original error with context that includes the path string; ensure changes reference BuilderError::Io and Error::IO so the Io branch forwards both path and error information instead of discarding path.crates/common/src/ncl_eval.rs (1)
23-29: ⚡ Quick winConsider handling potential
build()errors instead ofunwrap().While
BuilderError::NoInputsis unreachable since you always calladd_source_string, theBuilderError::Iovariant could still occur. Propagating the error would be more robust than panicking.♻️ Suggested approach
let mut program: Program<CacheImpl> = ProgramBuilder::new() .add_source_string(source, "toplevel") .extend_initial_env(self.vars.clone()) .with_reporter(NullReporter {}) .with_trace(std::io::stderr()) .build() - .unwrap(); + .map_err(|e| { + let files = Files::default(); + Box::new((files, Error::from(e))) + })?;This would require adjusting the error conversion, or you could document why
unwrap()is acceptable here if IO errors are truly impossible in this context (since we're using in-memory source strings).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/common/src/ncl_eval.rs` around lines 23 - 29, The current call to ProgramBuilder::build().unwrap() can panic on BuilderError::Io; replace the unwrap by propagating the error: change the surrounding function in ncl_eval.rs to return a Result<Program<CacheImpl>, E> (or your crate's error type), call ProgramBuilder::new()... .build()? (or .map_err(|e| convert_builder_error(e))?) to convert BuilderError::Io into your error type and return it, updating any callers accordingly; alternatively, if you prefer to retain the current signature, explicitly handle BuilderError::Io with a clear error conversion and a descriptive error return instead of unwrap. Ensure the conversion references ProgramBuilder, build, and BuilderError::Io so the code compiles and no panic occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/args/src/lib.rs`:
- Around line 165-167: The code is calling Number::try_from(...).unwrap() in the
ScalarArg::Number arm which will panic for non-finite f64 (NaN/inf); update
PrimitiveSpec::parse to validate floats are finite before attempting conversion
and reject non-finite inputs (return an Err or parse failure) and remove the
unwrap in the ScalarArg::Number handling so conversion errors are propagated
instead of panicking—specifically check f.is_finite() in PrimitiveSpec::parse,
return a clear parse error for non-finite values, and replace unwrap() in the
ScalarArg::Number -> NickelValue::number_posless(Number::try_from(...)) path
with proper error handling that forwards Number::try_from failures.
In `@crates/check/src/stack.rs`:
- Around line 26-30: The code selects an entry file into the variable entry_file
by checking for "harness.ncl" and falling back to "stack.ncl", but later
diagnostics still mention "stack.ncl"; update the failure/diagnostic message to
reference the actually selected entry file instead of hardcoding "stack.ncl".
Concretely, compute the chosen file name from entry_file (e.g.,
entry_file.file_name() or a small string like chosen = if harness exists {
"harness.ncl" } else { "stack.ncl" }) and use that chosen name in the
diagnostic/emission site (the code that reports the missing/invalid stack) so
messages align with the selected entry file; adjust any surrounding test strings
or formatters that currently hardcode "stack.ncl".
---
Nitpick comments:
In `@crates/common/src/ncl_eval.rs`:
- Around line 23-29: The current call to ProgramBuilder::build().unwrap() can
panic on BuilderError::Io; replace the unwrap by propagating the error: change
the surrounding function in ncl_eval.rs to return a Result<Program<CacheImpl>,
E> (or your crate's error type), call ProgramBuilder::new()... .build()? (or
.map_err(|e| convert_builder_error(e))?) to convert BuilderError::Io into your
error type and return it, updating any callers accordingly; alternatively, if
you prefer to retain the current signature, explicitly handle BuilderError::Io
with a clear error conversion and a descriptive error return instead of unwrap.
Ensure the conversion references ProgramBuilder, build, and BuilderError::Io so
the code compiles and no panic occurs.
In `@crates/decode/src/load.rs`:
- Around line 235-238: The mapping of BuilderError::Io drops the path, losing
context; update the error mapping in the closure that handles BuilderError (the
match arm for BuilderError::Io { path, error }) to preserve the path by either
populating a new or existing Error variant with the path (e.g., extend Error::IO
to include path or create Error::IOWithPath) or by wrapping the original error
with context that includes the path string; ensure changes reference
BuilderError::Io and Error::IO so the Io branch forwards both path and error
information instead of discarding path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31ad8f8e-e532-497c-9de6-f813fac1ac57
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcrates/args/Cargo.tomlcrates/args/src/lib.rscrates/check/src/lib.rscrates/check/src/profile.rscrates/check/src/stack.rscrates/common/src/ncl_eval.rscrates/common/src/target.rscrates/decode/src/load.rscrates/stdlib/minimal-ncl/config.ncl
| ScalarArg::Number(f) => { | ||
| NickelValue::number_posless(nickel_lang_core::term::Number::try_from(*f).unwrap()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does nickel-lang-core Number::try_from fail for NaN and Infinity f64 values?
💡 Result:
No definitive evidence found in the searched sources that directly answers whether nickel-lang-core’s Number::try_from fails (returns Err) specifically for f64 NaN and ±Infinity. What I can say from the evidence gathered: 1) The available nickel-lang-core documentation pages in the search results don’t expose the implementation or behavior of Number::try_from for f64 NaN/Infinity (the rustdoc snippets shown are general crate docs, not the conversion implementation). 2) The search turned up Nickel issues/PRs about YAML/serialization panics and large numbers, but nothing in those results discusses NaN/Infinity handling for Number::try_from. 3) The only authoritative “NaN/Infinity” behavior I found in the evidence is about Rust f64 itself (NaN/Infinity are representable) and about Malachite float conversions, neither of which is directly tied to nickel-lang-core’s Number::try_from. If you want, I can do one more targeted search by opening the exact source file that defines nickel-lang-core::term::Number::try_from (or searching within the nickel-lang-core crate sources for “try_from” and “Number::try_from”), then verify the NaN/Infinity branches precisely.
Citations:
- 1: https://docs.rs/nickel-lang-core/latest/nickel_lang_core/
- 2: Fix panic when deserializing a too-large yaml number nickel-lang/nickel#2485
- 3: Exporting to yaml does not consistently quote strings that can be interpreted as scientific notation nickel-lang/nickel#2481
- 4: https://docs.rs/malachite-float/latest/src/malachite_float/conversion/primitive_float_from_float.rs.html
- 5: https://docs.rs/nickel-lang/latest/nickel_lang/all.html
🏁 Script executed:
# Find Cargo.toml to understand dependencies
fd -t f "Cargo.toml" | head -20Repository: gominimal/minimal
Length of output: 556
🏁 Script executed:
# Look at the structure of the crates directory
ls -la crates/Repository: gominimal/minimal
Length of output: 1245
🏁 Script executed:
# Check the specific crate's Cargo.toml
cat crates/args/Cargo.tomlRepository: gominimal/minimal
Length of output: 388
🏁 Script executed:
# Check the root Cargo.toml for workspace definition
cat Cargo.toml | head -80Repository: gominimal/minimal
Length of output: 2456
🏁 Script executed:
# Search for nickel-lang-core definition
rg "nickel-lang-core" --type toml | head -20Repository: gominimal/minimal
Length of output: 1312
🏁 Script executed:
# View the file at the exact location mentioned in the review
cat -n crates/args/src/lib.rs | sed -n '150,180p'Repository: gominimal/minimal
Length of output: 1210
🏁 Script executed:
# Get more context around the function
cat -n crates/args/src/lib.rs | sed -n '140,195p'Repository: gominimal/minimal
Length of output: 2194
🏁 Script executed:
# Search for the parsing logic and PrimitiveSpec
rg "PrimitiveSpec" crates/args/src/lib.rs -A 10 -B 2Repository: gominimal/minimal
Length of output: 6864
🏁 Script executed:
# Search for where ScalarArg::Number is created
rg "ScalarArg::Number" crates/args/src/lib.rs -B 5 -A 2Repository: gominimal/minimal
Length of output: 3000
🏁 Script executed:
# Search for any tests in this file that might use NaN or Infinity
rg "NaN|Infinity" crates/args/src/lib.rsRepository: gominimal/minimal
Length of output: 43
🏁 Script executed:
# Check if there are any error handling tests for number conversion
rg "to_nickel" crates/args/src/lib.rs -A 3 -B 3Repository: gominimal/minimal
Length of output: 982
🌐 Web query:
nickel-lang Number::try_from implementation f64 NaN Infinity
💡 Result:
I couldn’t find (in the sources surfaced by the searches) Nickel’s specific Number::try_from implementation that handles f64 inputs like NaN, +Infinity, or -Infinity. What I can support from authoritative Nickel sources is: 1) Nickel’s user-facing number type is arbitrary-precision rationals internally, but during deserialization it goes through 64-bit floating point numbers (“Numbers are deserialized as 64-bit floating point numbers, in line with common JSON implementations”) [1]. 2) Nickel’s stdlib documentation explicitly notes that when operating with pow, if exponent/result fallbacks to float are used, the docs exclude NaN and infinity from that float conversion path [2]. So, the likely behavior of Number::try_from(f64) with NaN/Infinity is somewhere in Nickel’s Rust core conversions (probably involving Malachite’s float/Float conversions), but the exact match arm / error-or-success behavior for NaN and Infinity is not present in the results I retrieved. If you want, paste a link to the Rust file/line where Number::try_from (or the TryFrom<f64> for Number impl) lives in your Nickel version, and I can precisely extract and summarize how it treats NaN and ±Infinity with line-level citations.
Citations:
🏁 Script executed:
# Search for test cases that might exercise to_nickel or provide insight
rg "test" crates/args/src/lib.rs -i | head -30Repository: gominimal/minimal
Length of output: 291
🏁 Script executed:
# Look at the test section of the file
cat -n crates/args/src/lib.rs | tail -200 | head -100Repository: gominimal/minimal
Length of output: 3957
🏁 Script executed:
# Check if there are any clues in Cargo.lock about the exact version/commit of nickel-lang-core
grep -A 10 "name = \"nickel-lang-core\"" Cargo.lock | head -15Repository: gominimal/minimal
Length of output: 371
🏁 Script executed:
# Look for any edge case testing or NaN/Infinity handling in tests
rg "parse.*[Nn]a[Nn]|parse.*[Ii]nf" crates/args/src/lib.rsRepository: gominimal/minimal
Length of output: 43
🏁 Script executed:
# Check what happens when parsing edge case strings
cat -n crates/args/src/lib.rs | sed -n '940,1000p'Repository: gominimal/minimal
Length of output: 2345
🏁 Script executed:
# Test what Rust's f64 parser does with NaN/Infinity strings
python3 << 'EOF'
# Simulate what Rust's f64 parser does
test_strings = ["NaN", "Infinity", "-Infinity", "inf", "-inf", "nan", "1.5", "abc"]
for s in test_strings:
try:
f = float(s)
print(f"{s:20} -> {f:20} (finite: {str(float('nan')).lower() not in str(f).lower() and str(float('inf')).lower() not in str(f).lower()})")
except ValueError as e:
print(f"{s:20} -> ERROR: {e}")
EOFRepository: gominimal/minimal
Length of output: 559
🌐 Web query:
Rust f64 parsing "NaN" "Infinity" parse method
💡 Result:
In Rust, the f64::from_str method—which is invoked when calling str::parse::—fully supports parsing special floating-point values, including "NaN", "Infinity", and their signed variants [1][2]. When parsing a string to f64, the input is treated in a case-insensitive manner [1]. The parser accepts the following variations [1]: - "inf", "infinity", "+inf", "+infinity", "-inf", "-infinity" (all map to the corresponding infinity value) [1][2] - "nan", "+nan", "-nan" (all map to NaN) [1] The parsing logic follows a specific EBNF grammar [1]. If a string does not match the valid floating-point format, the method returns an Err(ParseFloatError) [1][3]. Note that these special values are only supported by the standard from_str implementation for f32 and f64; they are not supported by from_str_radix, which strictly parses numeric characters [2].
Citations:
- 1: https://doc.rust-lang.org/1.62.1/core/primitive.f64.html
- 2: https://www.cs.brandeis.edu/~cs146a/rust/doc-02-21-2015/std/primitive.f64.html
- 3: https://doc.rust-lang.org/std/num/struct.ParseFloatError.html
🌐 Web query:
site:github.com/nickel-lang/nickel Number try_from f64 implementation
💡 Result:
There is no standard library Number type in Rust that implements TryFrom. The TryFrom trait in Rust's standard library is designed for safe, fallible conversions between types. While specific crates or third-party libraries may define their own Number types and implement TryFrom for them, it is not a feature of the Rust standard library itself. In the context of the Nickel configuration language (which is implemented in Rust), developers may work with arbitrary-precision numbers or other custom number representations [1][2], but these are specific to that project and not a general Rust language standard [3]. If you are encountering this in code, it is likely a custom implementation within the specific codebase you are examining [1][3].
Citations:
- 1: Exporting to yaml does not consistently quote strings that can be interpreted as scientific notation nickel-lang/nickel#2481
- 2: Use arbitrary precision numbers instead of floats as the default representation nickel-lang/nickel#1163
- 3: Add std.cast nickel-lang/nickel#2184
🌐 Web query:
nickel-lang-core Number type finite nan infinity constraints
💡 Result:
In Nickel, the Number type is implemented as an arbitrary-precision rational number [1][2]. Because it uses this representation, it inherently does not support IEEE 754 concepts such as NaN (Not-a-Number), positive infinity, or negative infinity [1][2]. Key characteristics of the Nickel Number type include: - Exact Arithmetic: Basic arithmetic operations (addition, subtraction, multiplication, and division) are performed on rationals and do not incur rounding errors [2]. - Conversion Behavior: While Nickel defaults to arbitrary-precision rationals, certain operations—specifically non-integral exponentiation via std.number.pow—may convert operands to 64-bit floating-point numbers to perform the calculation [1][2]. In these specific cases, if the resulting value exceeds the range of a 64-bit float, or if the operation involves values that would typically produce NaN or infinity, the behavior is governed by floating-point arithmetic; however, these resulting values are converted back to rationals, and the language is designed to avoid IEEE 754 artifacts like NaN and infinity in its primary representation [1]. - Constraints: Since Nickel does not natively represent NaN or infinity as Number values, standard programs do not encounter them. If you need to enforce constraints such as finite range, you must define custom contracts [3][4]. For example, you can use a contract to restrict a number to a specific range (e.g., value >= 0 && value <= 65535) [3]. In summary, Nickel's core design prioritizes precision through rationals over IEEE 754 floating-point behavior, effectively eliminating NaN and infinity from the Number type's standard domain [1][2].
Citations:
- 1: https://nickel-lang.org/stdlib/std-number/
- 2: https://nickel-lang.org/user-manual/syntax/
- 3: https://nickel-lang.org/user-manual/contracts/
- 4: https://github.com/tweag/nickel/blob/master/RATIONALE.md
🏁 Script executed:
# Check if there are any existing tests for NaN/Infinity in the parsing
rg "NaN|Infinity|to_nickel" crates/ --type rustRepository: gominimal/minimal
Length of output: 811
unwrap() on Number::try_from will panic for non-finite floats.
Nickel's Number type represents arbitrary-precision rationals and does not support NaN, positive infinity, or negative infinity. The parsing code accepts these values—Rust's f64::parse() successfully parses "NaN", "Infinity", and "-Infinity"—so a user can provide such input, which will then panic when Number::try_from(*f).unwrap() is called. Validate inputs in PrimitiveSpec::parse to reject non-finite numbers:
Proposed fix
PrimitiveSpec::Number => {
let f: f64 = s
.parse()
.map_err(|_| format!("expected a number, got `{s}`"))?;
+ if !f.is_finite() {
+ return Err(format!("expected a finite number, got `{s}`"));
+ }
Ok(ScalarArg::Number(f))
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/args/src/lib.rs` around lines 165 - 167, The code is calling
Number::try_from(...).unwrap() in the ScalarArg::Number arm which will panic for
non-finite f64 (NaN/inf); update PrimitiveSpec::parse to validate floats are
finite before attempting conversion and reject non-finite inputs (return an Err
or parse failure) and remove the unwrap in the ScalarArg::Number handling so
conversion errors are propagated instead of panicking—specifically check
f.is_finite() in PrimitiveSpec::parse, return a clear parse error for non-finite
values, and replace unwrap() in the ScalarArg::Number ->
NickelValue::number_posless(Number::try_from(...)) path with proper error
handling that forwards Number::try_from failures.
| let entry_file = if stacks_dir.join(&stack).join("harness.ncl").exists() { | ||
| stacks_dir.join(&stack).join("harness.ncl") | ||
| } else { | ||
| Program::new_from_source( | ||
| stacks_dir.join(&stack).join("stack.ncl") | ||
| }; |
There was a problem hiding this comment.
Keep stack-name diagnostics aligned with the selected entry file.
Line 26-Line 30 can choose harness.ncl, but the failure message later is still hardcoded to stack.ncl. This can mislead users during triage.
Suggested fix
pub(crate) async fn check_stack(
@@
- let entry_file = if stacks_dir.join(&stack).join("harness.ncl").exists() {
- stacks_dir.join(&stack).join("harness.ncl")
+ let (entry_file, entry_filename) = if stacks_dir.join(&stack).join("harness.ncl").exists() {
+ (stacks_dir.join(&stack).join("harness.ncl"), "harness.ncl")
} else {
- stacks_dir.join(&stack).join("stack.ncl")
+ (stacks_dir.join(&stack).join("stack.ncl"), "stack.ncl")
};
@@
- out.push(check_stack_name(stack.clone(), ctx, &mut program)?);
+ out.push(check_stack_name(stack.clone(), entry_filename, ctx, &mut program)?);-fn check_stack_name(
+fn check_stack_name(
stack: String,
+ entry_filename: &str,
ctx: &CheckCtx,
program: &mut Program<CacheImpl>,
) -> Result<CheckResult, Error> {
@@
return Ok(CheckResult::stack_name_fail(format!(
- "stack defined in {}/stack.ncl has name {}",
+ "stack defined in {}/{} has name {}",
stack,
+ entry_filename,
s.as_str()
)));📝 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.
| let entry_file = if stacks_dir.join(&stack).join("harness.ncl").exists() { | |
| stacks_dir.join(&stack).join("harness.ncl") | |
| } else { | |
| Program::new_from_source( | |
| stacks_dir.join(&stack).join("stack.ncl") | |
| }; | |
| let (entry_file, entry_filename) = if stacks_dir.join(&stack).join("harness.ncl").exists() { | |
| (stacks_dir.join(&stack).join("harness.ncl"), "harness.ncl") | |
| } else { | |
| (stacks_dir.join(&stack).join("stack.ncl"), "stack.ncl") | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/check/src/stack.rs` around lines 26 - 30, The code selects an entry
file into the variable entry_file by checking for "harness.ncl" and falling back
to "stack.ncl", but later diagnostics still mention "stack.ncl"; update the
failure/diagnostic message to reference the actually selected entry file instead
of hardcoding "stack.ncl". Concretely, compute the chosen file name from
entry_file (e.g., entry_file.file_name() or a small string like chosen = if
harness exists { "harness.ncl" } else { "stack.ncl" }) and use that chosen name
in the diagnostic/emission site (the code that reports the missing/invalid
stack) so messages align with the selected entry file; adjust any surrounding
test strings or formatters that currently hardcode "stack.ncl".
Now that nickel-lang/nickel#2611 is merged, we can remove the hack where we wrote injected config to disk so we could access it in nickel.
Also cleans up the string eval path which was accomplishing the same by accumulating nickel source literals as a string.
Summary by CodeRabbit
Refactor
Chores