From 157494e803d100a96e4e3c3079548fd3f98596af Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Wed, 2 Oct 2024 04:05:48 -0700 Subject: [PATCH] Make `get_env_var` return a reference to a `Value` (#13987) # Description Title says it all, changes `EngineState::get_env_var` to return a `Option<&'a Value>` instead of an owned `Option`. This avoids some unnecessary clones. I also made a similar change to the `PluginExecutionContext` trait. --- .../src/completions/completion_common.rs | 2 +- crates/nu-cli/src/prompt.rs | 12 ++++----- crates/nu-cli/src/prompt_update.rs | 8 ++++-- crates/nu-cli/src/repl.rs | 26 ++++++++++++++----- crates/nu-cmd-base/src/hook.rs | 17 +++++------- crates/nu-cmd-plugin/src/util.rs | 5 ++-- crates/nu-command/src/filesystem/cd.rs | 2 +- crates/nu-command/src/network/http/client.rs | 3 ++- crates/nu-command/src/viewers/griddle.rs | 2 +- crates/nu-command/src/viewers/table.rs | 2 +- crates/nu-engine/src/env.rs | 8 +++--- crates/nu-explore/src/nu_common/lscolor.rs | 2 +- crates/nu-plugin-engine/src/context.rs | 6 ++--- crates/nu-plugin-engine/src/interface/mod.rs | 4 ++- crates/nu-protocol/src/engine/engine_state.rs | 8 +++--- crates/nu-protocol/src/engine/stack.rs | 16 ++++++++---- src/test_bins.rs | 2 +- 17 files changed, 74 insertions(+), 51 deletions(-) diff --git a/crates/nu-cli/src/completions/completion_common.rs b/crates/nu-cli/src/completions/completion_common.rs index 77ae165c73..6dd59da853 100644 --- a/crates/nu-cli/src/completions/completion_common.rs +++ b/crates/nu-cli/src/completions/completion_common.rs @@ -180,7 +180,7 @@ pub fn complete_item( && engine_state.config.use_ansi_coloring) .then(|| { let ls_colors_env_str = match stack.get_env_var(engine_state, "LS_COLORS") { - Some(v) => env_to_string("LS_COLORS", &v, engine_state, stack).ok(), + Some(v) => env_to_string("LS_COLORS", v, engine_state, stack).ok(), None => None, }; get_ls_colors(ls_colors_env_str) diff --git a/crates/nu-cli/src/prompt.rs b/crates/nu-cli/src/prompt.rs index 744640b76f..b590a4436d 100644 --- a/crates/nu-cli/src/prompt.rs +++ b/crates/nu-cli/src/prompt.rs @@ -1,10 +1,7 @@ use crate::prompt_update::{ POST_PROMPT_MARKER, PRE_PROMPT_MARKER, VSCODE_POST_PROMPT_MARKER, VSCODE_PRE_PROMPT_MARKER, }; -use nu_protocol::{ - engine::{EngineState, Stack}, - Value, -}; +use nu_protocol::engine::{EngineState, Stack}; #[cfg(windows)] use nu_utils::enable_vt_processing; use reedline::{ @@ -124,8 +121,11 @@ impl Prompt for NushellPrompt { .replace('\n', "\r\n"); if self.shell_integration_osc633 { - if self.stack.get_env_var(&self.engine_state, "TERM_PROGRAM") - == Some(Value::test_string("vscode")) + if self + .stack + .get_env_var(&self.engine_state, "TERM_PROGRAM") + .and_then(|v| v.as_str().ok()) + == Some("vscode") { // We're in vscode and we have osc633 enabled format!("{VSCODE_PRE_PROMPT_MARKER}{prompt}{VSCODE_POST_PROMPT_MARKER}").into() diff --git a/crates/nu-cli/src/prompt_update.rs b/crates/nu-cli/src/prompt_update.rs index d96bf40b8f..95280af828 100644 --- a/crates/nu-cli/src/prompt_update.rs +++ b/crates/nu-cli/src/prompt_update.rs @@ -68,7 +68,7 @@ fn get_prompt_string( .get_env_var(engine_state, prompt) .and_then(|v| match v { Value::Closure { val, .. } => { - let result = ClosureEvalOnce::new(engine_state, stack, *val) + let result = ClosureEvalOnce::new(engine_state, stack, val.as_ref().clone()) .run_with_input(PipelineData::Empty); trace!( @@ -119,7 +119,11 @@ pub(crate) fn update_prompt( // Now that we have the prompt string lets ansify it. // <133 A><133 B><133 C> let left_prompt_string = if config.shell_integration.osc633 { - if stack.get_env_var(engine_state, "TERM_PROGRAM") == Some(Value::test_string("vscode")) { + if stack + .get_env_var(engine_state, "TERM_PROGRAM") + .and_then(|v| v.as_str().ok()) + == Some("vscode") + { // We're in vscode and we have osc633 enabled Some(format!( "{VSCODE_PRE_PROMPT_MARKER}{configured_left_prompt_string}{VSCODE_POST_PROMPT_MARKER}" diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 087a44114d..3e2b01cfd1 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -512,8 +512,10 @@ fn loop_iteration(ctx: LoopContext) -> (bool, Stack, Reedline) { drop(repl); if shell_integration_osc633 { - if stack.get_env_var(engine_state, "TERM_PROGRAM") - == Some(Value::test_string("vscode")) + if stack + .get_env_var(engine_state, "TERM_PROGRAM") + .and_then(|v| v.as_str().ok()) + == Some("vscode") { start_time = Instant::now(); @@ -835,7 +837,7 @@ fn do_auto_cd( let shells = stack.get_env_var(engine_state, "NUSHELL_SHELLS"); let mut shells = if let Some(v) = shells { - v.into_list().unwrap_or_else(|_| vec![cwd]) + v.clone().into_list().unwrap_or_else(|_| vec![cwd]) } else { vec![cwd] }; @@ -1027,7 +1029,11 @@ fn run_shell_integration_osc633( if let Ok(path) = current_dir_str(engine_state, stack) { // Supported escape sequences of Microsoft's Visual Studio Code (vscode) // https://code.visualstudio.com/docs/terminal/shell-integration#_supported-escape-sequences - if stack.get_env_var(engine_state, "TERM_PROGRAM") == Some(Value::test_string("vscode")) { + if stack + .get_env_var(engine_state, "TERM_PROGRAM") + .and_then(|v| v.as_str().ok()) + == Some("vscode") + { let start_time = Instant::now(); // If we're in vscode, run their specific ansi escape sequence. @@ -1225,7 +1231,11 @@ fn get_command_finished_marker( .and_then(|e| e.as_i64().ok()); if shell_integration_osc633 { - if stack.get_env_var(engine_state, "TERM_PROGRAM") == Some(Value::test_string("vscode")) { + if stack + .get_env_var(engine_state, "TERM_PROGRAM") + .and_then(|v| v.as_str().ok()) + == Some("vscode") + { // We're in vscode and we have osc633 enabled format!( "{}{}{}", @@ -1274,7 +1284,11 @@ fn run_finaliziation_ansi_sequence( ) { if shell_integration_osc633 { // Only run osc633 if we are in vscode - if stack.get_env_var(engine_state, "TERM_PROGRAM") == Some(Value::test_string("vscode")) { + if stack + .get_env_var(engine_state, "TERM_PROGRAM") + .and_then(|v| v.as_str().ok()) + == Some("vscode") + { let start_time = Instant::now(); run_ansi_sequence(&get_command_finished_marker( diff --git a/crates/nu-cmd-base/src/hook.rs b/crates/nu-cmd-base/src/hook.rs index 064db0e5bc..e9637d16ad 100644 --- a/crates/nu-cmd-base/src/hook.rs +++ b/crates/nu-cmd-base/src/hook.rs @@ -18,17 +18,12 @@ pub fn eval_env_change_hook( match hook { Value::Record { val, .. } => { for (env_name, hook_value) in &*val { - let before = engine_state - .previous_env_vars - .get(env_name) - .cloned() - .unwrap_or_default(); - - let after = stack - .get_env_var(engine_state, env_name) - .unwrap_or_default(); - + let before = engine_state.previous_env_vars.get(env_name); + let after = stack.get_env_var(engine_state, env_name); if before != after { + let before = before.cloned().unwrap_or_default(); + let after = after.cloned().unwrap_or_default(); + eval_hook( engine_state, stack, @@ -39,7 +34,7 @@ pub fn eval_env_change_hook( )?; Arc::make_mut(&mut engine_state.previous_env_vars) - .insert(env_name.to_string(), after); + .insert(env_name.clone(), after); } } } diff --git a/crates/nu-cmd-plugin/src/util.rs b/crates/nu-cmd-plugin/src/util.rs index 7d5282235f..6808a74a3c 100644 --- a/crates/nu-cmd-plugin/src/util.rs +++ b/crates/nu-cmd-plugin/src/util.rs @@ -95,8 +95,9 @@ pub(crate) fn get_plugin_dirs( let working_set = StateWorkingSet::new(engine_state); let value = working_set .find_variable(b"$NU_PLUGIN_DIRS") - .and_then(|var_id| working_set.get_constant(var_id).ok().cloned()) - .or_else(|| stack.get_env_var(engine_state, "NU_PLUGIN_DIRS")); + .and_then(|var_id| working_set.get_constant(var_id).ok()) + .or_else(|| stack.get_env_var(engine_state, "NU_PLUGIN_DIRS")) + .cloned(); // TODO: avoid this clone // Get all of the strings in the list, if possible value diff --git a/crates/nu-command/src/filesystem/cd.rs b/crates/nu-command/src/filesystem/cd.rs index ed63f06d74..2af932bcb4 100644 --- a/crates/nu-command/src/filesystem/cd.rs +++ b/crates/nu-command/src/filesystem/cd.rs @@ -107,7 +107,7 @@ impl Command for Cd { // Set OLDPWD. // We're using `Stack::get_env_var()` instead of `EngineState::cwd()` to avoid a conversion roundtrip. if let Some(oldpwd) = stack.get_env_var(engine_state, "PWD") { - stack.add_env_var("OLDPWD".into(), oldpwd) + stack.add_env_var("OLDPWD".into(), oldpwd.clone()) } match have_permission(&path) { diff --git a/crates/nu-command/src/network/http/client.rs b/crates/nu-command/src/network/http/client.rs index e53a1f4413..8dd4cc5601 100644 --- a/crates/nu-command/src/network/http/client.rs +++ b/crates/nu-command/src/network/http/client.rs @@ -314,7 +314,7 @@ fn send_form_request( Value::List { ref vals, .. } => { if vals.len() % 2 != 0 { return Err(ShellErrorOrRequestError::ShellError(ShellError::IncorrectValue { - msg: "Body type 'list' for form requests requires paired values. E.g.: [foo, 10]".into(), + msg: "Body type 'list' for form requests requires paired values. E.g.: [foo, 10]".into(), val_span: body.span(), call_span: span, })); @@ -901,6 +901,7 @@ fn retrieve_http_proxy_from_env(engine_state: &EngineState, stack: &mut Stack) - .or(stack.get_env_var(engine_state, "https_proxy")) .or(stack.get_env_var(engine_state, "HTTPS_PROXY")) .or(stack.get_env_var(engine_state, "ALL_PROXY")) + .cloned() .and_then(|proxy| proxy.coerce_into_string().ok()) } diff --git a/crates/nu-command/src/viewers/griddle.rs b/crates/nu-command/src/viewers/griddle.rs index f75864b388..2774903d0f 100644 --- a/crates/nu-command/src/viewers/griddle.rs +++ b/crates/nu-command/src/viewers/griddle.rs @@ -69,7 +69,7 @@ prints out the list properly."# let icons_param: bool = call.has_flag(engine_state, stack, "icons")?; let config = &stack.get_config(engine_state); let env_str = match stack.get_env_var(engine_state, "LS_COLORS") { - Some(v) => Some(env_to_string("LS_COLORS", &v, engine_state, stack)?), + Some(v) => Some(env_to_string("LS_COLORS", v, engine_state, stack)?), None => None, }; diff --git a/crates/nu-command/src/viewers/table.rs b/crates/nu-command/src/viewers/table.rs index 5bdb847212..44e96eb060 100644 --- a/crates/nu-command/src/viewers/table.rs +++ b/crates/nu-command/src/viewers/table.rs @@ -623,7 +623,7 @@ fn handle_row_stream( let ls_colors_env_str = match input.stack.get_env_var(input.engine_state, "LS_COLORS") { Some(v) => Some(env_to_string( "LS_COLORS", - &v, + v, input.engine_state, input.stack, )?), diff --git a/crates/nu-engine/src/env.rs b/crates/nu-engine/src/env.rs index f93e78af88..c4460f4695 100644 --- a/crates/nu-engine/src/env.rs +++ b/crates/nu-engine/src/env.rs @@ -240,7 +240,7 @@ pub fn path_str( } }?; - env_to_string(pathname, &pathval, engine_state, stack) + env_to_string(pathname, pathval, engine_state, stack) } pub const DIR_VAR_PARSER_INFO: &str = "dirs_var"; @@ -274,7 +274,7 @@ pub fn find_in_dirs_env( ) -> Result, ShellError> { // Choose whether to use file-relative or PWD-relative path let cwd = if let Some(pwd) = stack.get_env_var(engine_state, "FILE_PWD") { - match env_to_string("FILE_PWD", &pwd, engine_state, stack) { + match env_to_string("FILE_PWD", pwd, engine_state, stack) { Ok(cwd) => { if Path::new(&cwd).is_absolute() { cwd @@ -294,7 +294,7 @@ pub fn find_in_dirs_env( engine_state.cwd_as_string(Some(stack))? }; - let check_dir = |lib_dirs: Option| -> Option { + let check_dir = |lib_dirs: Option<&Value>| -> Option { if let Ok(p) = canonicalize_with(filename, &cwd) { return Some(p); } @@ -316,7 +316,7 @@ pub fn find_in_dirs_env( .flatten() }; - let lib_dirs = dirs_var.and_then(|var_id| engine_state.get_var(var_id).const_val.clone()); + let lib_dirs = dirs_var.and_then(|var_id| engine_state.get_var(var_id).const_val.as_ref()); // TODO: remove (see #8310) let lib_dirs_fallback = stack.get_env_var(engine_state, "NU_LIB_DIRS"); diff --git a/crates/nu-explore/src/nu_common/lscolor.rs b/crates/nu-explore/src/nu_common/lscolor.rs index 393ace7883..7a3400dae0 100644 --- a/crates/nu-explore/src/nu_common/lscolor.rs +++ b/crates/nu-explore/src/nu_common/lscolor.rs @@ -11,7 +11,7 @@ use nu_utils::get_ls_colors; pub fn create_lscolors(engine_state: &EngineState, stack: &Stack) -> LsColors { let colors = stack .get_env_var(engine_state, "LS_COLORS") - .and_then(|v| env_to_string("LS_COLORS", &v, engine_state, stack).ok()); + .and_then(|v| env_to_string("LS_COLORS", v, engine_state, stack).ok()); get_ls_colors(colors) } diff --git a/crates/nu-plugin-engine/src/context.rs b/crates/nu-plugin-engine/src/context.rs index 4a6af0b879..4c262fd3ed 100644 --- a/crates/nu-plugin-engine/src/context.rs +++ b/crates/nu-plugin-engine/src/context.rs @@ -25,7 +25,7 @@ pub trait PluginExecutionContext: Send + Sync { /// Get plugin configuration fn get_plugin_config(&self) -> Result, ShellError>; /// Get an environment variable from `$env` - fn get_env_var(&self, name: &str) -> Result, ShellError>; + fn get_env_var(&self, name: &str) -> Result, ShellError>; /// Get all environment variables fn get_env_vars(&self) -> Result, ShellError>; /// Get current working directory @@ -125,7 +125,7 @@ impl<'a> PluginExecutionContext for PluginExecutionCommandContext<'a> { })) } - fn get_env_var(&self, name: &str) -> Result, ShellError> { + fn get_env_var(&self, name: &str) -> Result, ShellError> { Ok(self.stack.get_env_var(&self.engine_state, name)) } @@ -300,7 +300,7 @@ impl PluginExecutionContext for PluginExecutionBogusContext { Ok(None) } - fn get_env_var(&self, _name: &str) -> Result, ShellError> { + fn get_env_var(&self, _name: &str) -> Result, ShellError> { Err(ShellError::NushellFailed { msg: "get_env_var not implemented on bogus".into(), }) diff --git a/crates/nu-plugin-engine/src/interface/mod.rs b/crates/nu-plugin-engine/src/interface/mod.rs index b27ea0be3d..93d75d0ec9 100644 --- a/crates/nu-plugin-engine/src/interface/mod.rs +++ b/crates/nu-plugin-engine/src/interface/mod.rs @@ -1276,7 +1276,9 @@ pub(crate) fn handle_engine_call( } EngineCall::GetEnvVar(name) => { let value = context.get_env_var(&name)?; - Ok(value.map_or_else(EngineCallResponse::empty, EngineCallResponse::value)) + Ok(value + .cloned() + .map_or_else(EngineCallResponse::empty, EngineCallResponse::value)) } EngineCall::GetEnvVars => context.get_env_vars().map(EngineCallResponse::ValueMap), EngineCall::GetCurrentDir => { diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index 665a79a329..ec616c3403 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -935,14 +935,14 @@ impl EngineState { let pwd = if let Some(stack) = stack { stack.get_env_var(self, "PWD") } else { - self.get_env_var("PWD").cloned() + self.get_env_var("PWD") }; let pwd = pwd.ok_or_else(|| error("$env.PWD not found", ""))?; - if let Value::String { val, .. } = pwd { - let path = AbsolutePathBuf::try_from(val) - .map_err(|path| error("$env.PWD is not an absolute path", path))?; + if let Ok(pwd) = pwd.as_str() { + let path = AbsolutePathBuf::try_from(pwd) + .map_err(|_| error("$env.PWD is not an absolute path", pwd))?; // Technically, a root path counts as "having trailing slashes", but // for the purpose of PWD, a root path is acceptable. diff --git a/crates/nu-protocol/src/engine/stack.rs b/crates/nu-protocol/src/engine/stack.rs index 391e8633c8..296c2eee55 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -211,7 +211,7 @@ impl Stack { /// /// The config will be updated with successfully parsed values even if an error occurs. pub fn update_config(&mut self, engine_state: &EngineState) -> Result<(), ShellError> { - if let Some(mut config) = self.get_env_var(engine_state, "config") { + if let Some(mut config) = self.get_env_var(engine_state, "config").cloned() { let existing_config = self.get_config(engine_state); let (new_config, error) = config.parse_as_config(&existing_config); self.config = Some(new_config.into()); @@ -451,12 +451,16 @@ impl Stack { result } - pub fn get_env_var(&self, engine_state: &EngineState, name: &str) -> Option { + pub fn get_env_var<'a>( + &'a self, + engine_state: &'a EngineState, + name: &str, + ) -> Option<&'a Value> { for scope in self.env_vars.iter().rev() { for active_overlay in self.active_overlays.iter().rev() { if let Some(env_vars) = scope.get(active_overlay) { if let Some(v) = env_vars.get(name) { - return Some(v.clone()); + return Some(v); } } } @@ -472,7 +476,7 @@ impl Stack { if !is_hidden { if let Some(env_vars) = engine_state.env_vars.get(active_overlay) { if let Some(v) = env_vars.get(name) { - return Some(v.clone()); + return Some(v); } } } @@ -842,7 +846,9 @@ mod test { ); assert_eq!( - original.get_env_var(&engine_state, "ADDED_IN_CHILD"), + original + .get_env_var(&engine_state, "ADDED_IN_CHILD") + .cloned(), Some(string_value("New Env Var")), ); } diff --git a/src/test_bins.rs b/src/test_bins.rs index 2fef7e02b7..c4ba37ed8e 100644 --- a/src/test_bins.rs +++ b/src/test_bins.rs @@ -348,7 +348,7 @@ pub fn nu_repl() { .coerce_str() .unwrap_or_else(|err| outcome_err(&engine_state, &err)); let _ = std::env::set_current_dir(path.as_ref()); - engine_state.add_env_var("PWD".into(), cwd); + engine_state.add_env_var("PWD".into(), cwd.clone()); } top_stack = Arc::new(Stack::with_changes_from_child(top_stack, stack)); }