Fix hiding logic; Fix hiding with predecls

* Hiding logic is simplified and fixed so you can hide and unhide the
  same def repeatedly.
* Separates predeclared ids into its own data structure to protect them
  from hiding. Otherwise, you could hide the predeclared variable and
  the actual def would panic.

crates/nu-parser/src/parse_keywords.rs

@@ -42,7 +42,7 @@ pub fn parse_def_predecl(working_set: &mut StateWorkingSet, spans: &[Span]) {
             signature.name = name;
             let decl = signature.predeclare();
 
-            working_set.add_decl(decl);
+            working_set.add_predecl(decl);
         }
     }
 }
@@ -95,15 +95,15 @@ pub fn parse_def(
                     call.positional.push(block);
 
                     if let (Some(name), Some(mut signature), Some(block_id)) =
-                        (name, signature, block_id)
+                        (&name, signature, block_id)
                     {
                         let decl_id = working_set
-                            .find_decl(name.as_bytes())
+                            .find_predecl(name.as_bytes())
                             .expect("internal error: predeclaration failed to add definition");
 
                         let declaration = working_set.get_decl_mut(decl_id);
 
-                        signature.name = name;
+                        signature.name = name.clone();
 
                         *declaration = signature.into_block_command(block_id);
                     }
@@ -118,6 +118,19 @@ pub fn parse_def(
                 }
                 working_set.exit_scope();
 
+                if let Some(name) = name {
+                    // It's OK if it returns None: The decl was already merged in previous parse
+                    // pass.
+                    working_set.merge_predecl(name.as_bytes());
+                } else {
+                    error = error.or_else(|| {
+                        Some(ParseError::UnknownState(
+                            "Could not get string from string expression".into(),
+                            *name_span,
+                        ))
+                    });
+                }
+
                 call
             } else {
                 let err_span = Span {
@@ -581,18 +594,10 @@ pub fn parse_hide(
         let name_bytes: Vec<u8> = working_set.get_span_contents(spans[1]).into();
 
         // TODO: Do the import pattern stuff for bulk-hiding
-        // TODO: move this error into error = error.or pattern
-        let _decl_id = if let Some(id) = working_set.find_decl(&name_bytes) {
-            id
-        } else {
-            return (
-                garbage_statement(spans),
-                Some(ParseError::UnknownCommand(spans[1])),
-            );
-        };
 
-        // Hide the definitions
+        if working_set.hide_decl(&name_bytes).is_none() {
-        working_set.hide_decl(name_bytes);
+            error = error.or_else(|| Some(ParseError::UnknownCommand(spans[1])));
+        }
 
         // Create the Hide command call
         let hide_decl_id = working_set

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

@@ -1,7 +1,10 @@
 use super::Command;
 use crate::{ast::Block, BlockId, DeclId, Span, Type, VarId};
 use core::panic;
-use std::{collections::HashMap, slice::Iter};
+use std::{
+    collections::{HashMap, HashSet},
+    slice::Iter,
+};
 
 pub struct EngineState {
     files: Vec<(String, usize, usize)>,
@@ -18,7 +21,7 @@ pub struct ScopeFrame {
     decls: HashMap<Vec<u8>, DeclId>,
     aliases: HashMap<Vec<u8>, Vec<Span>>,
     modules: HashMap<Vec<u8>, BlockId>,
-    hiding: HashMap<Vec<u8>, usize>, // defines what is being hidden and its "hiding strength"
+    hiding: HashSet<DeclId>,
 }
 
 impl ScopeFrame {
@@ -28,7 +31,7 @@ impl ScopeFrame {
             decls: HashMap::new(),
             aliases: HashMap::new(),
             modules: HashMap::new(),
-            hiding: HashMap::new(),
+            hiding: HashSet::new(),
         }
     }
 
@@ -84,7 +87,7 @@ impl EngineState {
                 last.modules.insert(item.0, item.1);
             }
             for item in first.hiding.into_iter() {
-                last.hiding.insert(item.0, item.1);
+                last.hiding.insert(item);
             }
         }
     }
@@ -129,21 +132,13 @@ impl EngineState {
     }
 
     pub fn find_decl(&self, name: &[u8]) -> Option<DeclId> {
-        let mut hiding_strength = 0;
+        let mut hiding: HashSet<DeclId> = HashSet::new();
-        // println!("state: starting finding {}", String::from_utf8_lossy(&name));
 
         for scope in self.scope.iter().rev() {
-            // println!("hiding map: {:?}", scope.hiding);
+            hiding.extend(&scope.hiding);
-            // check if we're hiding the declin this scope
-            if let Some(strength) = scope.hiding.get(name) {
-                hiding_strength += strength;
-            }
 
             if let Some(decl_id) = scope.decls.get(name) {
-                // if we're hiding this decl, do not return it and reduce the hiding strength
+                if !hiding.contains(decl_id) {
-                if hiding_strength > 0 {
-                    hiding_strength -= 1;
-                } else {
                     return Some(*decl_id);
                 }
             }
@@ -242,9 +237,10 @@ pub struct StateWorkingSet<'a> {
 pub struct StateDelta {
     files: Vec<(String, usize, usize)>,
     pub(crate) file_contents: Vec<u8>,
-    vars: Vec<Type>,              // indexed by VarId
+    vars: Vec<Type>,                    // indexed by VarId
-    decls: Vec<Box<dyn Command>>, // indexed by DeclId
+    decls: Vec<Box<dyn Command>>,       // indexed by DeclId
-    blocks: Vec<Block>,           // indexed by BlockId
+    blocks: Vec<Block>,                 // indexed by BlockId
+    predecls: HashMap<Vec<u8>, DeclId>, // this should get erased after every def call
     pub scope: Vec<ScopeFrame>,
 }
 
@@ -262,12 +258,10 @@ impl StateDelta {
     }
 
     pub fn enter_scope(&mut self) {
-        // println!("enter scope");
         self.scope.push(ScopeFrame::new());
     }
 
     pub fn exit_scope(&mut self) {
-        // println!("exit scope");
         self.scope.pop();
     }
 }
@@ -280,6 +274,7 @@ impl<'a> StateWorkingSet<'a> {
                 file_contents: vec![],
                 vars: vec![],
                 decls: vec![],
+                predecls: HashMap::new(),
                 blocks: vec![],
                 scope: vec![ScopeFrame::new()],
             },
@@ -301,7 +296,6 @@ impl<'a> StateWorkingSet<'a> {
 
     pub fn add_decl(&mut self, decl: Box<dyn Command>) -> DeclId {
         let name = decl.name().as_bytes().to_vec();
-        // println!("adding {}", String::from_utf8_lossy(&name));
 
         self.delta.decls.push(decl);
         let decl_id = self.num_decls() - 1;
@@ -312,33 +306,63 @@ impl<'a> StateWorkingSet<'a> {
             .last_mut()
             .expect("internal error: missing required scope frame");
 
-        // reset "hiding strength" to 0 => not hidden
-        if let Some(strength) = scope_frame.hiding.get_mut(&name) {
-            *strength = 0;
-            // println!(" strength: {}", strength);
-        }
-
         scope_frame.decls.insert(name, decl_id);
 
         decl_id
     }
 
-    pub fn hide_decl(&mut self, name: Vec<u8>) {
+    pub fn add_predecl(&mut self, decl: Box<dyn Command>) {
-        let scope_frame = self
+        let name = decl.name().as_bytes().to_vec();
+
+        self.delta.decls.push(decl);
+        let decl_id = self.num_decls() - 1;
+
+        self.delta.predecls.insert(name, decl_id);
+    }
+
+    pub fn find_predecl(&mut self, name: &[u8]) -> Option<DeclId> {
+        self.delta.predecls.get(name).copied()
+    }
+
+    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.decls.insert(name.into(), decl_id);
+
+            return Some(decl_id);
+        }
+
+        None
+    }
+
+    pub fn hide_decl(&mut self, name: &[u8]) -> Option<DeclId> {
+        // Since we can mutate scope frames in delta, remove the id directly
+        for scope in self.delta.scope.iter_mut().rev() {
+            if let Some(decl_id) = scope.decls.remove(name) {
+                return Some(decl_id);
+            }
+        }
+
+        // We cannot mutate the permanent state => store the information in the current scope frame
+        let last_scope_frame = self
             .delta
             .scope
             .last_mut()
             .expect("internal error: missing required scope frame");
 
-        if let Some(strength) = scope_frame.hiding.get_mut(&name) {
+        for scope in self.permanent_state.scope.iter().rev() {
-            *strength += 1;
+            if let Some(decl_id) = scope.decls.get(name) {
-            // println!("hiding {}, strength: {}", String::from_utf8_lossy(&name), strength);
+                last_scope_frame.hiding.insert(*decl_id);
-        } else {
+                return Some(*decl_id);
-            // println!("hiding {}, strength: 1", String::from_utf8_lossy(&name));
+            }
-            scope_frame.hiding.insert(name, 1);
         }
 
-        // println!("hiding map: {:?}", scope_frame.hiding);
+        None
     }
 
     pub fn add_block(&mut self, block: Block) -> BlockId {
@@ -448,46 +472,25 @@ impl<'a> StateWorkingSet<'a> {
     }
 
     pub fn find_decl(&self, name: &[u8]) -> Option<DeclId> {
-        let mut hiding_strength = 0;
+        let mut hiding: HashSet<DeclId> = HashSet::new();
-        // println!("set: starting finding {}", String::from_utf8_lossy(&name));
+
+        if let Some(decl_id) = self.delta.predecls.get(name) {
+            return Some(*decl_id);
+        }
 
         for scope in self.delta.scope.iter().rev() {
-            // println!("delta frame");
+            hiding.extend(&scope.hiding);
-            // println!("hiding map: {:?}", scope.hiding);
-            // check if we're hiding the declin this scope
-            if let Some(strength) = scope.hiding.get(name) {
-                hiding_strength += strength;
-                // println!(" was hiding, strength {}", hiding_strength);
-            }
 
             if let Some(decl_id) = scope.decls.get(name) {
-                // if we're hiding this decl, do not return it and reduce the hiding strength
+                return Some(*decl_id);
-                if hiding_strength > 0 {
-                    hiding_strength -= 1;
-                    // println!(" decl found, strength {}", hiding_strength);
-                } else {
-                    // println!(" decl found, return");
-                    return Some(*decl_id);
-                }
             }
         }
 
         for scope in self.permanent_state.scope.iter().rev() {
-            // println!("perma frame");
+            hiding.extend(&scope.hiding);
-            // println!("hiding map: {:?}", scope.hiding);
-            // check if we're hiding the declin this scope
-            if let Some(strength) = scope.hiding.get(name) {
-                hiding_strength += strength;
-                // println!(" was hiding, strength {}", hiding_strength);
-            }
 
             if let Some(decl_id) = scope.decls.get(name) {
-                // if we're hiding this decl, do not return it and reduce the hiding strength
+                if !hiding.contains(decl_id) {
-                if hiding_strength > 0 {
-                    hiding_strength -= 1;
-                    // println!(" decl found, strength {}", hiding_strength);
-                } else {
-                    // println!(" decl found, return");
                     return Some(*decl_id);
                 }
             }