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.

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<String> = stack
                     .get_env_var_names(engine_state)
                     .iter()

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<Value> {
+    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) {
+                    if env_vars.remove(name).is_some() {
-                        return Some(v);
+                        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 {

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() -> TestResult {
-fn hides_def_runs_env_1() -> 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(

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