Hiding of environment variables (#362)

* Remember environment variables from previous scope

* Re-introduce env var hiding

Right now, hiding decls is broken

* Re-introduce hidden field of import patterns

All tests pass now.

* Remove/Address tests TODOs

* Fix test typo; Report hiding error

* Add a few more tests

* Fix wrong expected test result

crates/nu-command/src/core_commands/hide.rs

@@ -1,6 +1,6 @@
-use nu_protocol::ast::Call;
+use nu_protocol::ast::{Call, Expr, Expression, ImportPatternMember};
 use nu_protocol::engine::{Command, EngineState, Stack};
-use nu_protocol::{Category, PipelineData, Signature, SyntaxShape};
+use nu_protocol::{Category, PipelineData, ShellError, Signature, SyntaxShape};
 
 #[derive(Clone)]
 pub struct Hide;
@@ -20,13 +20,94 @@ impl Command for Hide {
         "Hide definitions in the current scope"
     }
 
+    fn extra_usage(&self) -> &str {
+        "If there is a definition and an environment variable with the same name in the current scope, first the definition will be hidden, then the environment variable."
+    }
+
     fn run(
         &self,
-        _engine_state: &EngineState,
-        _stack: &mut Stack,
+        engine_state: &EngineState,
+        stack: &mut Stack,
         call: &Call,
         _input: PipelineData,
     ) -> Result<nu_protocol::PipelineData, nu_protocol::ShellError> {
+        let import_pattern = if let Some(Expression {
+            expr: Expr::ImportPattern(pat),
+            ..
+        }) = call.positional.get(0)
+        {
+            pat
+        } else {
+            return Err(ShellError::InternalError(
+                "Got something else than import pattern".into(),
+            ));
+        };
+
+        let head_name_str = if let Ok(s) = String::from_utf8(import_pattern.head.name.clone()) {
+            s
+        } else {
+            return Err(ShellError::NonUtf8(import_pattern.head.span));
+        };
+
+        if let Some(overlay_id) = engine_state.find_overlay(&import_pattern.head.name) {
+            // The first word is a module
+            let overlay = engine_state.get_overlay(overlay_id);
+
+            let env_vars_to_hide = if import_pattern.members.is_empty() {
+                overlay.env_vars_with_head(&import_pattern.head.name)
+            } else {
+                match &import_pattern.members[0] {
+                    ImportPatternMember::Glob { .. } => {
+                        overlay.env_vars_with_head(&import_pattern.head.name)
+                    }
+                    ImportPatternMember::Name { name, span } => {
+                        let mut output = vec![];
+
+                        if let Some((name, id)) =
+                            overlay.env_var_with_head(name, &import_pattern.head.name)
+                        {
+                            output.push((name, id));
+                        } else if !overlay.has_decl(name) {
+                            return Err(ShellError::EnvVarNotFoundAtRuntime(*span));
+                        }
+
+                        output
+                    }
+                    ImportPatternMember::List { names } => {
+                        let mut output = vec![];
+
+                        for (name, span) in names {
+                            if let Some((name, id)) =
+                                overlay.env_var_with_head(name, &import_pattern.head.name)
+                            {
+                                output.push((name, id));
+                            } else if !overlay.has_decl(name) {
+                                return Err(ShellError::EnvVarNotFoundAtRuntime(*span));
+                            }
+                        }
+
+                        output
+                    }
+                }
+            };
+
+            for (name, _) in env_vars_to_hide {
+                let name = if let Ok(s) = String::from_utf8(name.clone()) {
+                    s
+                } else {
+                    return Err(ShellError::NonUtf8(import_pattern.span()));
+                };
+
+                if stack.remove_env_var(&name).is_none() {
+                    return Err(ShellError::NotFound(call.positional[0].span));
+                }
+            }
+        } else if !import_pattern.hidden.contains(&import_pattern.head.name)
+            && stack.remove_env_var(&head_name_str).is_none()
+        {
+            return Err(ShellError::NotFound(call.positional[0].span));
+        }
+
         Ok(PipelineData::new(call.head))
     }
 }

crates/nu-command/src/env/with_env.rs

@@ -163,10 +163,10 @@ fn with_env(
     for (k, v) in env {
         match v {
             EnvVar::Nothing => {
-                stack.env_vars.remove(&k);
+                stack.remove_env_var(&k);
             }
             EnvVar::Proper(s) => {
-                stack.env_vars.insert(k, s);
+                stack.add_env_var(k, s);
             }
         }
     }

crates/nu-engine/src/eval.rs

@@ -468,9 +468,9 @@ pub fn eval_variable(
         let mut output_cols = vec![];
         let mut output_vals = vec![];
 
-        let env_columns: Vec<_> = stack.get_env_vars().keys().map(|x| x.to_string()).collect();
-        let env_values: Vec<_> = stack
-            .get_env_vars()
+        let env_vars = stack.get_env_vars();
+        let env_columns: Vec<_> = env_vars.keys().map(|x| x.to_string()).collect();
+        let env_values: Vec<_> = env_vars
             .values()
             .map(|x| Value::String {
                 val: x.to_string(),

crates/nu-parser/src/parse_keywords.rs

@@ -6,7 +6,7 @@ use nu_protocol::{
     engine::StateWorkingSet,
     span, Exportable, Overlay, Span, SyntaxShape, Type, CONFIG_VARIABLE_ID,
 };
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::path::Path;
 
 #[cfg(feature = "plugin")]
@@ -711,6 +711,7 @@ pub fn parse_use(
                                     span: spans[1],
                                 },
                                 members: import_pattern.members,
+                                hidden: HashSet::new(),
                             },
                             overlay,
                         )
@@ -836,11 +837,14 @@ pub fn parse_hide(
                         },
                     )
                 } else {
-                    // TODO: Or it could be an env var
-                    return (
-                        garbage_statement(spans),
-                        Some(ParseError::ModuleNotFound(spans[1])),
-                    );
+                    // Or it could be an env var
+                    (
+                        false,
+                        Overlay {
+                            decls: HashMap::new(),
+                            env_vars: HashMap::new(),
+                        },
+                    )
                 }
             } else {
                 return (
@@ -893,6 +897,8 @@ pub fn parse_hide(
         // TODO: `use spam; use spam foo; hide foo` will hide both `foo` and `spam foo` since
         // they point to the same DeclId. Do we want to keep it that way?
         working_set.hide_decls(&decls_to_hide);
+        let import_pattern = import_pattern
+            .with_hidden(decls_to_hide.iter().map(|(name, _)| name.clone()).collect());
 
         // Create the Hide command call
         let hide_decl_id = working_set

crates/nu-parser/src/parser.rs

@@ -20,6 +20,8 @@ use crate::parse_keywords::{
     parse_alias, parse_def, parse_def_predecl, parse_hide, parse_let, parse_module, parse_use,
 };
 
+use std::collections::HashSet;
+
 #[cfg(feature = "plugin")]
 use crate::parse_keywords::parse_plugin;
 
@@ -1807,6 +1809,7 @@ pub fn parse_import_pattern(
                     span: Span::unknown(),
                 },
                 members: vec![],
+                hidden: HashSet::new(),
             },
             Some(ParseError::WrongImportPattern(span(spans))),
         );
@@ -1823,6 +1826,7 @@ pub fn parse_import_pattern(
                         span: *head_span,
                     },
                     members: vec![ImportPatternMember::Glob { span: *tail_span }],
+                    hidden: HashSet::new(),
                 },
                 error,
             )
@@ -1850,6 +1854,7 @@ pub fn parse_import_pattern(
                                 span: *head_span,
                             },
                             members: vec![ImportPatternMember::List { names: output }],
+                            hidden: HashSet::new(),
                         },
                         error,
                     )
@@ -1861,6 +1866,7 @@ pub fn parse_import_pattern(
                             span: *head_span,
                         },
                         members: vec![],
+                        hidden: HashSet::new(),
                     },
                     Some(ParseError::ExportNotFound(result.span)),
                 ),
@@ -1877,6 +1883,7 @@ pub fn parse_import_pattern(
                         name: tail.to_vec(),
                         span: *tail_span,
                     }],
+                    hidden: HashSet::new(),
                 },
                 error,
             )
@@ -1889,6 +1896,7 @@ pub fn parse_import_pattern(
                     span: *head_span,
                 },
                 members: vec![],
+                hidden: HashSet::new(),
             },
             None,
         )

crates/nu-protocol/src/ast/import_pattern.rs

@@ -1,4 +1,5 @@
 use crate::{span, Span};
+use std::collections::HashSet;
 
 #[derive(Debug, Clone)]
 pub enum ImportPatternMember {
@@ -17,6 +18,9 @@ pub struct ImportPatternHead {
 pub struct ImportPattern {
     pub head: ImportPatternHead,
     pub members: Vec<ImportPatternMember>,
+    // communicate to eval which decls/aliases were hidden during `parse_hide()` so it does not
+    // interpret these as env var names:
+    pub hidden: HashSet<Vec<u8>>,
 }
 
 impl ImportPattern {
@@ -37,4 +41,12 @@ impl ImportPattern {
 
         span(&spans)
     }
+
+    pub fn with_hidden(self, hidden: HashSet<Vec<u8>>) -> Self {
+        ImportPattern {
+            head: self.head,
+            members: self.members,
+            hidden,
+        }
+    }
 }

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

@@ -21,8 +21,10 @@ use crate::{Config, ShellError, Value, VarId, CONFIG_VARIABLE_ID};
 /// use the Stack as a way of representing the local and closure-captured state.
 #[derive(Debug, Clone)]
 pub struct Stack {
+    /// Variables
     pub vars: HashMap<VarId, Value>,
-    pub env_vars: HashMap<String, String>,
+    /// Environment variables arranged as a stack to be able to recover values from parent scopes
+    pub env_vars: Vec<HashMap<String, String>>,
 }
 
 impl Default for Stack {
@@ -35,7 +37,7 @@ impl Stack {
     pub fn new() -> Stack {
         Stack {
             vars: HashMap::new(),
-            env_vars: HashMap::new(),
+            env_vars: vec![],
         }
     }
 
@@ -52,7 +54,11 @@ impl Stack {
     }
 
     pub fn add_env_var(&mut self, var: String, value: String) {
-        self.env_vars.insert(var, value);
+        if let Some(scope) = self.env_vars.last_mut() {
+            scope.insert(var, value);
+        } else {
+            self.env_vars.push(HashMap::from([(var, value)]));
+        }
     }
 
     pub fn collect_captures(&self, captures: &[VarId]) -> Stack {
@@ -68,6 +74,7 @@ impl Stack {
 
         // FIXME: this is probably slow
         output.env_vars = self.env_vars.clone();
+        output.env_vars.push(HashMap::new());
 
         let config = self
             .get_var(CONFIG_VARIABLE_ID)
@@ -77,19 +84,35 @@ impl Stack {
         output
     }
 
+    /// Flatten the env var scope frames into one frame
     pub fn get_env_vars(&self) -> HashMap<String, String> {
-        self.env_vars.clone()
+        let mut result = HashMap::new();
+
+        for scope in &self.env_vars {
+            result.extend(scope.clone());
+        }
+
+        result
     }
 
     pub fn get_env_var(&self, name: &str) -> Option<String> {
-        if let Some(v) = self.env_vars.get(name) {
-            return Some(v.to_string());
+        for scope in self.env_vars.iter().rev() {
+            if let Some(v) = scope.get(name) {
+                return Some(v.to_string());
+            }
         }
+
         None
     }
 
     pub fn remove_env_var(&mut self, name: &str) -> Option<String> {
-        self.env_vars.remove(name)
+        for scope in self.env_vars.iter_mut().rev() {
+            if let Some(v) = scope.remove(name) {
+                return Some(v);
+            }
+        }
+
+        None
     }
 
     pub fn get_config(&self) -> Result<Config, ShellError> {
@@ -109,9 +132,11 @@ impl Stack {
         for (var, val) in &self.vars {
             println!("  {}: {:?}", var, val);
         }
-        println!("env vars:");
-        for (var, val) in &self.env_vars {
-            println!("  {}: {:?}", var, val);
+        for (i, scope) in self.env_vars.iter().rev().enumerate() {
+            println!("env vars, scope {} (from the last);", i);
+            for (var, val) in scope {
+                println!("  {}: {:?}", var, val);
+            }
         }
     }
 }

src/main.rs

@@ -126,7 +126,7 @@ fn main() -> Result<()> {
         let mut stack = nu_protocol::engine::Stack::new();
 
         for (k, v) in std::env::vars() {
-            stack.env_vars.insert(k, v);
+            stack.add_env_var(k, v);
         }
 
         // Set up our initial config to start from
@@ -170,7 +170,7 @@ fn main() -> Result<()> {
         let mut stack = nu_protocol::engine::Stack::new();
 
         for (k, v) in std::env::vars() {
-            stack.env_vars.insert(k, v);
+            stack.add_env_var(k, v);
         }
 
         // Set up our initial config to start from

src/tests.rs

@@ -474,6 +474,22 @@ fn module_env_imports_5() -> TestResult {
     )
 }
 
+#[test]
+fn module_def_and_env_imports_1() -> TestResult {
+    run_test(
+        r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; $nu.env.foo"#,
+        "foo",
+    )
+}
+
+#[test]
+fn module_def_and_env_imports_2() -> TestResult {
+    run_test(
+        r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; foo"#,
+        "bar",
+    )
+}
+
 #[test]
 fn module_def_import_uses_internal_command() -> TestResult {
     run_test(
@@ -496,7 +512,7 @@ fn hides_def() -> TestResult {
     fail_test(r#"def foo [] { "foo" }; hide foo; foo"#, not_found_msg())
 }
 
-#[ignore]
+#[test]
 fn hides_env() -> TestResult {
     fail_test(
         r#"let-env foo = "foo"; hide foo; $nu.env.foo"#,
@@ -514,7 +530,7 @@ fn hides_def_then_redefines() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_then_redefines() -> TestResult {
     run_test(
         r#"let-env foo = "foo"; hide foo; let-env foo = "bar"; $nu.env.foo"#,
@@ -554,7 +570,7 @@ fn hides_def_in_scope_4() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_in_scope_1() -> TestResult {
     fail_test(
         r#"let-env foo = "foo"; do { hide foo; $nu.env.foo }"#,
@@ -562,16 +578,15 @@ fn hides_env_in_scope_1() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_in_scope_2() -> TestResult {
-    // TODO: Revisit this -- 'hide foo' should restore the env, not hide it completely
     run_test(
         r#"let-env foo = "foo"; do { let-env foo = "bar"; hide foo; $nu.env.foo }"#,
         "foo",
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_in_scope_3() -> TestResult {
     fail_test(
         r#"let-env foo = "foo"; do { hide foo; let-env foo = "bar"; hide foo; $nu.env.foo }"#,
@@ -579,9 +594,8 @@ fn hides_env_in_scope_3() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_in_scope_4() -> TestResult {
-    // TODO: Revisit this -- 'hide foo' should restore the env, not hide it completely
     fail_test(
         r#"let-env foo = "foo"; do { let-env foo = "bar"; hide foo; hide foo; $nu.env.foo }"#,
         "did you mean",
@@ -590,31 +604,38 @@ fn hides_env_in_scope_4() -> TestResult {
 
 #[test]
 fn hide_def_twice_not_allowed() -> TestResult {
-    fail_test(r#"def foo [] { "foo" }; hide foo; hide foo"#, "not found")
+    fail_test(
+        r#"def foo [] { "foo" }; hide foo; hide foo"#,
+        "did not find",
+    )
 }
 
-#[ignore]
+#[test]
 fn hide_env_twice_not_allowed() -> TestResult {
     fail_test(r#"let-env foo = "foo"; hide foo; hide foo"#, "did not find")
 }
 
-#[ignore]
-fn hides_def_runs_env() -> TestResult {
-    // TODO: We need some precedence system to handle this. Currently, 'hide foo' hides both the
-    // def and env var.
+#[test]
+fn hides_def_runs_env_1() -> TestResult {
     run_test(
         r#"let-env foo = "bar"; def foo [] { "foo" }; hide foo; $nu.env.foo"#,
         "bar",
     )
 }
 
-#[ignore]
+#[test]
+fn hides_def_runs_env_2() -> TestResult {
+    run_test(
+        r#"def foo [] { "foo" }; let-env foo = "bar"; hide foo; $nu.env.foo"#,
+        "bar",
+    )
+}
+
+#[test]
 fn hides_def_and_env() -> TestResult {
-    // TODO: We need some precedence system to handle this. Currently, 'hide foo' hides both the
-    // def and env var.
     fail_test(
         r#"let-env foo = "bar"; def foo [] { "foo" }; hide foo; hide foo; $nu.env.foo"#,
-        not_found_msg(),
+        "did you mean",
     )
 }
 
@@ -666,7 +687,7 @@ fn hides_def_import_6() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_import_1() -> TestResult {
     fail_test(
         r#"module spam { export env foo { "foo" } }; use spam; hide spam foo; $nu.env.'spam foo'"#,
@@ -674,7 +695,7 @@ fn hides_env_import_1() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_import_2() -> TestResult {
     fail_test(
         r#"module spam { export env foo { "foo" } }; use spam; hide spam *; $nu.env.'spam foo'"#,
@@ -682,15 +703,15 @@ fn hides_env_import_2() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_import_3() -> TestResult {
     fail_test(
-        r#"module spam { export env foo { "foo" }; } use spam; hide spam [foo]; $nu.env.'spam foo'"#,
+        r#"module spam { export env foo { "foo" } }; use spam; hide spam [foo]; $nu.env.'spam foo'"#,
         "did you mean",
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_import_4() -> TestResult {
     fail_test(
         r#"module spam { export env foo { "foo" } }; use spam foo; hide foo; $nu.env.foo"#,
@@ -698,7 +719,7 @@ fn hides_env_import_4() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_import_5() -> TestResult {
     fail_test(
         r#"module spam { export env foo { "foo" } }; use spam *; hide foo; $nu.env.foo"#,
@@ -706,7 +727,7 @@ fn hides_env_import_5() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hides_env_import_6() -> TestResult {
     fail_test(
         r#"module spam { export env foo { "foo" } }; use spam; hide spam; $nu.env.'spam foo'"#,
@@ -714,6 +735,30 @@ fn hides_env_import_6() -> TestResult {
     )
 }
 
+#[test]
+fn hides_def_runs_env_import() -> TestResult {
+    run_test(
+        r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; hide foo; $nu.env.foo"#,
+        "foo",
+    )
+}
+
+#[test]
+fn hides_def_and_env_import_1() -> TestResult {
+    fail_test(
+        r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; hide foo; hide foo; $nu.env.foo"#,
+        "did you mean",
+    )
+}
+
+#[test]
+fn hides_def_and_env_import_2() -> TestResult {
+    fail_test(
+        r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; hide foo; hide foo; foo"#,
+        not_found_msg(),
+    )
+}
+
 #[test]
 fn def_twice_should_fail() -> TestResult {
     fail_test(
@@ -730,7 +775,7 @@ fn use_def_import_after_hide() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn use_env_import_after_hide() -> TestResult {
     run_test(
         r#"module spam { export env foo { "foo" } }; use spam foo; hide foo; use spam foo; $nu.env.foo"#,
@@ -746,9 +791,8 @@ fn hide_shadowed_decl() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hide_shadowed_env() -> TestResult {
-    // TODO: waiting for a fix
     run_test(
         r#"module spam { export env foo { "bar" } }; let-env foo = "foo"; do { use spam foo; hide foo; $nu.env.foo }"#,
         "foo",
@@ -763,7 +807,7 @@ fn hides_all_decls_within_scope() -> TestResult {
     )
 }
 
-#[ignore]
+#[test]
 fn hides_all_envs_within_scope() -> TestResult {
     fail_test(
         r#"module spam { export env foo { "bar" } }; let-env foo = "foo"; use spam foo; hide foo; $nu.env.foo"#,