From 8aa2632661157a4b969de514e0aca490f431a8aa Mon Sep 17 00:00:00 2001 From: WindSoilder Date: Thu, 12 Jan 2023 17:22:30 +0800 Subject: [PATCH] Support redirect `err` and `out` to different streams (#7685) # Description Closes: #7364 # User-Facing Changes Given the following shell script: ```bash x=$(printf '=%.0s' {1..100}) echo $x echo $x 1>&2 ``` It supports the following command: ``` bash test.sh out> out.txt err> err.txt ``` Then both `out.txt` and `err.txt` will contain `=`(100 times) ## About the change The core idea is that when doing lite-parsing, introduce a new variant `LiteElement::SeparateRedirection` if we meet two Redirection token(which is generated by `lex` function), During converting from lite block to block, `LiteElement::SeparateRedirection` will be converted to `PipelineElement::SeparateRedirection`. Then in the block eval process, if we get `PipelineElement::SeparateRedirection`, we invoke `save` command with `--stderr` arguments to acthive our behavior. ## What happened internally? Take the following command as example: ``` ^ls out> out.txt err> err.txt ``` lex parsing result(`Tokens`) are not changed, but `LiteBlock` and `Block` is changed after this pr. ### LiteBlock before ```rust LiteBlock { block: [ LitePipeline { commands: [ Command(None, LiteCommand { comments: [], parts: [Span { start: 39041, end: 39044 }] }), // actually the span of first Redirection is wrong too.. Redirection(Span { start: 39058, end: 39062 }, Stdout, LiteCommand { comments: [], parts: [Span { start: 39050, end: 39057 }] }), Redirection(Span { start: 39058, end: 39062 }, Stderr, LiteCommand { comments: [], parts: [Span { start: 39063, end: 39070 }] }) ] }] } ``` ### LiteBlock after ```rust LiteBlock { block: [ LitePipeline { commands: [ Command( None, LiteCommand { comments: [], parts: [Span { start: 38525, end: 38528 }] }), // new one! two Redirection merged into one SeparateRedirection. SeparateRedirection { out: (Span { start: 38529, end: 38533 }, LiteCommand { comments: [], parts: [Span { start: 38534, end: 38541 }] }), err: (Span { start: 38542, end: 38546 }, LiteCommand { comments: [], parts: [Span { start: 38547, end: 38554 }] }) } ] }] } ``` ### Block before ```rust Pipeline { elements: [ Expression(None, Expression { expr: ExternalCall(Expression { expr: String("ls"), span: Span { start: 39042, end: 39044 }, ty: String, custom_completion: None }, [], false), span: Span { start: 39041, end: 39044 }, ty: Any, custom_completion: None }), Redirection(Span { start: 39058, end: 39062 }, Stdout, Expression { expr: String("out.txt"), span: Span { start: 39050, end: 39057 }, ty: String, custom_completion: None }), Redirection(Span { start: 39058, end: 39062 }, Stderr, Expression { expr: String("err.txt"), span: Span { start: 39063, end: 39070 }, ty: String, custom_completion: None })] } ``` ### Block after ```rust Pipeline { elements: [ Expression(None, Expression { expr: ExternalCall(Expression { expr: String("ls"), span: Span { start: 38526, end: 38528 }, ty: String, custom_completion: None }, [], false), span: Span { start: 38525, end: 38528 }, ty: Any, custom_completion: None }), // new one! SeparateRedirection SeparateRedirection { out: (Span { start: 38529, end: 38533 }, Expression { expr: String("out.txt"), span: Span { start: 38534, end: 38541 }, ty: String, custom_completion: None }), err: (Span { start: 38542, end: 38546 }, Expression { expr: String("err.txt"), span: Span { start: 38547, end: 38554 }, ty: String, custom_completion: None }) } ] } ``` # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- crates/nu-cli/src/completions/completer.rs | 3 +- crates/nu-cli/src/syntax_highlight.rs | 3 +- crates/nu-command/src/formats/from/nuon.rs | 6 +- .../nu-command/tests/commands/redirection.rs | 66 ++++++++++++++++ crates/nu-engine/src/eval.rs | 60 +++++++++++++++ crates/nu-parser/src/flatten.rs | 10 +++ crates/nu-parser/src/lite_parser.rs | 72 ++++++++++++++++- crates/nu-parser/src/parse_keywords.rs | 3 + crates/nu-parser/src/parser.rs | 77 ++++++++++++++++++- crates/nu-protocol/src/ast/pipeline.rs | 31 +++++++- 10 files changed, 317 insertions(+), 14 deletions(-) diff --git a/crates/nu-cli/src/completions/completer.rs b/crates/nu-cli/src/completions/completer.rs index 9d74c9aa07..5ee7452f0c 100644 --- a/crates/nu-cli/src/completions/completer.rs +++ b/crates/nu-cli/src/completions/completer.rs @@ -128,7 +128,8 @@ impl NuCompleter { PipelineElement::Expression(_, expr) | PipelineElement::Redirection(_, _, expr) | PipelineElement::And(_, expr) - | PipelineElement::Or(_, expr) => { + | PipelineElement::Or(_, expr) + | PipelineElement::SeparateRedirection { out: (_, expr), .. } => { let flattened: Vec<_> = flatten_expression(&working_set, &expr); let span_offset: usize = alias_offset.iter().sum(); let mut spans: Vec = vec![]; diff --git a/crates/nu-cli/src/syntax_highlight.rs b/crates/nu-cli/src/syntax_highlight.rs index 25e0384cf0..6dacdfba9c 100644 --- a/crates/nu-cli/src/syntax_highlight.rs +++ b/crates/nu-cli/src/syntax_highlight.rs @@ -234,7 +234,8 @@ fn find_matching_block_end_in_block( PipelineElement::Expression(_, e) | PipelineElement::Redirection(_, _, e) | PipelineElement::And(_, e) - | PipelineElement::Or(_, e) => { + | PipelineElement::Or(_, e) + | PipelineElement::SeparateRedirection { out: (_, e), .. } => { if e.span.contains(global_cursor_offset) { if let Some(pos) = find_matching_block_end_in_expr( line, diff --git a/crates/nu-command/src/formats/from/nuon.rs b/crates/nu-command/src/formats/from/nuon.rs index aec990ab89..9514e253e9 100644 --- a/crates/nu-command/src/formats/from/nuon.rs +++ b/crates/nu-command/src/formats/from/nuon.rs @@ -137,7 +137,11 @@ impl Command for FromNuon { PipelineElement::Expression(_, expression) | PipelineElement::Redirection(_, _, expression) | PipelineElement::And(_, expression) - | PipelineElement::Or(_, expression) => expression, + | PipelineElement::Or(_, expression) + | PipelineElement::SeparateRedirection { + out: (_, expression), + .. + } => expression, } } }; diff --git a/crates/nu-command/tests/commands/redirection.rs b/crates/nu-command/tests/commands/redirection.rs index 439dcf93ad..08da592100 100644 --- a/crates/nu-command/tests/commands/redirection.rs +++ b/crates/nu-command/tests/commands/redirection.rs @@ -64,3 +64,69 @@ fn redirect_out() { assert!(output.out.contains("hello")); }) } + +#[test] +fn separate_redirection() { + use nu_test_support::fs::{file_contents, Stub::FileWithContent}; + use nu_test_support::playground::Playground; + Playground::setup( + "external with both stdout and stderr messages, to different file", + |dirs, sandbox| { + let script_body = r#" + echo message + echo message 1>&2 + "#; + let expect_body = "message"; + + #[cfg(not(windows))] + { + sandbox.with_files(vec![FileWithContent("test.sh", script_body)]); + nu!( + cwd: dirs.test(), + r#"bash test.sh out> out.txt err> err.txt"# + ); + } + #[cfg(windows)] + { + sandbox.with_files(vec![FileWithContent("test.bat", script_body)]); + nu!( + cwd: dirs.test(), + r#"cmd /D /c test.bat out> out.txt err> err.txt"# + ); + } + // check for stdout redirection file. + let expected_out_file = dirs.test().join("out.txt"); + let actual = file_contents(expected_out_file); + assert!(actual.contains(expect_body)); + + // check for stderr redirection file. + let expected_err_file = dirs.test().join("err.txt"); + let actual = file_contents(expected_err_file); + assert!(actual.contains(expect_body)); + }, + ) +} + +#[cfg(not(windows))] +#[test] +fn redirection_with_pipeline_works() { + use nu_test_support::fs::{file_contents, Stub::FileWithContent}; + use nu_test_support::playground::Playground; + Playground::setup( + "external with stdout message with pipeline should write data", + |dirs, sandbox| { + let script_body = r"echo message"; + let expect_body = "message"; + sandbox.with_files(vec![FileWithContent("test.sh", script_body)]); + + nu!( + cwd: dirs.test(), + r#"bash test.sh out> out.txt | describe"# + ); + // check for stdout redirection file. + let expected_out_file = dirs.test().join("out.txt"); + let actual = file_contents(expected_out_file); + assert!(actual.contains(expect_body)); + }, + ) +} diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 06c5721cd1..87687f3ee1 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -828,6 +828,64 @@ pub fn eval_element_with_input( } _ => Err(ShellError::CommandNotFound(*span)), }, + PipelineElement::SeparateRedirection { + out: (out_span, out_expr), + err: (err_span, err_expr), + } => match (&out_expr.expr, &err_expr.expr) { + (Expr::String(_), Expr::String(_)) => { + if let Some(save_command) = engine_state.find_decl(b"save", &[]) { + eval_call( + engine_state, + stack, + &Call { + decl_id: save_command, + head: *out_span, + arguments: vec![ + Argument::Positional(out_expr.clone()), + Argument::Named(( + Spanned { + item: "stderr".into(), + span: *err_span, + }, + None, + Some(err_expr.clone()), + )), + Argument::Named(( + Spanned { + item: "raw".into(), + span: *out_span, + }, + None, + None, + )), + Argument::Named(( + Spanned { + item: "force".into(), + span: *out_span, + }, + None, + None, + )), + ], + redirect_stdout: false, + redirect_stderr: false, + parser_info: vec![], + }, + input, + ) + .map(|x| (x, false)) + } else { + Err(ShellError::CommandNotFound(*out_span)) + } + } + (_out_other, err_other) => { + if let Expr::String(_) = err_other { + Err(ShellError::CommandNotFound(*out_span)) + } else { + Err(ShellError::CommandNotFound(*err_span)) + } + } + }, PipelineElement::And(_, expr) => eval_expression_with_input( engine_state, stack, @@ -902,6 +960,7 @@ pub fn eval_block( pipeline.elements[i + 1], PipelineElement::Redirection(_, Redirection::Stderr, _) | PipelineElement::Redirection(_, Redirection::StdoutAndStderr, _) + | PipelineElement::SeparateRedirection { .. } ))); // if eval internal command failed, it can just make early return with `Err(ShellError)`. @@ -917,6 +976,7 @@ pub fn eval_block( PipelineElement::Redirection(_, Redirection::Stdout, _) | PipelineElement::Redirection(_, Redirection::StdoutAndStderr, _) | PipelineElement::Expression(..) + | PipelineElement::SeparateRedirection { .. } )), redirect_stderr, ); diff --git a/crates/nu-parser/src/flatten.rs b/crates/nu-parser/src/flatten.rs index b504e8a3d4..58b30b507c 100644 --- a/crates/nu-parser/src/flatten.rs +++ b/crates/nu-parser/src/flatten.rs @@ -455,6 +455,16 @@ pub fn flatten_pipeline_element( output.append(&mut flatten_expression(working_set, expr)); output } + PipelineElement::SeparateRedirection { + out: (out_span, out_expr), + err: (err_span, err_expr), + } => { + let mut output = vec![(*out_span, FlatShape::Redirection)]; + output.append(&mut flatten_expression(working_set, out_expr)); + output.push((*err_span, FlatShape::Redirection)); + output.append(&mut flatten_expression(working_set, err_expr)); + output + } PipelineElement::And(span, expr) => { let mut output = vec![(*span, FlatShape::And)]; output.append(&mut flatten_expression(working_set, expr)); diff --git a/crates/nu-parser/src/lite_parser.rs b/crates/nu-parser/src/lite_parser.rs index e4d3cf8306..0a1f97e7bf 100644 --- a/crates/nu-parser/src/lite_parser.rs +++ b/crates/nu-parser/src/lite_parser.rs @@ -38,6 +38,11 @@ impl LiteCommand { pub enum LiteElement { Command(Option, LiteCommand), Redirection(Span, Redirection, LiteCommand), + // SeparateRedirection variant can only be generated by two different Redirection variant + SeparateRedirection { + out: (Span, LiteCommand), + err: (Span, LiteCommand), + }, } #[derive(Debug)] @@ -60,6 +65,10 @@ impl LitePipeline { self.commands.push(element); } + pub fn insert(&mut self, index: usize, element: LiteElement) { + self.commands.insert(index, element); + } + pub fn is_empty(&self) -> bool { self.commands.is_empty() } @@ -81,13 +90,65 @@ impl LiteBlock { Self { block: vec![] } } - pub fn push(&mut self, pipeline: LitePipeline) { + pub fn push(&mut self, mut pipeline: LitePipeline) { + // once we push `pipeline` to our block + // the block takes ownership of `pipeline`, which means that + // our `pipeline` is complete on collecting commands. + self.merge_redirections(&mut pipeline); + self.block.push(pipeline); } pub fn is_empty(&self) -> bool { self.block.is_empty() } + + fn merge_redirections(&self, pipeline: &mut LitePipeline) { + // In case our command may contains both stdout and stderr redirection. + // We pick them out and Combine them into one LiteElement::SeparateRedirection variant. + let mut stdout_index = None; + let mut stderr_index = None; + for (index, cmd) in pipeline.commands.iter().enumerate() { + if let LiteElement::Redirection(_span, redirection, _target_cmd) = cmd { + match *redirection { + Redirection::Stderr => stderr_index = Some(index), + Redirection::Stdout => stdout_index = Some(index), + Redirection::StdoutAndStderr => {} + } + } + } + + if let (Some(out_indx), Some(err_indx)) = (stdout_index, stderr_index) { + let (out_redirect, err_redirect, new_indx) = { + // to avoid panic, we need to remove commands which have larger index first. + if out_indx > err_indx { + let out_redirect = pipeline.commands.remove(out_indx); + let err_redirect = pipeline.commands.remove(err_indx); + (out_redirect, err_redirect, err_indx) + } else { + let err_redirect = pipeline.commands.remove(err_indx); + let out_redirect = pipeline.commands.remove(out_indx); + (out_redirect, err_redirect, out_indx) + } + }; + // `out_redirect` and `err_redirect` should always be `LiteElement::Redirection` + if let ( + LiteElement::Redirection(out_span, _, out_command), + LiteElement::Redirection(err_span, _, err_command), + ) = (out_redirect, err_redirect) + { + // using insert with specific index to keep original + // pipeline commands order. + pipeline.insert( + new_indx, + LiteElement::SeparateRedirection { + out: (out_span, out_command), + err: (err_span, err_command), + }, + ) + } + } + } } pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option) { @@ -130,21 +191,24 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option) { match last_connector { TokenContents::OutGreaterThan => { curr_pipeline.push(LiteElement::Redirection( - token.span, + last_connector_span + .expect("internal error: redirection missing span information"), Redirection::Stdout, curr_command, )); } TokenContents::ErrGreaterThan => { curr_pipeline.push(LiteElement::Redirection( - token.span, + last_connector_span + .expect("internal error: redirection missing span information"), Redirection::Stderr, curr_command, )); } TokenContents::OutErrGreaterThan => { curr_pipeline.push(LiteElement::Redirection( - token.span, + last_connector_span + .expect("internal error: redirection missing span information"), Redirection::StdoutAndStderr, curr_command, )); diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index d7316df578..faa4e27bd3 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -1459,6 +1459,9 @@ pub fn parse_module_block( pipeline } LiteElement::Redirection(_, _, command) => garbage_pipeline(&command.parts), + LiteElement::SeparateRedirection { + out: (_, command), .. + } => garbage_pipeline(&command.parts), } } else { error = Some(ParseError::Expected("not a pipeline".into(), span)); diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 2ef67d4e25..2a4898bff4 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -3950,7 +3950,11 @@ pub fn parse_table_expression( } _ => { match &output.block[0].commands[0] { - LiteElement::Command(_, command) | LiteElement::Redirection(_, _, command) => { + LiteElement::Command(_, command) + | LiteElement::Redirection(_, _, command) + | LiteElement::SeparateRedirection { + out: (_, command), .. + } => { let mut table_headers = vec![]; let (headers, err) = parse_value( @@ -3971,7 +3975,10 @@ pub fn parse_table_expression( match &output.block[1].commands[0] { LiteElement::Command(_, command) - | LiteElement::Redirection(_, _, command) => { + | LiteElement::Redirection(_, _, command) + | LiteElement::SeparateRedirection { + out: (_, command), .. + } => { let mut rows = vec![]; for part in &command.parts { let (values, err) = parse_value( @@ -5431,7 +5438,11 @@ pub fn parse_block( for pipeline in &lite_block.block { if pipeline.commands.len() == 1 { match &pipeline.commands[0] { - LiteElement::Command(_, command) | LiteElement::Redirection(_, _, command) => { + LiteElement::Command(_, command) + | LiteElement::Redirection(_, _, command) + | LiteElement::SeparateRedirection { + out: (_, command), .. + } => { if let Some(err) = parse_def_predecl(working_set, &command.parts, expand_aliases_denylist) { @@ -5484,6 +5495,40 @@ pub fn parse_block( PipelineElement::Redirection(*span, redirection.clone(), expr) } + LiteElement::SeparateRedirection { + out: (out_span, out_command), + err: (err_span, err_command), + } => { + trace!("parsing: pipeline element: separate redirection"); + let (out_expr, out_err) = parse_string( + working_set, + out_command.parts[0], + expand_aliases_denylist, + ); + + working_set.type_scope.add_type(out_expr.ty.clone()); + + if error.is_none() { + error = out_err; + } + + let (err_expr, err_err) = parse_string( + working_set, + err_command.parts[0], + expand_aliases_denylist, + ); + + working_set.type_scope.add_type(err_expr.ty.clone()); + + if error.is_none() { + error = err_err; + } + + PipelineElement::SeparateRedirection { + out: (*out_span, out_expr), + err: (*err_span, err_expr), + } + } }) .collect::>(); @@ -5504,7 +5549,11 @@ pub fn parse_block( Pipeline { elements: output } } else { match &pipeline.commands[0] { - LiteElement::Command(_, command) | LiteElement::Redirection(_, _, command) => { + LiteElement::Command(_, command) + | LiteElement::Redirection(_, _, command) + | LiteElement::SeparateRedirection { + out: (_, command), .. + } => { let (mut pipeline, err) = parse_builtin_commands( working_set, command, @@ -5647,6 +5696,19 @@ pub fn discover_captures_in_pipeline_element( | PipelineElement::Or(_, expression) => { discover_captures_in_expr(working_set, expression, seen, seen_blocks) } + PipelineElement::SeparateRedirection { + out: (_, out_expr), + err: (_, err_expr), + } => { + let mut result = discover_captures_in_expr(working_set, out_expr, seen, seen_blocks)?; + result.append(&mut discover_captures_in_expr( + working_set, + err_expr, + seen, + seen_blocks, + )?); + Ok(result) + } } } @@ -5904,6 +5966,13 @@ fn wrap_element_with_collect( wrap_expr_with_collect(working_set, expression), ) } + PipelineElement::SeparateRedirection { + out: (out_span, out_exp), + err: (err_span, err_exp), + } => PipelineElement::SeparateRedirection { + out: (*out_span, wrap_expr_with_collect(working_set, out_exp)), + err: (*err_span, wrap_expr_with_collect(working_set, err_exp)), + }, PipelineElement::And(span, expression) => { PipelineElement::And(*span, wrap_expr_with_collect(working_set, expression)) } diff --git a/crates/nu-protocol/src/ast/pipeline.rs b/crates/nu-protocol/src/ast/pipeline.rs index a7df63bc1b..ee6194615e 100644 --- a/crates/nu-protocol/src/ast/pipeline.rs +++ b/crates/nu-protocol/src/ast/pipeline.rs @@ -14,6 +14,10 @@ pub enum Redirection { pub enum PipelineElement { Expression(Option, Expression), Redirection(Span, Redirection, Expression), + SeparateRedirection { + out: (Span, Expression), + err: (Span, Expression), + }, And(Span, Expression), Or(Span, Expression), } @@ -24,6 +28,10 @@ impl PipelineElement { PipelineElement::Expression(None, expression) => expression.span, PipelineElement::Expression(Some(span), expression) | PipelineElement::Redirection(span, _, expression) + | PipelineElement::SeparateRedirection { + out: (span, expression), + .. + } | PipelineElement::And(span, expression) | PipelineElement::Or(span, expression) => Span { start: span.start, @@ -37,6 +45,10 @@ impl PipelineElement { | PipelineElement::Redirection(_, _, expression) | PipelineElement::And(_, expression) | PipelineElement::Or(_, expression) => expression.has_in_variable(working_set), + PipelineElement::SeparateRedirection { + out: (_, out_expr), + err: (_, err_expr), + } => out_expr.has_in_variable(working_set) || err_expr.has_in_variable(working_set), } } @@ -48,6 +60,17 @@ impl PipelineElement { | PipelineElement::Or(_, expression) => { expression.replace_in_variable(working_set, new_var_id) } + PipelineElement::SeparateRedirection { + out: (_, out_expr), + err: (_, err_expr), + } => { + if out_expr.has_in_variable(working_set) { + out_expr.replace_in_variable(working_set, new_var_id) + } + if err_expr.has_in_variable(working_set) { + err_expr.replace_in_variable(working_set, new_var_id) + } + } } } @@ -61,9 +84,11 @@ impl PipelineElement { PipelineElement::Expression(_, expression) | PipelineElement::Redirection(_, _, expression) | PipelineElement::And(_, expression) - | PipelineElement::Or(_, expression) => { - expression.replace_span(working_set, replaced, new_span) - } + | PipelineElement::Or(_, expression) + | PipelineElement::SeparateRedirection { + out: (_, expression), + .. + } => expression.replace_span(working_set, replaced, new_span), } } }