From 77c520e10b37828795f2c59707ef22bb1b6b6fc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Sun, 10 Oct 2021 14:31:13 +0300 Subject: [PATCH] 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 | 6 ++- crates/nu-protocol/src/engine/engine_state.rs | 39 +++++++++++-------- src/tests.rs | 18 ++++++++- 3 files changed, 44 insertions(+), 19 deletions(-) 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 c3b07c97c8..830cd65697 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -56,6 +56,7 @@ impl Visibility { #[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>, @@ -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(), @@ -303,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, } @@ -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) + 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); } @@ -545,13 +552,13 @@ impl<'a> StateWorkingSet<'a> { pub fn find_decl(&self, name: &[u8]) -> Option { 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); } 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")