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<Value>`. This avoids
some unnecessary clones.

I also made a similar change to the `PluginExecutionContext` trait.

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)

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::{
+use nu_protocol::engine::{EngineState, Stack};
-    engine::{EngineState, Stack},
-    Value,
-};
 #[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")
+                if self
-                    == Some(Value::test_string("vscode"))
+                    .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()

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><prompt><133 B><command><133 C><command output>
     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}"

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")
+                if stack
-                    == Some(Value::test_string("vscode"))
+                    .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(

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
+                    let before = engine_state.previous_env_vars.get(env_name);
-                        .previous_env_vars
+                    let after = stack.get_env_var(engine_state, env_name);
-                        .get(env_name)
-                        .cloned()
-                        .unwrap_or_default();
-
-                    let after = stack
-                        .get_env_var(engine_state, env_name)
-                        .unwrap_or_default();
-
                     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);
                     }
                 }
             }

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())
+        .and_then(|var_id| working_set.get_constant(var_id).ok())
-        .or_else(|| stack.get_env_var(engine_state, "NU_PLUGIN_DIRS"));
+        .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

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) {

crates/nu-command/src/network/http/client.rs

@@ -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())
 }
 

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,
         };
 

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,
                 )?),

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<Option<PathBuf>, 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<Value>| -> Option<PathBuf> {
+    let check_dir = |lib_dirs: Option<&Value>| -> Option<PathBuf> {
         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");
 

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)
 }

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<Option<Value>, ShellError>;
     /// Get an environment variable from `$env`
-    fn get_env_var(&self, name: &str) -> Result<Option<Value>, ShellError>;
+    fn get_env_var(&self, name: &str) -> Result<Option<&Value>, ShellError>;
     /// Get all environment variables
     fn get_env_vars(&self) -> Result<HashMap<String, Value>, ShellError>;
     /// Get current working directory
@@ -125,7 +125,7 @@ impl<'a> PluginExecutionContext for PluginExecutionCommandContext<'a> {
             }))
     }
 
-    fn get_env_var(&self, name: &str) -> Result<Option<Value>, ShellError> {
+    fn get_env_var(&self, name: &str) -> Result<Option<&Value>, 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<Option<Value>, ShellError> {
+    fn get_env_var(&self, _name: &str) -> Result<Option<&Value>, ShellError> {
         Err(ShellError::NushellFailed {
             msg: "get_env_var not implemented on bogus".into(),
         })

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 => {

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 {
+        if let Ok(pwd) = pwd.as_str() {
-            let path = AbsolutePathBuf::try_from(val)
+            let path = AbsolutePathBuf::try_from(pwd)
-                .map_err(|path| error("$env.PWD is not an absolute path", path))?;
+                .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.

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<Value> {
+    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")),
         );
     }

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));
     }