From 6ccd547d814464b9500a260d68b0dd94320e20a7 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Thu, 18 Apr 2024 11:21:05 +0000 Subject: [PATCH] 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 | 8 +- crates/nu-command/src/formats/from/nuon.rs | 25 ++- crates/nu-parser/src/flatten.rs | 47 +++--- crates/nu-parser/src/parser.rs | 174 ++++++++++++-------- crates/nu-protocol/src/ast/expr.rs | 24 ++- crates/nu-protocol/src/ast/expression.rs | 18 +- crates/nu-protocol/src/debugger/profiler.rs | 1 - crates/nu-protocol/src/eval_base.rs | 15 +- 8 files changed, 179 insertions(+), 133 deletions(-) diff --git a/crates/nu-cli/src/syntax_highlight.rs b/crates/nu-cli/src/syntax_highlight.rs index 20addeab10..7bb6abd7ca 100644 --- a/crates/nu-cli/src/syntax_highlight.rs +++ b/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 { - find_in_expr_or_continue!(inner_expr); + for item in list { + let expr = item.expr(); + find_in_expr_or_continue!(expr); } None } diff --git a/crates/nu-command/src/formats/from/nuon.rs b/crates/nu-command/src/formats/from/nuon.rs index 52ef62bb48..70919200db 100644 --- a/crates/nu-command/src/formats/from/nuon.rs +++ b/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(), diff --git a/crates/nu-parser/src/flatten.rs b/crates/nu-parser/src/flatten.rs index 067052840d..9a5c0afcdd 100644 --- a/crates/nu-parser/src/flatten.rs +++ b/crates/nu-parser/src/flatten.rs @@ -1,7 +1,8 @@ use nu_protocol::{ ast::{ - Argument, Block, Expr, Expression, ExternalArgument, ImportPatternMember, MatchPattern, - PathMember, Pattern, Pipeline, PipelineElement, PipelineRedirection, RecordItem, + Argument, Block, Expr, Expression, ExternalArgument, ImportPatternMember, ListItem, + 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 { - let flattened = flatten_expression(working_set, l); + for item in list { + match item { + ListItem::Item(expr) => { + let flattened = flatten_expression(working_set, expr); - if let Some(first) = flattened.first() { - if first.0.start > last_end { - output.push((Span::new(last_end, first.0.start), FlatShape::List)); + if let Some(first) = flattened.first() { + if first.0.start > last_end { + 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 - } } } diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 8ea77e6051..8f32dff3fb 100644 --- a/crates/nu-parser/src/parser.rs +++ b/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 { - let contents = working_set.get_span_contents(expr.span); - output.push((trim_quotes(contents).to_vec(), expr.span)); + for item in list { + match item { + 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 { - expr: Expr::Spread(Box::new(spread_arg)), - span, - ty: elem_ty.clone(), - custom_completion: None, - }; - (spread_expr, elem_ty) + (ListItem::Spread(span, spread_arg), 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, 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::>() + .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 = { - let Expression { - expr: Expr::List(vals), - .. - } = head - else { - unreachable!("head must be a list by now") - }; - - vals - }; + let head = parse_table_row(working_set, first.span); let errors = working_set.parse_errors.len(); - let rows = rest - .iter() - .fold(Vec::with_capacity(rest.len()), |mut acc, it| { - use std::cmp::Ordering; - let text = working_set.get_span_contents(it.span).to_vec(); - match text.as_slice() { - 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") - }; + let (head, rows) = match head { + Ok((head, _)) => { + let rows = rest + .iter() + .filter_map(|it| { + use std::cmp::Ordering; - match item.len().cmp(&head.len()) { - Ordering::Less => { - let err = ParseError::MissingColumns(head.len(), span); - working_set.error(err); - } - Ordering::Greater => { - let span = { - let start = item[head.len()].span.start; - let end = span.end; - Span::new(start, end) + match working_set.get_span_contents(it.span) { + b"," => None, + text 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"), }; - 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); - acc - } - } - }); + (head, rows) + } + 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) => { - for expr in exprs { - discover_captures_in_expr(working_set, expr, seen, seen_blocks, output)?; + Expr::List(list) => { + for item in list { + 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(()) } diff --git a/crates/nu-protocol/src/ast/expr.rs b/crates/nu-protocol/src/ast/expr.rs index 85839d63a1..95ca72ef00 100644 --- a/crates/nu-protocol/src/ast/expr.rs +++ b/crates/nu-protocol/src/ast/expr.rs @@ -34,7 +34,7 @@ pub enum Expr { Block(BlockId), Closure(BlockId), MatchBlock(Vec<(MatchPattern, Expression)>), - List(Vec), + List(Vec), Table(Vec, Vec>), Record(Vec), Keyword(Vec, Span, Box), @@ -50,7 +50,6 @@ pub enum Expr { Overlay(Option), // block ID of the overlay's origin module Signature(Box), StringInterpolation(Vec), - Spread(Box), 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 + } +} diff --git a/crates/nu-protocol/src/ast/expression.rs b/crates/nu-protocol/src/ast/expression.rs index 70e5e51804..4e91ba5e62 100644 --- a/crates/nu-protocol/src/ast/expression.rs +++ b/crates/nu-protocol/src/ast/expression.rs @@ -86,13 +86,6 @@ impl Expression { } } - pub fn as_list(&self) -> Option> { - 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 { - if l.has_in_variable(working_set) { + for item in list { + 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 { - l.replace_span(working_set, replaced, new_span) + for item in list { + 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), } } } diff --git a/crates/nu-protocol/src/debugger/profiler.rs b/crates/nu-protocol/src/debugger/profiler.rs index a76c027e14..427c51d9c3 100644 --- a/crates/nu-protocol/src/debugger/profiler.rs +++ b/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(), diff --git a/crates/nu-protocol/src/eval_base.rs b/crates/nu-protocol/src/eval_base.rs index 7c240c0b1f..2c74ff9f30 100644 --- a/crates/nu-protocol/src/eval_base.rs +++ b/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 { - match &expr.expr { - Expr::Spread(expr) => match Self::eval::(state, mut_state, expr)? { - Value::List { mut vals, .. } => output.append(&mut vals), + for item in list { + match item { + ListItem::Item(expr) => output.push(Self::eval::(state, mut_state, expr)?), + ListItem::Spread(_, expr) => match Self::eval::(state, mut_state, expr)? { + Value::List { vals, .. } => output.extend(vals), _ => return Err(ShellError::CannotSpreadAsList { span: expr.span }), }, - _ => output.push(Self::eval::(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), }