From ba6a214bfa1563027a6e63e57b53d3c090d8aef8 Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Fri, 12 Jul 2024 20:34:42 -0700 Subject: [PATCH] move redirections into let/mut and get tests passing --- crates/nu-parser/src/lite_parser.rs | 14 ++- crates/nu-parser/src/parser.rs | 120 ++++++++++++++------------ crates/nu-parser/tests/test_parser.rs | 63 ++++++++++++-- 3 files changed, 135 insertions(+), 62 deletions(-) diff --git a/crates/nu-parser/src/lite_parser.rs b/crates/nu-parser/src/lite_parser.rs index 9499468489..c9e9578698 100644 --- a/crates/nu-parser/src/lite_parser.rs +++ b/crates/nu-parser/src/lite_parser.rs @@ -2,7 +2,7 @@ //! can be parsed. use crate::{Token, TokenContents}; -use itertools::Either; +use itertools::{Either, Itertools}; use nu_protocol::{ast::RedirectionSource, ParseError, Span}; use std::mem; @@ -52,7 +52,9 @@ impl LiteRedirection { pub fn spans(&self) -> impl Iterator { match self { LiteRedirection::Single { target, .. } => Either::Left(target.spans()), - LiteRedirection::Separate { out, err } => Either::Right(out.spans().chain(err.spans())), + LiteRedirection::Separate { out, err } => { + Either::Right(out.spans().chain(err.spans()).sorted()) + } } } } @@ -132,6 +134,14 @@ impl LiteCommand { Ok(()) } + + pub fn parts_including_redirection(&self) -> impl Iterator + '_ { + self.parts.iter().copied().chain( + self.redirection + .iter() + .flat_map(|redirection| redirection.spans()), + ) + } } #[derive(Debug, Clone, Default)] diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index fb2c66a0b4..dc1f2e8b6a 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -5394,9 +5394,19 @@ pub fn parse_builtin_commands( match name { b"def" => parse_def(working_set, lite_command, None).0, b"extern" => parse_extern(working_set, lite_command, None), - b"let" => parse_let(working_set, &lite_command.parts), + b"let" => parse_let( + working_set, + &lite_command + .parts_including_redirection() + .collect::>(), + ), b"const" => parse_const(working_set, &lite_command.parts), - b"mut" => parse_mut(working_set, &lite_command.parts), + b"mut" => parse_mut( + working_set, + &lite_command + .parts_including_redirection() + .collect::>(), + ), b"for" => { let expr = parse_for(working_set, lite_command); Pipeline::from_vec(vec![expr]) @@ -5648,12 +5658,7 @@ pub(crate) fn redirecting_builtin_error( } } -pub fn parse_pipeline( - working_set: &mut StateWorkingSet, - pipeline: &LitePipeline, - is_subexpression: bool, - pipeline_index: usize, -) -> Pipeline { +pub fn parse_pipeline(working_set: &mut StateWorkingSet, pipeline: &LitePipeline) -> Pipeline { let first_command = pipeline.commands.get(0); let first_command_name = first_command .and_then(|command| command.parts.get(0)) @@ -5661,55 +5666,60 @@ pub fn parse_pipeline( if pipeline.commands.len() > 1 { // Special case: allow "let" or "mut" to consume the whole pipeline, if this is a pipeline - // with multiple commands, and the let/mut is well-formed - if matches!(first_command_name, Some(b"let" | b"mut")) - && first_command.is_some_and(|command| command.parts.len() > 3) - { + // with multiple commands + if matches!(first_command_name, Some(b"let" | b"mut")) { // Merge the pipeline into one command let first_command = first_command.expect("must be Some"); - let remainder_span = Span::concat(&first_command.parts[3..]); + + let remainder_span = first_command + .parts_including_redirection() + .skip(3) + .chain( + pipeline.commands[1..] + .iter() + .flat_map(|command| command.parts_including_redirection()), + ) + .reduce(Span::append); + + let parts = first_command + .parts + .iter() + .take(3) // the let/mut start itself + .copied() + .chain(remainder_span) // everything else + .collect(); + + let comments = pipeline + .commands + .iter() + .flat_map(|command| command.comments.iter()) + .copied() + .collect(); + let new_command = LiteCommand { pipe: None, - comments: pipeline - .commands - .iter() - .flat_map(|command| command.comments.iter()) - .copied() - .collect(), - parts: first_command.parts[0..3] // the let/mut start itself - .iter() - .copied() - .chain(std::iter::once(remainder_span)) // the first command - .chain( - first_command - .redirection - .iter() - .flat_map(|redirection| redirection.spans()), - ) - .chain( - pipeline.commands[1..] - .iter() - .flat_map(|command| command.parts.iter().copied()), - ) - .collect(), + comments, + parts, redirection: None, }; parse_builtin_commands(working_set, &new_command) } else { // Parse a normal multi command pipeline - let mut elements: Vec<_> = pipeline + let elements: Vec<_> = pipeline .commands .iter() - .map(|element| parse_pipeline_element(working_set, element)) + .enumerate() + .map(|(index, element)| { + let element = parse_pipeline_element(working_set, element); + // Handle $in for pipeline elements beyond the first one + if index > 0 && element.has_in_variable(working_set) { + wrap_element_with_collect(working_set, element.clone()) + } else { + element + } + }) .collect(); - // Handle $in for anything other than the first element, which is handled by block parsing - for element in elements.iter_mut().skip(1) { - if element.has_in_variable(working_set) { - *element = wrap_element_with_collect(working_set, element); - } - } - Pipeline { elements } } } else { @@ -5747,8 +5757,8 @@ pub fn parse_block( let mut block = Block::new_with_capacity(lite_block.block.len()); block.span = Some(span); - for (idx, lite_pipeline) in lite_block.block.iter().enumerate() { - let pipeline = parse_pipeline(working_set, lite_pipeline, is_subexpression, idx); + for lite_pipeline in &lite_block.block { + let pipeline = parse_pipeline(working_set, lite_pipeline); block.pipelines.push(pipeline); } @@ -6206,28 +6216,28 @@ pub fn discover_captures_in_expr( fn wrap_redirection_with_collect( working_set: &mut StateWorkingSet, - target: &RedirectionTarget, + target: RedirectionTarget, ) -> RedirectionTarget { match target { RedirectionTarget::File { expr, append, span } => RedirectionTarget::File { - expr: wrap_expr_with_collect(working_set, expr.clone()), - span: *span, - append: *append, + expr: wrap_expr_with_collect(working_set, expr), + span, + append, }, - RedirectionTarget::Pipe { span } => RedirectionTarget::Pipe { span: *span }, + RedirectionTarget::Pipe { span } => RedirectionTarget::Pipe { span }, } } fn wrap_element_with_collect( working_set: &mut StateWorkingSet, - element: &PipelineElement, + element: PipelineElement, ) -> PipelineElement { PipelineElement { pipe: element.pipe, - expr: wrap_expr_with_collect(working_set, element.expr.clone()), - redirection: element.redirection.as_ref().map(|r| match r { + expr: wrap_expr_with_collect(working_set, element.expr), + redirection: element.redirection.map(|r| match r { PipelineRedirection::Single { source, target } => PipelineRedirection::Single { - source: *source, + source, target: wrap_redirection_with_collect(working_set, target), }, PipelineRedirection::Separate { out, err } => PipelineRedirection::Separate { diff --git a/crates/nu-parser/tests/test_parser.rs b/crates/nu-parser/tests/test_parser.rs index 7762863ba1..4d6778d201 100644 --- a/crates/nu-parser/tests/test_parser.rs +++ b/crates/nu-parser/tests/test_parser.rs @@ -41,6 +41,41 @@ impl Command for Let { } } +#[cfg(test)] +#[derive(Clone)] +pub struct Mut; + +#[cfg(test)] +impl Command for Mut { + fn name(&self) -> &str { + "mut" + } + + fn usage(&self) -> &str { + "Mock mut command." + } + + fn signature(&self) -> nu_protocol::Signature { + Signature::build("mut") + .required("var_name", SyntaxShape::VarWithOptType, "variable name") + .required( + "initial_value", + SyntaxShape::Keyword(b"=".to_vec(), Box::new(SyntaxShape::MathExpression)), + "equals sign followed by value", + ) + } + + fn run( + &self, + _engine_state: &EngineState, + _stack: &mut Stack, + _call: &Call, + _input: PipelineData, + ) -> Result { + todo!() + } +} + fn test_int( test_tag: &str, // name of sub-test test: &[u8], // input expression @@ -1149,11 +1184,29 @@ fn test_nothing_comparison_eq() { fn test_redirection_with_letmut(#[case] phase: &[u8]) { let engine_state = EngineState::new(); let mut working_set = StateWorkingSet::new(&engine_state); - let _block = parse(&mut working_set, None, phase, true); - assert!(matches!( - working_set.parse_errors.first(), - Some(ParseError::RedirectingBuiltinCommand(_, _, _)) - )); + working_set.add_decl(Box::new(Let)); + working_set.add_decl(Box::new(Mut)); + + let block = parse(&mut working_set, None, phase, true); + assert!( + working_set.parse_errors.is_empty(), + "parse errors: {:?}", + working_set.parse_errors + ); + assert_eq!(1, block.pipelines[0].elements.len()); + + let element = &block.pipelines[0].elements[0]; + assert!(element.redirection.is_none()); // it should be in the let block, not here + + if let Expr::Call(call) = &element.expr.expr { + let arg = call.positional_nth(1).expect("no positional args"); + let block_id = arg.as_block().expect("arg 1 is not a block"); + let block = working_set.get_block(block_id); + let inner_element = &block.pipelines[0].elements[0]; + assert!(inner_element.redirection.is_some()); + } else { + panic!("expected Call: {:?}", block.pipelines[0].elements[0]) + } } #[rstest]