mirror of
https://github.com/nushell/nushell.git
synced 2025-06-20 18:08:36 +02:00
Better error on spawn failure caused by null bytes (#15911)
# Description When attempting to pass a null byte in a commandline argument, Nu currently fails with: ``` > ^echo (char -i 0) Error: nu:🐚:io::invalid_input × I/O error ╰─▶ × Could not spawn foreground child ╭──── 1 │ crates/nu-command/src/system/run_external.rs:284:17 · ─────────────────────────┬───────────────────────── · ╰── Invalid input parameter ╰──── ``` This does not explain which input parameter is invalid, or why. Since Nu does not typically seem to escape null bytes when printing values containing them, this can make it a bit tricky to track down the problem. After this change, it fails with: ``` > ^echo (char -i 0) Error: nu:🐚:io::invalid_input × I/O error ╰─▶ × Could not spawn foreground child: nul byte found in provided data ╭──── 1 │ crates/nu-command/src/system/run_external.rs:282:17 · ─────────────────────────┬───────────────────────── · ╰── Invalid input parameter ╰──── ``` which is more useful. This could be improved further but this is niche enough that is probably not necessary. This might make some other errors unnecessarily verbose but seems like the better default. I did check that attempting to execute a non-executable file still has a reasonable error: the error message for that failure is not affected by this change. It is still an "internal" error (referencing the Nu code triggering it, not the user's input) because the `call.head` span available to this code is for the command, not its arguments. Using it would result in ``` × I/O error ╰─▶ × Could not spawn foreground child: nul byte found in provided data ╭─[entry #1:1:2] 1 │ ^echo (char -i 0) · ──┬─ · ╰── Invalid input parameter ╰──── ``` which is actively misleading because "echo" does not contain the nul byte. # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> Haven't tried to write a test yet: it's tricky because the better error message comes from the Rust stdlib (so a straightforward integration test checking for the specific message would be brittle)... # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. -->
This commit is contained in:
parent
81e86c40e1
commit
bd3930d00d
@ -275,11 +275,8 @@ impl Command for External {
|
|||||||
);
|
);
|
||||||
|
|
||||||
let mut child = child.map_err(|err| {
|
let mut child = child.map_err(|err| {
|
||||||
IoError::new_internal(
|
let context = format!("Could not spawn foreground child: {err}");
|
||||||
err,
|
IoError::new_internal(err, context, nu_protocol::location!())
|
||||||
"Could not spawn foreground child",
|
|
||||||
nu_protocol::location!(),
|
|
||||||
)
|
|
||||||
})?;
|
})?;
|
||||||
|
|
||||||
if let Some(thread_job) = engine_state.current_thread_job() {
|
if let Some(thread_job) = engine_state.current_thread_job() {
|
||||||
|
Loading…
x
Reference in New Issue
Block a user