diff --git a/crates/nu-command/src/commands/env/let_env.rs b/crates/nu-command/src/commands/env/let_env.rs index 2cedb38e4..30885a92e 100644 --- a/crates/nu-command/src/commands/env/let_env.rs +++ b/crates/nu-command/src/commands/env/let_env.rs @@ -1,5 +1,7 @@ +use std::convert::TryInto; + use crate::prelude::*; -use nu_engine::{evaluate_baseline_expr, WholeStreamCommand}; +use nu_engine::{evaluate_baseline_expr, EnvVar, WholeStreamCommand}; use nu_errors::ShellError; use nu_protocol::{hir::CapturedBlock, hir::ClassifiedCommand, Signature, SyntaxShape}; @@ -90,9 +92,7 @@ pub fn set_env(args: CommandArgs) -> Result { ctx.scope.exit_scope(); - let value = value?; - let value = value.as_string()?; - + let value: EnvVar = value?.try_into()?; let name = name.item; // Note: this is a special case for setting the context from a command diff --git a/crates/nu-command/src/commands/env/load_env.rs b/crates/nu-command/src/commands/env/load_env.rs index 57f276e6d..e7a4eb1e1 100644 --- a/crates/nu-command/src/commands/env/load_env.rs +++ b/crates/nu-command/src/commands/env/load_env.rs @@ -1,5 +1,7 @@ +use std::convert::TryInto; + use crate::prelude::*; -use nu_engine::WholeStreamCommand; +use nu_engine::{EnvVar, WholeStreamCommand}; use nu_errors::ShellError; use nu_protocol::{Signature, SyntaxShape, Value}; @@ -60,14 +62,17 @@ fn load_env_from_table( for (key, value) in value.row_entries() { if key == "name" { - var_name = Some(value.as_string()?); + var_name = Some(value); } else if key == "value" { - var_value = Some(value.as_string()?); + var_value = Some(value); } } match (var_name, var_value) { - (Some(name), Some(value)) => ctx.scope.add_env_var(name, value), + (Some(name), Some(value)) => { + let env_var: EnvVar = value.try_into()?; + ctx.scope.add_env_var(name.as_string()?, env_var); + } _ => { return Err(ShellError::labeled_error( r#"Expected each row in the table to have a "name" and "value" field."#, diff --git a/crates/nu-command/src/commands/env/with_env.rs b/crates/nu-command/src/commands/env/with_env.rs index 79893cbbd..3fd293e70 100644 --- a/crates/nu-command/src/commands/env/with_env.rs +++ b/crates/nu-command/src/commands/env/with_env.rs @@ -1,5 +1,8 @@ +use std::convert::TryInto; + use crate::prelude::*; use nu_engine::run_block; +use nu_engine::EnvVar; use nu_engine::WholeStreamCommand; use nu_errors::ShellError; use nu_protocol::{ @@ -73,20 +76,20 @@ fn with_env(args: CommandArgs) -> Result { let variable: Value = args.req(0)?; let block: CapturedBlock = args.req(1)?; - let mut env = IndexMap::new(); + let mut env: IndexMap = IndexMap::new(); match &variable.value { UntaggedValue::Table(table) => { if table.len() == 1 { // single row([[X W]; [Y Z]]) for (k, v) in table[0].row_entries() { - env.insert(k.clone(), v.convert_to_string()); + env.insert(k.clone(), v.try_into()?); } } else { // primitive values([X Y W Z]) for row in table.chunks(2) { if row.len() == 2 && row[0].is_primitive() && row[1].is_primitive() { - env.insert(row[0].convert_to_string(), row[1].convert_to_string()); + env.insert(row[0].convert_to_string(), (&row[1]).try_into()?); } } } @@ -94,7 +97,7 @@ fn with_env(args: CommandArgs) -> Result { // when get object by `open x.json` or `from json` UntaggedValue::Row(row) => { for (k, v) in &row.entries { - env.insert(k.clone(), v.convert_to_string()); + env.insert(k.clone(), v.try_into()?); } } _ => { diff --git a/crates/nu-command/tests/commands/with_env.rs b/crates/nu-command/tests/commands/with_env.rs index 084652d2e..46f135235 100644 --- a/crates/nu-command/tests/commands/with_env.rs +++ b/crates/nu-command/tests/commands/with_env.rs @@ -64,3 +64,37 @@ fn with_env_shorthand_nested_quotes() { assert_eq!(actual.out, "-arg \"hello world\""); } + +#[test] +fn with_env_hides_variables_in_parent_scope() { + let actual = nu!( + cwd: "tests/fixtures/formats", + r#" + let-env FOO = "1" + echo $nu.env.FOO + with-env [FOO $nothing] { + echo $nu.env.FOO + } + echo $nu.env.FOO + "# + ); + + assert_eq!(actual.out, "11"); + assert!(actual.err.contains("error")); + assert!(actual.err.contains("Unknown column")); +} + +#[test] +fn with_env_shorthand_can_not_hide_variables() { + let actual = nu!( + cwd: "tests/fixtures/formats", + r#" + let-env FOO = "1" + echo $nu.env.FOO + FOO=$nothing echo $nu.env.FOO + echo $nu.env.FOO + "# + ); + + assert_eq!(actual.out, "1$nothing1"); +} diff --git a/crates/nu-completion/src/variable.rs b/crates/nu-completion/src/variable.rs index e00d3c62a..4d450101b 100644 --- a/crates/nu-completion/src/variable.rs +++ b/crates/nu-completion/src/variable.rs @@ -89,7 +89,7 @@ mod tests { use super::{Completer, Suggestion as S, VariableCompleter}; use crate::matchers::case_insensitive::Matcher as CaseInsensitiveMatcher; - use indexmap::IndexMap; + use nu_engine::EnvVar; use nu_engine::{ evaluation_context::EngineState, ConfigHolder, EvaluationContext, FakeHost, Host, Scope, ShellManager, @@ -121,7 +121,12 @@ mod tests { fn create_context_with_host(host: Box) -> EvaluationContext { let scope = Scope::new(); - let env_vars = host.vars().iter().cloned().collect::>(); + let env_vars = host + .vars() + .iter() + .cloned() + .map(|(k, v)| (k, EnvVar::from(v))) + .collect(); scope.add_env(env_vars); EvaluationContext { diff --git a/crates/nu-engine/src/evaluate/envvar.rs b/crates/nu-engine/src/evaluate/envvar.rs new file mode 100644 index 000000000..068f204b0 --- /dev/null +++ b/crates/nu-engine/src/evaluate/envvar.rs @@ -0,0 +1,50 @@ +use std::convert::TryFrom; + +use nu_errors::ShellError; +use nu_protocol::{SpannedTypeName, Value}; + +#[derive(Debug, Clone)] +pub enum EnvVar { + Proper(String), + Nothing, +} + +impl TryFrom for EnvVar { + type Error = ShellError; + + fn try_from(value: Value) -> Result { + if value.value.is_none() { + Ok(EnvVar::Nothing) + } else if value.is_primitive() { + Ok(EnvVar::Proper(value.convert_to_string())) + } else { + Err(ShellError::type_error( + "primitive", + value.spanned_type_name(), + )) + } + } +} + +impl TryFrom<&Value> for EnvVar { + type Error = ShellError; + + fn try_from(value: &Value) -> Result { + if value.value.is_none() { + Ok(EnvVar::Nothing) + } else if value.is_primitive() { + Ok(EnvVar::Proper(value.convert_to_string())) + } else { + Err(ShellError::type_error( + "primitive", + value.spanned_type_name(), + )) + } + } +} + +impl From for EnvVar { + fn from(string: String) -> Self { + EnvVar::Proper(string) + } +} diff --git a/crates/nu-engine/src/evaluate/mod.rs b/crates/nu-engine/src/evaluate/mod.rs index dbf8b3b6c..5bfc7fded 100644 --- a/crates/nu-engine/src/evaluate/mod.rs +++ b/crates/nu-engine/src/evaluate/mod.rs @@ -1,4 +1,5 @@ pub(crate) mod block; +pub(crate) mod envvar; pub(crate) mod evaluate_args; pub mod evaluator; pub(crate) mod expr; diff --git a/crates/nu-engine/src/evaluate/scope.rs b/crates/nu-engine/src/evaluate/scope.rs index 5da6af9fe..3586a778c 100644 --- a/crates/nu-engine/src/evaluate/scope.rs +++ b/crates/nu-engine/src/evaluate/scope.rs @@ -1,4 +1,7 @@ -use crate::whole_stream_command::{whole_stream_command, Command}; +use crate::{ + evaluate::envvar::EnvVar, + whole_stream_command::{whole_stream_command, Command}, +}; use indexmap::IndexMap; use nu_errors::ShellError; use nu_parser::ParserScope; @@ -203,6 +206,7 @@ impl Scope { } } + // This is used for starting processes, keep it string -> string pub fn get_env_vars(&self) -> IndexMap { //FIXME: should this be an iterator? let mut output = IndexMap::new(); @@ -216,12 +220,21 @@ impl Scope { } output + .into_iter() + .filter_map(|(k, v)| match v { + EnvVar::Proper(s) => Some((k, s)), + EnvVar::Nothing => None, + }) + .collect() } pub fn get_env(&self, name: &str) -> Option { for frame in self.frames.lock().iter().rev() { if let Some(v) = frame.env.get(name) { - return Some(v.clone()); + return match v { + EnvVar::Proper(string) => Some(string.clone()), + EnvVar::Nothing => None, + }; } } @@ -252,36 +265,36 @@ impl Scope { } } - pub fn add_env_var(&self, name: impl Into, value: String) { + pub fn add_env_var(&self, name: impl Into, value: impl Into) { if let Some(frame) = self.frames.lock().last_mut() { - frame.env.insert(name.into(), value); + frame.env.insert(name.into(), value.into()); } } pub fn remove_env_var(&self, name: impl Into) -> Option { if let Some(frame) = self.frames.lock().last_mut() { if let Some(val) = frame.env.remove_entry(&name.into()) { - return Some(val.1); + return Some(val.0); } } None } - pub fn add_env(&self, env_vars: IndexMap) { + pub fn add_env(&self, env_vars: IndexMap) { if let Some(frame) = self.frames.lock().last_mut() { frame.env.extend(env_vars) } } - pub fn add_env_to_base(&self, env_vars: IndexMap) { + pub fn add_env_to_base(&self, env_vars: IndexMap) { if let Some(frame) = self.frames.lock().first_mut() { frame.env.extend(env_vars) } } - pub fn add_env_var_to_base(&self, name: impl Into, value: String) { + pub fn add_env_var_to_base(&self, name: impl Into, value: impl Into) { if let Some(frame) = self.frames.lock().first_mut() { - frame.env.insert(name.into(), value); + frame.env.insert(name.into(), value.into()); } } @@ -426,7 +439,7 @@ impl ParserScope for Scope { #[derive(Debug, Clone)] pub struct ScopeFrame { pub vars: IndexMap, - pub env: IndexMap, + pub env: IndexMap, pub commands: IndexMap, pub custom_commands: IndexMap>, pub aliases: IndexMap>>, diff --git a/crates/nu-engine/src/evaluation_context.rs b/crates/nu-engine/src/evaluation_context.rs index 5553b077a..473dc4ff2 100644 --- a/crates/nu-engine/src/evaluation_context.rs +++ b/crates/nu-engine/src/evaluation_context.rs @@ -1,3 +1,4 @@ +use crate::evaluate::envvar::EnvVar; use crate::evaluate::evaluator::Variable; use crate::evaluate::scope::{Scope, ScopeFrame}; use crate::shell::palette::ThemedPalette; @@ -66,7 +67,12 @@ impl EvaluationContext { pub fn basic() -> EvaluationContext { let scope = Scope::new(); let host = BasicHost {}; - let env_vars = host.vars().iter().cloned().collect::>(); + let env_vars: IndexMap = host + .vars() + .iter() + .cloned() + .map(|(k, v)| (k, v.into())) + .collect(); scope.add_env(env_vars); EvaluationContext { @@ -239,7 +245,12 @@ impl EvaluationContext { let tag = config::cfg_path_to_scope_tag(cfg_path.get_path()); self.scope.enter_scope_with_tag(tag); - self.scope.add_env(cfg.env_map()); + let config_env = cfg.env_map(); + let env_vars = config_env + .into_iter() + .map(|(k, v)| (k, EnvVar::from(v))) + .collect(); + self.scope.add_env(env_vars); if let Some(path) = joined_paths { self.scope.add_env_var(NATIVE_PATH_ENV_VAR, path); } @@ -348,10 +359,16 @@ impl EvaluationContext { let tag = config::cfg_path_to_scope_tag(&cfg.file_path); let mut frame = ScopeFrame::with_tag(tag.clone()); - - frame.env = cfg.env_map(); + let config_env = cfg.env_map(); + let env_vars = config_env + .into_iter() + .map(|(k, v)| (k, EnvVar::from(v))) + .collect(); + frame.env = env_vars; if let Some(path) = joined_paths { - frame.env.insert(NATIVE_PATH_ENV_VAR.to_string(), path); + frame + .env + .insert(NATIVE_PATH_ENV_VAR.to_string(), path.into()); } frame.exitscripts = exit_scripts; diff --git a/crates/nu-engine/src/lib.rs b/crates/nu-engine/src/lib.rs index 618b053fd..607ab63e1 100644 --- a/crates/nu-engine/src/lib.rs +++ b/crates/nu-engine/src/lib.rs @@ -23,6 +23,7 @@ pub use crate::documentation::{generate_docs, get_brief_help, get_documentation, pub use crate::env::host::FakeHost; pub use crate::env::host::Host; pub use crate::evaluate::block::run_block; +pub use crate::evaluate::envvar::EnvVar; pub use crate::evaluate::scope::Scope; pub use crate::evaluate::{evaluator, evaluator::evaluate_baseline_expr}; pub use crate::evaluation_context::EvaluationContext; diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs index d07c9b36f..52fd069d3 100644 --- a/tests/shell/pipeline/commands/internal.rs +++ b/tests/shell/pipeline/commands/internal.rs @@ -424,6 +424,43 @@ fn let_env_variable() { assert_eq!(actual.out, "hello world"); } +#[test] +fn let_env_hides_variable() { + let actual = nu!( + cwd: ".", + r#" + let-env TESTENVVAR = "hello world" + echo $nu.env.TESTENVVAR + let-env TESTENVVAR = $nothing + echo $nu.env.TESTENVVAR + "# + ); + + assert_eq!(actual.out, "hello world"); + assert!(actual.err.contains("error")); + assert!(actual.err.contains("Unknown column")); +} + +#[test] +fn let_env_hides_variable_in_parent_scope() { + let actual = nu!( + cwd: ".", + r#" + let-env TESTENVVAR = "hello world" + echo $nu.env.TESTENVVAR + do { + let-env TESTENVVAR = $nothing + echo $nu.env.TESTENVVAR + } + echo $nu.env.TESTENVVAR + "# + ); + + assert_eq!(actual.out, "hello worldhello world"); + assert!(actual.err.contains("error")); + assert!(actual.err.contains("Unknown column")); +} + #[test] fn unlet_env_variable() { let actual = nu!( @@ -443,10 +480,31 @@ fn unlet_nonexistent_variable() { cwd: ".", r#" unlet-env NONEXISTENT_VARIABLE - " + "# ); - assert!(actual.err.contains("did you mean")); + assert!(actual.err.contains("error")); + assert!(actual.err.contains("Not an environment variable")); +} + +#[test] +fn unlet_variable_in_parent_scope() { + let actual = nu!( + cwd: ".", + r#" + let-env DEBUG = "1" + echo $nu.env.DEBUG + do { + let-env DEBUG = "2" + echo $nu.env.DEBUG + unlet-env DEBUG + echo $nu.env.DEBUG + } + echo $nu.env.DEBUG + "# + ); + + assert_eq!(actual.out, "1211"); } #[test] @@ -534,6 +592,41 @@ fn proper_shadow_load_env_aliases() { assert_eq!(actual.out, "truefalsetrue"); } +#[test] +fn load_env_can_hide_var_envs() { + let actual = nu!( + cwd: ".", + r#" + let-env DEBUG = "1" + echo $nu.env.DEBUG + load-env [[name, value]; [DEBUG $nothing]] + echo $nu.env.DEBUG + "# + ); + assert_eq!(actual.out, "1"); + assert!(actual.err.contains("error")); + assert!(actual.err.contains("Unknown column")); +} + +#[test] +fn load_env_can_hide_var_envs_in_parent_scope() { + let actual = nu!( + cwd: ".", + r#" + let-env DEBUG = "1" + echo $nu.env.DEBUG + do { + load-env [[name, value]; [DEBUG $nothing]] + echo $nu.env.DEBUG + } + echo $nu.env.DEBUG + "# + ); + assert_eq!(actual.out, "11"); + assert!(actual.err.contains("error")); + assert!(actual.err.contains("Unknown column")); +} + #[test] fn proper_shadow_let_aliases() { let actual = nu!(