From 40741254f696e01b4e1b57fe5be63ab93f17efc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Sun, 10 Oct 2021 13:10:37 +0300 Subject: [PATCH 1/2] Rewrite hiding system Hiding definitions now should work correctly with repeated use of 'use', 'def' and 'hide' keywords. The key change is that 'hide foo' will hide all definitions of foo that were defined/used within the scope (those from other scopes are still available). This makes the logic simpler and I found it leads to a simpler mental map: you don't need to remember the order of defined/used commands withing the scope -- it just hides all. --- crates/nu-protocol/src/engine/engine_state.rs | 80 ++++++++++++++----- 1 file changed, 59 insertions(+), 21 deletions(-) diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index c93c0ace7d..c3b07c97c8 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,13 +12,54 @@ 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>, decls: HashMap, DeclId>, aliases: HashMap, Vec>, modules: HashMap, BlockId>, - hiding: HashSet, + visibility: Visibility, } impl ScopeFrame { @@ -31,7 +69,7 @@ impl ScopeFrame { decls: HashMap::new(), aliases: HashMap::new(), modules: HashMap::new(), - hiding: HashSet::new(), + visibility: Visibility::new(), } } @@ -86,9 +124,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 +168,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); } } @@ -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 } @@ -367,11 +404,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 +423,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 +469,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 +543,14 @@ impl<'a> StateWorkingSet<'a> { } pub fn find_decl(&self, name: &[u8]) -> Option { - let mut hiding: HashSet = HashSet::new(); + 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() { - hiding.extend(&scope.hiding); + visibility.append(&scope.visibility); if let Some(decl_id) = scope.decls.get(name) { return Some(*decl_id); @@ -520,10 +558,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); } } 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 2/2] 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")