Add ListItem type for Expr::List (#12529)

# Description
This PR adds a `ListItem` enum to our set of AST types. It encodes the
two possible expressions inside of list expression: a singular item or a
spread. This is similar to the existing `RecordItem` enum. Adding
`ListItem` allows us to remove the existing `Expr::Spread` case which
was previously used for list spreads. As a consequence, this guarantees
(via the type system) that spreads can only ever occur inside lists,
records, or as command args.

This PR also does a little bit of cleanup in relevant parser code.

crates/nu-cli/src/syntax_highlight.rs

@@ -360,7 +360,6 @@ fn find_matching_block_end_in_expr(
             Expr::MatchBlock(_) => None,
             Expr::Nothing => None,
             Expr::Garbage => None,
-            Expr::Spread(_) => None,
 
             Expr::Table(hdr, rows) => {
                 if expr_last == global_cursor_offset {
@@ -468,7 +467,7 @@ fn find_matching_block_end_in_expr(
                 None
             }
 
-            Expr::List(inner_expr) => {
+            Expr::List(list) => {
                 if expr_last == global_cursor_offset {
                     // cursor is at list end
                     Some(expr_first)
@@ -477,8 +476,9 @@ fn find_matching_block_end_in_expr(
                     Some(expr_last)
                 } else {
                     // cursor is inside list
-                    for inner_expr in inner_expr {
+                    for item in list {
-                        find_in_expr_or_continue!(inner_expr);
+                        let expr = item.expr();
+                        find_in_expr_or_continue!(expr);
                     }
                     None
                 }

crates/nu-command/src/formats/from/nuon.rs

@@ -1,6 +1,6 @@
 use nu_engine::command_prelude::*;
 use nu_protocol::{
-    ast::{Expr, Expression, RecordItem},
+    ast::{Expr, Expression, ListItem, RecordItem},
     engine::StateWorkingSet,
     Range, Unit,
 };
@@ -253,8 +253,21 @@ fn convert_to_value(
         }),
         Expr::List(vals) => {
             let mut output = vec![];
-            for val in vals {
+
-                output.push(convert_to_value(val, span, original_text)?);
+            for item in vals {
+                match item {
+                    ListItem::Item(expr) => {
+                        output.push(convert_to_value(expr, span, original_text)?);
+                    }
+                    ListItem::Spread(_, inner) => {
+                        return Err(ShellError::OutsideSpannedLabeledError {
+                            src: original_text.to_string(),
+                            error: "Error when loading".into(),
+                            msg: "spread operator not supported in nuon".into(),
+                            span: inner.span,
+                        });
+                    }
+                }
             }
 
             Ok(Value::list(output, span))
@@ -351,12 +364,6 @@ fn convert_to_value(
             msg: "signatures not supported in nuon".into(),
             span: expr.span,
         }),
-        Expr::Spread(..) => Err(ShellError::OutsideSpannedLabeledError {
-            src: original_text.to_string(),
-            error: "Error when loading".into(),
-            msg: "spread operator not supported in nuon".into(),
-            span: expr.span,
-        }),
         Expr::String(s) => Ok(Value::string(s, span)),
         Expr::StringInterpolation(..) => Err(ShellError::OutsideSpannedLabeledError {
             src: original_text.to_string(),

crates/nu-parser/src/flatten.rs

@@ -1,7 +1,8 @@
 use nu_protocol::{
     ast::{
-        Argument, Block, Expr, Expression, ExternalArgument, ImportPatternMember, MatchPattern,
+        Argument, Block, Expr, Expression, ExternalArgument, ImportPatternMember, ListItem,
-        PathMember, Pattern, Pipeline, PipelineElement, PipelineRedirection, RecordItem,
+        MatchPattern, PathMember, Pattern, Pipeline, PipelineElement, PipelineRedirection,
+        RecordItem,
     },
     engine::StateWorkingSet,
     DeclId, Span, VarId,
@@ -377,20 +378,31 @@ pub fn flatten_expression(
             let mut last_end = outer_span.start;
 
             let mut output = vec![];
-            for l in list {
+            for item in list {
-                let flattened = flatten_expression(working_set, l);
+                match item {
+                    ListItem::Item(expr) => {
+                        let flattened = flatten_expression(working_set, expr);
 
-                if let Some(first) = flattened.first() {
+                        if let Some(first) = flattened.first() {
-                    if first.0.start > last_end {
+                            if first.0.start > last_end {
-                        output.push((Span::new(last_end, first.0.start), FlatShape::List));
+                                output.push((Span::new(last_end, first.0.start), FlatShape::List));
+                            }
+                        }
+
+                        if let Some(last) = flattened.last() {
+                            last_end = last.0.end;
+                        }
+
+                        output.extend(flattened);
+                    }
+                    ListItem::Spread(_, expr) => {
+                        let mut output = vec![(
+                            Span::new(expr.span.start, expr.span.start + 3),
+                            FlatShape::Operator,
+                        )];
+                        output.extend(flatten_expression(working_set, expr));
                     }
                 }
-
-                if let Some(last) = flattened.last() {
-                    last_end = last.0.end;
-                }
-
-                output.extend(flattened);
             }
 
             if last_end < outer_span.end {
@@ -545,15 +557,6 @@ pub fn flatten_expression(
         Expr::VarDecl(var_id) => {
             vec![(expr.span, FlatShape::VarDecl(*var_id))]
         }
-
-        Expr::Spread(inner_expr) => {
-            let mut output = vec![(
-                Span::new(expr.span.start, expr.span.start + 3),
-                FlatShape::Operator,
-            )];
-            output.extend(flatten_expression(working_set, inner_expr));
-            output
-        }
     }
 }
 

crates/nu-parser/src/parser.rs

@@ -2807,9 +2807,19 @@ pub fn parse_import_pattern(working_set: &mut StateWorkingSet, spans: &[Span]) -
                     ..
                 } = result
                 {
-                    for expr in list {
+                    for item in list {
-                        let contents = working_set.get_span_contents(expr.span);
+                        match item {
-                        output.push((trim_quotes(contents).to_vec(), expr.span));
+                            ListItem::Item(expr) => {
+                                let contents = working_set.get_span_contents(expr.span);
+                                output.push((trim_quotes(contents).to_vec(), expr.span));
+                            }
+                            ListItem::Spread(_, spread) => {
+                                working_set.error(ParseError::WrongImportPattern(
+                                    "cannot spread in an import pattern".into(),
+                                    spread.span,
+                                ))
+                            }
+                        }
                     }
 
                     import_pattern
@@ -3823,13 +3833,7 @@ pub fn parse_list_expression(
                         _ => Type::Any,
                     };
                     let span = Span::new(curr_span.start, spread_arg.span.end);
-                    let spread_expr = Expression {
+                    (ListItem::Spread(span, spread_arg), elem_ty)
-                        expr: Expr::Spread(Box::new(spread_arg)),
-                        span,
-                        ty: elem_ty.clone(),
-                        custom_completion: None,
-                    };
-                    (spread_expr, elem_ty)
                 } else {
                     let arg = parse_multispan_value(
                         working_set,
@@ -3838,7 +3842,7 @@ pub fn parse_list_expression(
                         element_shape,
                     );
                     let ty = arg.ty.clone();
-                    (arg, ty)
+                    (ListItem::Item(arg), ty)
                 };
 
                 if let Some(ref ctype) = contained_type {
@@ -3868,6 +3872,29 @@ pub fn parse_list_expression(
     }
 }
 
+fn parse_table_row(
+    working_set: &mut StateWorkingSet,
+    span: Span,
+) -> Result<(Vec<Expression>, Span), Span> {
+    let list = parse_list_expression(working_set, span, &SyntaxShape::Any);
+    let Expression {
+        expr: Expr::List(list),
+        span,
+        ..
+    } = list
+    else {
+        unreachable!("the item must be a list")
+    };
+
+    list.into_iter()
+        .map(|item| match item {
+            ListItem::Item(expr) => Ok(expr),
+            ListItem::Spread(_, spread) => Err(spread.span),
+        })
+        .collect::<Result<_, _>>()
+        .map(|exprs| (exprs, span))
+}
+
 fn parse_table_expression(working_set: &mut StateWorkingSet, span: Span) -> Expression {
     let bytes = working_set.get_span_contents(span);
     let inner_span = {
@@ -3905,71 +3932,75 @@ fn parse_table_expression(working_set: &mut StateWorkingSet, span: Span) -> Expr
     {
         return parse_list_expression(working_set, span, &SyntaxShape::Any);
     };
-    let head = parse_list_expression(working_set, first.span, &SyntaxShape::Any);
+    let head = parse_table_row(working_set, first.span);
-    let head = {
-        let Expression {
-            expr: Expr::List(vals),
-            ..
-        } = head
-        else {
-            unreachable!("head must be a list by now")
-        };
-
-        vals
-    };
 
     let errors = working_set.parse_errors.len();
 
-    let rows = rest
+    let (head, rows) = match head {
-        .iter()
+        Ok((head, _)) => {
-        .fold(Vec::with_capacity(rest.len()), |mut acc, it| {
+            let rows = rest
-            use std::cmp::Ordering;
+                .iter()
-            let text = working_set.get_span_contents(it.span).to_vec();
+                .filter_map(|it| {
-            match text.as_slice() {
+                    use std::cmp::Ordering;
-                b"," => acc,
-                _ if !&text.starts_with(b"[") => {
-                    let err = ParseError::LabeledErrorWithHelp {
-                        error: String::from("Table item not list"),
-                        label: String::from("not a list"),
-                        span: it.span,
-                        help: String::from("All table items must be lists"),
-                    };
-                    working_set.error(err);
-                    acc
-                }
-                _ => {
-                    let ls = parse_list_expression(working_set, it.span, &SyntaxShape::Any);
-                    let Expression {
-                        expr: Expr::List(item),
-                        span,
-                        ..
-                    } = ls
-                    else {
-                        unreachable!("the item must be a list")
-                    };
 
-                    match item.len().cmp(&head.len()) {
+                    match working_set.get_span_contents(it.span) {
-                        Ordering::Less => {
+                        b"," => None,
-                            let err = ParseError::MissingColumns(head.len(), span);
+                        text if !text.starts_with(b"[") => {
-                            working_set.error(err);
+                            let err = ParseError::LabeledErrorWithHelp {
-                        }
+                                error: String::from("Table item not list"),
-                        Ordering::Greater => {
+                                label: String::from("not a list"),
-                            let span = {
+                                span: it.span,
-                                let start = item[head.len()].span.start;
+                                help: String::from("All table items must be lists"),
-                                let end = span.end;
-                                Span::new(start, end)
                             };
-                            let err = ParseError::ExtraColumns(head.len(), span);
                             working_set.error(err);
+                            None
                         }
-                        Ordering::Equal => {}
+                        _ => match parse_table_row(working_set, it.span) {
+                            Ok((list, span)) => {
+                                match list.len().cmp(&head.len()) {
+                                    Ordering::Less => {
+                                        let err = ParseError::MissingColumns(head.len(), span);
+                                        working_set.error(err);
+                                    }
+                                    Ordering::Greater => {
+                                        let span = {
+                                            let start = list[head.len()].span.start;
+                                            let end = span.end;
+                                            Span::new(start, end)
+                                        };
+                                        let err = ParseError::ExtraColumns(head.len(), span);
+                                        working_set.error(err);
+                                    }
+                                    Ordering::Equal => {}
+                                }
+                                Some(list)
+                            }
+                            Err(span) => {
+                                let err = ParseError::LabeledError(
+                                    String::from("Cannot spread in a table row"),
+                                    String::from("invalid spread here"),
+                                    span,
+                                );
+                                working_set.error(err);
+                                None
+                            }
+                        },
                     }
+                })
+                .collect();
 
-                    acc.push(item);
+            (head, rows)
-                    acc
+        }
-                }
+        Err(span) => {
-            }
+            let err = ParseError::LabeledError(
-        });
+                String::from("Cannot spread in a table row"),
+                String::from("invalid spread here"),
+                span,
+            );
+            working_set.error(err);
+            (Vec::new(), Vec::new())
+        }
+    };
 
     let ty = if working_set.parse_errors.len() == errors {
         let (ty, errs) = table_type(&head, &rows);
@@ -5950,9 +5981,9 @@ pub fn discover_captures_in_expr(
         Expr::Keyword(_, _, expr) => {
             discover_captures_in_expr(working_set, expr, seen, seen_blocks, output)?;
         }
-        Expr::List(exprs) => {
+        Expr::List(list) => {
-            for expr in exprs {
+            for item in list {
-                discover_captures_in_expr(working_set, expr, seen, seen_blocks, output)?;
+                discover_captures_in_expr(working_set, item.expr(), seen, seen_blocks, output)?;
             }
         }
         Expr::Operator(_) => {}
@@ -6071,9 +6102,6 @@ pub fn discover_captures_in_expr(
         Expr::VarDecl(var_id) => {
             seen.push(*var_id);
         }
-        Expr::Spread(expr) => {
-            discover_captures_in_expr(working_set, expr, seen, seen_blocks, output)?;
-        }
     }
     Ok(())
 }

crates/nu-protocol/src/ast/expr.rs

@@ -34,7 +34,7 @@ pub enum Expr {
     Block(BlockId),
     Closure(BlockId),
     MatchBlock(Vec<(MatchPattern, Expression)>),
-    List(Vec<Expression>),
+    List(Vec<ListItem>),
     Table(Vec<Expression>, Vec<Vec<Expression>>),
     Record(Vec<RecordItem>),
     Keyword(Vec<u8>, Span, Box<Expression>),
@@ -50,7 +50,6 @@ pub enum Expr {
     Overlay(Option<BlockId>), // block ID of the overlay's origin module
     Signature(Box<Signature>),
     StringInterpolation(Vec<Expression>),
-    Spread(Box<Expression>),
     Nothing,
     Garbage,
 }
@@ -99,7 +98,6 @@ impl Expr {
             | Expr::ImportPattern(_)
             | Expr::Overlay(_)
             | Expr::Signature(_)
-            | Expr::Spread(_)
             | Expr::Garbage => {
                 // These should be impossible to pipe to,
                 // but even it is, the pipeline output is not used in any way.
@@ -129,3 +127,23 @@ pub enum RecordItem {
     /// Span for the "..." and the expression that's being spread
     Spread(Span, Expression),
 }
+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub enum ListItem {
+    /// A normal expression
+    Item(Expression),
+    /// Span for the "..." and the expression that's being spread
+    Spread(Span, Expression),
+}
+
+impl ListItem {
+    pub fn expr(&self) -> &Expression {
+        let (ListItem::Item(expr) | ListItem::Spread(_, expr)) = self;
+        expr
+    }
+
+    pub fn expr_mut(&mut self) -> &mut Expression {
+        let (ListItem::Item(expr) | ListItem::Spread(_, expr)) = self;
+        expr
+    }
+}

crates/nu-protocol/src/ast/expression.rs

@@ -86,13 +86,6 @@ impl Expression {
         }
     }
 
-    pub fn as_list(&self) -> Option<Vec<Expression>> {
-        match &self.expr {
-            Expr::List(list) => Some(list.clone()),
-            _ => None,
-        }
-    }
-
     pub fn as_keyword(&self) -> Option<&Expression> {
         match &self.expr {
             Expr::Keyword(_, _, expr) => Some(expr),
@@ -213,8 +206,8 @@ impl Expression {
             Expr::Int(_) => false,
             Expr::Keyword(_, _, expr) => expr.has_in_variable(working_set),
             Expr::List(list) => {
-                for l in list {
+                for item in list {
-                    if l.has_in_variable(working_set) {
+                    if item.expr().has_in_variable(working_set) {
                         return true;
                     }
                 }
@@ -304,7 +297,6 @@ impl Expression {
             Expr::ValueWithUnit(expr, _) => expr.has_in_variable(working_set),
             Expr::Var(var_id) => *var_id == IN_VARIABLE_ID,
             Expr::VarDecl(_) => false,
-            Expr::Spread(expr) => expr.has_in_variable(working_set),
         }
     }
 
@@ -394,8 +386,9 @@ impl Expression {
             Expr::Int(_) => {}
             Expr::Keyword(_, _, expr) => expr.replace_span(working_set, replaced, new_span),
             Expr::List(list) => {
-                for l in list {
+                for item in list {
-                    l.replace_span(working_set, replaced, new_span)
+                    item.expr_mut()
+                        .replace_span(working_set, replaced, new_span);
                 }
             }
             Expr::Operator(_) => {}
@@ -456,7 +449,6 @@ impl Expression {
             Expr::ValueWithUnit(expr, _) => expr.replace_span(working_set, replaced, new_span),
             Expr::Var(_) => {}
             Expr::VarDecl(_) => {}
-            Expr::Spread(expr) => expr.replace_span(working_set, replaced, new_span),
         }
     }
 }

crates/nu-protocol/src/debugger/profiler.rs

@@ -253,7 +253,6 @@ fn expr_to_string(engine_state: &EngineState, expr: &Expr) -> String {
         Expr::Record(_) => "record".to_string(),
         Expr::RowCondition(_) => "row condition".to_string(),
         Expr::Signature(_) => "signature".to_string(),
-        Expr::Spread(_) => "spread".to_string(),
         Expr::String(_) => "string".to_string(),
         Expr::StringInterpolation(_) => "string interpolation".to_string(),
         Expr::Subexpression(_) => "subexpression".to_string(),

crates/nu-protocol/src/eval_base.rs

@@ -1,7 +1,7 @@
 use crate::{
     ast::{
         eval_operator, Assignment, Bits, Boolean, Call, Comparison, Expr, Expression,
-        ExternalArgument, Math, Operator, RecordItem,
+        ExternalArgument, ListItem, Math, Operator, RecordItem,
     },
     debugger::DebugContext,
     Config, IntoInterruptiblePipelineData, Range, Record, ShellError, Span, Value, VarId,
@@ -40,15 +40,15 @@ pub trait Eval {
                 value.follow_cell_path(&cell_path.tail, false)
             }
             Expr::DateTime(dt) => Ok(Value::date(*dt, expr.span)),
-            Expr::List(x) => {
+            Expr::List(list) => {
                 let mut output = vec![];
-                for expr in x {
+                for item in list {
-                    match &expr.expr {
+                    match item {
-                        Expr::Spread(expr) => match Self::eval::<D>(state, mut_state, expr)? {
+                        ListItem::Item(expr) => output.push(Self::eval::<D>(state, mut_state, expr)?),
-                            Value::List { mut vals, .. } => output.append(&mut vals),
+                        ListItem::Spread(_, expr) => match Self::eval::<D>(state, mut_state, expr)? {
+                            Value::List { vals, .. } => output.extend(vals),
                             _ => return Err(ShellError::CannotSpreadAsList { span: expr.span }),
                         },
-                        _ => output.push(Self::eval::<D>(state, mut_state, expr)?),
                     }
                 }
                 Ok(Value::list(output, expr.span))
@@ -295,7 +295,6 @@ pub trait Eval {
             | Expr::VarDecl(_)
             | Expr::ImportPattern(_)
             | Expr::Signature(_)
-            | Expr::Spread(_)
             | Expr::Operator(_)
             | Expr::Garbage => Self::unreachable(expr),
         }