Change append operator to concatenation operator (#14344)

# Description

The "append" operator currently serves as both the append operator and
the concatenation operator. This dual role creates ambiguity when
operating on nested lists.

```nu
[1 2] ++ 3     # appends a value to a list [1 2 3]
[1 2] ++ [3 4] # concatenates two lists    [1 2 3 4]

[[1 2] [3 4]] ++ [5 6]
# does this give [[1 2] [3 4] [5 6]]
# or             [[1 2] [3 4] 5 6]  
```

Another problem is that `++=` can change the type of a variable:
```nu
mut str = 'hello '
$str ++= ['world']
($str | describe) == list<string>
```

Note that appending is only relevant for lists, but concatenation is
relevant for lists, strings, and binary values. Additionally, appending
can be expressed in terms of concatenation (see example below). So, this
PR changes the `++` operator to only perform concatenation.

# User-Facing Changes

Using the `++` operator with a list and a non-list value will now be a
compile time or runtime error.
```nu
mut list = []
$list ++= 1 # error
```
Instead, concatenate a list with one element:
```nu
$list ++= [1]
```
Or use `append`:
```nu
$list = $list | append 1
```

# After Submitting

Update book and docs.

---------

Co-authored-by: Douglas <32344964+NotTheDr01ds@users.noreply.github.com>
This commit is contained in:
Ian Manske 2024-11-24 10:59:54 -08:00 committed by GitHub
parent dd3a3a2717
commit 4d3283e235
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
20 changed files with 131 additions and 201 deletions

View File

@ -60,10 +60,6 @@ impl Completer for OperatorCompletion {
("bit-shr", "Bitwise shift right"),
("in", "Is a member of (doesn't use regex)"),
("not-in", "Is not a member of (doesn't use regex)"),
(
"++",
"Appends two lists, a list and a value, two strings, or two binary values",
),
],
Expr::String(_) => vec![
("=~", "Contains regex match"),
@ -72,7 +68,7 @@ impl Completer for OperatorCompletion {
("not-like", "Does not contain regex match"),
(
"++",
"Appends two lists, a list and a value, two strings, or two binary values",
"Concatenates two lists, two strings, or two binary values",
),
("in", "Is a member of (doesn't use regex)"),
("not-in", "Is not a member of (doesn't use regex)"),
@ -95,10 +91,6 @@ impl Completer for OperatorCompletion {
("**", "Power of"),
("in", "Is a member of (doesn't use regex)"),
("not-in", "Is not a member of (doesn't use regex)"),
(
"++",
"Appends two lists, a list and a value, two strings, or two binary values",
),
],
Expr::Bool(_) => vec![
(
@ -113,15 +105,11 @@ impl Completer for OperatorCompletion {
("not", "Negates a value or expression"),
("in", "Is a member of (doesn't use regex)"),
("not-in", "Is not a member of (doesn't use regex)"),
(
"++",
"Appends two lists, a list and a value, two strings, or two binary values",
),
],
Expr::FullCellPath(path) => match path.head.expr {
Expr::List(_) => vec![(
"++",
"Appends two lists, a list and a value, two strings, or two binary values",
"Concatenates two lists, two strings, or two binary values",
)],
Expr::Var(id) => get_variable_completions(id, working_set),
_ => vec![],
@ -161,7 +149,7 @@ pub fn get_variable_completions<'a>(
Type::List(_) | Type::String | Type::Binary => vec![
(
"++=",
"Appends a list, a value, a string, or a binary value to a variable.",
"Concatenates two lists, two strings, or two binary values",
),
("=", "Assigns a value to a variable."),
],

View File

@ -31,7 +31,7 @@ impl Command for HelpOperators {
let mut operators = [
Operator::Assignment(Assignment::Assign),
Operator::Assignment(Assignment::PlusAssign),
Operator::Assignment(Assignment::AppendAssign),
Operator::Assignment(Assignment::ConcatAssign),
Operator::Assignment(Assignment::MinusAssign),
Operator::Assignment(Assignment::MultiplyAssign),
Operator::Assignment(Assignment::DivideAssign),
@ -48,7 +48,7 @@ impl Command for HelpOperators {
Operator::Comparison(Comparison::StartsWith),
Operator::Comparison(Comparison::EndsWith),
Operator::Math(Math::Plus),
Operator::Math(Math::Append),
Operator::Math(Math::Concat),
Operator::Math(Math::Minus),
Operator::Math(Math::Multiply),
Operator::Math(Math::Divide),
@ -144,8 +144,8 @@ fn description(operator: &Operator) -> &'static str {
Operator::Comparison(Comparison::StartsWith) => "Checks if a string starts with another.",
Operator::Comparison(Comparison::EndsWith) => "Checks if a string ends with another.",
Operator::Math(Math::Plus) => "Adds two values.",
Operator::Math(Math::Append) => {
"Appends two lists, a list and a value, two strings, or two binary values."
Operator::Math(Math::Concat) => {
"Concatenates two lists, two strings, or two binary values."
}
Operator::Math(Math::Minus) => "Subtracts two values.",
Operator::Math(Math::Multiply) => "Multiplies two values.",
@ -163,8 +163,8 @@ fn description(operator: &Operator) -> &'static str {
Operator::Bits(Bits::ShiftRight) => "Bitwise shifts a value right by another.",
Operator::Assignment(Assignment::Assign) => "Assigns a value to a variable.",
Operator::Assignment(Assignment::PlusAssign) => "Adds a value to a variable.",
Operator::Assignment(Assignment::AppendAssign) => {
"Appends a list, a value, a string, or a binary value to a variable."
Operator::Assignment(Assignment::ConcatAssign) => {
"Concatenates two lists, two strings, or two binary values."
}
Operator::Assignment(Assignment::MinusAssign) => "Subtracts a value from a variable.",
Operator::Assignment(Assignment::MultiplyAssign) => "Multiplies a variable by a value.",

View File

@ -1,88 +0,0 @@
use nu_test_support::nu;
#[test]
fn append_assign_int() {
let actual = nu!(r#"
mut a = [1 2];
$a ++= [3 4];
$a == [1 2 3 4]
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn append_assign_string() {
let actual = nu!(r#"
mut a = [a b];
$a ++= [c d];
$a == [a b c d]
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn append_assign_any() {
let actual = nu!(r#"
mut a = [1 2 a];
$a ++= [b 3];
$a == [1 2 a b 3]
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn append_assign_both_empty() {
let actual = nu!(r#"
mut a = [];
$a ++= [];
$a == []
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn append_assign_type_mismatch() {
let actual = nu!(r#"
mut a = [1 2];
$a ++= [a];
$a == [1 2 "a"]
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn append_assign_single_element() {
let actual = nu!(r#"
mut a = ["list" "and"];
$a ++= "a single element";
$a == ["list" "and" "a single element"]
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn append_assign_to_single_element() {
let actual = nu!(r#"
mut a = "string";
$a ++= ["and" "the" "list"];
$a == ["string" "and" "the" "list"]
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn append_assign_single_to_single() {
let actual = nu!(r#"
mut a = 1;
$a ++= "and a single element";
"#);
assert!(actual.err.contains("nu::parser::unsupported_operation"));
}

View File

@ -0,0 +1,76 @@
use nu_test_support::nu;
#[test]
fn concat_assign_list_int() {
let actual = nu!(r#"
mut a = [1 2];
$a ++= [3 4];
$a == [1 2 3 4]
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn concat_assign_list_string() {
let actual = nu!(r#"
mut a = [a b];
$a ++= [c d];
$a == [a b c d]
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn concat_assign_any() {
let actual = nu!(r#"
mut a = [1 2 a];
$a ++= [b 3];
$a == [1 2 a b 3]
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn concat_assign_both_empty() {
let actual = nu!(r#"
mut a = [];
$a ++= [];
$a == []
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn concat_assign_string() {
let actual = nu!(r#"
mut a = 'hello';
$a ++= ' world';
$a == 'hello world'
"#);
assert_eq!(actual.out, "true")
}
#[test]
fn concat_assign_type_mismatch() {
let actual = nu!(r#"
mut a = [];
$a ++= 'str'
"#);
assert!(actual.err.contains("nu::parser::unsupported_operation"));
}
#[test]
fn concat_assign_runtime_type_mismatch() {
let actual = nu!(r#"
mut a = [];
$a ++= if true { 'str' }
"#);
assert!(actual.err.contains("nu::shell::type_mismatch"));
}

View File

@ -1 +1 @@
mod append_assign;
mod concat;

View File

@ -483,7 +483,7 @@ fn compound_where_paren() {
// TODO: these ++ tests are not really testing *math* functionality, maybe find another place for them
#[test]
fn adding_lists() {
fn concat_lists() {
let actual = nu!(pipeline(
r#"
[1 3] ++ [5 6] | to nuon
@ -494,29 +494,7 @@ fn adding_lists() {
}
#[test]
fn adding_list_and_value() {
let actual = nu!(pipeline(
r#"
[1 3] ++ 5 | to nuon
"#
));
assert_eq!(actual.out, "[1, 3, 5]");
}
#[test]
fn adding_value_and_list() {
let actual = nu!(pipeline(
r#"
1 ++ [3 5] | to nuon
"#
));
assert_eq!(actual.out, "[1, 3, 5]");
}
#[test]
fn adding_tables() {
fn concat_tables() {
let actual = nu!(pipeline(
r#"
[[a b]; [1 2]] ++ [[c d]; [10 11]] | to nuon
@ -526,7 +504,7 @@ fn adding_tables() {
}
#[test]
fn append_strings() {
fn concat_strings() {
let actual = nu!(pipeline(
r#"
"foo" ++ "bar"
@ -536,7 +514,7 @@ fn append_strings() {
}
#[test]
fn append_binary_values() {
fn concat_binary_values() {
let actual = nu!(pipeline(
r#"
0x[01 02] ++ 0x[03 04] | to nuon

View File

@ -411,12 +411,9 @@ fn url_join_with_params_invalid_table() {
"host": "localhost",
"params": (
[
["key", "value"];
["par_1", "aaa"],
["par_2", "bbb"],
["par_1", "ccc"],
["par_2", "ddd"],
] ++ ["not a record"]
{ key: foo, value: bar }
"not a record"
]
),
"port": "1234",
} | url join

View File

@ -155,7 +155,7 @@ pub(crate) fn decompose_assignment(assignment: Assignment) -> Option<Operator> {
match assignment {
Assignment::Assign => None,
Assignment::PlusAssign => Some(Operator::Math(Math::Plus)),
Assignment::AppendAssign => Some(Operator::Math(Math::Append)),
Assignment::ConcatAssign => Some(Operator::Math(Math::Concat)),
Assignment::MinusAssign => Some(Operator::Math(Math::Minus)),
Assignment::MultiplyAssign => Some(Operator::Math(Math::Multiply)),
Assignment::DivideAssign => Some(Operator::Math(Math::Divide)),

View File

@ -547,9 +547,9 @@ impl Eval for EvalRuntime {
let lhs = eval_expression::<D>(engine_state, stack, lhs)?;
lhs.div(op_span, &rhs, op_span)?
}
Assignment::AppendAssign => {
Assignment::ConcatAssign => {
let lhs = eval_expression::<D>(engine_state, stack, lhs)?;
lhs.append(op_span, &rhs, op_span)?
lhs.concat(op_span, &rhs, op_span)?
}
};

View File

@ -956,7 +956,7 @@ fn binary_op(
},
Operator::Math(mat) => match mat {
Math::Plus => lhs_val.add(op_span, &rhs_val, span)?,
Math::Append => lhs_val.append(op_span, &rhs_val, span)?,
Math::Concat => lhs_val.concat(op_span, &rhs_val, span)?,
Math::Minus => lhs_val.sub(op_span, &rhs_val, span)?,
Math::Multiply => lhs_val.mul(op_span, &rhs_val, span)?,
Math::Divide => lhs_val.div(op_span, &rhs_val, span)?,

View File

@ -4887,7 +4887,7 @@ pub fn parse_assignment_operator(working_set: &mut StateWorkingSet, span: Span)
let operator = match contents {
b"=" => Operator::Assignment(Assignment::Assign),
b"+=" => Operator::Assignment(Assignment::PlusAssign),
b"++=" => Operator::Assignment(Assignment::AppendAssign),
b"++=" => Operator::Assignment(Assignment::ConcatAssign),
b"-=" => Operator::Assignment(Assignment::MinusAssign),
b"*=" => Operator::Assignment(Assignment::MultiplyAssign),
b"/=" => Operator::Assignment(Assignment::DivideAssign),
@ -5013,7 +5013,7 @@ pub fn parse_operator(working_set: &mut StateWorkingSet, span: Span) -> Expressi
b"=~" | b"like" => Operator::Comparison(Comparison::RegexMatch),
b"!~" | b"not-like" => Operator::Comparison(Comparison::NotRegexMatch),
b"+" => Operator::Math(Math::Plus),
b"++" => Operator::Math(Math::Append),
b"++" => Operator::Math(Math::Concat),
b"-" => Operator::Math(Math::Minus),
b"*" => Operator::Math(Math::Multiply),
b"/" => Operator::Math(Math::Divide),

View File

@ -132,7 +132,7 @@ pub fn math_result_type(
)
}
},
Operator::Math(Math::Append) => check_append(working_set, lhs, rhs, op),
Operator::Math(Math::Concat) => check_concat(working_set, lhs, rhs, op),
Operator::Math(Math::Minus) => match (&lhs.ty, &rhs.ty) {
(Type::Int, Type::Int) => (Type::Int, None),
(Type::Float, Type::Int) => (Type::Float, None),
@ -935,8 +935,8 @@ pub fn math_result_type(
)
}
},
Operator::Assignment(Assignment::AppendAssign) => {
check_append(working_set, lhs, rhs, op)
Operator::Assignment(Assignment::ConcatAssign) => {
check_concat(working_set, lhs, rhs, op)
}
Operator::Assignment(_) => match (&lhs.ty, &rhs.ty) {
(x, y) if x == y => (Type::Nothing, None),
@ -1085,7 +1085,7 @@ pub fn check_block_input_output(working_set: &StateWorkingSet, block: &Block) ->
output_errors
}
fn check_append(
fn check_concat(
working_set: &mut StateWorkingSet,
lhs: &Expression,
rhs: &Expression,
@ -1099,23 +1099,17 @@ fn check_append(
(Type::List(Box::new(Type::Any)), None)
}
}
(Type::List(a), b) | (b, Type::List(a)) => {
if a == &Box::new(b.clone()) {
(Type::List(a.clone()), None)
} else {
(Type::List(Box::new(Type::Any)), None)
}
}
(Type::Table(a), Type::Table(_)) => (Type::Table(a.clone()), None),
(Type::String, Type::String) => (Type::String, None),
(Type::Binary, Type::Binary) => (Type::Binary, None),
(Type::Any, _) | (_, Type::Any) => (Type::Any, None),
(Type::Table(_) | Type::String | Type::Binary, _) => {
(Type::Table(_) | Type::List(_) | Type::String | Type::Binary, _)
| (_, Type::Table(_) | Type::List(_) | Type::String | Type::Binary) => {
*op = Expression::garbage(working_set, op.span);
(
Type::Any,
Some(ParseError::UnsupportedOperationRHS(
"append".into(),
"concatenation".into(),
op.span,
lhs.span,
lhs.ty.clone(),
@ -1129,7 +1123,7 @@ fn check_append(
(
Type::Any,
Some(ParseError::UnsupportedOperationLHS(
"append".into(),
"concatenation".into(),
op.span,
lhs.span,
lhs.ty.clone(),

View File

@ -1485,7 +1485,7 @@ fn prepare_plugin_call_custom_value_op() {
span,
},
CustomValueOp::Operation(
Operator::Math(Math::Append).into_spanned(span),
Operator::Math(Math::Concat).into_spanned(span),
cv_ok_val.clone(),
),
),
@ -1498,7 +1498,7 @@ fn prepare_plugin_call_custom_value_op() {
span,
},
CustomValueOp::Operation(
Operator::Math(Math::Append).into_spanned(span),
Operator::Math(Math::Concat).into_spanned(span),
cv_bad_val.clone(),
),
),

View File

@ -24,7 +24,7 @@ pub enum Comparison {
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub enum Math {
Plus,
Append,
Concat,
Minus,
Multiply,
Divide,
@ -53,7 +53,7 @@ pub enum Bits {
pub enum Assignment {
Assign,
PlusAssign,
AppendAssign,
ConcatAssign,
MinusAssign,
MultiplyAssign,
DivideAssign,
@ -90,7 +90,7 @@ impl Operator {
| Self::Comparison(Comparison::NotEqual)
| Self::Comparison(Comparison::In)
| Self::Comparison(Comparison::NotIn)
| Self::Math(Math::Append) => 80,
| Self::Math(Math::Concat) => 80,
Self::Bits(Bits::BitAnd) => 75,
Self::Bits(Bits::BitXor) => 70,
Self::Bits(Bits::BitOr) => 60,
@ -107,7 +107,7 @@ impl Display for Operator {
match self {
Operator::Assignment(Assignment::Assign) => write!(f, "="),
Operator::Assignment(Assignment::PlusAssign) => write!(f, "+="),
Operator::Assignment(Assignment::AppendAssign) => write!(f, "++="),
Operator::Assignment(Assignment::ConcatAssign) => write!(f, "++="),
Operator::Assignment(Assignment::MinusAssign) => write!(f, "-="),
Operator::Assignment(Assignment::MultiplyAssign) => write!(f, "*="),
Operator::Assignment(Assignment::DivideAssign) => write!(f, "/="),
@ -124,7 +124,7 @@ impl Display for Operator {
Operator::Comparison(Comparison::In) => write!(f, "in"),
Operator::Comparison(Comparison::NotIn) => write!(f, "not-in"),
Operator::Math(Math::Plus) => write!(f, "+"),
Operator::Math(Math::Append) => write!(f, "++"),
Operator::Math(Math::Concat) => write!(f, "++"),
Operator::Math(Math::Minus) => write!(f, "-"),
Operator::Math(Math::Multiply) => write!(f, "*"),
Operator::Math(Math::Divide) => write!(f, "/"),

View File

@ -238,7 +238,7 @@ pub trait Eval {
Math::Minus => lhs.sub(op_span, &rhs, expr_span),
Math::Multiply => lhs.mul(op_span, &rhs, expr_span),
Math::Divide => lhs.div(op_span, &rhs, expr_span),
Math::Append => lhs.append(op_span, &rhs, expr_span),
Math::Concat => lhs.concat(op_span, &rhs, expr_span),
Math::Modulo => lhs.modulo(op_span, &rhs, expr_span),
Math::FloorDivision => lhs.floor_div(op_span, &rhs, expr_span),
Math::Pow => lhs.pow(op_span, &rhs, expr_span),

View File

@ -2503,34 +2503,20 @@ impl Value {
}
}
pub fn append(&self, op: Span, rhs: &Value, span: Span) -> Result<Value, ShellError> {
pub fn concat(&self, op: Span, rhs: &Value, span: Span) -> Result<Value, ShellError> {
match (self, rhs) {
(Value::List { vals: lhs, .. }, Value::List { vals: rhs, .. }) => {
let mut lhs = lhs.clone();
let mut rhs = rhs.clone();
lhs.append(&mut rhs);
Ok(Value::list(lhs, span))
}
(Value::List { vals: lhs, .. }, val) => {
let mut lhs = lhs.clone();
lhs.push(val.clone());
Ok(Value::list(lhs, span))
}
(val, Value::List { vals: rhs, .. }) => {
let mut rhs = rhs.clone();
rhs.insert(0, val.clone());
Ok(Value::list(rhs, span))
Ok(Value::list([lhs.as_slice(), rhs.as_slice()].concat(), span))
}
(Value::String { val: lhs, .. }, Value::String { val: rhs, .. }) => {
Ok(Value::string(lhs.to_string() + rhs, span))
}
(Value::Binary { val: lhs, .. }, Value::Binary { val: rhs, .. }) => {
let mut val = lhs.clone();
val.extend(rhs);
Ok(Value::binary(val, span))
Ok(Value::string([lhs.as_str(), rhs.as_str()].join(""), span))
}
(Value::Binary { val: lhs, .. }, Value::Binary { val: rhs, .. }) => Ok(Value::binary(
[lhs.as_slice(), rhs.as_slice()].concat(),
span,
)),
(Value::Custom { val: lhs, .. }, rhs) => {
lhs.operation(self.span(), Operator::Math(Math::Append), op, rhs)
lhs.operation(self.span(), Operator::Math(Math::Concat), op, rhs)
}
_ => Err(ShellError::OperatorMismatch {
op_span: op,

View File

@ -37,7 +37,7 @@ def get-all-operators [] { return [
[Assignment, =, Assign, "Assigns a value to a variable.", 10]
[Assignment, +=, PlusAssign, "Adds a value to a variable.", 10]
[Assignment, ++=, AppendAssign, "Appends a list or a value to a variable.", 10]
[Assignment, ++=, ConcatAssign, "Concatenate two lists, two strings, or two binary values.", 10]
[Assignment, -=, MinusAssign, "Subtracts a value from a variable.", 10]
[Assignment, *=, MultiplyAssign, "Multiplies a variable by a value.", 10]
[Assignment, /=, DivideAssign, "Divides a variable by a value.", 10]
@ -55,7 +55,7 @@ def get-all-operators [] { return [
[Comparison, ends-with, EndsWith, "Checks if a string ends with another.", 80]
[Comparison, not, UnaryNot, "Negates a value or expression.", 0]
[Math, +, Plus, "Adds two values.", 90]
[Math, ++, Append, "Appends two lists or a list and a value.", 80]
[Math, ++, Concat, "Concatenate two lists, two strings, or two binary values.", 80]
[Math, -, Minus, "Subtracts two values.", 90]
[Math, *, Multiply, "Multiplies two values.", 95]
[Math, /, Divide, "Divides two values.", 95]

View File

@ -112,7 +112,7 @@ impl CustomValue for CoolCustomValue {
) -> Result<Value, ShellError> {
match operator {
// Append the string inside `cool`
ast::Operator::Math(ast::Math::Append) => {
ast::Operator::Math(ast::Math::Concat) => {
if let Some(right) = right
.as_custom_value()
.ok()

View File

@ -154,7 +154,6 @@ fn const_unary_operator(#[case] inp: &[&str], #[case] expect: &str) {
#[case(&[r#"const x = "a" ++ "b" "#, "$x"], "ab")]
#[case(&[r#"const x = [1,2] ++ [3]"#, "$x | describe"], "list<int>")]
#[case(&[r#"const x = 0x[1,2] ++ 0x[3]"#, "$x | describe"], "binary")]
#[case(&[r#"const x = 0x[1,2] ++ [3]"#, "$x | describe"], "list<any>")]
#[case(&["const x = 1 < 2", "$x"], "true")]
#[case(&["const x = (3 * 200) > (2 * 100)", "$x"], "true")]
#[case(&["const x = (3 * 200) < (2 * 100)", "$x"], "false")]

View File

@ -159,7 +159,7 @@ fn record_spread() {
#[test]
fn binary_op_example() {
test_eval(
"(([1 2] ++ [3 4]) == [1 2 3 4]) and (([1 2 3] ++ 4) == ([1] ++ [2 3 4]))",
"(([1 2] ++ [3 4]) == [1 2 3 4]) and (([1] ++ [2 3 4]) == [1 2 3 4])",
Eq("true"),
)
}