From 88817a8f104a9815c9554c6d25f1fa4fe115eb80 Mon Sep 17 00:00:00 2001 From: Hristo Filaretov Date: Thu, 26 Aug 2021 15:15:58 +0200 Subject: [PATCH] Allow environment variables to be hidden (#3950) * Allow environment variables to be hidden This change allows environment variables in Nushell to have a value of `Nothing`, which can be set by the user by passing `$nothing` to `let-env` and friends. Environment variables with a value of Nothing behave as if they are not set at all. This allows a user to shadow the value of an environment variable in a parent scope, effectively removing it from their current scope. This was not possible before, because a scope can not affect its parent scopes. This is a workaround for issues like #3920. Additionally, this allows a user to simultaneously set, change and remove multiple environment variables via `load-env`. Any environment variables set to $nothing will be hidden and thus act as if they are removed. This simplifies working with virtual environments, which rely on setting multiple environment variables, including PATH, to specific values, and remove/change them on deactivation. One surprising behavior is that an environment variable set to $nothing will act as if it is not set when querying it (via $nu.env.X), but it is still possible to remove it entirely via `unlet-env`. If the same environment variable is present in the parent scope, the value in the parent scope will be visible to the user. This might be surprising behavior to users who are not familiar with the implementation details. An additional corner case is the the shorthand form of `with-env` does not work with this feature. Using `X=$nothing` will set $nu.env.X to the string "$nothing". The long-form works as expected: `with-env [X $nothing] {...}`. * Remove unused import * Allow all primitives to be convert to strings --- crates/nu-command/src/commands/env/let_env.rs | 8 +- .../nu-command/src/commands/env/load_env.rs | 13 ++- .../nu-command/src/commands/env/with_env.rs | 11 ++- crates/nu-command/tests/commands/with_env.rs | 34 +++++++ crates/nu-completion/src/variable.rs | 9 +- crates/nu-engine/src/evaluate/envvar.rs | 50 ++++++++++ crates/nu-engine/src/evaluate/mod.rs | 1 + crates/nu-engine/src/evaluate/scope.rs | 33 +++++-- crates/nu-engine/src/evaluation_context.rs | 27 +++++- crates/nu-engine/src/lib.rs | 1 + tests/shell/pipeline/commands/internal.rs | 97 ++++++++++++++++++- 11 files changed, 253 insertions(+), 31 deletions(-) create mode 100644 crates/nu-engine/src/evaluate/envvar.rs 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!(