diff --git a/crates/nu-command/src/core_commands/hide.rs b/crates/nu-command/src/core_commands/hide.rs index 700280e663..818649f156 100644 --- a/crates/nu-command/src/core_commands/hide.rs +++ b/crates/nu-command/src/core_commands/hide.rs @@ -1,6 +1,6 @@ -use nu_protocol::ast::Call; +use nu_protocol::ast::{Call, Expr, Expression, ImportPatternMember}; use nu_protocol::engine::{Command, EngineState, Stack}; -use nu_protocol::{Category, PipelineData, Signature, SyntaxShape}; +use nu_protocol::{Category, PipelineData, ShellError, Signature, SyntaxShape}; #[derive(Clone)] pub struct Hide; @@ -20,13 +20,94 @@ impl Command for Hide { "Hide definitions in the current scope" } + fn extra_usage(&self) -> &str { + "If there is a definition and an environment variable with the same name in the current scope, first the definition will be hidden, then the environment variable." + } + fn run( &self, - _engine_state: &EngineState, - _stack: &mut Stack, + engine_state: &EngineState, + stack: &mut Stack, call: &Call, _input: PipelineData, ) -> Result { + let import_pattern = if let Some(Expression { + expr: Expr::ImportPattern(pat), + .. + }) = call.positional.get(0) + { + pat + } else { + return Err(ShellError::InternalError( + "Got something else than import pattern".into(), + )); + }; + + let head_name_str = if let Ok(s) = String::from_utf8(import_pattern.head.name.clone()) { + s + } else { + return Err(ShellError::NonUtf8(import_pattern.head.span)); + }; + + if let Some(overlay_id) = engine_state.find_overlay(&import_pattern.head.name) { + // The first word is a module + let overlay = engine_state.get_overlay(overlay_id); + + let env_vars_to_hide = if import_pattern.members.is_empty() { + overlay.env_vars_with_head(&import_pattern.head.name) + } else { + match &import_pattern.members[0] { + ImportPatternMember::Glob { .. } => { + overlay.env_vars_with_head(&import_pattern.head.name) + } + ImportPatternMember::Name { name, span } => { + let mut output = vec![]; + + if let Some((name, id)) = + overlay.env_var_with_head(name, &import_pattern.head.name) + { + output.push((name, id)); + } else if !overlay.has_decl(name) { + return Err(ShellError::EnvVarNotFoundAtRuntime(*span)); + } + + output + } + ImportPatternMember::List { names } => { + let mut output = vec![]; + + for (name, span) in names { + if let Some((name, id)) = + overlay.env_var_with_head(name, &import_pattern.head.name) + { + output.push((name, id)); + } else if !overlay.has_decl(name) { + return Err(ShellError::EnvVarNotFoundAtRuntime(*span)); + } + } + + output + } + } + }; + + for (name, _) in env_vars_to_hide { + let name = if let Ok(s) = String::from_utf8(name.clone()) { + s + } else { + return Err(ShellError::NonUtf8(import_pattern.span())); + }; + + if stack.remove_env_var(&name).is_none() { + return Err(ShellError::NotFound(call.positional[0].span)); + } + } + } else if !import_pattern.hidden.contains(&import_pattern.head.name) + && stack.remove_env_var(&head_name_str).is_none() + { + return Err(ShellError::NotFound(call.positional[0].span)); + } + Ok(PipelineData::new(call.head)) } } diff --git a/crates/nu-command/src/env/with_env.rs b/crates/nu-command/src/env/with_env.rs index 3f9b3aeece..3f59c1f917 100644 --- a/crates/nu-command/src/env/with_env.rs +++ b/crates/nu-command/src/env/with_env.rs @@ -163,10 +163,10 @@ fn with_env( for (k, v) in env { match v { EnvVar::Nothing => { - stack.env_vars.remove(&k); + stack.remove_env_var(&k); } EnvVar::Proper(s) => { - stack.env_vars.insert(k, s); + stack.add_env_var(k, s); } } } diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index e5fa30b7a4..59c421427d 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -468,9 +468,9 @@ pub fn eval_variable( let mut output_cols = vec![]; let mut output_vals = vec![]; - let env_columns: Vec<_> = stack.get_env_vars().keys().map(|x| x.to_string()).collect(); - let env_values: Vec<_> = stack - .get_env_vars() + let env_vars = stack.get_env_vars(); + let env_columns: Vec<_> = env_vars.keys().map(|x| x.to_string()).collect(); + let env_values: Vec<_> = env_vars .values() .map(|x| Value::String { val: x.to_string(), diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 8a3402fc72..8ee625ffd5 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -6,7 +6,7 @@ use nu_protocol::{ engine::StateWorkingSet, span, Exportable, Overlay, Span, SyntaxShape, Type, CONFIG_VARIABLE_ID, }; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::Path; #[cfg(feature = "plugin")] @@ -711,6 +711,7 @@ pub fn parse_use( span: spans[1], }, members: import_pattern.members, + hidden: HashSet::new(), }, overlay, ) @@ -836,11 +837,14 @@ pub fn parse_hide( }, ) } else { - // TODO: Or it could be an env var - return ( - garbage_statement(spans), - Some(ParseError::ModuleNotFound(spans[1])), - ); + // Or it could be an env var + ( + false, + Overlay { + decls: HashMap::new(), + env_vars: HashMap::new(), + }, + ) } } else { return ( @@ -893,6 +897,8 @@ pub fn parse_hide( // TODO: `use spam; use spam foo; hide foo` will hide both `foo` and `spam foo` since // they point to the same DeclId. Do we want to keep it that way? working_set.hide_decls(&decls_to_hide); + let import_pattern = import_pattern + .with_hidden(decls_to_hide.iter().map(|(name, _)| name.clone()).collect()); // Create the Hide command call let hide_decl_id = working_set diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index ed67800ded..1631cee1ae 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -20,6 +20,8 @@ use crate::parse_keywords::{ parse_alias, parse_def, parse_def_predecl, parse_hide, parse_let, parse_module, parse_use, }; +use std::collections::HashSet; + #[cfg(feature = "plugin")] use crate::parse_keywords::parse_plugin; @@ -1807,6 +1809,7 @@ pub fn parse_import_pattern( span: Span::unknown(), }, members: vec![], + hidden: HashSet::new(), }, Some(ParseError::WrongImportPattern(span(spans))), ); @@ -1823,6 +1826,7 @@ pub fn parse_import_pattern( span: *head_span, }, members: vec![ImportPatternMember::Glob { span: *tail_span }], + hidden: HashSet::new(), }, error, ) @@ -1850,6 +1854,7 @@ pub fn parse_import_pattern( span: *head_span, }, members: vec![ImportPatternMember::List { names: output }], + hidden: HashSet::new(), }, error, ) @@ -1861,6 +1866,7 @@ pub fn parse_import_pattern( span: *head_span, }, members: vec![], + hidden: HashSet::new(), }, Some(ParseError::ExportNotFound(result.span)), ), @@ -1877,6 +1883,7 @@ pub fn parse_import_pattern( name: tail.to_vec(), span: *tail_span, }], + hidden: HashSet::new(), }, error, ) @@ -1889,6 +1896,7 @@ pub fn parse_import_pattern( span: *head_span, }, members: vec![], + hidden: HashSet::new(), }, None, ) diff --git a/crates/nu-protocol/src/ast/import_pattern.rs b/crates/nu-protocol/src/ast/import_pattern.rs index 0029c453b1..28f9a4a232 100644 --- a/crates/nu-protocol/src/ast/import_pattern.rs +++ b/crates/nu-protocol/src/ast/import_pattern.rs @@ -1,4 +1,5 @@ use crate::{span, Span}; +use std::collections::HashSet; #[derive(Debug, Clone)] pub enum ImportPatternMember { @@ -17,6 +18,9 @@ pub struct ImportPatternHead { pub struct ImportPattern { pub head: ImportPatternHead, pub members: Vec, + // communicate to eval which decls/aliases were hidden during `parse_hide()` so it does not + // interpret these as env var names: + pub hidden: HashSet>, } impl ImportPattern { @@ -37,4 +41,12 @@ impl ImportPattern { span(&spans) } + + pub fn with_hidden(self, hidden: HashSet>) -> Self { + ImportPattern { + head: self.head, + members: self.members, + hidden, + } + } } diff --git a/crates/nu-protocol/src/engine/stack.rs b/crates/nu-protocol/src/engine/stack.rs index 4457809fd3..4a2e79ab75 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -21,8 +21,10 @@ use crate::{Config, ShellError, Value, VarId, CONFIG_VARIABLE_ID}; /// use the Stack as a way of representing the local and closure-captured state. #[derive(Debug, Clone)] pub struct Stack { + /// Variables pub vars: HashMap, - pub env_vars: HashMap, + /// Environment variables arranged as a stack to be able to recover values from parent scopes + pub env_vars: Vec>, } impl Default for Stack { @@ -35,7 +37,7 @@ impl Stack { pub fn new() -> Stack { Stack { vars: HashMap::new(), - env_vars: HashMap::new(), + env_vars: vec![], } } @@ -52,7 +54,11 @@ impl Stack { } pub fn add_env_var(&mut self, var: String, value: String) { - self.env_vars.insert(var, value); + if let Some(scope) = self.env_vars.last_mut() { + scope.insert(var, value); + } else { + self.env_vars.push(HashMap::from([(var, value)])); + } } pub fn collect_captures(&self, captures: &[VarId]) -> Stack { @@ -68,6 +74,7 @@ impl Stack { // FIXME: this is probably slow output.env_vars = self.env_vars.clone(); + output.env_vars.push(HashMap::new()); let config = self .get_var(CONFIG_VARIABLE_ID) @@ -77,19 +84,35 @@ impl Stack { output } + /// Flatten the env var scope frames into one frame pub fn get_env_vars(&self) -> HashMap { - self.env_vars.clone() + let mut result = HashMap::new(); + + for scope in &self.env_vars { + result.extend(scope.clone()); + } + + result } pub fn get_env_var(&self, name: &str) -> Option { - if let Some(v) = self.env_vars.get(name) { - return Some(v.to_string()); + for scope in self.env_vars.iter().rev() { + if let Some(v) = scope.get(name) { + return Some(v.to_string()); + } } + None } pub fn remove_env_var(&mut self, name: &str) -> Option { - self.env_vars.remove(name) + for scope in self.env_vars.iter_mut().rev() { + if let Some(v) = scope.remove(name) { + return Some(v); + } + } + + None } pub fn get_config(&self) -> Result { @@ -109,9 +132,11 @@ impl Stack { for (var, val) in &self.vars { println!(" {}: {:?}", var, val); } - println!("env vars:"); - for (var, val) in &self.env_vars { - println!(" {}: {:?}", var, val); + for (i, scope) in self.env_vars.iter().rev().enumerate() { + println!("env vars, scope {} (from the last);", i); + for (var, val) in scope { + println!(" {}: {:?}", var, val); + } } } } diff --git a/src/main.rs b/src/main.rs index 9bda4f8b75..d5dbe49588 100644 --- a/src/main.rs +++ b/src/main.rs @@ -126,7 +126,7 @@ fn main() -> Result<()> { let mut stack = nu_protocol::engine::Stack::new(); for (k, v) in std::env::vars() { - stack.env_vars.insert(k, v); + stack.add_env_var(k, v); } // Set up our initial config to start from @@ -170,7 +170,7 @@ fn main() -> Result<()> { let mut stack = nu_protocol::engine::Stack::new(); for (k, v) in std::env::vars() { - stack.env_vars.insert(k, v); + stack.add_env_var(k, v); } // Set up our initial config to start from diff --git a/src/tests.rs b/src/tests.rs index 17777777c3..aacf23f3ff 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -474,6 +474,22 @@ fn module_env_imports_5() -> TestResult { ) } +#[test] +fn module_def_and_env_imports_1() -> TestResult { + run_test( + r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; $nu.env.foo"#, + "foo", + ) +} + +#[test] +fn module_def_and_env_imports_2() -> TestResult { + run_test( + r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; foo"#, + "bar", + ) +} + #[test] fn module_def_import_uses_internal_command() -> TestResult { run_test( @@ -496,7 +512,7 @@ fn hides_def() -> TestResult { fail_test(r#"def foo [] { "foo" }; hide foo; foo"#, not_found_msg()) } -#[ignore] +#[test] fn hides_env() -> TestResult { fail_test( r#"let-env foo = "foo"; hide foo; $nu.env.foo"#, @@ -514,7 +530,7 @@ fn hides_def_then_redefines() -> TestResult { ) } -#[ignore] +#[test] fn hides_env_then_redefines() -> TestResult { run_test( r#"let-env foo = "foo"; hide foo; let-env foo = "bar"; $nu.env.foo"#, @@ -554,7 +570,7 @@ fn hides_def_in_scope_4() -> TestResult { ) } -#[ignore] +#[test] fn hides_env_in_scope_1() -> TestResult { fail_test( r#"let-env foo = "foo"; do { hide foo; $nu.env.foo }"#, @@ -562,16 +578,15 @@ fn hides_env_in_scope_1() -> TestResult { ) } -#[ignore] +#[test] fn hides_env_in_scope_2() -> TestResult { - // TODO: Revisit this -- 'hide foo' should restore the env, not hide it completely run_test( r#"let-env foo = "foo"; do { let-env foo = "bar"; hide foo; $nu.env.foo }"#, "foo", ) } -#[ignore] +#[test] fn hides_env_in_scope_3() -> TestResult { fail_test( r#"let-env foo = "foo"; do { hide foo; let-env foo = "bar"; hide foo; $nu.env.foo }"#, @@ -579,9 +594,8 @@ fn hides_env_in_scope_3() -> TestResult { ) } -#[ignore] +#[test] fn hides_env_in_scope_4() -> TestResult { - // TODO: Revisit this -- 'hide foo' should restore the env, not hide it completely fail_test( r#"let-env foo = "foo"; do { let-env foo = "bar"; hide foo; hide foo; $nu.env.foo }"#, "did you mean", @@ -590,31 +604,38 @@ fn hides_env_in_scope_4() -> TestResult { #[test] fn hide_def_twice_not_allowed() -> TestResult { - fail_test(r#"def foo [] { "foo" }; hide foo; hide foo"#, "not found") + fail_test( + r#"def foo [] { "foo" }; hide foo; hide foo"#, + "did not find", + ) } -#[ignore] +#[test] fn hide_env_twice_not_allowed() -> TestResult { fail_test(r#"let-env foo = "foo"; hide foo; hide foo"#, "did not find") } -#[ignore] -fn hides_def_runs_env() -> TestResult { - // TODO: We need some precedence system to handle this. Currently, 'hide foo' hides both the - // def and env var. +#[test] +fn hides_def_runs_env_1() -> TestResult { run_test( r#"let-env foo = "bar"; def foo [] { "foo" }; hide foo; $nu.env.foo"#, "bar", ) } -#[ignore] +#[test] +fn hides_def_runs_env_2() -> TestResult { + run_test( + r#"def foo [] { "foo" }; let-env foo = "bar"; hide foo; $nu.env.foo"#, + "bar", + ) +} + +#[test] fn hides_def_and_env() -> TestResult { - // TODO: We need some precedence system to handle this. Currently, 'hide foo' hides both the - // def and env var. fail_test( r#"let-env foo = "bar"; def foo [] { "foo" }; hide foo; hide foo; $nu.env.foo"#, - not_found_msg(), + "did you mean", ) } @@ -666,7 +687,7 @@ fn hides_def_import_6() -> TestResult { ) } -#[ignore] +#[test] fn hides_env_import_1() -> TestResult { fail_test( r#"module spam { export env foo { "foo" } }; use spam; hide spam foo; $nu.env.'spam foo'"#, @@ -674,7 +695,7 @@ fn hides_env_import_1() -> TestResult { ) } -#[ignore] +#[test] fn hides_env_import_2() -> TestResult { fail_test( r#"module spam { export env foo { "foo" } }; use spam; hide spam *; $nu.env.'spam foo'"#, @@ -682,15 +703,15 @@ fn hides_env_import_2() -> TestResult { ) } -#[ignore] +#[test] fn hides_env_import_3() -> TestResult { fail_test( - r#"module spam { export env foo { "foo" }; } use spam; hide spam [foo]; $nu.env.'spam foo'"#, + r#"module spam { export env foo { "foo" } }; use spam; hide spam [foo]; $nu.env.'spam foo'"#, "did you mean", ) } -#[ignore] +#[test] fn hides_env_import_4() -> TestResult { fail_test( r#"module spam { export env foo { "foo" } }; use spam foo; hide foo; $nu.env.foo"#, @@ -698,7 +719,7 @@ fn hides_env_import_4() -> TestResult { ) } -#[ignore] +#[test] fn hides_env_import_5() -> TestResult { fail_test( r#"module spam { export env foo { "foo" } }; use spam *; hide foo; $nu.env.foo"#, @@ -706,7 +727,7 @@ fn hides_env_import_5() -> TestResult { ) } -#[ignore] +#[test] fn hides_env_import_6() -> TestResult { fail_test( r#"module spam { export env foo { "foo" } }; use spam; hide spam; $nu.env.'spam foo'"#, @@ -714,6 +735,30 @@ fn hides_env_import_6() -> TestResult { ) } +#[test] +fn hides_def_runs_env_import() -> TestResult { + run_test( + r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; hide foo; $nu.env.foo"#, + "foo", + ) +} + +#[test] +fn hides_def_and_env_import_1() -> TestResult { + fail_test( + r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; hide foo; hide foo; $nu.env.foo"#, + "did you mean", + ) +} + +#[test] +fn hides_def_and_env_import_2() -> TestResult { + fail_test( + r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; hide foo; hide foo; foo"#, + not_found_msg(), + ) +} + #[test] fn def_twice_should_fail() -> TestResult { fail_test( @@ -730,7 +775,7 @@ fn use_def_import_after_hide() -> TestResult { ) } -#[ignore] +#[test] fn use_env_import_after_hide() -> TestResult { run_test( r#"module spam { export env foo { "foo" } }; use spam foo; hide foo; use spam foo; $nu.env.foo"#, @@ -746,9 +791,8 @@ fn hide_shadowed_decl() -> TestResult { ) } -#[ignore] +#[test] fn hide_shadowed_env() -> TestResult { - // TODO: waiting for a fix run_test( r#"module spam { export env foo { "bar" } }; let-env foo = "foo"; do { use spam foo; hide foo; $nu.env.foo }"#, "foo", @@ -763,7 +807,7 @@ fn hides_all_decls_within_scope() -> TestResult { ) } -#[ignore] +#[test] fn hides_all_envs_within_scope() -> TestResult { fail_test( r#"module spam { export env foo { "bar" } }; let-env foo = "foo"; use spam foo; hide foo; $nu.env.foo"#,