From 7f758d3e51aa901dd427a25460a9f8431f803c71 Mon Sep 17 00:00:00 2001 From: Maxim Zhiburt Date: Tue, 30 May 2023 03:03:00 +0300 Subject: [PATCH] Merge stack before printing (#9304) Could you @fdncred try it? close?: #9264 --------- Signed-off-by: Maxim Zhiburt Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com> --- crates/nu-cli/src/config_files.rs | 7 +++++- crates/nu-cli/src/print.rs | 6 +++++ crates/nu-cli/src/repl.rs | 13 ++++++---- crates/nu-cli/src/util.rs | 4 ++++ crates/nu-cli/tests/completions.rs | 3 ++- .../tests/support/completions_helpers.rs | 13 ++++++---- crates/nu-cmd-lang/src/example_support.rs | 6 ++++- crates/nu-command/src/hook.rs | 4 +++- crates/nu-command/src/viewers/table.rs | 6 +++++ crates/nu-protocol/src/engine/engine_state.rs | 23 +++++++++++++----- crates/nu-std/src/lib.rs | 3 ++- src/config_files.rs | 24 +++++++++---------- src/test_bins.rs | 6 ++++- 13 files changed, 84 insertions(+), 34 deletions(-) diff --git a/crates/nu-cli/src/config_files.rs b/crates/nu-cli/src/config_files.rs index 5199a1b051..0a2910bfa3 100644 --- a/crates/nu-cli/src/config_files.rs +++ b/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); } diff --git a/crates/nu-cli/src/print.rs b/crates/nu-cli/src/print.rs index 1a6d7d146f..385770b75a 100644 --- a/crates/nu-cli/src/print.rs +++ b/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 { diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 8088d51a0d..13837880bb 100644 --- a/crates/nu-cli/src/repl.rs +++ b/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, diff --git a/crates/nu-cli/src/util.rs b/crates/nu-cli/src/util.rs index 9cdbdf1573..febd39a181 100644 --- a/crates/nu-cli/src/util.rs +++ b/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 { diff --git a/crates/nu-cli/tests/completions.rs b/crates/nu-cli/tests/completions.rs index d6608975f4..f2ea030909 100644 --- a/crates/nu-cli/tests/completions.rs +++ b/crates/nu-cli/tests/completions.rs @@ -768,7 +768,8 @@ fn run_external_completion(block: &str, input: &str) -> Vec { 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; diff --git a/crates/nu-cli/tests/support/completions_helpers.rs b/crates/nu-cli/tests/support/completions_helpers.rs index 43f79a4d9f..5a0f3f974a 100644 --- a/crates/nu-cli/tests/support/completions_helpers.rs +++ b/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(()) } diff --git a/crates/nu-cmd-lang/src/example_support.rs b/crates/nu-cmd-lang/src/example_support.rs index c531719715..a34a958cc8 100644 --- a/crates/nu-cmd-lang/src/example_support.rs +++ b/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); diff --git a/crates/nu-command/src/hook.rs b/crates/nu-command/src/hook.rs index 40b9a77a03..7ab366f69e 100644 --- a/crates/nu-command/src/hook.rs +++ b/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) } diff --git a/crates/nu-command/src/viewers/table.rs b/crates/nu-command/src/viewers/table.rs index 82e734adfe..5c4737b21e 100644 --- a/crates/nu-command/src/viewers/table.rs +++ b/crates/nu-command/src/viewers/table.rs @@ -102,6 +102,12 @@ impl Command for Table { call: &Call, input: PipelineData, ) -> Result { + // 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 = 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"); diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index cb7456b1f8..002f1b8fa1 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/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, - ) -> 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) -> 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 { + 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 { diff --git a/crates/nu-std/src/lib.rs b/crates/nu-std/src/lib.rs index bb3db92d36..2bf1b26910 100644 --- a/crates/nu-std/src/lib.rs +++ b/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(()) } diff --git a/src/config_files.rs b/src/config_files.rs index 3793e4e570..635a791894 100644 --- a/src/config_files.rs +++ b/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); } diff --git a/src/test_bins.rs b/src/test_bins.rs index 3f14519ca2..6fedeb4c92 100644 --- a/src/test_bins.rs +++ b/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); }