diff --git a/crates/nu-cli/src/syntax_highlight.rs b/crates/nu-cli/src/syntax_highlight.rs index d62484afd..c3e88190a 100644 --- a/crates/nu-cli/src/syntax_highlight.rs +++ b/crates/nu-cli/src/syntax_highlight.rs @@ -385,6 +385,7 @@ fn find_matching_block_end_in_expr( Argument::Named((_, _, opt_expr)) => opt_expr.as_ref(), Argument::Positional(inner_expr) => Some(inner_expr), Argument::Unknown(inner_expr) => Some(inner_expr), + Argument::Spread(inner_expr) => Some(inner_expr), }; if let Some(inner_expr) = opt_expr { diff --git a/crates/nu-command/src/bytes/build_.rs b/crates/nu-command/src/bytes/build_.rs index e39e15e21..99ea3cac9 100644 --- a/crates/nu-command/src/bytes/build_.rs +++ b/crates/nu-command/src/bytes/build_.rs @@ -1,4 +1,4 @@ -use nu_engine::eval_expression; +use nu_engine::{eval_expression, CallExt}; use nu_protocol::ast::Call; use nu_protocol::engine::{Command, EngineState, Stack}; use nu_protocol::{ @@ -48,8 +48,7 @@ impl Command for BytesBuild { _input: PipelineData, ) -> Result { let mut output = vec![]; - for expr in call.positional_iter() { - let val = eval_expression(engine_state, stack, expr)?; + for val in call.rest_iter_flattened(0, |expr| eval_expression(engine_state, stack, expr))? { match val { Value::Binary { mut val, .. } => output.append(&mut val), // Explicitly propagate errors instead of dropping them. diff --git a/crates/nu-command/src/debug/explain.rs b/crates/nu-command/src/debug/explain.rs index 6395e9133..3d1018f3d 100644 --- a/crates/nu-command/src/debug/explain.rs +++ b/crates/nu-command/src/debug/explain.rs @@ -191,6 +191,24 @@ fn get_arguments(engine_state: &EngineState, stack: &mut Stack, call: Call) -> V let arg_value_name_span_start = evaled_span.start as i64; let arg_value_name_span_end = evaled_span.end as i64; + let record = record! { + "arg_type" => Value::string(arg_type, span), + "name" => Value::string(arg_value_name, inner_expr.span), + "type" => Value::string(arg_value_type, span), + "span_start" => Value::int(arg_value_name_span_start, span), + "span_end" => Value::int(arg_value_name_span_end, span), + }; + arg_value.push(Value::record(record, inner_expr.span)); + } + Argument::Spread(inner_expr) => { + let arg_type = "spread"; + let evaluated_expression = get_expression_as_value(engine_state, stack, inner_expr); + let arg_value_name = debug_string_without_formatting(&evaluated_expression); + let arg_value_type = &evaluated_expression.get_type().to_string(); + let evaled_span = evaluated_expression.span(); + let arg_value_name_span_start = evaled_span.start as i64; + let arg_value_name_span_end = evaled_span.end as i64; + let record = record! { "arg_type" => Value::string(arg_type, span), "name" => Value::string(arg_value_name, inner_expr.span), diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index cfd9e71eb..90b9b85d8 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -1,8 +1,9 @@ use nu_cmd_base::hook::eval_hook; use nu_engine::env_to_strings; +use nu_engine::eval_expression; use nu_engine::CallExt; use nu_protocol::{ - ast::{Call, Expr, Expression}, + ast::{Call, Expr}, did_you_mean, engine::{Command, EngineState, Stack}, Category, Example, ListStream, PipelineData, RawStream, ShellError, Signature, Span, Spanned, @@ -113,7 +114,6 @@ pub fn create_external_command( trim_end_newline: bool, ) -> Result { let name: Spanned = call.req(engine_state, stack, 0)?; - let args: Vec = call.rest(engine_state, stack, 1)?; // Translate environment variables from Values to Strings let env_vars_str = env_to_strings(engine_state, stack)?; @@ -132,11 +132,24 @@ pub fn create_external_command( } let mut spanned_args = vec![]; - let args_expr: Vec = call.positional_iter().skip(1).cloned().collect(); let mut arg_keep_raw = vec![]; - for (one_arg, one_arg_expr) in args.into_iter().zip(args_expr) { - match one_arg { + for (arg, spread) in call.rest_iter(1) { + // TODO: Disallow automatic spreading entirely later. This match block will + // have to be refactored, and lists will have to be disallowed in the parser too + match eval_expression(engine_state, stack, arg)? { Value::List { vals, .. } => { + if !spread { + nu_protocol::report_error_new( + engine_state, + &ShellError::GenericError { + error: "Automatically spreading lists is deprecated".into(), + msg: "Spreading lists automatically when calling external commands is deprecated and will be removed in 0.91.".into(), + span: Some(arg.span), + help: Some("Use the spread operator (put a '...' before the argument)".into()), + inner: vec![], + }, + ); + } // turn all the strings in the array into params. // Example: one_arg may be something like ["ls" "-a"] // convert it to "ls" "-a" @@ -147,15 +160,20 @@ pub fn create_external_command( } } val => { - spanned_args.push(value_as_spanned(val)?); - match one_arg_expr.expr { - // refer to `parse_dollar_expr` function - // the expression type of $variable_name, $"($variable_name)" - // will be Expr::StringInterpolation, Expr::FullCellPath - Expr::StringInterpolation(_) | Expr::FullCellPath(_) => arg_keep_raw.push(true), - _ => arg_keep_raw.push(false), + if spread { + return Err(ShellError::CannotSpreadAsList { span: arg.span }); + } else { + spanned_args.push(value_as_spanned(val)?); + match arg.expr { + // refer to `parse_dollar_expr` function + // the expression type of $variable_name, $"($variable_name)" + // will be Expr::StringInterpolation, Expr::FullCellPath + Expr::StringInterpolation(_) | Expr::FullCellPath(_) => { + arg_keep_raw.push(true) + } + _ => arg_keep_raw.push(false), + } } - {} } } } diff --git a/crates/nu-command/tests/commands/def.rs b/crates/nu-command/tests/commands/def.rs index f4fafc4d7..67e9c82d6 100644 --- a/crates/nu-command/tests/commands/def.rs +++ b/crates/nu-command/tests/commands/def.rs @@ -197,7 +197,7 @@ fn def_wrapped_with_block() { #[test] fn def_wrapped_from_module() { let actual = nu!(r#"module spam { - export def --wrapped my-echo [...rest] { ^echo $rest } + export def --wrapped my-echo [...rest] { ^echo ...$rest } } use spam diff --git a/crates/nu-command/tests/commands/exec.rs b/crates/nu-command/tests/commands/exec.rs index 860bc8a61..deb268659 100644 --- a/crates/nu-command/tests/commands/exec.rs +++ b/crates/nu-command/tests/commands/exec.rs @@ -49,7 +49,7 @@ fn exec_misc_values() { let actual = nu!( cwd: dirs.test(), pipeline( r#" - nu -c 'let x = "abc"; exec nu --testbin cococo $x [ a b c ]' + nu -c 'let x = "abc"; exec nu --testbin cococo $x ...[ a b c ]' "# )); diff --git a/crates/nu-command/tests/commands/run_external.rs b/crates/nu-command/tests/commands/run_external.rs index 4e6130639..638d56497 100644 --- a/crates/nu-command/tests/commands/run_external.rs +++ b/crates/nu-command/tests/commands/run_external.rs @@ -328,7 +328,7 @@ fn redirect_combine() { let actual = nu!( cwd: dirs.test(), pipeline( r#" - run-external --redirect-combine sh [-c 'echo Foo; echo >&2 Bar'] + run-external --redirect-combine sh ...[-c 'echo Foo; echo >&2 Bar'] "# )); diff --git a/crates/nu-engine/src/call_ext.rs b/crates/nu-engine/src/call_ext.rs index f21821f3d..c8d2dc321 100644 --- a/crates/nu-engine/src/call_ext.rs +++ b/crates/nu-engine/src/call_ext.rs @@ -1,8 +1,8 @@ use nu_protocol::{ - ast::Call, + ast::{Call, Expression}, engine::{EngineState, Stack, StateWorkingSet}, eval_const::eval_constant, - FromValue, ShellError, + FromValue, ShellError, Value, }; use crate::eval_expression; @@ -34,6 +34,10 @@ pub trait CallExt { starting_pos: usize, ) -> Result, ShellError>; + fn rest_iter_flattened(&self, start: usize, eval: F) -> Result, ShellError> + where + F: FnMut(&Expression) -> Result; + fn opt( &self, engine_state: &EngineState, @@ -98,8 +102,9 @@ impl CallExt for Call { ) -> Result, ShellError> { let mut output = vec![]; - for expr in self.positional_iter().skip(starting_pos) { - let result = eval_expression(engine_state, stack, expr)?; + for result in self.rest_iter_flattened(starting_pos, |expr| { + eval_expression(engine_state, stack, expr) + })? { output.push(FromValue::from_value(result)?); } @@ -113,14 +118,36 @@ impl CallExt for Call { ) -> Result, ShellError> { let mut output = vec![]; - for expr in self.positional_iter().skip(starting_pos) { - let result = eval_constant(working_set, expr)?; + for result in + self.rest_iter_flattened(starting_pos, |expr| eval_constant(working_set, expr))? + { output.push(FromValue::from_value(result)?); } Ok(output) } + fn rest_iter_flattened(&self, start: usize, mut eval: F) -> Result, ShellError> + where + F: FnMut(&Expression) -> Result, + { + let mut output = Vec::new(); + + for (expr, spread) in self.rest_iter(start) { + let result = eval(expr)?; + if spread { + match result { + Value::List { mut vals, .. } => output.append(&mut vals), + _ => return Err(ShellError::CannotSpreadAsList { span: expr.span }), + } + } else { + output.push(result); + } + } + + Ok(output) + } + fn opt( &self, engine_state: &EngineState, diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 4fe6f17e3..550eee6e1 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -1,9 +1,9 @@ -use crate::{current_dir_str, get_full_help}; +use crate::{call_ext::CallExt, current_dir_str, get_full_help}; use nu_path::expand_path_with; use nu_protocol::{ ast::{ - Argument, Assignment, Block, Call, Expr, Expression, PathMember, PipelineElement, - Redirection, + Argument, Assignment, Block, Call, Expr, Expression, ExternalArgument, PathMember, + PipelineElement, Redirection, }, engine::{Closure, EngineState, Stack}, eval_base::Eval, @@ -66,11 +66,11 @@ pub fn eval_call( if let Some(rest_positional) = decl.signature().rest_positional { let mut rest_items = vec![]; - for arg in call.positional_iter().skip( + for result in call.rest_iter_flattened( decl.signature().required_positional.len() + decl.signature().optional_positional.len(), - ) { - let result = eval_expression(engine_state, caller_stack, arg)?; + |expr| eval_expression(engine_state, caller_stack, expr), + )? { rest_items.push(result); } @@ -182,7 +182,7 @@ fn eval_external( engine_state: &EngineState, stack: &mut Stack, head: &Expression, - args: &[Expression], + args: &[ExternalArgument], input: PipelineData, redirect_target: RedirectTarget, is_subexpression: bool, @@ -198,7 +198,10 @@ fn eval_external( call.add_positional(head.clone()); for arg in args { - call.add_positional(arg.clone()) + match arg { + ExternalArgument::Regular(expr) => call.add_positional(expr.clone()), + ExternalArgument::Spread(expr) => call.add_spread(expr.clone()), + } } match redirect_target { @@ -947,7 +950,7 @@ impl Eval for EvalRuntime { engine_state: &EngineState, stack: &mut Stack, head: &Expression, - args: &[Expression], + args: &[ExternalArgument], is_subexpression: bool, _: Span, ) -> Result { diff --git a/crates/nu-parser/src/flatten.rs b/crates/nu-parser/src/flatten.rs index 6a82a32af..dc37822a0 100644 --- a/crates/nu-parser/src/flatten.rs +++ b/crates/nu-parser/src/flatten.rs @@ -1,6 +1,6 @@ use nu_protocol::ast::{ - Block, Expr, Expression, ImportPatternMember, MatchPattern, PathMember, Pattern, Pipeline, - PipelineElement, RecordItem, + Argument, Block, Expr, Expression, ExternalArgument, ImportPatternMember, MatchPattern, + PathMember, Pattern, Pipeline, PipelineElement, RecordItem, }; use nu_protocol::{engine::StateWorkingSet, Span}; use nu_protocol::{DeclId, VarId}; @@ -193,17 +193,28 @@ pub fn flatten_expression( } let mut args = vec![]; - for positional in call.positional_iter() { - let flattened = flatten_expression(working_set, positional); - args.extend(flattened); - } - for named in call.named_iter() { - if named.0.span.end != 0 { - // Ignore synthetic flags - args.push((named.0.span, FlatShape::Flag)); - } - if let Some(expr) = &named.2 { - args.extend(flatten_expression(working_set, expr)); + for arg in &call.arguments { + match arg { + Argument::Positional(positional) | Argument::Unknown(positional) => { + let flattened = flatten_expression(working_set, positional); + args.extend(flattened); + } + Argument::Named(named) => { + if named.0.span.end != 0 { + // Ignore synthetic flags + args.push((named.0.span, FlatShape::Flag)); + } + if let Some(expr) = &named.2 { + args.extend(flatten_expression(working_set, expr)); + } + } + Argument::Spread(expr) => { + args.push(( + Span::new(expr.span.start - 3, expr.span.start), + FlatShape::Operator, + )); + args.extend(flatten_expression(working_set, expr)); + } } } // sort these since flags and positional args can be intermixed @@ -231,15 +242,24 @@ pub fn flatten_expression( for arg in args { //output.push((*arg, FlatShape::ExternalArg)); match arg { - Expression { - expr: Expr::String(..), - span, - .. - } => { - output.push((*span, FlatShape::ExternalArg)); - } - _ => { - output.extend(flatten_expression(working_set, arg)); + ExternalArgument::Regular(expr) => match expr { + Expression { + expr: Expr::String(..), + span, + .. + } => { + output.push((*span, FlatShape::ExternalArg)); + } + _ => { + output.extend(flatten_expression(working_set, expr)); + } + }, + ExternalArgument::Spread(expr) => { + output.push(( + Span::new(expr.span.start - 3, expr.span.start), + FlatShape::Operator, + )); + output.extend(flatten_expression(working_set, expr)); } } } diff --git a/crates/nu-parser/src/known_external.rs b/crates/nu-parser/src/known_external.rs index df0ee6122..d808d264a 100644 --- a/crates/nu-parser/src/known_external.rs +++ b/crates/nu-parser/src/known_external.rs @@ -106,6 +106,7 @@ impl Command for KnownExternal { } } Argument::Unknown(unknown) => extern_call.add_unknown(unknown.clone()), + Argument::Spread(args) => extern_call.add_spread(args.clone()), } } diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 9c203d37d..c3795e5ad 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -12,9 +12,9 @@ use nu_engine::DIR_VAR_PARSER_INFO; use nu_protocol::{ ast::{ Argument, Assignment, Bits, Block, Boolean, Call, CellPath, Comparison, Expr, Expression, - FullCellPath, ImportPattern, ImportPatternHead, ImportPatternMember, MatchPattern, Math, - Operator, PathMember, Pattern, Pipeline, PipelineElement, RangeInclusion, RangeOperator, - RecordItem, + ExternalArgument, FullCellPath, ImportPattern, ImportPatternHead, ImportPatternMember, + MatchPattern, Math, Operator, PathMember, Pattern, Pipeline, PipelineElement, + RangeInclusion, RangeOperator, RecordItem, }, engine::StateWorkingSet, eval_const::eval_constant, @@ -266,13 +266,22 @@ pub fn check_name<'a>(working_set: &mut StateWorkingSet, spans: &'a [Span]) -> O } } -fn parse_external_arg(working_set: &mut StateWorkingSet, span: Span) -> Expression { +fn parse_external_arg(working_set: &mut StateWorkingSet, span: Span) -> ExternalArgument { let contents = working_set.get_span_contents(span); if contents.starts_with(b"$") || contents.starts_with(b"(") { - parse_dollar_expr(working_set, span) + ExternalArgument::Regular(parse_dollar_expr(working_set, span)) } else if contents.starts_with(b"[") { - parse_list_expression(working_set, span, &SyntaxShape::Any) + ExternalArgument::Regular(parse_list_expression(working_set, span, &SyntaxShape::Any)) + } else if contents.len() > 3 + && contents.starts_with(b"...") + && (contents[3] == b'$' || contents[3] == b'[' || contents[3] == b'(') + { + ExternalArgument::Spread(parse_value( + working_set, + Span::new(span.start + 3, span.end), + &SyntaxShape::List(Box::new(SyntaxShape::Any)), + )) } else { // Eval stage trims the quotes, so we don't have to do the same thing when parsing. let contents = if contents.starts_with(b"\"") { @@ -285,12 +294,12 @@ fn parse_external_arg(working_set: &mut StateWorkingSet, span: Span) -> Expressi String::from_utf8_lossy(contents).to_string() }; - Expression { + ExternalArgument::Regular(Expression { expr: Expr::String(contents), span, ty: Type::String, custom_completion: None, - } + }) } } @@ -978,6 +987,49 @@ pub fn parse_internal_call( continue; } + { + let contents = working_set.get_span_contents(spans[spans_idx]); + + if contents.len() > 3 + && contents.starts_with(b"...") + && (contents[3] == b'$' || contents[3] == b'[' || contents[3] == b'(') + { + if signature.rest_positional.is_none() && !signature.allows_unknown_args { + working_set.error(ParseError::UnexpectedSpreadArg( + signature.call_signature(), + arg_span, + )); + call.add_positional(Expression::garbage(arg_span)); + } else if positional_idx < signature.required_positional.len() { + working_set.error(ParseError::MissingPositional( + signature.required_positional[positional_idx].name.clone(), + Span::new(spans[spans_idx].start, spans[spans_idx].start), + signature.call_signature(), + )); + call.add_positional(Expression::garbage(arg_span)); + } else { + let rest_shape = match &signature.rest_positional { + Some(arg) => arg.shape.clone(), + None => SyntaxShape::Any, + }; + // Parse list of arguments to be spread + let args = parse_value( + working_set, + Span::new(arg_span.start + 3, arg_span.end), + &SyntaxShape::List(Box::new(rest_shape)), + ); + + call.add_spread(args); + // Let the parser know that it's parsing rest arguments now + positional_idx = + signature.required_positional.len() + signature.optional_positional.len(); + } + + spans_idx += 1; + continue; + } + } + // Parse a positional arg if there is one if let Some(positional) = signature.get_positional(positional_idx) { let end = calculate_end_span(working_set, &signature, spans, spans_idx, positional_idx); @@ -5885,22 +5937,27 @@ pub fn discover_captures_in_expr( } } - for named in call.named_iter() { - if let Some(arg) = &named.2 { - discover_captures_in_expr(working_set, arg, seen, seen_blocks, output)?; + for arg in &call.arguments { + match arg { + Argument::Named(named) => { + if let Some(arg) = &named.2 { + discover_captures_in_expr(working_set, arg, seen, seen_blocks, output)?; + } + } + Argument::Positional(expr) + | Argument::Unknown(expr) + | Argument::Spread(expr) => { + discover_captures_in_expr(working_set, expr, seen, seen_blocks, output)?; + } } } - - for positional in call.positional_iter() { - discover_captures_in_expr(working_set, positional, seen, seen_blocks, output)?; - } } Expr::CellPath(_) => {} Expr::DateTime(_) => {} - Expr::ExternalCall(head, exprs, _) => { + Expr::ExternalCall(head, args, _) => { discover_captures_in_expr(working_set, head, seen, seen_blocks, output)?; - for expr in exprs { + for ExternalArgument::Regular(expr) | ExternalArgument::Spread(expr) in args { discover_captures_in_expr(working_set, expr, seen, seen_blocks, output)?; } } diff --git a/crates/nu-plugin/src/protocol/evaluated_call.rs b/crates/nu-plugin/src/protocol/evaluated_call.rs index 9366f758d..cf968deb8 100644 --- a/crates/nu-plugin/src/protocol/evaluated_call.rs +++ b/crates/nu-plugin/src/protocol/evaluated_call.rs @@ -1,4 +1,4 @@ -use nu_engine::eval_expression; +use nu_engine::{eval_expression, CallExt}; use nu_protocol::{ ast::Call, engine::{EngineState, Stack}, @@ -33,10 +33,8 @@ impl EvaluatedCall { engine_state: &EngineState, stack: &mut Stack, ) -> Result { - let positional = call - .positional_iter() - .map(|expr| eval_expression(engine_state, stack, expr)) - .collect::, ShellError>>()?; + let positional = + call.rest_iter_flattened(0, |expr| eval_expression(engine_state, stack, expr))?; let mut named = Vec::with_capacity(call.named_len()); for (string, _, expr) in call.named_iter() { diff --git a/crates/nu-protocol/src/ast/call.rs b/crates/nu-protocol/src/ast/call.rs index dc1c20924..f3f19667a 100644 --- a/crates/nu-protocol/src/ast/call.rs +++ b/crates/nu-protocol/src/ast/call.rs @@ -10,6 +10,7 @@ pub enum Argument { Positional(Expression), Named((Spanned, Option>, Option)), Unknown(Expression), // unknown argument used in "fall-through" signatures + Spread(Expression), // a list spread to fill in rest arguments } impl Argument { @@ -30,10 +31,17 @@ impl Argument { Span::new(start, end) } Argument::Unknown(e) => e.span, + Argument::Spread(e) => e.span, } } } +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub enum ExternalArgument { + Regular(Expression), + Spread(Expression), +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Call { /// identifier of the declaration to call @@ -85,6 +93,7 @@ impl Call { Argument::Named(named) => Some(named), Argument::Positional(_) => None, Argument::Unknown(_) => None, + Argument::Spread(_) => None, }) } @@ -96,6 +105,7 @@ impl Call { Argument::Named(named) => Some(named), Argument::Positional(_) => None, Argument::Unknown(_) => None, + Argument::Spread(_) => None, }) } @@ -118,26 +128,45 @@ impl Call { self.arguments.push(Argument::Unknown(unknown)); } + pub fn add_spread(&mut self, args: Expression) { + self.arguments.push(Argument::Spread(args)); + } + pub fn positional_iter(&self) -> impl Iterator { - self.arguments.iter().filter_map(|arg| match arg { - Argument::Named(_) => None, - Argument::Positional(positional) => Some(positional), - Argument::Unknown(unknown) => Some(unknown), - }) + self.arguments + .iter() + .take_while(|arg| match arg { + Argument::Spread(_) => false, // Don't include positional arguments given to rest parameter + _ => true, + }) + .filter_map(|arg| match arg { + Argument::Named(_) => None, + Argument::Positional(positional) => Some(positional), + Argument::Unknown(unknown) => Some(unknown), + Argument::Spread(_) => None, + }) } pub fn positional_iter_mut(&mut self) -> impl Iterator { - self.arguments.iter_mut().filter_map(|arg| match arg { - Argument::Named(_) => None, - Argument::Positional(positional) => Some(positional), - Argument::Unknown(unknown) => Some(unknown), - }) + self.arguments + .iter_mut() + .take_while(|arg| match arg { + Argument::Spread(_) => false, // Don't include positional arguments given to rest parameter + _ => true, + }) + .filter_map(|arg| match arg { + Argument::Named(_) => None, + Argument::Positional(positional) => Some(positional), + Argument::Unknown(unknown) => Some(unknown), + Argument::Spread(_) => None, + }) } pub fn positional_nth(&self, i: usize) -> Option<&Expression> { self.positional_iter().nth(i) } + // TODO this method is never used. Delete? pub fn positional_nth_mut(&mut self, i: usize) -> Option<&mut Expression> { self.positional_iter_mut().nth(i) } @@ -146,6 +175,24 @@ impl Call { self.positional_iter().count() } + /// Returns every argument to the rest parameter, as well as whether each argument + /// is spread or a normal positional argument (true for spread, false for normal) + pub fn rest_iter(&self, start: usize) -> impl Iterator { + // todo maybe rewrite to be more elegant or something + let args = self + .arguments + .iter() + .filter_map(|arg| match arg { + Argument::Named(_) => None, + Argument::Positional(positional) => Some((positional, false)), + Argument::Unknown(unknown) => Some((unknown, false)), + Argument::Spread(args) => Some((args, true)), + }) + .collect::>(); + let spread_start = args.iter().position(|(_, spread)| *spread).unwrap_or(start); + args.into_iter().skip(start.min(spread_start)) + } + pub fn get_parser_info(&self, name: &str) -> Option<&Expression> { self.parser_info.get(name) } diff --git a/crates/nu-protocol/src/ast/expr.rs b/crates/nu-protocol/src/ast/expr.rs index 8879262d7..ef8349ff3 100644 --- a/crates/nu-protocol/src/ast/expr.rs +++ b/crates/nu-protocol/src/ast/expr.rs @@ -1,7 +1,10 @@ use chrono::FixedOffset; use serde::{Deserialize, Serialize}; -use super::{Call, CellPath, Expression, FullCellPath, MatchPattern, Operator, RangeOperator}; +use super::{ + Call, CellPath, Expression, ExternalArgument, FullCellPath, MatchPattern, Operator, + RangeOperator, +}; use crate::{ast::ImportPattern, BlockId, Signature, Span, Spanned, Unit, VarId}; #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -19,7 +22,7 @@ pub enum Expr { Var(VarId), VarDecl(VarId), Call(Box), - ExternalCall(Box, Vec, bool), // head, args, is_subexpression + ExternalCall(Box, Vec, bool), // head, args, is_subexpression Operator(Operator), RowCondition(BlockId), UnaryNot(Box), diff --git a/crates/nu-protocol/src/ast/expression.rs b/crates/nu-protocol/src/ast/expression.rs index d03306caa..9361e0bba 100644 --- a/crates/nu-protocol/src/ast/expression.rs +++ b/crates/nu-protocol/src/ast/expression.rs @@ -1,6 +1,6 @@ use serde::{Deserialize, Serialize}; -use super::{Expr, RecordItem}; +use super::{Argument, Expr, ExternalArgument, RecordItem}; use crate::ast::ImportPattern; use crate::DeclId; use crate::{engine::StateWorkingSet, BlockId, Signature, Span, Type, VarId, IN_VARIABLE_ID}; @@ -162,15 +162,21 @@ impl Expression { Expr::Binary(_) => false, Expr::Bool(_) => false, Expr::Call(call) => { - for positional in call.positional_iter() { - if positional.has_in_variable(working_set) { - return true; - } - } - for named in call.named_iter() { - if let Some(expr) = &named.2 { - if expr.has_in_variable(working_set) { - return true; + for arg in &call.arguments { + match arg { + Argument::Positional(expr) + | Argument::Unknown(expr) + | Argument::Spread(expr) => { + if expr.has_in_variable(working_set) { + return true; + } + } + Argument::Named(named) => { + if let Some(expr) = &named.2 { + if expr.has_in_variable(working_set) { + return true; + } + } } } } @@ -182,8 +188,8 @@ impl Expression { if head.has_in_variable(working_set) { return true; } - for arg in args { - if arg.has_in_variable(working_set) { + for ExternalArgument::Regular(expr) | ExternalArgument::Spread(expr) in args { + if expr.has_in_variable(working_set) { return true; } } @@ -346,12 +352,18 @@ impl Expression { if replaced.contains_span(call.head) { call.head = new_span; } - for positional in call.positional_iter_mut() { - positional.replace_span(working_set, replaced, new_span); - } - for named in call.named_iter_mut() { - if let Some(expr) = &mut named.2 { - expr.replace_span(working_set, replaced, new_span) + for arg in call.arguments.iter_mut() { + match arg { + Argument::Positional(expr) + | Argument::Unknown(expr) + | Argument::Spread(expr) => { + expr.replace_span(working_set, replaced, new_span); + } + Argument::Named(named) => { + if let Some(expr) = &mut named.2 { + expr.replace_span(working_set, replaced, new_span); + } + } } } } @@ -359,8 +371,8 @@ impl Expression { Expr::DateTime(_) => {} Expr::ExternalCall(head, args, _) => { head.replace_span(working_set, replaced, new_span); - for arg in args { - arg.replace_span(working_set, replaced, new_span) + for ExternalArgument::Regular(expr) | ExternalArgument::Spread(expr) in args { + expr.replace_span(working_set, replaced, new_span); } } Expr::Filepath(_) => {} diff --git a/crates/nu-protocol/src/eval_base.rs b/crates/nu-protocol/src/eval_base.rs index da4862a42..dedc8abd1 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, Math, - Operator, RecordItem, + eval_operator, Assignment, Bits, Boolean, Call, Comparison, Expr, Expression, + ExternalArgument, Math, Operator, RecordItem, }, Range, Record, ShellError, Span, Value, VarId, }; @@ -319,7 +319,7 @@ pub trait Eval { state: Self::State<'_>, mut_state: &mut Self::MutState, head: &Expression, - args: &[Expression], + args: &[ExternalArgument], is_subexpression: bool, span: Span, ) -> Result; diff --git a/crates/nu-protocol/src/eval_const.rs b/crates/nu-protocol/src/eval_const.rs index e954518aa..091bf5432 100644 --- a/crates/nu-protocol/src/eval_const.rs +++ b/crates/nu-protocol/src/eval_const.rs @@ -1,5 +1,5 @@ use crate::{ - ast::{Assignment, Block, Call, Expr, Expression, PipelineElement}, + ast::{Assignment, Block, Call, Expr, Expression, ExternalArgument, PipelineElement}, engine::{EngineState, StateWorkingSet}, eval_base::Eval, record, HistoryFileFormat, PipelineData, Record, ShellError, Span, Value, VarId, @@ -317,7 +317,7 @@ impl Eval for EvalConst { _: &StateWorkingSet, _: &mut (), _: &Expression, - _: &[Expression], + _: &[ExternalArgument], _: bool, span: Span, ) -> Result { diff --git a/crates/nu-protocol/src/parse_error.rs b/crates/nu-protocol/src/parse_error.rs index d5acc47a8..f9ad99f5a 100644 --- a/crates/nu-protocol/src/parse_error.rs +++ b/crates/nu-protocol/src/parse_error.rs @@ -484,9 +484,12 @@ pub enum ParseError { #[label("...and here")] Option, ), - #[error("Unexpected spread operator outside list")] - #[diagnostic(code(nu::parser::unexpected_spread_operator))] - UnexpectedSpread(#[label("Spread operator not allowed here")] Span), + #[error("This command does not have a ...rest parameter")] + #[diagnostic( + code(nu::parser::unexpected_spread_arg), + help("To spread arguments, the command needs to define a multi-positional parameter in its signature, such as ...rest") + )] + UnexpectedSpreadArg(String, #[label = "unexpected spread argument"] Span), } impl ParseError { @@ -573,7 +576,7 @@ impl ParseError { ParseError::InvalidLiteral(_, _, s) => *s, ParseError::LabeledErrorWithHelp { span: s, .. } => *s, ParseError::RedirectionInLetMut(s, _) => *s, - ParseError::UnexpectedSpread(s) => *s, + ParseError::UnexpectedSpreadArg(_, s) => *s, } } } diff --git a/crates/nu-protocol/src/shell_error.rs b/crates/nu-protocol/src/shell_error.rs index 29aca7802..25c1d0d86 100644 --- a/crates/nu-protocol/src/shell_error.rs +++ b/crates/nu-protocol/src/shell_error.rs @@ -1281,15 +1281,15 @@ This is an internal Nushell error, please file an issue https://github.com/nushe span: Span, }, - /// Tried spreading a non-list inside a list. + /// Tried spreading a non-list inside a list or command call. /// /// ## Resolution /// - /// Only lists can be spread inside lists. Try converting the value to a list before spreading. + /// Only lists can be spread inside lists and command calls. Try converting the value to a list before spreading. #[error("Not a list")] #[diagnostic( code(nu::shell::cannot_spread_as_list), - help("Only lists can be spread inside lists. Try converting the value to a list before spreading") + help("Only lists can be spread inside lists and command calls. Try converting the value to a list before spreading.") )] CannotSpreadAsList { #[label = "cannot spread value"] diff --git a/crates/nu-test-support/src/lib.rs b/crates/nu-test-support/src/lib.rs index 5e2ab8a44..531983092 100644 --- a/crates/nu-test-support/src/lib.rs +++ b/crates/nu-test-support/src/lib.rs @@ -45,7 +45,7 @@ pub fn pipeline(commands: &str) -> String { } pub fn nu_repl_code(source_lines: &[&str]) -> String { - let mut out = String::from("nu --testbin=nu_repl [ "); + let mut out = String::from("nu --testbin=nu_repl ...[ "); for line in source_lines.iter() { // convert each "line" to really be a single line to prevent nu! macro joining the newlines diff --git a/src/tests/test_known_external.rs b/src/tests/test_known_external.rs index fdffb681e..586ee40cd 100644 --- a/src/tests/test_known_external.rs +++ b/src/tests/test_known_external.rs @@ -99,8 +99,8 @@ fn known_external_misc_values() -> TestResult { run_test( r#" let x = 'abc' - extern echo [] - echo $x [ a b c ] + extern echo [...args] + echo $x ...[ a b c ] "#, "abc a b c", ) diff --git a/src/tests/test_spread.rs b/src/tests/test_spread.rs index 86fb8351d..31a6183b1 100644 --- a/src/tests/test_spread.rs +++ b/src/tests/test_spread.rs @@ -1,4 +1,5 @@ use crate::tests::{fail_test, run_test, TestResult}; +use nu_test_support::nu; #[test] fn spread_in_list() -> TestResult { @@ -24,30 +25,6 @@ fn spread_in_list() -> TestResult { ) } -#[test] -fn const_spread_in_list() -> TestResult { - run_test(r#"const x = [...[]]; $x | to nuon"#, "[]").unwrap(); - run_test( - r#"const x = [1 2 ...[[3] {x: 1}] 5]; $x | to nuon"#, - "[1, 2, [3], {x: 1}, 5]", - ) - .unwrap(); - run_test( - r#"const x = [...([f o o]) 10]; $x | to nuon"#, - "[f, o, o, 10]", - ) - .unwrap(); - run_test( - r#"const l = [1, 2, [3]]; const x = [...$l $l]; $x | to nuon"#, - "[1, 2, [3], [1, 2, [3]]]", - ) - .unwrap(); - run_test( - r#"[ ...[ ...[ ...[ a ] b ] c ] d ] | to nuon"#, - "[a, b, c, d]", - ) -} - #[test] fn not_spread() -> TestResult { run_test(r#"def ... [x] { $x }; ... ..."#, "...").unwrap(); @@ -95,15 +72,6 @@ fn spread_in_record() -> TestResult { ) } -#[test] -fn const_spread_in_record() -> TestResult { - run_test(r#"const x = {...{...{...{}}}}; $x | to nuon"#, "{}").unwrap(); - run_test( - r#"const x = {foo: bar ...{a: {x: 1}} b: 3}; $x | to nuon"#, - "{foo: bar, a: {x: 1}, b: 3}", - ) -} - #[test] fn duplicate_cols() -> TestResult { fail_test(r#"{a: 1, ...{a: 3}}"#, "column used twice").unwrap(); @@ -111,16 +79,6 @@ fn duplicate_cols() -> TestResult { fail_test(r#"{...{a: 0, x: 2}, ...{x: 5}}"#, "column used twice") } -#[test] -fn const_duplicate_cols() -> TestResult { - fail_test(r#"const _ = {a: 1, ...{a: 3}}"#, "column used twice").unwrap(); - fail_test(r#"const _ = {...{a: 4, x: 3}, x: 1}"#, "column used twice").unwrap(); - fail_test( - r#"const _ = {...{a: 0, x: 2}, ...{x: 5}}"#, - "column used twice", - ) -} - #[test] fn bad_spread_on_non_record() -> TestResult { fail_test(r#"let x = 5; { ...$x }"#, "cannot spread").unwrap(); @@ -139,3 +97,96 @@ fn spread_type_record() -> TestResult { "type_mismatch", ) } + +#[test] +fn spread_external_args() { + assert_eq!( + nu!(r#"nu --testbin cococo ...[1 "foo"] 2 ...[3 "bar"]"#).out, + "1 foo 2 3 bar", + ); + // exec doesn't have rest parameters but allows unknown arguments + assert_eq!( + nu!(r#"exec nu --testbin cococo "foo" ...[5 6]"#).out, + "foo 5 6" + ); +} + +#[test] +fn spread_internal_args() -> TestResult { + run_test( + r#" + let list = ["foo" 4] + def f [a b c? d? ...x] { [$a $b $c $d $x] | to nuon } + f 1 2 ...[5 6] 7 ...$list"#, + "[1, 2, null, null, [5, 6, 7, foo, 4]]", + ) + .unwrap(); + run_test( + r#" + def f [a b c? d? ...x] { [$a $b $c $d $x] | to nuon } + f 1 2 3 ...[5 6]"#, + "[1, 2, 3, null, [5, 6]]", + ) + .unwrap(); + run_test( + r#" + def f [--flag: int ...x] { [$flag $x] | to nuon } + f 2 ...[foo] 4 --flag 5 6 ...[7 8]"#, + "[5, [2, foo, 4, 6, 7, 8]]", + ) + .unwrap(); + run_test( + r#" + def f [a b? --flag: int ...x] { [$a $b $flag $x] | to nuon } + f 1 ...[foo] 4 --flag 5 6 ...[7 8]"#, + "[1, null, 5, [foo, 4, 6, 7, 8]]", + ) +} + +#[test] +fn bad_spread_internal_args() -> TestResult { + fail_test( + r#" + def f [a b c? d? ...x] { echo $a $b $c $d $x } + f 1 ...[5 6]"#, + "Missing required positional argument", + ) + .unwrap(); + fail_test( + r#" + def f [a b?] { echo a b c d } + f ...[5 6]"#, + "unexpected spread argument", + ) +} + +#[test] +fn spread_non_list_args() { + fail_test(r#"echo ...(1)"#, "cannot spread value").unwrap(); + assert!(nu!(r#"nu --testbin cococo ...(1)"#) + .err + .contains("cannot spread value")); +} + +#[test] +fn spread_args_type() -> TestResult { + fail_test(r#"def f [...x: int] {}; f ...["abc"]"#, "expected int") +} + +#[test] +fn explain_spread_args() -> TestResult { + run_test( + r#"(explain { || echo ...[1 2] }).cmd_args.0 | select arg_type name type | to nuon"#, + r#"[[arg_type, name, type]; [spread, "[1 2]", list]]"#, + ) +} + +#[test] +fn deprecate_implicit_spread_for_externals() { + // TODO: When automatic spreading is removed, test that list literals fail at parse time + let result = nu!(r#"nu --testbin cococo [1 2]"#); + assert!(result + .err + .contains("Automatically spreading lists is deprecated")); + assert_eq!(result.out, "1 2"); +} diff --git a/tests/shell/pipeline/commands/external.rs b/tests/shell/pipeline/commands/external.rs index 66e9057be..3672c6181 100644 --- a/tests/shell/pipeline/commands/external.rs +++ b/tests/shell/pipeline/commands/external.rs @@ -354,7 +354,7 @@ mod nu_commands { #[test] fn command_list_arg_test() { let actual = nu!(" - nu ['-c' 'version'] + nu ...['-c' 'version'] "); assert!(actual.out.contains("version")); @@ -365,7 +365,7 @@ mod nu_commands { #[test] fn command_cell_path_arg_test() { let actual = nu!(" - nu ([ '-c' 'version' ]) + nu ...([ '-c' 'version' ]) "); assert!(actual.out.contains("version")); @@ -436,7 +436,7 @@ mod external_command_arguments { let actual = nu!( cwd: dirs.test(), pipeline( " - nu --testbin cococo (ls | get name) + nu --testbin cococo ...(ls | get name) " ));