From 9b41f9ecb838e0fd06053844ec4cd3ba948af169 Mon Sep 17 00:00:00 2001 From: Leon Date: Wed, 7 Dec 2022 03:51:55 +1000 Subject: [PATCH] Allow `$env` and mutable records to be mutated by `=` (closes #7110) (#7318) # Description Closes #7110. ~~Note that unlike "real" `mut` vars, $env can be deeply mutated via stuff like `$env.PYTHON_IO_ENCODING = utf8` or `$env.config.history.max_size = 2000`. So, it's a slightly awkward special case, arguably justifiable because of what $env represents (the environment variables of your system, which is essentially "outside" normal Nushell regulations).~~ EDIT: Now allows all `mut` vars to be deeply mutated using `=`, on request. # User-Facing Changes See above. # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- crates/nu-command/src/core_commands/mut_.rs | 5 ++ crates/nu-command/tests/commands/mut_.rs | 72 +++++++++++++++++---- crates/nu-engine/src/eval.rs | 47 ++++++++------ crates/nu-parser/src/parser.rs | 36 +++++------ crates/nu-protocol/src/shell_error.rs | 12 ++++ crates/nu-protocol/src/value/mod.rs | 16 +++-- 6 files changed, 129 insertions(+), 59 deletions(-) diff --git a/crates/nu-command/src/core_commands/mut_.rs b/crates/nu-command/src/core_commands/mut_.rs index 5c6dd5847d..91d65e57b1 100644 --- a/crates/nu-command/src/core_commands/mut_.rs +++ b/crates/nu-command/src/core_commands/mut_.rs @@ -83,6 +83,11 @@ impl Command for Mut { example: "mut x = 10; $x = 12", result: None, }, + Example { + description: "Upsert a value inside a mutable data structure", + example: "mut a = {b:{c:1}}; $a.b.c = 2", + result: None, + }, Example { description: "Set a mutable variable to the result of an expression", example: "mut x = 10 + 100", diff --git a/crates/nu-command/tests/commands/mut_.rs b/crates/nu-command/tests/commands/mut_.rs index 0034f6d999..99f2ba6dc9 100644 --- a/crates/nu-command/tests/commands/mut_.rs +++ b/crates/nu-command/tests/commands/mut_.rs @@ -36,18 +36,6 @@ fn capture_of_mutable_var() { assert!(actual.err.contains("capture of mutable variable")); } -#[test] -fn mut_a_field() { - let actual = nu!( - cwd: ".", pipeline( - r#" - mut y = {abc: 123}; $y.abc = 456; $y.abc - "# - )); - - assert_eq!(actual.out, "456"); -} - #[test] fn mut_add_assign() { let actual = nu!( @@ -95,3 +83,63 @@ fn mut_divide_assign() { assert_eq!(actual.out, "4"); } + +#[test] +fn mut_path_insert() { + let actual = nu!( + cwd: ".", pipeline( + r#" + mut y = {abc: 123}; $y.abc = 456; $y.abc + "# + )); + + assert_eq!(actual.out, "456"); +} + +#[test] +fn mut_path_insert_list() { + let actual = nu!( + cwd: ".", pipeline( + r#" + mut a = [0 1 2]; $a.3 = 3; $a | to nuon + "# + )); + + assert_eq!(actual.out, "[0, 1, 2, 3]"); +} + +#[test] +fn mut_path_upsert() { + let actual = nu!( + cwd: ".", pipeline( + r#" + mut a = {b:[{c:1}]}; $a.b.0.d = 11; $a.b.0.d + "# + )); + + assert_eq!(actual.out, "11"); +} + +#[test] +fn mut_path_upsert_list() { + let actual = nu!( + cwd: ".", pipeline( + r#" + mut a = [[[3] 2] 1]; $a.0.0.1 = 0; $a.0.2 = 0; $a.2 = 0; $a | to nuon + "# + )); + + assert_eq!(actual.out, "[[[3, 0], 2, 0], 1, 0]"); +} + +#[test] +fn mut_path_operator_assign() { + let actual = nu!( + cwd: ".", pipeline( + r#" + mut a = {b:1}; $a.b += 3; $a.b -= 2; $a.b *= 10; $a.b /= 4; $a.b + "# + )); + + assert_eq!(actual.out, "5"); +} diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index ff094fc8e2..a6d8fa50e6 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -491,30 +491,37 @@ pub fn eval_expression( } Expr::FullCellPath(cell_path) => match &cell_path.head.expr { Expr::Var(var_id) | Expr::VarDecl(var_id) => { - if var_id == &ENV_VARIABLE_ID { - // let mut lhs = - // eval_expression(engine_state, stack, &cell_path.head)?; - //lhs.update_data_at_cell_path(&cell_path.tail, rhs)?; - match &cell_path.tail[0] { - PathMember::String { val, .. } => { - stack.add_env_var(val.to_string(), rhs); - } - PathMember::Int { val, .. } => { - stack.add_env_var(val.to_string(), rhs); + // The $env variable is considered "mutable" in Nushell. + // As such, give it special treatment here. + let is_env = var_id == &ENV_VARIABLE_ID; + if is_env || engine_state.get_var(*var_id).mutable { + let mut lhs = + eval_expression(engine_state, stack, &cell_path.head)?; + + lhs.upsert_data_at_cell_path(&cell_path.tail, rhs)?; + if is_env { + // The special $env treatment: for something like $env.config.history.max_size = 2000, + // get $env.config (or whichever one it is) AFTER the above mutation, and set it + // as the "config" environment variable. + let vardata = lhs.follow_cell_path( + &[cell_path.tail[0].clone()], + false, + )?; + match &cell_path.tail[0] { + PathMember::String { val, .. } => { + stack.add_env_var(val.to_string(), vardata); + } + // In case someone really wants an integer env-var + PathMember::Int { val, .. } => { + stack.add_env_var(val.to_string(), vardata); + } } + } else { + stack.vars.insert(*var_id, lhs); } Ok(Value::nothing(cell_path.head.span)) } else { - let var_info = engine_state.get_var(*var_id); - if var_info.mutable { - let mut lhs = - eval_expression(engine_state, stack, &cell_path.head)?; - lhs.update_data_at_cell_path(&cell_path.tail, rhs)?; - stack.vars.insert(*var_id, lhs); - Ok(Value::nothing(cell_path.head.span)) - } else { - Err(ShellError::AssignmentRequiresMutableVar(lhs.span)) - } + Err(ShellError::AssignmentRequiresMutableVar(lhs.span)) } } _ => Err(ShellError::AssignmentRequiresVar(lhs.span)), diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index b58ef00078..bf5d1f6386 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -1997,28 +1997,22 @@ pub fn parse_full_cell_path( ); error = error.or(err); - if !tail.is_empty() { - ( - Expression { - ty: head.ty.clone(), // FIXME. How to access the last type of tail? - expr: Expr::FullCellPath(Box::new(FullCellPath { head, tail })), - span: full_cell_span, - custom_completion: None, + ( + Expression { + // FIXME: Get the type of the data at the tail using follow_cell_path() (or something) + ty: if !tail.is_empty() { + // Until the aforementioned fix is implemented, this is necessary to allow mutable list upserts + // such as $a.1 = 2 to work correctly. + Type::Any + } else { + head.ty.clone() }, - error, - ) - } else { - let ty = head.ty.clone(); - ( - Expression { - expr: Expr::FullCellPath(Box::new(FullCellPath { head, tail })), - ty, - span: full_cell_span, - custom_completion: None, - }, - error, - ) - } + expr: Expr::FullCellPath(Box::new(FullCellPath { head, tail })), + span: full_cell_span, + custom_completion: None, + }, + error, + ) } else { (garbage(span), error) } diff --git a/crates/nu-protocol/src/shell_error.rs b/crates/nu-protocol/src/shell_error.rs index 511e3d86bb..22bb63533b 100644 --- a/crates/nu-protocol/src/shell_error.rs +++ b/crates/nu-protocol/src/shell_error.rs @@ -376,6 +376,18 @@ Either make sure {0} is a string, or add a 'to_string' entry for it in ENV_CONVE #[diagnostic(code(nu::shell::access_beyond_end), url(docsrs))] AccessBeyondEnd(usize, #[label = "index too large (max: {0})"] Span), + /// You attempted to insert data at a list position higher than the end. + /// + /// ## Resolution + /// + /// To insert data into a list, assign to the last used index + 1. + #[error("Inserted at wrong row number (should be {0}).")] + #[diagnostic(code(nu::shell::access_beyond_end), url(docsrs))] + InsertAfterNextFreeIndex( + usize, + #[label = "can't insert at index (the next available index is {0})"] Span, + ), + /// You attempted to access an index when it's empty. /// /// ## Resolution diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs index 210590cbfa..7ace6ad945 100644 --- a/crates/nu-protocol/src/value/mod.rs +++ b/crates/nu-protocol/src/value/mod.rs @@ -878,10 +878,12 @@ impl Value { Value::List { vals, .. } => { if let Some(v) = vals.get_mut(*row_num) { v.upsert_data_at_cell_path(&cell_path[1..], new_val)? - } else if vals.is_empty() { - return Err(ShellError::AccessEmptyContent(*span)); + } else if vals.len() == *row_num && cell_path.len() == 1 { + // If the upsert is at 1 + the end of the list, it's OK. + // Otherwise, it's prohibited. + vals.push(new_val); } else { - return Err(ShellError::AccessBeyondEnd(vals.len() - 1, *span)); + return Err(ShellError::InsertAfterNextFreeIndex(vals.len(), *span)); } } v => return Err(ShellError::NotAList(*span, v.span()?)), @@ -1266,10 +1268,12 @@ impl Value { Value::List { vals, .. } => { if let Some(v) = vals.get_mut(*row_num) { v.insert_data_at_cell_path(&cell_path[1..], new_val)? - } else if vals.is_empty() { - return Err(ShellError::AccessEmptyContent(*span)); + } else if vals.len() == *row_num && cell_path.len() == 1 { + // If the insert is at 1 + the end of the list, it's OK. + // Otherwise, it's prohibited. + vals.push(new_val); } else { - return Err(ShellError::AccessBeyondEnd(vals.len() - 1, *span)); + return Err(ShellError::InsertAfterNextFreeIndex(vals.len(), *span)); } } v => return Err(ShellError::NotAList(*span, v.span()?)),