diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 3abad8e122..05fe97e5eb 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/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) { // 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]); diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index c93c0ace7d..830cd65697 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -1,10 +1,7 @@ use super::Command; use crate::{ast::Block, BlockId, DeclId, Example, Signature, Span, Type, VarId}; use core::panic; -use std::{ - collections::{HashMap, HashSet}, - slice::Iter, -}; +use std::{collections::HashMap, slice::Iter}; pub struct EngineState { files: Vec<(String, usize, usize)>, @@ -15,23 +12,66 @@ pub struct EngineState { pub scope: Vec, } +// Tells whether a decl etc. is visible or not +// TODO: When adding new exportables (env vars, aliases, etc.), parametrize the ID type with generics +#[derive(Debug, Clone)] +struct Visibility { + ids: HashMap, +} + +impl Visibility { + fn new() -> Self { + Visibility { + ids: HashMap::new(), + } + } + + fn is_id_visible(&self, id: &DeclId) -> bool { + *self.ids.get(id).unwrap_or(&true) // by default it's visible + } + + fn hide_id(&mut self, id: &DeclId) { + self.ids.insert(*id, false); + } + + fn use_id(&mut self, id: &DeclId) { + self.ids.insert(*id, true); + } + + fn merge_with(&mut self, other: Visibility) { + // overwrite own values with the other + self.ids.extend(other.ids); + } + + fn append(&mut self, other: &Visibility) { + // take new values from other but keep own values + for (id, visible) in other.ids.iter() { + if !self.ids.contains_key(id) { + self.ids.insert(*id, *visible); + } + } + } +} + #[derive(Debug)] pub struct ScopeFrame { pub vars: HashMap, VarId>, + predecls: HashMap, DeclId>, // temporary storage for predeclarations decls: HashMap, DeclId>, aliases: HashMap, Vec>, modules: HashMap, BlockId>, - hiding: HashSet, + visibility: Visibility, } impl ScopeFrame { pub fn new() -> Self { Self { vars: HashMap::new(), + predecls: HashMap::new(), decls: HashMap::new(), aliases: HashMap::new(), modules: HashMap::new(), - hiding: HashSet::new(), + visibility: Visibility::new(), } } @@ -86,9 +126,7 @@ impl EngineState { for item in first.modules.into_iter() { last.modules.insert(item.0, item.1); } - for item in first.hiding.into_iter() { - last.hiding.insert(item); - } + last.visibility.merge_with(first.visibility); } } @@ -132,13 +170,13 @@ impl EngineState { } pub fn find_decl(&self, name: &[u8]) -> Option { - let mut hiding: HashSet = HashSet::new(); + let mut visibility: Visibility = Visibility::new(); for scope in self.scope.iter().rev() { - hiding.extend(&scope.hiding); + visibility.append(&scope.visibility); if let Some(decl_id) = scope.decls.get(name) { - if !hiding.contains(decl_id) { + if visibility.is_id_visible(decl_id) { return Some(*decl_id); } } @@ -267,10 +305,9 @@ pub struct StateWorkingSet<'a> { pub struct StateDelta { files: Vec<(String, usize, usize)>, pub(crate) file_contents: Vec, - vars: Vec, // indexed by VarId - decls: Vec>, // indexed by DeclId - blocks: Vec, // indexed by BlockId - predecls: HashMap, DeclId>, // this should get erased after every def call + vars: Vec, // indexed by VarId + decls: Vec>, // indexed by DeclId + blocks: Vec, // indexed by BlockId pub scope: Vec, } @@ -304,7 +341,6 @@ impl<'a> StateWorkingSet<'a> { file_contents: vec![], vars: vec![], decls: vec![], - predecls: HashMap::new(), blocks: vec![], scope: vec![ScopeFrame::new()], }, @@ -337,6 +373,7 @@ impl<'a> StateWorkingSet<'a> { .expect("internal error: missing required scope frame"); scope_frame.decls.insert(name, decl_id); + scope_frame.visibility.use_id(&decl_id); decl_id } @@ -347,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) + 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 { - 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"); + 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); } @@ -367,11 +411,11 @@ impl<'a> StateWorkingSet<'a> { } pub fn hide_decl(&mut self, name: &[u8]) -> Option { - let mut hiding: HashSet = HashSet::new(); + let mut visibility: Visibility = Visibility::new(); // Since we can mutate scope frames in delta, remove the id directly for scope in self.delta.scope.iter_mut().rev() { - hiding.extend(&scope.hiding); + visibility.append(&scope.visibility); if let Some(decl_id) = scope.decls.remove(name) { return Some(decl_id); @@ -386,12 +430,12 @@ impl<'a> StateWorkingSet<'a> { .expect("internal error: missing required scope frame"); for scope in self.permanent_state.scope.iter().rev() { - hiding.extend(&scope.hiding); + visibility.append(&scope.visibility); if let Some(decl_id) = scope.decls.get(name) { - if !hiding.contains(decl_id) { + if visibility.is_id_visible(decl_id) { // Hide decl only if it's not already hidden - last_scope_frame.hiding.insert(*decl_id); + last_scope_frame.visibility.hide_id(decl_id); return Some(*decl_id); } } @@ -432,6 +476,7 @@ impl<'a> StateWorkingSet<'a> { for (name, decl_id) in overlay { scope_frame.decls.insert(name, decl_id); + scope_frame.visibility.use_id(&decl_id); } } @@ -505,14 +550,14 @@ impl<'a> StateWorkingSet<'a> { } pub fn find_decl(&self, name: &[u8]) -> Option { - let mut hiding: HashSet = HashSet::new(); - - if let Some(decl_id) = self.delta.predecls.get(name) { - return Some(*decl_id); - } + let mut visibility: Visibility = Visibility::new(); for scope in self.delta.scope.iter().rev() { - hiding.extend(&scope.hiding); + 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); @@ -520,10 +565,10 @@ impl<'a> StateWorkingSet<'a> { } for scope in self.permanent_state.scope.iter().rev() { - hiding.extend(&scope.hiding); + visibility.append(&scope.visibility); if let Some(decl_id) = scope.decls.get(name) { - if !hiding.contains(decl_id) { + if visibility.is_id_visible(decl_id) { return Some(*decl_id); } } diff --git a/src/tests.rs b/src/tests.rs index eb431e8a47..7ba1b952a6 100644 --- a/src/tests.rs +++ b/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")