Improve with-env robustness (#12523)

# Description
Work for #7149

- **Error `with-env` given uneven count in list form**
- **Fix `with-env` `CantConvert` to record**
- **Error `with-env` when given protected env vars**
- **Deprecate list/table input of vars to `with-env`**
- **Remove examples for deprecated input**

# User-Facing Changes

## Deprecation of the following forms

```
> with-env [MYENV "my env value"] { $env.MYENV }
my env value

> with-env [X Y W Z] { $env.X }
Y

> with-env [[X W]; [Y Z]] { $env.W }
Z
```

## recommended standardized form

```
# Set by key-value record
> with-env {X: "Y", W: "Z"} { [$env.X $env.W] }
╭───┬───╮
│ 0 │ Y │
│ 1 │ Z │
╰───┴───╯
```

## (Side effect) Repeated definitions in an env shorthand are now
disallowed

```
> FOO=bar FOO=baz $env
Error: nu:🐚:column_defined_twice

  × Record field or table column used twice: FOO
   ╭─[entry #1:1:1]
 1 │ FOO=bar FOO=baz $env
   · ─┬─     ─┬─
   ·  │       ╰── field redefined here
   ·  ╰── field first defined here
   ╰────
```
This commit is contained in:
Stefan Holderbach 2024-04-16 13:08:58 +02:00 committed by GitHub
parent 5f818eaefe
commit c9e9b138eb
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 57 additions and 49 deletions

View File

@ -41,31 +41,14 @@ impl Command for WithEnv {
}
fn examples(&self) -> Vec<Example> {
vec![
Example {
description: "Set the MYENV environment variable",
example: r#"with-env [MYENV "my env value"] { $env.MYENV }"#,
result: Some(Value::test_string("my env value")),
},
Example {
description: "Set by primitive value list",
example: r#"with-env [X Y W Z] { $env.X }"#,
result: Some(Value::test_string("Y")),
},
Example {
description: "Set by single row table",
example: r#"with-env [[X W]; [Y Z]] { $env.W }"#,
result: Some(Value::test_string("Z")),
},
Example {
description: "Set by key-value record",
example: r#"with-env {X: "Y", W: "Z"} { [$env.X $env.W] }"#,
result: Some(Value::list(
vec![Value::test_string("Y"), Value::test_string("Z")],
Span::test_data(),
)),
},
]
vec![Example {
description: "Set by key-value record",
example: r#"with-env {X: "Y", W: "Z"} { [$env.X $env.W] }"#,
result: Some(Value::list(
vec![Value::test_string("Y"), Value::test_string("Z")],
Span::test_data(),
)),
}]
}
}
@ -85,6 +68,16 @@ fn with_env(
match &variable {
Value::List { vals: table, .. } => {
nu_protocol::report_error_new(
engine_state,
&ShellError::GenericError {
error: "Deprecated argument type".into(),
msg: "providing the variables to `with-env` as a list or single row table has been deprecated".into(),
span: Some(variable.span()),
help: Some("use the record form instead".into()),
inner: vec![],
},
);
if table.len() == 1 {
// single row([[X W]; [Y Z]])
match &table[0] {
@ -95,7 +88,7 @@ fn with_env(
}
x => {
return Err(ShellError::CantConvert {
to_type: "string list or single row".into(),
to_type: "record".into(),
from_type: x.get_type().to_string(),
span: call
.positional_nth(1)
@ -111,7 +104,13 @@ fn with_env(
if row.len() == 2 {
env.insert(row[0].coerce_string()?, row[1].clone());
}
// TODO: else error?
if row.len() == 1 {
return Err(ShellError::IncorrectValue {
msg: format!("Missing value for $env.{}", row[0].coerce_string()?),
val_span: row[0].span(),
call_span: call.head,
});
}
}
}
}
@ -123,7 +122,7 @@ fn with_env(
}
x => {
return Err(ShellError::CantConvert {
to_type: "string list or single row".into(),
to_type: "record".into(),
from_type: x.get_type().to_string(),
span: call
.positional_nth(1)
@ -134,6 +133,16 @@ fn with_env(
}
};
// TODO: factor list of prohibited env vars into common place
for prohibited in ["PWD", "FILE_PWD", "CURRENT_FILE"] {
if env.contains_key(prohibited) {
return Err(ShellError::AutomaticEnvVarSetManually {
envvar_name: prohibited.into(),
span: call.head,
});
}
}
for (k, v) in env {
stack.add_env_var(k, v);
}

View File

@ -63,7 +63,7 @@ fn let_pipeline_redirects_externals() {
#[test]
fn let_err_pipeline_redirects_externals() {
let actual = nu!(
r#"let x = with-env [FOO "foo"] {nu --testbin echo_env_stderr FOO e>| str length}; $x"#
r#"let x = with-env { FOO: "foo" } {nu --testbin echo_env_stderr FOO e>| str length}; $x"#
);
assert_eq!(actual.out, "3");
}
@ -71,7 +71,7 @@ fn let_err_pipeline_redirects_externals() {
#[test]
fn let_outerr_pipeline_redirects_externals() {
let actual = nu!(
r#"let x = with-env [FOO "foo"] {nu --testbin echo_env_stderr FOO o+e>| str length}; $x"#
r#"let x = with-env { FOO: "foo" } {nu --testbin echo_env_stderr FOO o+e>| str length}; $x"#
);
assert_eq!(actual.out, "3");
}

View File

@ -90,7 +90,7 @@ fn folding_with_tables() {
"
echo [10 20 30 40]
| reduce --fold [] { |it, acc|
with-env [value $it] {
with-env { value: $it } {
echo $acc | append (10 * ($env.value | into int))
}
}

View File

@ -2,7 +2,7 @@ use nu_test_support::nu;
#[test]
fn with_env_extends_environment() {
let actual = nu!("with-env [FOO BARRRR] {echo $env} | get FOO");
let actual = nu!("with-env { FOO: BARRRR } {echo $env} | get FOO");
assert_eq!(actual.out, "BARRRR");
}
@ -32,7 +32,7 @@ fn with_env_shorthand_trims_quotes() {
fn with_env_and_shorthand_same_result() {
let actual_shorthand = nu!("FOO='BARRRR' echo $env | get FOO");
let actual_normal = nu!("with-env [FOO BARRRR] {echo $env} | get FOO");
let actual_normal = nu!("with-env { FOO: BARRRR } {echo $env} | get FOO");
assert_eq!(actual_shorthand.out, actual_normal.out);
}
@ -50,7 +50,7 @@ fn with_env_hides_variables_in_parent_scope() {
let actual = nu!(r#"
$env.FOO = "1"
print $env.FOO
with-env [FOO null] {
with-env { FOO: null } {
echo $env.FOO
}
print $env.FOO

View File

@ -5164,9 +5164,8 @@ pub fn parse_expression(working_set: &mut StateWorkingSet, spans: &[Span]) -> Ex
}
};
let with_env = working_set.find_decl(b"with-env");
if !shorthand.is_empty() {
let with_env = working_set.find_decl(b"with-env");
if let Some(decl_id) = with_env {
let mut block = Block::default();
let ty = output.ty.clone();
@ -5176,13 +5175,12 @@ pub fn parse_expression(working_set: &mut StateWorkingSet, spans: &[Span]) -> Ex
let mut env_vars = vec![];
for sh in shorthand {
env_vars.push(sh.0);
env_vars.push(sh.1);
env_vars.push(RecordItem::Pair(sh.0, sh.1));
}
let arguments = vec![
Argument::Positional(Expression {
expr: Expr::List(env_vars),
expr: Expr::Record(env_vars),
span: span(&spans[..pos]),
ty: Type::Any,
custom_completion: None,

View File

@ -22,7 +22,7 @@ use dt.nu [datetime-diff, pretty-print-duration]
# # Example
# - adding some dummy paths to an empty PATH
# ```nushell
# >_ with-env [PATH []] {
# >_ with-env { PATH: [] } {
# std path add "foo"
# std path add "bar" "baz"
# std path add "fooo" --append

View File

@ -6,7 +6,7 @@ def path_add [] {
let path_name = if "PATH" in $env { "PATH" } else { "Path" }
with-env [$path_name []] {
with-env {$path_name: []} {
def get_path [] { $env | get $path_name }
assert equal (get_path) []

View File

@ -1,4 +1,4 @@
use crate::tests::{run_test, TestResult};
use crate::tests::{fail_test, run_test, TestResult};
#[test]
fn shorthand_env_1() -> TestResult {
@ -7,7 +7,7 @@ fn shorthand_env_1() -> TestResult {
#[test]
fn shorthand_env_2() -> TestResult {
run_test(r#"FOO=BAZ FOO=MOO $env.FOO"#, "MOO")
fail_test(r#"FOO=BAZ FOO=MOO $env.FOO"#, "defined_twice")
}
#[test]

View File

@ -121,7 +121,7 @@ fn load_env_pwd_env_var_fails() {
#[test]
fn passes_with_env_env_var_to_external_process() {
let actual = nu!("
with-env [FOO foo] {nu --testbin echo_env FOO}
with-env { FOO: foo } {nu --testbin echo_env FOO}
");
assert_eq!(actual.out, "foo");
}

View File

@ -133,26 +133,27 @@ fn command_not_found_error_shows_not_found_1() {
#[test]
fn command_substitution_wont_output_extra_newline() {
let actual = nu!(r#"
with-env [FOO "bar"] { echo $"prefix (nu --testbin echo_env FOO) suffix" }
with-env { FOO: "bar" } { echo $"prefix (nu --testbin echo_env FOO) suffix" }
"#);
assert_eq!(actual.out, "prefix bar suffix");
let actual = nu!(r#"
with-env [FOO "bar"] { (nu --testbin echo_env FOO) }
with-env { FOO: "bar" } { (nu --testbin echo_env FOO) }
"#);
assert_eq!(actual.out, "bar");
}
#[test]
fn basic_err_pipe_works() {
let actual = nu!(r#"with-env [FOO "bar"] { nu --testbin echo_env_stderr FOO e>| str length }"#);
let actual =
nu!(r#"with-env { FOO: "bar" } { nu --testbin echo_env_stderr FOO e>| str length }"#);
assert_eq!(actual.out, "3");
}
#[test]
fn basic_outerr_pipe_works() {
let actual = nu!(
r#"with-env [FOO "bar"] { nu --testbin echo_env_mixed out-err FOO FOO o+e>| str length }"#
r#"with-env { FOO: "bar" } { nu --testbin echo_env_mixed out-err FOO FOO o+e>| str length }"#
);
assert_eq!(actual.out, "7");
}
@ -160,7 +161,7 @@ fn basic_outerr_pipe_works() {
#[test]
fn err_pipe_with_failed_external_works() {
let actual =
nu!(r#"with-env [FOO "bar"] { nu --testbin echo_env_stderr_fail FOO e>| str length }"#);
nu!(r#"with-env { FOO: "bar" } { nu --testbin echo_env_stderr_fail FOO e>| str length }"#);
assert_eq!(actual.out, "3");
}