From b0775b3f1eca2bf98d0aaa01bd90daac9234a3f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Sun, 12 Feb 2023 19:48:51 +0200 Subject: [PATCH] Fix hidden env vars not being hidden in closures (#8055) # Description This one fixes env not being hidden inside closure, reported in the conversation under https://github.com/nushell/nushell/issues/6593 https://github.com/nushell/nushell/issues/6593 https://github.com/nushell/nushell/issues/7937 still persist. These seems a bit more involved and might need hidden env tracking also in the engine state... I'm not yet sure what's causing it. Also re-enables some env-related tests and removes unused Value clone. # User-Facing Changes Just a bugfix # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- .../nu-command/src/core_commands/hide_env.rs | 2 +- crates/nu-protocol/src/engine/stack.rs | 16 +++++------ src/tests/test_hiding.rs | 12 +------- tests/shell/environment/env.rs | 28 +++++++++++-------- 4 files changed, 27 insertions(+), 31 deletions(-) diff --git a/crates/nu-command/src/core_commands/hide_env.rs b/crates/nu-command/src/core_commands/hide_env.rs index 4a060706d1..9fa2715a96 100644 --- a/crates/nu-command/src/core_commands/hide_env.rs +++ b/crates/nu-command/src/core_commands/hide_env.rs @@ -45,7 +45,7 @@ impl Command for HideEnv { let ignore_errors = call.has_flag("ignore-errors"); for name in env_var_names { - if stack.remove_env_var(engine_state, &name.item).is_none() && !ignore_errors { + if !stack.remove_env_var(engine_state, &name.item) && !ignore_errors { let all_names: Vec = stack .get_env_var_names(engine_state) .iter() diff --git a/crates/nu-protocol/src/engine/stack.rs b/crates/nu-protocol/src/engine/stack.rs index 0f43218630..85e6c223aa 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -163,7 +163,7 @@ impl Stack { Stack { vars: captures.clone(), env_vars, - env_hidden: HashMap::new(), + env_hidden: self.env_hidden.clone(), active_overlays: self.active_overlays.clone(), recursion_count: self.recursion_count.to_owned(), profiling_config: self.profiling_config.clone(), @@ -189,7 +189,7 @@ impl Stack { Stack { vars, env_vars, - env_hidden: HashMap::new(), + env_hidden: self.env_hidden.clone(), active_overlays: self.active_overlays.clone(), recursion_count: self.recursion_count.to_owned(), profiling_config: self.profiling_config.clone(), @@ -348,12 +348,12 @@ impl Stack { false } - pub fn remove_env_var(&mut self, engine_state: &EngineState, name: &str) -> Option { + pub fn remove_env_var(&mut self, engine_state: &EngineState, name: &str) -> bool { for scope in self.env_vars.iter_mut().rev() { for active_overlay in self.active_overlays.iter().rev() { if let Some(env_vars) = scope.get_mut(active_overlay) { - if let Some(v) = env_vars.remove(name) { - return Some(v); + if env_vars.remove(name).is_some() { + return true; } } } @@ -361,7 +361,7 @@ impl Stack { for active_overlay in self.active_overlays.iter().rev() { if let Some(env_vars) = engine_state.env_vars.get(active_overlay) { - if let Some(val) = env_vars.get(name) { + if env_vars.get(name).is_some() { if let Some(env_hidden) = self.env_hidden.get_mut(active_overlay) { env_hidden.insert(name.into()); } else { @@ -369,12 +369,12 @@ impl Stack { .insert(active_overlay.into(), HashSet::from([name.into()])); } - return Some(val.clone()); + return true; } } } - None + false } pub fn has_env_overlay(&self, name: &str, engine_state: &EngineState) -> bool { diff --git a/src/tests/test_hiding.rs b/src/tests/test_hiding.rs index 3455ed81fb..224791787f 100644 --- a/src/tests/test_hiding.rs +++ b/src/tests/test_hiding.rs @@ -173,23 +173,13 @@ fn hide_env_twice_allowed() -> TestResult { } #[test] -#[ignore = "Re-enable after virtualenv update"] -fn hides_def_runs_env_1() -> TestResult { +fn hides_def_runs_env() -> TestResult { run_test( r#"let-env foo = "bar"; def foo [] { "foo" }; hide foo; $env.foo"#, "bar", ) } -#[test] -#[ignore = "Re-enable after virtualenv update"] -fn hides_def_runs_env_2() -> TestResult { - run_test( - r#"def foo [] { "foo" }; let-env foo = "bar"; hide foo; $env.foo"#, - "bar", - ) -} - #[test] fn hides_alias_runs_def_1() -> TestResult { run_test( diff --git a/tests/shell/environment/env.rs b/tests/shell/environment/env.rs index 0cad3caed2..c73556d00e 100644 --- a/tests/shell/environment/env.rs +++ b/tests/shell/environment/env.rs @@ -3,6 +3,7 @@ use super::support::Trusted; use nu_test_support::fs::Stub::FileWithContent; use nu_test_support::nu; use nu_test_support::playground::Playground; +use nu_test_support::{nu_repl_code, pipeline}; use serial_test::serial; @@ -106,17 +107,6 @@ fn load_env_pwd_env_var_fails() { assert!(actual.err.contains("automatic_env_var_set_manually")); } -// FIXME: for some reason Nu is attempting to execute foo in `let-env FOO = foo` -#[ignore] -#[test] -fn passes_let_env_env_var_to_external_process() { - let actual = nu!(cwd: ".", r#" - let-env FOO = foo - nu --testbin echo_env FOO - "#); - assert_eq!(actual.out, "foo"); -} - #[test] fn passes_with_env_env_var_to_external_process() { let actual = nu!(cwd: ".", r#" @@ -158,3 +148,19 @@ fn passes_env_from_local_cfg_to_external_process() { assert_eq!(actual.out, "foo"); }) } + +#[test] +fn hides_env_in_block() { + let inp = &[ + "let-env foo = 'foo'", + "hide-env foo", + "let b = {|| $env.foo }", + "do $b", + ]; + + let actual = nu!(cwd: "tests/shell/environment", pipeline(&inp.join("; "))); + let actual_repl = nu!(cwd: "tests/shell/environment", nu_repl_code(inp)); + + assert!(actual.err.contains("column_not_found")); + assert!(actual_repl.err.contains("column_not_found")); +}