From 6c649809d38a687cd63b59ab028b7f30207e5663 Mon Sep 17 00:00:00 2001 From: YizhePKU Date: Thu, 23 May 2024 10:05:27 +0800 Subject: [PATCH] Rewrite run_external.rs (#12921) This PR is a complete rewrite of `run_external.rs`. The main goal of the rewrite is improving readability, but it also fixes some bugs related to argument handling and the PATH variable (fixes https://github.com/nushell/nushell/issues/6011). I'll discuss some technical details to make reviewing easier. ## Argument handling Quoting arguments for external commands is hard. Like, *really* hard. We've had more than a dozen issues and PRs dedicated to quoting arguments (see Appendix) but the current implementation is still buggy. Here's a demonstration of the buggy behavior: ```nu let foo = "'bar'" ^touch $foo # This creates a file named `bar`, but it should be `'bar'` ^touch ...[ "'bar'" ] # Same ``` I'll describe how this PR deals with argument handling. First, we'll introduce the concept of **bare strings**. Bare strings are **string literals** that are either **unquoted** or **quoted by backticks** [^1]. Strings within a list literal are NOT considered bare strings, even if they are unquoted or quoted by backticks. When a bare string is used as an argument to external process, we need to perform tilde-expansion, glob-expansion, and inner-quotes-removal, in that order. "Inner-quotes-removal" means transforming from `--option="value"` into `--option=value`. ## `.bat` files and CMD built-ins On Windows, `.bat` files and `.cmd` files are considered executable, but they need `CMD.exe` as the interpreter. The Rust standard library supports running `.bat` files directly and will spawn `CMD.exe` under the hood (see [documentation](https://doc.rust-lang.org/std/process/index.html#windows-argument-splitting)). However, other extensions are not supported [^2]. Nushell also supports a selected number of CMD built-ins. The problem with CMD is that it uses a different set of quoting rules. Correctly quoting for CMD requires using [Command::raw_arg()](https://doc.rust-lang.org/std/os/windows/process/trait.CommandExt.html#tymethod.raw_arg) and manually quoting CMD special characters, on top of quoting from the Nushell side. ~~I decided that this is too complex and chose to reject special characters in CMD built-ins instead [^3]. Hopefully this will not affact real-world use cases.~~ I've implemented escaping that works reasonably well. ## `which-support` feature The `which` crate is now a hard dependency of `nu-command`, making the `which-support` feature essentially useless. The `which` crate is already a hard dependency of `nu-cli`, and we should consider removing the `which-support` feature entirely. ## Appendix Here's a list of quoting-related issues and PRs in rough chronological order. * https://github.com/nushell/nushell/issues/4609 * https://github.com/nushell/nushell/issues/4631 * https://github.com/nushell/nushell/issues/4601 * https://github.com/nushell/nushell/pull/5846 * https://github.com/nushell/nushell/issues/5978 * https://github.com/nushell/nushell/pull/6014 * https://github.com/nushell/nushell/issues/6154 * https://github.com/nushell/nushell/pull/6161 * https://github.com/nushell/nushell/issues/6399 * https://github.com/nushell/nushell/pull/6420 * https://github.com/nushell/nushell/pull/6426 * https://github.com/nushell/nushell/issues/6465 * https://github.com/nushell/nushell/issues/6559 * https://github.com/nushell/nushell/pull/6560 [^1]: The idea that backtick-quoted strings act like bare strings was introduced by Kubouch and briefly mentioned in [the language reference](https://www.nushell.sh/lang-guide/chapters/strings_and_text.html#backtick-quotes). [^2]: The documentation also said "running .bat scripts in this way may be removed in the future and so should not be relied upon", which is another reason to move away from this. But again, quoting for CMD is hard. [^3]: If anyone wants to try, the best resource I found on the topic is [this](https://daviddeley.com/autohotkey/parameters/parameters.htm). --- Cargo.lock | 1 + crates/nu-command/Cargo.toml | 5 +- .../nu-command/src/env/config/config_env.rs | 75 +- crates/nu-command/src/env/config/config_nu.rs | 75 +- crates/nu-command/src/env/config/mod.rs | 1 - crates/nu-command/src/env/config/utils.rs | 36 - crates/nu-command/src/system/exec.rs | 120 +- crates/nu-command/src/system/mod.rs | 2 +- crates/nu-command/src/system/run_external.rs | 1217 ++++++++--------- crates/nu-command/tests/commands/all.rs | 4 +- crates/nu-command/tests/commands/any.rs | 4 +- crates/nu-command/tests/commands/complete.rs | 2 +- .../nu-command/tests/commands/source_env.rs | 8 +- crates/nu-command/tests/commands/use_.rs | 10 +- src/test_bins.rs | 1 + tests/repl/test_known_external.rs | 12 +- tests/repl/test_parser.rs | 4 +- tests/repl/test_spread.rs | 7 +- tests/shell/pipeline/commands/external.rs | 6 +- tests/shell/pipeline/commands/internal.rs | 11 +- 20 files changed, 803 insertions(+), 798 deletions(-) delete mode 100644 crates/nu-command/src/env/config/utils.rs diff --git a/Cargo.lock b/Cargo.lock index 362d64ba99..e074e772a3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3012,6 +3012,7 @@ dependencies = [ "sha2", "sysinfo", "tabled", + "tempfile", "terminal_size", "titlecase", "toml 0.8.12", diff --git a/crates/nu-command/Cargo.toml b/crates/nu-command/Cargo.toml index 6010e16285..d3456764fd 100644 --- a/crates/nu-command/Cargo.toml +++ b/crates/nu-command/Cargo.toml @@ -99,7 +99,7 @@ uu_whoami = { workspace = true } uuid = { workspace = true, features = ["v4"] } v_htmlescape = { workspace = true } wax = { workspace = true } -which = { workspace = true, optional = true } +which = { workspace = true } unicode-width = { workspace = true } [target.'cfg(windows)'.dependencies] @@ -134,7 +134,7 @@ workspace = true plugin = ["nu-parser/plugin"] sqlite = ["rusqlite"] trash-support = ["trash"] -which-support = ["which"] +which-support = [] [dev-dependencies] nu-cmd-lang = { path = "../nu-cmd-lang", version = "0.93.1" } @@ -146,3 +146,4 @@ quickcheck = { workspace = true } quickcheck_macros = { workspace = true } rstest = { workspace = true, default-features = false } pretty_assertions = { workspace = true } +tempfile = { workspace = true } diff --git a/crates/nu-command/src/env/config/config_env.rs b/crates/nu-command/src/env/config/config_env.rs index 3c8dff487e..32bd7bba90 100644 --- a/crates/nu-command/src/env/config/config_env.rs +++ b/crates/nu-command/src/env/config/config_env.rs @@ -1,6 +1,7 @@ -use super::utils::gen_command; use nu_cmd_base::util::get_editor; use nu_engine::{command_prelude::*, env_to_strings}; +use nu_protocol::{process::ChildProcess, ByteStream}; +use nu_system::ForegroundChild; #[derive(Clone)] pub struct ConfigEnv; @@ -47,7 +48,7 @@ impl Command for ConfigEnv { engine_state: &EngineState, stack: &mut Stack, call: &Call, - input: PipelineData, + _input: PipelineData, ) -> Result { // `--default` flag handling if call.has_flag(engine_state, stack, "default")? { @@ -55,27 +56,59 @@ impl Command for ConfigEnv { return Ok(Value::string(nu_utils::get_default_env(), head).into_pipeline_data()); } - let env_vars_str = env_to_strings(engine_state, stack)?; - let nu_config = match engine_state.get_config_path("env-path") { - Some(path) => path, - None => { - return Err(ShellError::GenericError { - error: "Could not find $nu.env-path".into(), - msg: "Could not find $nu.env-path".into(), - span: None, - help: None, - inner: vec![], - }); - } + // Find the editor executable. + let (editor_name, editor_args) = get_editor(engine_state, stack, call.head)?; + let paths = nu_engine::env::path_str(engine_state, stack, call.head)?; + let cwd = engine_state.cwd(Some(stack))?; + let editor_executable = + crate::which(&editor_name, &paths, &cwd).ok_or(ShellError::ExternalCommand { + label: format!("`{editor_name}` not found"), + help: "Failed to find the editor executable".into(), + span: call.head, + })?; + + let Some(env_path) = engine_state.get_config_path("env-path") else { + return Err(ShellError::GenericError { + error: "Could not find $nu.env-path".into(), + msg: "Could not find $nu.env-path".into(), + span: None, + help: None, + inner: vec![], + }); }; + let env_path = env_path.to_string_lossy().to_string(); - let (item, config_args) = get_editor(engine_state, stack, call.head)?; + // Create the command. + let mut command = std::process::Command::new(editor_executable); - gen_command(call.head, nu_config, item, config_args, env_vars_str).run_with_input( - engine_state, - stack, - input, - true, - ) + // Configure PWD. + command.current_dir(cwd); + + // Configure environment variables. + let envs = env_to_strings(engine_state, stack)?; + command.env_clear(); + command.envs(envs); + + // Configure args. + command.arg(env_path); + command.args(editor_args); + + // Spawn the child process. On Unix, also put the child process to + // foreground if we're in an interactive session. + #[cfg(windows)] + let child = ForegroundChild::spawn(command)?; + #[cfg(unix)] + let child = ForegroundChild::spawn( + command, + engine_state.is_interactive, + &engine_state.pipeline_externals_state, + )?; + + // Wrap the output into a `PipelineData::ByteStream`. + let child = ChildProcess::new(child, None, false, call.head)?; + Ok(PipelineData::ByteStream( + ByteStream::child(child, call.head), + None, + )) } } diff --git a/crates/nu-command/src/env/config/config_nu.rs b/crates/nu-command/src/env/config/config_nu.rs index c43d8d0726..08695ca5bb 100644 --- a/crates/nu-command/src/env/config/config_nu.rs +++ b/crates/nu-command/src/env/config/config_nu.rs @@ -1,6 +1,7 @@ -use super::utils::gen_command; use nu_cmd_base::util::get_editor; use nu_engine::{command_prelude::*, env_to_strings}; +use nu_protocol::{process::ChildProcess, ByteStream}; +use nu_system::ForegroundChild; #[derive(Clone)] pub struct ConfigNu; @@ -51,7 +52,7 @@ impl Command for ConfigNu { engine_state: &EngineState, stack: &mut Stack, call: &Call, - input: PipelineData, + _input: PipelineData, ) -> Result { // `--default` flag handling if call.has_flag(engine_state, stack, "default")? { @@ -59,27 +60,59 @@ impl Command for ConfigNu { return Ok(Value::string(nu_utils::get_default_config(), head).into_pipeline_data()); } - let env_vars_str = env_to_strings(engine_state, stack)?; - let nu_config = match engine_state.get_config_path("config-path") { - Some(path) => path, - None => { - return Err(ShellError::GenericError { - error: "Could not find $nu.config-path".into(), - msg: "Could not find $nu.config-path".into(), - span: None, - help: None, - inner: vec![], - }); - } + // Find the editor executable. + let (editor_name, editor_args) = get_editor(engine_state, stack, call.head)?; + let paths = nu_engine::env::path_str(engine_state, stack, call.head)?; + let cwd = engine_state.cwd(Some(stack))?; + let editor_executable = + crate::which(&editor_name, &paths, &cwd).ok_or(ShellError::ExternalCommand { + label: format!("`{editor_name}` not found"), + help: "Failed to find the editor executable".into(), + span: call.head, + })?; + + let Some(config_path) = engine_state.get_config_path("config-path") else { + return Err(ShellError::GenericError { + error: "Could not find $nu.config-path".into(), + msg: "Could not find $nu.config-path".into(), + span: None, + help: None, + inner: vec![], + }); }; + let config_path = config_path.to_string_lossy().to_string(); - let (item, config_args) = get_editor(engine_state, stack, call.head)?; + // Create the command. + let mut command = std::process::Command::new(editor_executable); - gen_command(call.head, nu_config, item, config_args, env_vars_str).run_with_input( - engine_state, - stack, - input, - true, - ) + // Configure PWD. + command.current_dir(cwd); + + // Configure environment variables. + let envs = env_to_strings(engine_state, stack)?; + command.env_clear(); + command.envs(envs); + + // Configure args. + command.arg(config_path); + command.args(editor_args); + + // Spawn the child process. On Unix, also put the child process to + // foreground if we're in an interactive session. + #[cfg(windows)] + let child = ForegroundChild::spawn(command)?; + #[cfg(unix)] + let child = ForegroundChild::spawn( + command, + engine_state.is_interactive, + &engine_state.pipeline_externals_state, + )?; + + // Wrap the output into a `PipelineData::ByteStream`. + let child = ChildProcess::new(child, None, false, call.head)?; + Ok(PipelineData::ByteStream( + ByteStream::child(child, call.head), + None, + )) } } diff --git a/crates/nu-command/src/env/config/mod.rs b/crates/nu-command/src/env/config/mod.rs index 9cd93d2491..fa6a3b4ca3 100644 --- a/crates/nu-command/src/env/config/mod.rs +++ b/crates/nu-command/src/env/config/mod.rs @@ -2,7 +2,6 @@ mod config_; mod config_env; mod config_nu; mod config_reset; -mod utils; pub use config_::ConfigMeta; pub use config_env::ConfigEnv; pub use config_nu::ConfigNu; diff --git a/crates/nu-command/src/env/config/utils.rs b/crates/nu-command/src/env/config/utils.rs deleted file mode 100644 index 4b3ea43483..0000000000 --- a/crates/nu-command/src/env/config/utils.rs +++ /dev/null @@ -1,36 +0,0 @@ -use crate::ExternalCommand; -use nu_protocol::{OutDest, Span, Spanned}; -use std::{collections::HashMap, path::Path}; - -pub(crate) fn gen_command( - span: Span, - config_path: &Path, - item: String, - config_args: Vec, - env_vars_str: HashMap, -) -> ExternalCommand { - let name = Spanned { item, span }; - - let mut args = vec![Spanned { - item: config_path.to_string_lossy().to_string(), - span: Span::unknown(), - }]; - - let number_of_args = config_args.len() + 1; - - for arg in config_args { - args.push(Spanned { - item: arg, - span: Span::unknown(), - }) - } - - ExternalCommand { - name, - args, - arg_keep_raw: vec![false; number_of_args], - out: OutDest::Inherit, - err: OutDest::Inherit, - env_vars: env_vars_str, - } -} diff --git a/crates/nu-command/src/system/exec.rs b/crates/nu-command/src/system/exec.rs index 21c98f121b..35ab9428be 100644 --- a/crates/nu-command/src/system/exec.rs +++ b/crates/nu-command/src/system/exec.rs @@ -1,7 +1,4 @@ -use super::run_external::create_external_command; -#[allow(deprecated)] -use nu_engine::{command_prelude::*, current_dir}; -use nu_protocol::OutDest; +use nu_engine::{command_prelude::*, env_to_strings}; #[derive(Clone)] pub struct Exec; @@ -35,7 +32,66 @@ On Windows based systems, Nushell will wait for the command to finish and then e call: &Call, _input: PipelineData, ) -> Result { - exec(engine_state, stack, call) + let cwd = engine_state.cwd(Some(stack))?; + + // Find the absolute path to the executable. If the command is not + // found, display a helpful error message. + let name: Spanned = call.req(engine_state, stack, 0)?; + let executable = { + let paths = nu_engine::env::path_str(engine_state, stack, call.head)?; + let Some(executable) = crate::which(&name.item, &paths, &cwd) else { + return Err(crate::command_not_found( + &name.item, + call.head, + engine_state, + stack, + )); + }; + executable + }; + + // Create the command. + let mut command = std::process::Command::new(executable); + + // Configure PWD. + command.current_dir(cwd); + + // Configure environment variables. + let envs = env_to_strings(engine_state, stack)?; + command.env_clear(); + command.envs(envs); + + // Configure args. + let args = crate::eval_arguments_from_call(engine_state, stack, call)?; + command.args(args.into_iter().map(|s| s.item)); + + // Execute the child process, replacing/terminating the current process + // depending on platform. + #[cfg(unix)] + { + use std::os::unix::process::CommandExt; + + let err = command.exec(); + Err(ShellError::ExternalCommand { + label: "Failed to exec into new process".into(), + help: err.to_string(), + span: call.head, + }) + } + #[cfg(windows)] + { + let mut child = command.spawn().map_err(|err| ShellError::ExternalCommand { + label: "Failed to exec into new process".into(), + help: err.to_string(), + span: call.head, + })?; + let status = child.wait().map_err(|err| ShellError::ExternalCommand { + label: "Failed to wait for child process".into(), + help: err.to_string(), + span: call.head, + })?; + std::process::exit(status.code().expect("status.code() succeeds on Windows")) + } } fn examples(&self) -> Vec { @@ -53,57 +109,3 @@ On Windows based systems, Nushell will wait for the command to finish and then e ] } } - -fn exec( - engine_state: &EngineState, - stack: &mut Stack, - call: &Call, -) -> Result { - let mut external_command = create_external_command(engine_state, stack, call)?; - external_command.out = OutDest::Inherit; - external_command.err = OutDest::Inherit; - - #[allow(deprecated)] - let cwd = current_dir(engine_state, stack)?; - let mut command = external_command.spawn_simple_command(&cwd.to_string_lossy())?; - command.current_dir(cwd); - command.envs(external_command.env_vars); - - // this either replaces our process and should not return, - // or the exec fails and we get an error back - exec_impl(command, call.head) -} - -#[cfg(unix)] -fn exec_impl(mut command: std::process::Command, span: Span) -> Result { - use std::os::unix::process::CommandExt; - - let error = command.exec(); - - Err(ShellError::GenericError { - error: "Error on exec".into(), - msg: error.to_string(), - span: Some(span), - help: None, - inner: vec![], - }) -} - -#[cfg(windows)] -fn exec_impl(mut command: std::process::Command, span: Span) -> Result { - match command.spawn() { - Ok(mut child) => match child.wait() { - Ok(status) => std::process::exit(status.code().unwrap_or(0)), - Err(e) => Err(ShellError::ExternalCommand { - label: "Error in external command".into(), - help: e.to_string(), - span, - }), - }, - Err(e) => Err(ShellError::ExternalCommand { - label: "Error spawning external command".into(), - help: e.to_string(), - span, - }), - } -} diff --git a/crates/nu-command/src/system/mod.rs b/crates/nu-command/src/system/mod.rs index c0f890232d..a87aac8a07 100644 --- a/crates/nu-command/src/system/mod.rs +++ b/crates/nu-command/src/system/mod.rs @@ -33,7 +33,7 @@ pub use nu_check::NuCheck; pub use ps::Ps; #[cfg(windows)] pub use registry_query::RegistryQuery; -pub use run_external::{External, ExternalCommand}; +pub use run_external::{command_not_found, eval_arguments_from_call, which, External}; pub use sys::*; pub use uname::UName; pub use which_::Which; diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index b37d3a2fcb..873cee19b1 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -1,15 +1,18 @@ use nu_cmd_base::hook::eval_hook; use nu_engine::{command_prelude::*, env_to_strings, get_eval_expression}; -use nu_protocol::{ast::Expr, did_you_mean, process::ChildProcess, ByteStream, NuGlob, OutDest}; +use nu_protocol::{ + ast::{Expr, Expression}, + did_you_mean, + process::ChildProcess, + ByteStream, OutDest, +}; use nu_system::ForegroundChild; use nu_utils::IgnoreCaseExt; -use os_pipe::PipeReader; -use pathdiff::diff_paths; use std::{ - collections::HashMap, + borrow::Cow, io::Write, path::{Path, PathBuf}, - process::{Command as CommandSys, Stdio}, + process::Stdio, sync::Arc, thread, }; @@ -41,8 +44,131 @@ impl Command for External { call: &Call, input: PipelineData, ) -> Result { - let command = create_external_command(engine_state, stack, call)?; - command.run_with_input(engine_state, stack, input, false) + let cwd = engine_state.cwd(Some(stack))?; + + // Find the absolute path to the executable. On Windows, set the + // executable to "cmd.exe" if it's is a CMD internal command. If the + // command is not found, display a helpful error message. + let name: Spanned = call.req(engine_state, stack, 0)?; + let executable = if cfg!(windows) && is_cmd_internal_command(&name.item) { + PathBuf::from("cmd.exe") + } else { + let paths = nu_engine::env::path_str(engine_state, stack, call.head)?; + let Some(executable) = which(&name.item, &paths, &cwd) else { + return Err(command_not_found( + &name.item, + call.head, + engine_state, + stack, + )); + }; + executable + }; + + // Create the command. + let mut command = std::process::Command::new(executable); + + // Configure PWD. + command.current_dir(cwd); + + // Configure environment variables. + let envs = env_to_strings(engine_state, stack)?; + command.env_clear(); + command.envs(envs); + + // Configure args. + let args = eval_arguments_from_call(engine_state, stack, call)?; + #[cfg(windows)] + if is_cmd_internal_command(&name.item) { + use std::os::windows::process::CommandExt; + + // The /D flag disables execution of AutoRun commands from registry. + // The /C flag followed by a command name instructs CMD to execute + // that command and quit. + command.args(["/D", "/C", &name.item]); + for arg in &args { + command.raw_arg(escape_cmd_argument(arg)?.as_ref()); + } + } else { + command.args(args.into_iter().map(|s| s.item)); + } + #[cfg(not(windows))] + command.args(args.into_iter().map(|s| s.item)); + + // Configure stdout and stderr. If both are set to `OutDest::Pipe`, + // we'll setup a pipe that merge two streams into one. + let stdout = stack.stdout(); + let stderr = stack.stderr(); + let merged_stream = if matches!(stdout, OutDest::Pipe) && matches!(stderr, OutDest::Pipe) { + let (reader, writer) = os_pipe::pipe()?; + command.stdout(writer.try_clone()?); + command.stderr(writer); + Some(reader) + } else { + command.stdout(Stdio::try_from(stdout)?); + command.stderr(Stdio::try_from(stderr)?); + None + }; + + // Configure stdin. We'll try connecting input to the child process + // directly. If that's not possible, we'll setup a pipe and spawn a + // thread to copy data into the child process. + let data_to_copy_into_stdin = match input { + PipelineData::ByteStream(stream, metadata) => match stream.into_stdio() { + Ok(stdin) => { + command.stdin(stdin); + None + } + Err(stream) => { + command.stdin(Stdio::piped()); + Some(PipelineData::ByteStream(stream, metadata)) + } + }, + PipelineData::Empty => { + command.stdin(Stdio::inherit()); + None + } + value => { + command.stdin(Stdio::piped()); + Some(value) + } + }; + + // Spawn the child process. On Unix, also put the child process to + // foreground if we're in an interactive session. + #[cfg(windows)] + let mut child = ForegroundChild::spawn(command)?; + #[cfg(unix)] + let mut child = ForegroundChild::spawn( + command, + engine_state.is_interactive, + &engine_state.pipeline_externals_state, + )?; + + // If we need to copy data into the child process, do it now. + if let Some(data) = data_to_copy_into_stdin { + let stdin = child.as_mut().stdin.take().expect("stdin is piped"); + let engine_state = engine_state.clone(); + let stack = stack.clone(); + thread::Builder::new() + .name("external stdin worker".into()) + .spawn(move || { + let _ = write_pipeline_data(engine_state, stack, data, stdin); + }) + .err_span(call.head)?; + } + + // Wrap the output into a `PipelineData::ByteStream`. + let child = ChildProcess::new( + child, + merged_stream, + matches!(stderr, OutDest::Pipe), + call.head, + )?; + Ok(PipelineData::ByteStream( + ByteStream::child(child, call.head), + None, + )) } fn examples(&self) -> Vec { @@ -66,679 +192,514 @@ impl Command for External { } } -/// Creates ExternalCommand from a call -pub fn create_external_command( +/// Removes surrounding quotes from a string. Doesn't remove quotes from raw +/// strings. Returns the original string if it doesn't have matching quotes. +fn remove_quotes(s: &str) -> &str { + let quoted_by_double_quotes = s.len() >= 2 && s.starts_with('"') && s.ends_with('"'); + let quoted_by_single_quotes = s.len() >= 2 && s.starts_with('\'') && s.ends_with('\''); + let quoted_by_backticks = s.len() >= 2 && s.starts_with('`') && s.ends_with('`'); + if quoted_by_double_quotes || quoted_by_single_quotes || quoted_by_backticks { + &s[1..s.len() - 1] + } else { + s + } +} + +/// Evaluate all arguments from a call, performing expansions when necessary. +pub fn eval_arguments_from_call( engine_state: &EngineState, stack: &mut Stack, call: &Call, -) -> Result { - let name: Spanned = call.req(engine_state, stack, 0)?; - - // Translate environment variables from Values to Strings - let env_vars_str = env_to_strings(engine_state, stack)?; - - fn value_as_spanned(value: Value) -> Result, ShellError> { - let span = value.span(); - - value - .coerce_string() - .map(|item| Spanned { item, span }) - .map_err(|_| ShellError::ExternalCommand { - label: format!("Cannot convert {} to a string", value.get_type()), - help: "All arguments to an external command need to be string-compatible".into(), - span, - }) - } - - let eval_expression = get_eval_expression(engine_state); - - let mut spanned_args = vec![]; - let mut arg_keep_raw = vec![]; - for (arg, spread) in call.rest_iter(1) { - match eval_expression(engine_state, stack, arg)? { - Value::List { vals, .. } => { - if spread { - // turn all the strings in the array into params. - // Example: one_arg may be something like ["ls" "-a"] - // convert it to "ls" "-a" - for v in vals { - spanned_args.push(value_as_spanned(v)?); - // for arguments in list, it's always treated as a whole arguments - arg_keep_raw.push(true); - } - } else { - return Err(ShellError::CannotPassListToExternal { - arg: String::from_utf8_lossy(engine_state.get_span_contents(arg.span)) - .into(), - span: arg.span, - }); +) -> Result>, ShellError> { + let cwd = engine_state.cwd(Some(stack))?; + let mut args: Vec> = vec![]; + for (expr, spread) in call.rest_iter(1) { + if is_bare_string(expr) { + // If `expr` is a bare string, perform tilde-expansion, + // glob-expansion, and inner-quotes-removal, in that order. + for arg in eval_argument(engine_state, stack, expr, spread)? { + let tilde_expanded = expand_tilde(&arg); + for glob_expanded in expand_glob(&tilde_expanded, &cwd, expr.span)? { + let inner_quotes_removed = remove_inner_quotes(&glob_expanded); + args.push(inner_quotes_removed.into_owned().into_spanned(expr.span)); } } - val => { - if spread { - return Err(ShellError::CannotSpreadAsList { span: arg.span }); - } else { - spanned_args.push(value_as_spanned(val)?); - match arg.expr { - // refer to `parse_dollar_expr` function - // the expression type of $variable_name, $"($variable_name)" - // will be Expr::StringInterpolation, Expr::FullCellPath - Expr::StringInterpolation(_) | Expr::FullCellPath(_) => { - arg_keep_raw.push(true) - } - _ => arg_keep_raw.push(false), - } - } + } else { + for arg in eval_argument(engine_state, stack, expr, spread)? { + args.push(arg.into_spanned(expr.span)); } } } - - Ok(ExternalCommand { - name, - args: spanned_args, - arg_keep_raw, - out: stack.stdout().clone(), - err: stack.stderr().clone(), - env_vars: env_vars_str, - }) + Ok(args) } -#[derive(Clone)] -pub struct ExternalCommand { - pub name: Spanned, - pub args: Vec>, - pub arg_keep_raw: Vec, - pub out: OutDest, - pub err: OutDest, - pub env_vars: HashMap, -} - -impl ExternalCommand { - pub fn run_with_input( - &self, - engine_state: &EngineState, - stack: &mut Stack, - input: PipelineData, - reconfirm_command_name: bool, - ) -> Result { - let head = self.name.span; - - #[cfg(windows)] - let (child, reader, input) = { - // We may need to run `create_process` again, so we have to clone the underlying - // file or pipe in `input` here first. - let (input_consumed, stdin) = match &input { - PipelineData::ByteStream(stream, ..) => match stream.source() { - nu_protocol::ByteStreamSource::Read(_) => (false, Stdio::piped()), - nu_protocol::ByteStreamSource::File(file) => { - (true, file.try_clone().err_span(head)?.into()) - } - nu_protocol::ByteStreamSource::Child(child) => { - if let Some(nu_protocol::process::ChildPipe::Pipe(pipe)) = &child.stdout { - (true, pipe.try_clone().err_span(head)?.into()) - } else { - (false, Stdio::piped()) - } - } - }, - PipelineData::Empty => (false, Stdio::inherit()), - _ => (false, Stdio::piped()), - }; - - let mut input = input; - let (cmd, mut reader) = self.create_process(stdin, false, head)?; - let child = match ForegroundChild::spawn(cmd) { - Ok(child) => { - if input_consumed { - input = PipelineData::Empty; - } - Ok(child) - } - Err(err) => { - // Running external commands on Windows has 2 points of complication: - // 1. Some common Windows commands are actually built in to cmd.exe, not executables in their own right. - // 2. We need to let users run batch scripts etc. (.bat, .cmd) without typing their extension - - // To support these situations, we have a fallback path that gets run if a command - // fails to be run as a normal executable: - // 1. "shell out" to cmd.exe if the command is a known cmd.exe internal command - // 2. Otherwise, use `which-rs` to look for batch files etc. then run those in cmd.exe - - // set the default value, maybe we'll override it later - let mut child = Err(err); - - // This has the full list of cmd.exe "internal" commands: https://ss64.com/nt/syntax-internal.html - // I (Reilly) went through the full list and whittled it down to ones that are potentially useful: - const CMD_INTERNAL_COMMANDS: [&str; 9] = [ - "ASSOC", "CLS", "ECHO", "FTYPE", "MKLINK", "PAUSE", "START", "VER", "VOL", - ]; - let command_name = &self.name.item; - let looks_like_cmd_internal = CMD_INTERNAL_COMMANDS - .iter() - .any(|&cmd| command_name.eq_ignore_ascii_case(cmd)); - - let (data, stdin) = extract_stdio(input); - input = data; - - if looks_like_cmd_internal { - let (cmd, new_reader) = self.create_process(stdin, true, head)?; - reader = new_reader; - child = ForegroundChild::spawn(cmd); - } else { - #[cfg(feature = "which-support")] - { - // maybe it's a batch file (foo.cmd) and the user typed `foo`. Try to find it with `which-rs` - // TODO: clean this up with an if-let chain once those are stable - if let Ok(path) = - nu_engine::env::path_str(engine_state, stack, self.name.span) - { - if let Some(cwd) = self.env_vars.get("PWD") { - // append cwd to PATH so `which-rs` looks in the cwd too. - // this approximates what cmd.exe does. - let path_with_cwd = format!("{};{}", cwd, path); - if let Ok(which_path) = - which::which_in(&self.name.item, Some(path_with_cwd), cwd) - { - if let Some(file_name) = which_path.file_name() { - if !file_name - .to_string_lossy() - .eq_ignore_case(command_name) - { - // which-rs found an executable file with a slightly different name - // than the one the user tried. Let's try running it - let mut new_command = self.clone(); - new_command.name = Spanned { - item: file_name.to_string_lossy().to_string(), - span: self.name.span, - }; - let (cmd, new_reader) = new_command - .create_process(stdin, true, head)?; - reader = new_reader; - child = ForegroundChild::spawn(cmd); - } - } - } - } - } - } - } - - child - } - }; - - (child, reader, input) - }; - - #[cfg(unix)] - let (child, reader, input) = { - let (input, stdin) = extract_stdio(input); - let (cmd, reader) = self.create_process(stdin, false, head)?; - let child = ForegroundChild::spawn( - cmd, - engine_state.is_interactive, - &engine_state.pipeline_externals_state, - ); - (child, reader, input) - }; - - match child { - Err(err) => { - match err.kind() { - // If file not found, try suggesting alternative commands to the user - std::io::ErrorKind::NotFound => { - // recommend a replacement if the user tried a removed command - let command_name_lower = self.name.item.to_lowercase(); - let removed_from_nu = crate::removed_commands(); - if removed_from_nu.contains_key(&command_name_lower) { - let replacement = match removed_from_nu.get(&command_name_lower) { - Some(s) => s.clone(), - None => "".to_string(), - }; - return Err(ShellError::RemovedCommand { - removed: command_name_lower, - replacement, - span: self.name.span, - }); - } - - let suggestion = suggest_command(&self.name.item, engine_state); - let label = match suggestion { - Some(s) => { - if reconfirm_command_name { - format!( - "'{}' was not found; did you mean '{s}'?", - self.name.item - ) - } else { - let cmd_name = &self.name.item; - let maybe_module = engine_state - .which_module_has_decl(cmd_name.as_bytes(), &[]); - if let Some(module_name) = maybe_module { - let module_name = String::from_utf8_lossy(module_name); - let new_name = &[module_name.as_ref(), cmd_name].join(" "); - - if engine_state - .find_decl(new_name.as_bytes(), &[]) - .is_some() - { - format!("command '{cmd_name}' was not found but it was imported from module '{module_name}'; try using `{new_name}`") - } else { - format!("command '{cmd_name}' was not found but it exists in module '{module_name}'; try importing it with `use`") - } - } else { - // If command and suggestion are the same, display not found - if cmd_name == &s { - format!("'{cmd_name}' was not found") - } else { - format!("did you mean '{s}'?") - } - } - } - } - None => { - if reconfirm_command_name { - format!("executable '{}' was not found", self.name.item) - } else { - "executable was not found".into() - } - } - }; - - let mut err_str = err.to_string(); - if engine_state.is_interactive { - let mut engine_state = engine_state.clone(); - if let Some(hook) = engine_state.config.hooks.command_not_found.clone() - { - let canary = "ENTERED_COMMAND_NOT_FOUND"; - let stack = &mut stack.start_capture(); - if stack.has_env_var(&engine_state, canary) { - return Err(ShellError::ExternalCommand { - label: "command_not_found handler could not be run".into(), - help: "make sure the command_not_found closure itself does not use unknown commands".to_string(), - span: self.name.span, - }); - } - stack.add_env_var( - canary.to_string(), - Value::bool(true, Span::unknown()), - ); - match eval_hook( - &mut engine_state, - stack, - None, - vec![( - "cmd_name".into(), - Value::string(self.name.item.to_string(), self.name.span), - )], - &hook, - "command_not_found", - ) { - Ok(PipelineData::Value(Value::String { val, .. }, ..)) => { - err_str = format!("{}\n{}", err_str, val); - } - - Err(err) => { - stack.remove_env_var(&engine_state, canary); - return Err(err); - } - _ => {} - } - stack.remove_env_var(&engine_state, canary); - } - } - - Err(ShellError::ExternalCommand { - label, - help: err_str, - span: self.name.span, - }) - } - // otherwise, a default error message - _ => Err(ShellError::ExternalCommand { - label: "can't run executable".into(), - help: err.to_string(), - span: self.name.span, - }), - } - } - Ok(mut child) => { - if !input.is_nothing() { - let mut engine_state = engine_state.clone(); - let mut stack = stack.clone(); - - // Turn off color as we pass data through - Arc::make_mut(&mut engine_state.config).use_ansi_coloring = false; - - // Pipe input into the external command's stdin - if let Some(mut stdin_write) = child.as_mut().stdin.take() { - thread::Builder::new() - .name("external stdin worker".to_string()) - .spawn(move || { - let input = match input { - // Don't touch binary input or byte streams - input @ PipelineData::ByteStream(..) => input, - input @ PipelineData::Value(Value::Binary { .. }, ..) => input, - input => { - let stack = &mut stack.start_capture(); - // Attempt to render the input as a table before piping it to the external. - // This is important for pagers like `less`; - // they need to get Nu data rendered for display to users. - // - // TODO: should we do something different for list inputs? - // Users often expect those to be piped to *nix tools as raw strings separated by newlines - crate::Table.run( - &engine_state, - stack, - &Call::new(head), - input, - )? - } - }; - - if let PipelineData::ByteStream(stream, ..) = input { - stream.write_to(&mut stdin_write)?; - } else { - for value in input.into_iter() { - let buf = value.coerce_into_binary()?; - stdin_write.write_all(&buf)?; - } - } - - Ok::<_, ShellError>(()) - }) - .err_span(head)?; - } - } - - let child = - ChildProcess::new(child, reader, matches!(self.err, OutDest::Pipe), head)?; - - Ok(PipelineData::ByteStream( - ByteStream::child(child, head), - None, - )) - } - } +/// Evaluates an expression, coercing the values to strings. +/// +/// Note: The parser currently has a special hack that retains surrounding +/// quotes for string literals in `Expression`, so that we can decide whether +/// the expression is considered a bare string. The hack doesn't affect string +/// literals within lists or records. This function will remove the quotes +/// before evaluating the expression. +fn eval_argument( + engine_state: &EngineState, + stack: &mut Stack, + expr: &Expression, + spread: bool, +) -> Result, ShellError> { + // Remove quotes from string literals. + let mut expr = expr.clone(); + if let Expr::String(s) = &expr.expr { + expr.expr = Expr::String(remove_quotes(s).into()); } - pub fn create_process( - &self, - stdin: Stdio, - use_cmd: bool, - span: Span, - ) -> Result<(CommandSys, Option), ShellError> { - let mut process = if let Some(d) = self.env_vars.get("PWD") { - let mut process = if use_cmd { - self.spawn_cmd_command(d) + let eval = get_eval_expression(engine_state); + match eval(engine_state, stack, &expr)? { + Value::List { vals, .. } => { + if spread { + vals.into_iter() + .map(|val| val.coerce_into_string()) + .collect() } else { - self.create_command(d)? - }; - - // do not try to set current directory if cwd does not exist - if Path::new(&d).exists() { - process.current_dir(d); + Err(ShellError::CannotPassListToExternal { + arg: String::from_utf8_lossy(engine_state.get_span_contents(expr.span)).into(), + span: expr.span, + }) } - process - } else { - return Err(ShellError::GenericError{ - error: "Current directory not found".into(), - msg: "did not find PWD environment variable".into(), - span: Some(span), - help: Some(concat!( - "The environment variable 'PWD' was not found. ", - "It is required to define the current directory when running an external command." - ).into()), - inner:Vec::new(), - }); - }; - - process.envs(&self.env_vars); - - // If the external is not the last command, its output will get piped - // either as a string or binary - let reader = if matches!(self.out, OutDest::Pipe) && matches!(self.err, OutDest::Pipe) { - let (reader, writer) = os_pipe::pipe()?; - let writer_clone = writer.try_clone()?; - process.stdout(writer); - process.stderr(writer_clone); - Some(reader) - } else { - process.stdout(Stdio::try_from(&self.out)?); - process.stderr(Stdio::try_from(&self.err)?); - None - }; - - process.stdin(stdin); - - Ok((process, reader)) - } - - fn create_command(&self, cwd: &str) -> Result { - // in all the other cases shell out - if cfg!(windows) { - //TODO. This should be modifiable from the config file. - // We could give the option to call from powershell - // for minimal builds cwd is unused - if self.name.item.ends_with(".cmd") || self.name.item.ends_with(".bat") { - Ok(self.spawn_cmd_command(cwd)) + } + value => { + if spread { + Err(ShellError::CannotSpreadAsList { span: expr.span }) } else { - self.spawn_simple_command(cwd) + Ok(vec![value.coerce_into_string()?]) } - } else { - self.spawn_simple_command(cwd) } } - - /// Spawn a command without shelling out to an external shell - /// - /// Note that this function will not set the cwd or environment variables. - /// It only creates the command and adds arguments. - pub fn spawn_simple_command(&self, cwd: &str) -> Result { - let (head, _, _) = trim_enclosing_quotes(&self.name.item); - let head = nu_path::expand_to_real_path(head) - .to_string_lossy() - .to_string(); - - let mut process = std::process::Command::new(head); - process.env_clear(); - - for (arg, arg_keep_raw) in self.args.iter().zip(self.arg_keep_raw.iter()) { - trim_expand_and_apply_arg(&mut process, arg, arg_keep_raw, cwd); - } - - Ok(process) - } - - /// Spawn a cmd command with `cmd /c args...` - pub fn spawn_cmd_command(&self, cwd: &str) -> std::process::Command { - let mut process = std::process::Command::new("cmd"); - - // Disable AutoRun - // TODO: There should be a config option to enable/disable this - // Alternatively (even better) a config option to specify all the arguments to pass to cmd - process.arg("/D"); - - process.arg("/c"); - process.arg(&self.name.item); - for (arg, arg_keep_raw) in self.args.iter().zip(self.arg_keep_raw.iter()) { - // https://stackoverflow.com/questions/1200235/how-to-pass-a-quoted-pipe-character-to-cmd-exe - // cmd.exe needs to have a caret to escape a pipe - let arg = Spanned { - item: arg.item.replace('|', "^|"), - span: arg.span, - }; - - trim_expand_and_apply_arg(&mut process, &arg, arg_keep_raw, cwd) - } - - process - } } -fn trim_expand_and_apply_arg( - process: &mut CommandSys, - arg: &Spanned, - arg_keep_raw: &bool, - cwd: &str, -) { - // if arg is quoted, like "aa", 'aa', `aa`, or: - // if arg is a variable or String interpolation, like: $variable_name, $"($variable_name)" - // `as_a_whole` will be true, so nu won't remove the inner quotes. - let (trimmed_args, mut run_glob_expansion, mut keep_raw) = trim_enclosing_quotes(&arg.item); - if *arg_keep_raw { - keep_raw = true; - // it's a list or a variable, don't run glob expansion either - run_glob_expansion = false; - } - let mut arg = Spanned { - item: if keep_raw { - trimmed_args - } else { - remove_quotes(trimmed_args) - }, - span: arg.span, +/// Returns whether an expression is considered a bare string. +/// +/// Bare strings are defined as string literals that are either unquoted or +/// quoted by backticks. Raw strings or string interpolations don't count. +fn is_bare_string(expr: &Expression) -> bool { + let Expr::String(s) = &expr.expr else { + return false; }; - if !keep_raw { - arg.item = nu_path::expand_tilde(arg.item) - .to_string_lossy() - .to_string(); - } - let cwd = PathBuf::from(cwd); - if arg.item.contains('*') && run_glob_expansion { - // we need to run glob expansion, so it's NeedExpand. - let path = Spanned { - item: NuGlob::Expand(arg.item.clone()), - span: arg.span, - }; - if let Ok((prefix, matches)) = nu_engine::glob_from(&path, &cwd, arg.span, None) { - let matches: Vec<_> = matches.collect(); - - // FIXME: do we want to special-case this further? We might accidentally expand when they don't - // intend to - if matches.is_empty() { - process.arg(&arg.item); - } - for m in matches { - if let Ok(arg) = m { - let arg = if let Some(prefix) = &prefix { - if let Ok(remainder) = arg.strip_prefix(prefix) { - let new_prefix = if let Some(pfx) = diff_paths(prefix, &cwd) { - pfx - } else { - prefix.to_path_buf() - }; - - new_prefix.join(remainder).to_string_lossy().to_string() - } else { - arg.to_string_lossy().to_string() - } - } else { - arg.to_string_lossy().to_string() - }; - - process.arg(&arg); - } else { - process.arg(&arg.item); - } - } - } - } else { - process.arg(&arg.item); - } + let quoted_by_double_quotes = s.len() >= 2 && s.starts_with('"') && s.ends_with('"'); + let quoted_by_single_quotes = s.len() >= 2 && s.starts_with('\'') && s.ends_with('\''); + !quoted_by_double_quotes && !quoted_by_single_quotes } -/// Given an invalid command name, try to suggest an alternative -fn suggest_command(attempted_command: &str, engine_state: &EngineState) -> Option { - let commands = engine_state.get_signatures(false); - let command_folded_case = attempted_command.to_folded_case(); - let search_term_match = commands.iter().find(|sig| { +/// Performs tilde expansion on `arg`. Returns the original string if `arg` +/// doesn't start with tilde. +fn expand_tilde(arg: &str) -> String { + nu_path::expand_tilde(arg).to_string_lossy().to_string() +} + +/// Performs glob expansion on `arg`. If the expansion found no matches, returns +/// the original string as the expansion result. +/// +/// Note: This matches the default behavior of Bash, but is known to be +/// error-prone. We might want to change this behavior in the future. +fn expand_glob(arg: &str, cwd: &Path, span: Span) -> Result, ShellError> { + let paths = + nu_glob::glob_with_parent(arg, nu_glob::MatchOptions::default(), cwd).map_err(|err| { + ShellError::InvalidGlobPattern { + msg: err.msg.to_string(), + span, + } + })?; + + let mut result = vec![]; + for path in paths { + let path = path.map_err(|err| ShellError::IOErrorSpanned { + msg: format!("{}: {:?}", err.path().display(), err.error()), + span, + })?; + // Strip PWD from the resulting paths if possible. + let path_stripped = path.strip_prefix(cwd).unwrap_or(&path); + let path_string = path_stripped.to_string_lossy().to_string(); + result.push(path_string); + } + + if result.is_empty() { + result.push(arg.to_string()); + } + + Ok(result) +} + +/// Transforms `--option="value"` into `--option=value`. `value` can be quoted +/// with double quotes, single quotes, or backticks. Only removes the outermost +/// pair of quotes after the equal sign. +fn remove_inner_quotes(arg: &str) -> Cow<'_, str> { + // Check that `arg` is a long option. + if !arg.starts_with("--") { + return Cow::Borrowed(arg); + } + // Split `arg` on the first `=`. + let Some((option, value)) = arg.split_once('=') else { + return Cow::Borrowed(arg); + }; + // Check that `option` doesn't contain quotes. + if option.contains('"') || option.contains('\'') || option.contains('`') { + return Cow::Borrowed(arg); + } + // Remove the outermost pair of quotes from `value`. + let value = remove_quotes(value); + Cow::Owned(format!("{option}={value}")) +} + +/// Write `PipelineData` into `writer`. If `PipelineData` is not binary, it is +/// first rendered using the `table` command. +/// +/// Note: Avoid using this function when piping data from an external command to +/// another external command, because it copies data unnecessarily. Instead, +/// extract the pipe from the `PipelineData::ByteStream` of the first command +/// and hand it to the second command directly. +fn write_pipeline_data( + mut engine_state: EngineState, + mut stack: Stack, + data: PipelineData, + mut writer: impl Write, +) -> Result<(), ShellError> { + if let PipelineData::ByteStream(stream, ..) = data { + stream.write_to(writer)?; + } else if let PipelineData::Value(Value::Binary { val, .. }, ..) = data { + writer.write_all(&val)?; + } else { + stack.start_capture(); + + // Turn off color as we pass data through + Arc::make_mut(&mut engine_state.config).use_ansi_coloring = false; + + // Invoke the `table` command. + let output = + crate::Table.run(&engine_state, &mut stack, &Call::new(Span::unknown()), data)?; + + // Write the output. + for value in output { + let bytes = value.coerce_into_binary()?; + writer.write_all(&bytes)?; + } + } + Ok(()) +} + +/// Returns a helpful error message given an invalid command name, +pub fn command_not_found( + name: &str, + span: Span, + engine_state: &EngineState, + stack: &mut Stack, +) -> ShellError { + // Run the `command_not_found` hook if there is one. + if let Some(hook) = &engine_state.config.hooks.command_not_found { + let mut stack = stack.start_capture(); + // Set a special environment variable to avoid infinite loops when the + // `command_not_found` hook triggers itself. + let canary = "ENTERED_COMMAND_NOT_FOUND"; + if stack.has_env_var(engine_state, canary) { + return ShellError::ExternalCommand { + label: format!( + "Command {name} not found while running the `command_not_found` hook" + ), + help: "Make sure the `command_not_found` hook itself does not use unknown commands" + .into(), + span, + }; + } + stack.add_env_var(canary.into(), Value::bool(true, Span::unknown())); + + let output = eval_hook( + &mut engine_state.clone(), + &mut stack, + None, + vec![("cmd_name".into(), Value::string(name, span))], + hook, + "command_not_found", + ); + + // Remove the special environment variable that we just set. + stack.remove_env_var(engine_state, canary); + + match output { + Ok(PipelineData::Value(Value::String { val, .. }, ..)) => { + return ShellError::ExternalCommand { + label: format!("Command `{name}` not found"), + help: val, + span, + }; + } + Err(err) => { + return err; + } + _ => { + // The hook did not return a string, so ignore it. + } + } + } + + // If the name is one of the removed commands, recommend a replacement. + if let Some(replacement) = crate::removed_commands().get(&name.to_lowercase()) { + return ShellError::RemovedCommand { + removed: name.to_lowercase(), + replacement: replacement.clone(), + span, + }; + } + + // The command might be from another module. Try to find it. + if let Some(module) = engine_state.which_module_has_decl(name.as_bytes(), &[]) { + let module = String::from_utf8_lossy(module); + // Is the command already imported? + let full_name = format!("{module} {name}"); + if engine_state.find_decl(full_name.as_bytes(), &[]).is_some() { + return ShellError::ExternalCommand { + label: format!("Command `{name}` not found"), + help: format!("Did you mean `{full_name}`?"), + span, + }; + } else { + return ShellError::ExternalCommand { + label: format!("Command `{name}` not found"), + help: format!("A command with that name exists in module `{module}`. Try importing it with `use`"), + span, + }; + } + } + + // Try to match the name with the search terms of existing commands. + let signatures = engine_state.get_signatures(false); + if let Some(sig) = signatures.iter().find(|sig| { sig.search_terms .iter() - .any(|term| term.to_folded_case() == command_folded_case) - }); - match search_term_match { - Some(sig) => Some(sig.name.clone()), - None => { - let command_names: Vec = commands.iter().map(|sig| sig.name.clone()).collect(); - did_you_mean(&command_names, attempted_command) + .any(|term| term.to_folded_case() == name.to_folded_case()) + }) { + return ShellError::ExternalCommand { + label: format!("Command `{name}` not found"), + help: format!("Did you mean `{}`?", sig.name), + span, + }; + } + + // Try a fuzzy search on the names of all existing commands. + if let Some(cmd) = did_you_mean(signatures.iter().map(|sig| &sig.name), name) { + // The user is invoking an external command with the same name as a + // built-in command. Remind them of this. + if cmd == name { + return ShellError::ExternalCommand { + label: format!("Command `{name}` not found"), + help: "There is a built-in command with the same name".into(), + span, + }; } + return ShellError::ExternalCommand { + label: format!("Command `{name}` not found"), + help: format!("Did you mean `{cmd}`?"), + span, + }; + } + + // We found nothing useful. Give up and return a generic error message. + ShellError::ExternalCommand { + label: format!("Command `{name}` not found"), + help: format!("`{name}` is neither a Nushell built-in or a known external command"), + span, } } -/// This function returns a tuple with 3 items: -/// 1st item: trimmed string. -/// 2nd item: a boolean value indicate if it's ok to run glob expansion. -/// 3rd item: a boolean value indicate if we need to keep raw string. -fn trim_enclosing_quotes(input: &str) -> (String, bool, bool) { - let mut chars = input.chars(); - - match (chars.next(), chars.next_back()) { - (Some('"'), Some('"')) => (chars.collect(), false, true), - (Some('\''), Some('\'')) => (chars.collect(), false, true), - // We treat back-quoted strings as bare words, so there's no need to keep them as raw strings - (Some('`'), Some('`')) => (chars.collect(), true, false), - _ => (input.to_string(), true, false), - } +/// Searches for the absolute path of an executable by name. `.bat` and `.cmd` +/// files are recognized as executables on Windows. +/// +/// This is a wrapper around `which::which_in()` except that, on Windows, it +/// also searches the current directory before any PATH entries. +/// +/// Note: the `which.rs` crate always uses PATHEXT from the environment. As +/// such, changing PATHEXT within Nushell doesn't work without updating the +/// actual environment of the Nushell process. +pub fn which(name: &str, paths: &str, cwd: &Path) -> Option { + #[cfg(windows)] + let paths = format!("{};{}", cwd.display(), paths); + which::which_in(name, Some(paths), cwd).ok() } -fn remove_quotes(input: String) -> String { - let mut chars = input.chars(); - - match (chars.next_back(), input.contains('=')) { - (Some('"'), true) => chars - .collect::() - .replacen('"', "", 1) - .replace(r#"\""#, "\""), - (Some('\''), true) => chars.collect::().replacen('\'', "", 1), - _ => input, - } +/// Returns true if `name` is a (somewhat useful) CMD internal command. The full +/// list can be found at https://ss64.com/nt/syntax-internal.html +fn is_cmd_internal_command(name: &str) -> bool { + const COMMANDS: &[&str] = &[ + "ASSOC", "CLS", "ECHO", "FTYPE", "MKLINK", "PAUSE", "START", "VER", "VOL", + ]; + COMMANDS.iter().any(|cmd| cmd.eq_ignore_ascii_case(name)) } -fn extract_stdio(pipeline: PipelineData) -> (PipelineData, Stdio) { - match pipeline { - PipelineData::ByteStream(stream, metadata) => match stream.into_stdio() { - Ok(pipe) => (PipelineData::Empty, pipe), - Err(stream) => (PipelineData::ByteStream(stream, metadata), Stdio::piped()), - }, - PipelineData::Empty => (PipelineData::Empty, Stdio::inherit()), - data => (data, Stdio::piped()), +/// Returns true if a string contains CMD special characters. +#[cfg(windows)] +fn has_cmd_special_character(s: &str) -> bool { + const SPECIAL_CHARS: &[char] = &['<', '>', '&', '|', '^']; + SPECIAL_CHARS.iter().any(|c| s.contains(*c)) +} + +/// Escape an argument for CMD internal commands. The result can be safely +/// passed to `raw_arg()`. +#[cfg(windows)] +fn escape_cmd_argument(arg: &Spanned) -> Result, ShellError> { + let Spanned { item: arg, span } = arg; + if arg.contains('"') { + // If `arg` is already quoted by double quotes, confirm there's no + // embedded double quotes, then leave it as is. + if arg.chars().filter(|c| *c == '"').count() == 2 + && arg.starts_with('"') + && arg.ends_with('"') + { + Ok(Cow::Borrowed(arg)) + } else { + Err(ShellError::ExternalCommand { + label: "Arguments to CMD internal commands cannot contain embedded double quotes" + .into(), + help: "CMD doesn't support escaping double quotes inside double quotes".into(), + span: *span, + }) + } + } else if arg.contains(' ') || has_cmd_special_character(arg) { + // If `arg` contains space or special characters, quote the entire argument by double quotes. + Ok(Cow::Owned(format!("\"{arg}\""))) + } else { + Ok(Cow::Borrowed(arg)) } } #[cfg(test)] mod test { use super::*; + use nu_protocol::ast::ListItem; #[test] - fn remove_quotes_argument_with_equal_test() { - let input = r#"--file="my_file.txt""#.into(); - let res = remove_quotes(input); - - assert_eq!("--file=my_file.txt", res) + fn test_remove_quotes() { + assert_eq!(remove_quotes(r#""#), r#""#); + assert_eq!(remove_quotes(r#"'"#), r#"'"#); + assert_eq!(remove_quotes(r#"''"#), r#""#); + assert_eq!(remove_quotes(r#""foo""#), r#"foo"#); + assert_eq!(remove_quotes(r#"`foo '"' bar`"#), r#"foo '"' bar"#); + assert_eq!(remove_quotes(r#"'foo' bar"#), r#"'foo' bar"#); + assert_eq!(remove_quotes(r#"r#'foo'#"#), r#"r#'foo'#"#); } #[test] - fn argument_without_equal_test() { - let input = r#"--file "my_file.txt""#.into(); - let res = remove_quotes(input); + fn test_eval_argument() { + fn expression(expr: Expr) -> Expression { + Expression { + expr, + span: Span::unknown(), + ty: Type::Any, + custom_completion: None, + } + } - assert_eq!(r#"--file "my_file.txt""#, res) + fn eval(expr: Expr, spread: bool) -> Result, ShellError> { + let engine_state = EngineState::new(); + let mut stack = Stack::new(); + eval_argument(&engine_state, &mut stack, &expression(expr), spread) + } + + let actual = eval(Expr::String("".into()), false).unwrap(); + let expected = &[""]; + assert_eq!(actual, expected); + + let actual = eval(Expr::String("'foo'".into()), false).unwrap(); + let expected = &["foo"]; + assert_eq!(actual, expected); + + let actual = eval(Expr::RawString("'foo'".into()), false).unwrap(); + let expected = &["'foo'"]; + assert_eq!(actual, expected); + + let actual = eval(Expr::List(vec![]), true).unwrap(); + let expected: &[&str] = &[]; + assert_eq!(actual, expected); + + let actual = eval( + Expr::List(vec![ + ListItem::Item(expression(Expr::String("'foo'".into()))), + ListItem::Item(expression(Expr::String("bar".into()))), + ]), + true, + ) + .unwrap(); + let expected = &["'foo'", "bar"]; + assert_eq!(actual, expected); + + eval(Expr::String("".into()), true).unwrap_err(); + eval(Expr::List(vec![]), false).unwrap_err(); } #[test] - fn remove_quotes_argument_with_single_quotes_test() { - let input = r#"--file='my_file.txt'"#.into(); - let res = remove_quotes(input); + fn test_expand_glob() { + let tempdir = tempfile::tempdir().unwrap(); + let cwd = tempdir.path(); + std::fs::File::create(cwd.join("a.txt")).unwrap(); + std::fs::File::create(cwd.join("b.txt")).unwrap(); - assert_eq!("--file=my_file.txt", res) + let actual = expand_glob("*.txt", cwd, Span::unknown()).unwrap(); + let expected = &["a.txt", "b.txt"]; + assert_eq!(actual, expected); + + let actual = expand_glob("'*.txt'", cwd, Span::unknown()).unwrap(); + let expected = &["'*.txt'"]; + assert_eq!(actual, expected); + + expand_glob("[*.txt", cwd, Span::unknown()).unwrap_err(); } #[test] - fn argument_with_inner_quotes_test() { - let input = r#"sh -c 'echo a'"#.into(); - let res = remove_quotes(input); + fn test_remove_inner_quotes() { + let actual = remove_inner_quotes(r#"--option=value"#); + let expected = r#"--option=value"#; + assert_eq!(actual, expected); - assert_eq!("sh -c 'echo a'", res) + let actual = remove_inner_quotes(r#"--option="value""#); + let expected = r#"--option=value"#; + assert_eq!(actual, expected); + + let actual = remove_inner_quotes(r#"--option='value'"#); + let expected = r#"--option=value"#; + assert_eq!(actual, expected); + + let actual = remove_inner_quotes(r#"--option "value""#); + let expected = r#"--option "value""#; + assert_eq!(actual, expected); + } + + #[test] + fn test_write_pipeline_data() { + let engine_state = EngineState::new(); + let stack = Stack::new(); + + let mut buf = vec![]; + let input = PipelineData::Empty; + write_pipeline_data(engine_state.clone(), stack.clone(), input, &mut buf).unwrap(); + assert_eq!(buf, b""); + + let mut buf = vec![]; + let input = PipelineData::Value(Value::string("foo", Span::unknown()), None); + write_pipeline_data(engine_state.clone(), stack.clone(), input, &mut buf).unwrap(); + assert_eq!(buf, b"foo"); + + let mut buf = vec![]; + let input = PipelineData::Value(Value::binary(b"foo", Span::unknown()), None); + write_pipeline_data(engine_state.clone(), stack.clone(), input, &mut buf).unwrap(); + assert_eq!(buf, b"foo"); + + let mut buf = vec![]; + let input = PipelineData::ByteStream( + ByteStream::read( + b"foo".as_slice(), + Span::unknown(), + None, + ByteStreamType::Unknown, + ), + None, + ); + write_pipeline_data(engine_state.clone(), stack.clone(), input, &mut buf).unwrap(); + assert_eq!(buf, b"foo"); } } diff --git a/crates/nu-command/tests/commands/all.rs b/crates/nu-command/tests/commands/all.rs index bcf6f86a45..8ed5a6f34f 100644 --- a/crates/nu-command/tests/commands/all.rs +++ b/crates/nu-command/tests/commands/all.rs @@ -55,7 +55,9 @@ fn checks_if_all_returns_error_with_invalid_command() { "# )); - assert!(actual.err.contains("can't run executable") || actual.err.contains("did you mean")); + assert!( + actual.err.contains("Command `st` not found") && actual.err.contains("Did you mean `ast`?") + ); } #[test] diff --git a/crates/nu-command/tests/commands/any.rs b/crates/nu-command/tests/commands/any.rs index 0e43f26cfd..f8af25eb61 100644 --- a/crates/nu-command/tests/commands/any.rs +++ b/crates/nu-command/tests/commands/any.rs @@ -41,7 +41,9 @@ fn checks_if_any_returns_error_with_invalid_command() { "# )); - assert!(actual.err.contains("can't run executable") || actual.err.contains("did you mean")); + assert!( + actual.err.contains("Command `st` not found") && actual.err.contains("Did you mean `ast`?") + ); } #[test] diff --git a/crates/nu-command/tests/commands/complete.rs b/crates/nu-command/tests/commands/complete.rs index a5a8b6a256..329e95ad71 100644 --- a/crates/nu-command/tests/commands/complete.rs +++ b/crates/nu-command/tests/commands/complete.rs @@ -24,7 +24,7 @@ fn basic_exit_code() { #[test] fn error() { let actual = nu!("not-found | complete"); - assert!(actual.err.contains("executable was not found")); + assert!(actual.err.contains("Command `not-found` not found")); } #[test] diff --git a/crates/nu-command/tests/commands/source_env.rs b/crates/nu-command/tests/commands/source_env.rs index 4e8da55c78..9dd0320a45 100644 --- a/crates/nu-command/tests/commands/source_env.rs +++ b/crates/nu-command/tests/commands/source_env.rs @@ -283,13 +283,17 @@ fn source_env_is_scoped() { let actual = nu!(cwd: dirs.test(), &inp.join("; ")); - assert!(actual.err.contains("executable was not found")); + assert!(actual + .err + .contains("Command `no-name-similar-to-this` not found")); let inp = &[r#"source-env spam.nu"#, r#"nor-similar-to-this"#]; let actual = nu!(cwd: dirs.test(), &inp.join("; ")); - assert!(actual.err.contains("executable was not found")); + assert!(actual + .err + .contains("Command `nor-similar-to-this` not found")); }) } diff --git a/crates/nu-command/tests/commands/use_.rs b/crates/nu-command/tests/commands/use_.rs index 8cb783f96b..63b1f8391a 100644 --- a/crates/nu-command/tests/commands/use_.rs +++ b/crates/nu-command/tests/commands/use_.rs @@ -189,9 +189,7 @@ fn use_module_creates_accurate_did_you_mean_1() { let actual = nu!(r#" module spam { export def foo [] { "foo" } }; use spam; foo "#); - assert!(actual.err.contains( - "command 'foo' was not found but it was imported from module 'spam'; try using `spam foo`" - )); + assert!(actual.err.contains("Did you mean `spam foo`")); } #[test] @@ -199,9 +197,9 @@ fn use_module_creates_accurate_did_you_mean_2() { let actual = nu!(r#" module spam { export def foo [] { "foo" } }; foo "#); - assert!(actual.err.contains( - "command 'foo' was not found but it exists in module 'spam'; try importing it with `use`" - )); + assert!(actual + .err + .contains("A command with that name exists in module `spam`")); } #[test] diff --git a/src/test_bins.rs b/src/test_bins.rs index d96c74a911..7cd542e732 100644 --- a/src/test_bins.rs +++ b/src/test_bins.rs @@ -242,6 +242,7 @@ pub fn nu_repl() { let mut top_stack = Arc::new(Stack::new()); engine_state.add_env_var("PWD".into(), Value::test_string(cwd.to_string_lossy())); + engine_state.add_env_var("PATH".into(), Value::test_string("")); let mut last_output = String::new(); diff --git a/tests/repl/test_known_external.rs b/tests/repl/test_known_external.rs index c140f4ee90..96ea677d37 100644 --- a/tests/repl/test_known_external.rs +++ b/tests/repl/test_known_external.rs @@ -108,14 +108,14 @@ fn known_external_misc_values() -> TestResult { /// GitHub issue #7822 #[test] fn known_external_subcommand_from_module() -> TestResult { - let output = Command::new("cargo").arg("check").arg("-h").output()?; + let output = Command::new("cargo").arg("add").arg("-h").output()?; run_test( r#" module cargo { - export extern check [] + export extern add [] }; use cargo; - cargo check -h + cargo add -h "#, String::from_utf8(output.stdout)?.trim(), ) @@ -124,14 +124,14 @@ fn known_external_subcommand_from_module() -> TestResult { /// GitHub issue #7822 #[test] fn known_external_aliased_subcommand_from_module() -> TestResult { - let output = Command::new("cargo").arg("check").arg("-h").output()?; + let output = Command::new("cargo").arg("add").arg("-h").output()?; run_test( r#" module cargo { - export extern check [] + export extern add [] }; use cargo; - alias cc = cargo check; + alias cc = cargo add; cc -h "#, String::from_utf8(output.stdout)?.trim(), diff --git a/tests/repl/test_parser.rs b/tests/repl/test_parser.rs index c0e7279b7b..9d743b175d 100644 --- a/tests/repl/test_parser.rs +++ b/tests/repl/test_parser.rs @@ -647,7 +647,7 @@ fn duration_with_underscores_2() -> TestResult { #[test] fn duration_with_underscores_3() -> TestResult { - fail_test("1_000_d_ay", "executable was not found") + fail_test("1_000_d_ay", "Command `1_000_d_ay` not found") } #[test] @@ -667,7 +667,7 @@ fn filesize_with_underscores_2() -> TestResult { #[test] fn filesize_with_underscores_3() -> TestResult { - fail_test("42m_b", "executable was not found") + fail_test("42m_b", "Command `42m_b` not found") } #[test] diff --git a/tests/repl/test_spread.rs b/tests/repl/test_spread.rs index 419188be33..f0ce84d393 100644 --- a/tests/repl/test_spread.rs +++ b/tests/repl/test_spread.rs @@ -183,17 +183,14 @@ fn explain_spread_args() -> TestResult { #[test] fn disallow_implicit_spread_for_externals() -> TestResult { - fail_test( - r#"nu --testbin cococo [1 2]"#, - "Lists are not automatically spread", - ) + fail_test(r#"^echo [1 2]"#, "Lists are not automatically spread") } #[test] fn respect_shape() -> TestResult { fail_test( "def foo [...rest] { ...$rest }; foo bar baz", - "executable was not found", + "Command `...$rest` not found", ) .unwrap(); fail_test("module foo { ...$bar }", "expected_keyword").unwrap(); diff --git a/tests/shell/pipeline/commands/external.rs b/tests/shell/pipeline/commands/external.rs index 3cbed5a124..8557e9e327 100644 --- a/tests/shell/pipeline/commands/external.rs +++ b/tests/shell/pipeline/commands/external.rs @@ -72,7 +72,9 @@ fn correctly_escape_external_arguments() { fn escape_also_escapes_equals() { let actual = nu!("^MYFOONAME=MYBARVALUE"); - assert!(actual.err.contains("executable was not found")); + assert!(actual + .err + .contains("Command `MYFOONAME=MYBARVALUE` not found")); } #[test] @@ -127,7 +129,7 @@ fn command_not_found_error_shows_not_found_1() { export extern "foo" []; foo "#); - assert!(actual.err.contains("'foo' was not found")); + assert!(actual.err.contains("Command `foo` not found")); } #[test] diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs index 6c2226ff65..67d4ee31a9 100644 --- a/tests/shell/pipeline/commands/internal.rs +++ b/tests/shell/pipeline/commands/internal.rs @@ -989,7 +989,9 @@ fn hide_alias_hides_alias() { " )); - assert!(actual.err.contains("did you mean 'all'?")); + assert!( + actual.err.contains("Command `ll` not found") && actual.err.contains("Did you mean `all`?") + ); } mod parse { @@ -1035,7 +1037,7 @@ mod parse { fn ensure_backticks_are_bareword_command() { let actual = nu!("`8abc123`"); - assert!(actual.err.contains("was not found"),); + assert!(actual.err.contains("Command `8abc123` not found"),); } } @@ -1146,5 +1148,8 @@ fn command_not_found_error_shows_not_found_2() { export def --wrapped my-foo [...rest] { foo }; my-foo "#); - assert!(actual.err.contains("did you mean")); + assert!( + actual.err.contains("Command `foo` not found") + && actual.err.contains("Did you mean `for`?") + ); }