Make predeclarations scoped; Add hiding tests

In some rare cases, the global predeclarations would clash, for example:

  > module spam { export def foo [] { "foo" } }; def foo [] { "bar" }

In the example, the `foo [] { "bar" }` would get predeclared first, then
the predeclaration would be overwritten and consumed by `foo [] {"foo"}`
inside the module, then when parsing the actual `foo [] { "bar" }`, it
would not find its predeclaration.

crates/nu-parser/src/parse_keywords.rs

@@ -110,8 +110,10 @@ pub fn parse_def(
                             *declaration = signature.into_block_command(block_id);
                         } else {
                             error = error.or_else(|| {
+                                // TODO: Add InternalError variant
                                 Some(ParseError::UnknownState(
-                                    "Could not define hidden command".into(),
+                                    "Internal error: Predeclaration failed to add declaration"
+                                        .into(),
                                     spans[1],
                                 ))
                             });
@@ -318,7 +320,7 @@ pub fn parse_module(
     spans: &[Span],
 ) -> (Statement, Option<ParseError>) {
     // TODO: Currently, module is closing over its parent scope (i.e., defs in the parent scope are
-    // visible and usable in this module's scope). We might want to disable that. How?
+    // visible and usable in this module's scope). We want to disable that for files.
 
     let mut error = None;
     let bytes = working_set.get_span_contents(spans[0]);

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

@@ -56,6 +56,7 @@ impl Visibility {
 #[derive(Debug)]
 pub struct ScopeFrame {
     pub vars: HashMap<Vec<u8>, VarId>,
+    predecls: HashMap<Vec<u8>, DeclId>, // temporary storage for predeclarations
     decls: HashMap<Vec<u8>, DeclId>,
     aliases: HashMap<Vec<u8>, Vec<Span>>,
     modules: HashMap<Vec<u8>, BlockId>,
@@ -66,6 +67,7 @@ impl ScopeFrame {
     pub fn new() -> Self {
         Self {
             vars: HashMap::new(),
+            predecls: HashMap::new(),
             decls: HashMap::new(),
             aliases: HashMap::new(),
             modules: HashMap::new(),
@@ -306,7 +308,6 @@ pub struct StateDelta {
     vars: Vec<Type>,              // indexed by VarId
     decls: Vec<Box<dyn Command>>, // indexed by DeclId
     blocks: Vec<Block>,           // indexed by BlockId
-    predecls: HashMap<Vec<u8>, DeclId>, // this should get erased after every def call
     pub scope: Vec<ScopeFrame>,
 }
 
@@ -340,7 +341,6 @@ impl<'a> StateWorkingSet<'a> {
                 file_contents: vec![],
                 vars: vec![],
                 decls: vec![],
-                predecls: HashMap::new(),
                 blocks: vec![],
                 scope: vec![ScopeFrame::new()],
             },
@@ -384,18 +384,25 @@ impl<'a> StateWorkingSet<'a> {
         self.delta.decls.push(decl);
         let decl_id = self.num_decls() - 1;
 
-        self.delta.predecls.insert(name, decl_id)
-    }
-
-    pub fn merge_predecl(&mut self, name: &[u8]) -> Option<DeclId> {
-        if let Some(decl_id) = self.delta.predecls.remove(name) {
         let scope_frame = self
             .delta
             .scope
             .last_mut()
             .expect("internal error: missing required scope frame");
 
+        scope_frame.predecls.insert(name, decl_id)
+    }
+
+    pub fn merge_predecl(&mut self, name: &[u8]) -> Option<DeclId> {
+        let scope_frame = self
+            .delta
+            .scope
+            .last_mut()
+            .expect("internal error: missing required scope frame");
+
+        if let Some(decl_id) = scope_frame.predecls.remove(name) {
             scope_frame.decls.insert(name.into(), decl_id);
+            scope_frame.visibility.use_id(&decl_id);
 
             return Some(decl_id);
         }
@@ -545,13 +552,13 @@ impl<'a> StateWorkingSet<'a> {
     pub fn find_decl(&self, name: &[u8]) -> Option<DeclId> {
         let mut visibility: Visibility = Visibility::new();
 
-        if let Some(decl_id) = self.delta.predecls.get(name) {
-            return Some(*decl_id);
-        }
-
         for scope in self.delta.scope.iter().rev() {
             visibility.append(&scope.visibility);
 
+            if let Some(decl_id) = scope.predecls.get(name) {
+                return Some(*decl_id);
+            }
+
             if let Some(decl_id) = scope.decls.get(name) {
                 return Some(*decl_id);
             }

src/tests.rs

@@ -399,6 +399,7 @@ fn module_import_uses_internal_command() -> TestResult {
     )
 }
 
+// TODO: Test the use/hide tests also as separate lines in REPL (i.e., with  merging the delta in between)
 #[test]
 fn hides_def() -> TestResult {
     fail_test(r#"def foo [] { "foo" }; hide foo; foo"#, not_found_msg())
@@ -500,7 +501,6 @@ fn def_twice_should_fail() -> TestResult {
     )
 }
 
-// TODO: This test fails if executed each command on a separate line in REPL
 #[test]
 fn use_import_after_hide() -> TestResult {
     run_test(
@@ -509,6 +509,22 @@ fn use_import_after_hide() -> TestResult {
     )
 }
 
+#[test]
+fn hide_shadowed_decl() -> TestResult {
+    run_test(
+        r#"module spam { export def foo [] { "bar" } }; def foo [] { "foo" }; do { use spam.foo; hide foo; foo }"#,
+        "foo",
+    )
+}
+
+#[test]
+fn hides_all_decls_within_scope() -> TestResult {
+    fail_test(
+        r#"module spam { export def foo [] { "bar" } }; def foo [] { "foo" }; use spam.foo; hide foo; foo"#,
+        not_found_msg(),
+    )
+}
+
 #[test]
 fn from_json_1() -> TestResult {
     run_test(r#"('{"name": "Fred"}' | from json).name"#, "Fred")