Merge stack before printing (#9304)

Could you @fdncred try it?

close?: #9264

---------

Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>
Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>

crates/nu-cli/src/config_files.rs

@@ -100,7 +100,12 @@ pub fn eval_config_contents(
             // Merge the environment in case env vars changed in the config
             match nu_engine::env::current_dir(engine_state, stack) {
                 Ok(cwd) => {
-                    if let Err(e) = engine_state.merge_env(stack, cwd) {
+                    if let Err(e) = engine_state.merge_env(stack) {
+                        let working_set = StateWorkingSet::new(engine_state);
+                        report_error(&working_set, &e);
+                    }
+
+                    if let Err(e) = engine_state.set_current_working_dir(cwd) {
                         let working_set = StateWorkingSet::new(engine_state);
                         report_error(&working_set, &e);
                     }

crates/nu-cli/src/print.rs

@@ -53,6 +53,12 @@ Since this command has no output, there is no point in piping it with other comm
         let no_newline = call.has_flag("no-newline");
         let to_stderr = call.has_flag("stderr");
 
+        // We merge stack to make sure we render the changes if any were made in the `block`
+        //
+        // CONSIDERED TO BE A CODE SMELL AND IT BETTER BE RESOLVED UPWARDS THE CALLING STACK
+        let engine = engine_state.clone_with_env(stack)?;
+        let engine_state = &engine;
+
         // This will allow for easy printing of pipelines as well
         if !args.is_empty() {
             for arg in args {

crates/nu-cli/src/repl.rs

@@ -172,7 +172,8 @@ pub fn evaluate_repl(
             PipelineData::empty(),
             false,
         );
-        engine_state.merge_env(stack, get_guaranteed_cwd(engine_state, stack))?;
+        engine_state.merge_env(stack)?;
+        engine_state.set_current_working_dir(get_guaranteed_cwd(engine_state, stack))?;
     }
 
     engine_state.set_startup_time(entire_start_time.elapsed().as_nanos() as i64);
@@ -191,14 +192,18 @@ pub fn evaluate_repl(
     loop {
         let loop_start_time = std::time::Instant::now();
 
-        let cwd = get_guaranteed_cwd(engine_state, stack);
-
         start_time = std::time::Instant::now();
         // Before doing anything, merge the environment from the previous REPL iteration into the
         // permanent state.
-        if let Err(err) = engine_state.merge_env(stack, cwd) {
+        if let Err(err) = engine_state.merge_env(stack) {
             report_error_new(engine_state, &err);
         }
+
+        let cwd = get_guaranteed_cwd(engine_state, stack);
+        if let Err(err) = engine_state.set_current_working_dir(cwd) {
+            report_error_new(engine_state, &err);
+        }
+
         perf(
             "merge env",
             start_time,

crates/nu-cli/src/util.rs

@@ -246,6 +246,10 @@ pub fn eval_source(
 
     match b {
         Ok(pipeline_data) => {
+            // we merge stack here because the block could change the envirenemnt,
+            // and we need to render it while do print.
+            let _ = engine_state.merge_env(stack);
+
             let config = engine_state.get_config();
             let result;
             if let PipelineData::ExternalStream {

crates/nu-cli/tests/completions.rs

@@ -768,7 +768,8 @@ fn run_external_completion(block: &str, input: &str) -> Vec<Suggestion> {
     assert!(engine_state.merge_delta(delta).is_ok());
 
     // Merge environment into the permanent state
-    assert!(engine_state.merge_env(&mut stack, &dir).is_ok());
+    assert!(engine_state.merge_env(&mut stack).is_ok());
+    assert!(engine_state.set_current_working_dir(&dir).is_ok());
 
     let latest_block_id = engine_state.num_blocks() - 1;
 

crates/nu-cli/tests/support/completions_helpers.rs

@@ -61,8 +61,8 @@ pub fn new_engine() -> (PathBuf, String, EngineState, Stack) {
     );
 
     // Merge environment into the permanent state
-    let merge_result = engine_state.merge_env(&mut stack, &dir);
-    assert!(merge_result.is_ok());
+    assert!(engine_state.merge_env(&mut stack).is_ok());
+    assert!(engine_state.set_current_working_dir(&dir).is_ok());
 
     (dir, dir_str, engine_state, stack)
 }
@@ -100,8 +100,8 @@ pub fn new_quote_engine() -> (PathBuf, String, EngineState, Stack) {
     );
 
     // Merge environment into the permanent state
-    let merge_result = engine_state.merge_env(&mut stack, &dir);
-    assert!(merge_result.is_ok());
+    assert!(engine_state.merge_env(&mut stack).is_ok());
+    assert!(engine_state.set_current_working_dir(&dir).is_ok());
 
     (dir, dir_str, engine_state, stack)
 }
@@ -171,5 +171,8 @@ pub fn merge_input(
     .is_ok());
 
     // Merge environment into the permanent state
-    engine_state.merge_env(stack, &dir)
+    engine_state.merge_env(stack)?;
+    engine_state.set_current_working_dir(&dir)?;
+
+    Ok(())
 }

crates/nu-cmd-lang/src/example_support.rs

@@ -167,9 +167,13 @@ pub fn check_example_evaluates_to_expected_output(
     stack.add_env_var("PWD".to_string(), Value::test_string(cwd.to_string_lossy()));
 
     engine_state
-        .merge_env(&mut stack, cwd)
+        .merge_env(&mut stack)
         .expect("Error merging environment");
 
+    engine_state
+        .set_current_working_dir(cwd)
+        .expect("Error setting CWD");
+
     let empty_input = PipelineData::empty();
     let result = eval(example.example, empty_input, cwd, engine_state);
 

crates/nu-command/src/hook.rs

@@ -281,8 +281,10 @@ pub fn eval_hook(
         }
     }
 
+    engine_state.merge_env(stack)?;
+
     let cwd = get_guaranteed_cwd(engine_state, stack);
-    engine_state.merge_env(stack, cwd)?;
+    engine_state.set_current_working_dir(cwd)?;
 
     Ok(output)
 }

crates/nu-command/src/viewers/table.rs

@@ -102,6 +102,12 @@ impl Command for Table {
         call: &Call,
         input: PipelineData,
     ) -> Result<PipelineData, ShellError> {
+        // We merge stack to make sure we render the changes if any were made in the `block`
+        //
+        // CONSIDERED TO BE A CODE SMELL AND IT BETTER BE RESOLVED UPWARDS THE CALLING STACK
+        let engine = engine_state.clone_with_env(stack)?;
+        let engine_state = &engine;
+
         let start_num: Option<i64> = call.get_flag(engine_state, stack, "start-number")?;
         let row_offset = start_num.unwrap_or_default() as usize;
         let list: bool = call.has_flag("list");

crates/nu-protocol/src/engine/engine_state.rs

@@ -279,11 +279,7 @@ impl EngineState {
     }
 
     /// Merge the environment from the runtime Stack into the engine state
-    pub fn merge_env(
-        &mut self,
-        stack: &mut Stack,
-        cwd: impl AsRef<Path>,
-    ) -> Result<(), ShellError> {
+    pub fn merge_env(&mut self, stack: &mut Stack) -> Result<(), ShellError> {
         for mut scope in stack.env_vars.drain(..) {
             for (overlay_name, mut env) in scope.drain() {
                 if let Some(env_vars) = self.env_vars.get_mut(&overlay_name) {
@@ -310,12 +306,27 @@ impl EngineState {
             }
         }
 
+        Ok(())
+    }
+
+    /// Set a CWD.
+    pub fn set_current_working_dir(&mut self, cwd: impl AsRef<Path>) -> Result<(), ShellError> {
         // TODO: better error
         std::env::set_current_dir(cwd)?;
-
         Ok(())
     }
 
+    /// Merge the environment from the runtime Stack into the engine state
+    ///
+    /// A merge which does not consume the stack env.
+    pub fn clone_with_env(&self, stack: &Stack) -> Result<Self, ShellError> {
+        let mut engine = self.clone();
+        let mut stack = stack.clone();
+        engine.merge_env(&mut stack)?;
+
+        Ok(engine)
+    }
+
     /// Mark a starting point if it is a script (e.g., nu spam.nu)
     pub fn start_in_file(&mut self, file_path: Option<&str>) {
         self.currently_parsed_cwd = if let Some(path) = file_path {

crates/nu-std/src/lib.rs

@@ -98,7 +98,8 @@ use std dirs [
     )?;
 
     let cwd = current_dir(engine_state, &stack)?;
-    engine_state.merge_env(&mut stack, cwd)?;
+    engine_state.merge_env(&mut stack)?;
+    engine_state.set_current_working_dir(cwd)?;
 
     Ok(())
 }

src/config_files.rs

@@ -136,18 +136,7 @@ pub(crate) fn read_default_env_file(engine_state: &mut EngineState, stack: &mut
 
     info!("read_config_file {}:{}:{}", file!(), line!(), column!());
     // Merge the environment in case env vars changed in the config
-    match nu_engine::env::current_dir(engine_state, stack) {
-        Ok(cwd) => {
-            if let Err(e) = engine_state.merge_env(stack, cwd) {
-                let working_set = StateWorkingSet::new(engine_state);
-                report_error(&working_set, &e);
-            }
-        }
-        Err(e) => {
-            let working_set = StateWorkingSet::new(engine_state);
-            report_error(&working_set, &e);
-        }
-    }
+    merge_env(engine_state, stack)
 }
 
 fn eval_default_config(
@@ -172,9 +161,18 @@ fn eval_default_config(
     );
 
     // Merge the environment in case env vars changed in the config
+    merge_env(engine_state, stack)
+}
+
+fn merge_env(engine_state: &mut EngineState, stack: &mut Stack) {
     match nu_engine::env::current_dir(engine_state, stack) {
         Ok(cwd) => {
-            if let Err(e) = engine_state.merge_env(stack, cwd) {
+            if let Err(e) = engine_state.merge_env(stack) {
+                let working_set = StateWorkingSet::new(engine_state);
+                report_error(&working_set, &e);
+            }
+
+            if let Err(e) = engine_state.set_current_working_dir(cwd) {
                 let working_set = StateWorkingSet::new(engine_state);
                 report_error(&working_set, &e);
             }

src/test_bins.rs

@@ -188,7 +188,11 @@ pub fn nu_repl() {
 
         // Before doing anything, merge the environment from the previous REPL iteration into the
         // permanent state.
-        if let Err(err) = engine_state.merge_env(&mut stack, &cwd) {
+        if let Err(err) = engine_state.merge_env(&mut stack) {
+            outcome_err(&engine_state, &err);
+        }
+
+        if let Err(err) = engine_state.set_current_working_dir(&cwd) {
             outcome_err(&engine_state, &err);
         }