From 18d7e646606251fce9c4578298a2103c4ce18963 Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Fri, 11 Nov 2022 09:05:34 +1300 Subject: [PATCH] Convert 'for' to a statement (#7086) --- crates/nu-command/src/core_commands/for_.rs | 158 +++++++------------- crates/nu-parser/src/parser.rs | 11 +- src/tests/test_engine.rs | 5 - src/tests/test_iteration.rs | 5 - tests/shell/pipeline/commands/internal.rs | 12 ++ 5 files changed, 77 insertions(+), 114 deletions(-) diff --git a/crates/nu-command/src/core_commands/for_.rs b/crates/nu-command/src/core_commands/for_.rs index 158429c31b..0d9ce32287 100644 --- a/crates/nu-command/src/core_commands/for_.rs +++ b/crates/nu-command/src/core_commands/for_.rs @@ -1,10 +1,7 @@ use nu_engine::{eval_block, eval_expression, CallExt}; use nu_protocol::ast::Call; -use nu_protocol::engine::{Closure, Command, EngineState, Stack}; -use nu_protocol::{ - Category, Example, IntoInterruptiblePipelineData, ListStream, PipelineData, Signature, Span, - SyntaxShape, Type, Value, -}; +use nu_protocol::engine::{Block, Command, EngineState, Stack}; +use nu_protocol::{Category, Example, ListStream, PipelineData, Signature, SyntaxShape, Value}; #[derive(Clone)] pub struct For; @@ -20,7 +17,6 @@ impl Command for For { fn signature(&self) -> nu_protocol::Signature { Signature::build("for") - .input_output_types(vec![(Type::Nothing, Type::List(Box::new(Type::Any)))]) .required( "var_name", SyntaxShape::VarWithOptType, @@ -71,72 +67,22 @@ impl Command for For { .expect("internal error: missing keyword"); let values = eval_expression(engine_state, stack, keyword_expr)?; - let capture_block: Closure = call.req(engine_state, stack, 2)?; + let block: Block = call.req(engine_state, stack, 2)?; let numbered = call.has_flag("numbered"); let ctrlc = engine_state.ctrlc.clone(); let engine_state = engine_state.clone(); - let block = engine_state.get_block(capture_block.block_id).clone(); - let mut stack = stack.captures_to_stack(&capture_block.captures); - let orig_env_vars = stack.env_vars.clone(); - let orig_env_hidden = stack.env_hidden.clone(); + let block = engine_state.get_block(block.block_id).clone(); let redirect_stdout = call.redirect_stdout; let redirect_stderr = call.redirect_stderr; match values { Value::List { vals, .. } => { - Ok(ListStream::from_stream(vals.into_iter(), ctrlc.clone()) - .enumerate() - .map(move |(idx, x)| { - // with_env() is used here to ensure that each iteration uses - // a different set of environment variables. - // Hence, a 'cd' in the first loop won't affect the next loop. - stack.with_env(&orig_env_vars, &orig_env_hidden); - - stack.add_var( - var_id, - if numbered { - Value::Record { - cols: vec!["index".into(), "item".into()], - vals: vec![ - Value::Int { - val: idx as i64, - span: head, - }, - x, - ], - span: head, - } - } else { - x - }, - ); - - //let block = engine_state.get_block(block_id); - match eval_block( - &engine_state, - &mut stack, - &block, - PipelineData::new(head), - redirect_stdout, - redirect_stderr, - ) { - Ok(pipeline_data) => pipeline_data.into_value(head), - Err(error) => Value::Error { error }, - } - }) - .filter(|x| !x.is_nothing()) - .into_pipeline_data(ctrlc)) - } - Value::Range { val, .. } => Ok(val - .into_range_iter(ctrlc.clone())? - .enumerate() - .map(move |(idx, x)| { + for (idx, x) in ListStream::from_stream(vals.into_iter(), ctrlc).enumerate() { // with_env() is used here to ensure that each iteration uses // a different set of environment variables. // Hence, a 'cd' in the first loop won't affect the next loop. - stack.with_env(&orig_env_vars, &orig_env_hidden); stack.add_var( var_id, @@ -158,78 +104,84 @@ impl Command for For { ); //let block = engine_state.get_block(block_id); - match eval_block( + eval_block( &engine_state, - &mut stack, + stack, &block, PipelineData::new(head), redirect_stdout, redirect_stderr, - ) { - Ok(pipeline_data) => pipeline_data.into_value(head), - Err(error) => Value::Error { error }, - } - }) - .filter(|x| !x.is_nothing()) - .into_pipeline_data(ctrlc)), + )? + .into_value(head); + } + } + Value::Range { val, .. } => { + for (idx, x) in val.into_range_iter(ctrlc)?.enumerate() { + stack.add_var( + var_id, + if numbered { + Value::Record { + cols: vec!["index".into(), "item".into()], + vals: vec![ + Value::Int { + val: idx as i64, + span: head, + }, + x, + ], + span: head, + } + } else { + x + }, + ); + + //let block = engine_state.get_block(block_id); + eval_block( + &engine_state, + stack, + &block, + PipelineData::new(head), + redirect_stdout, + redirect_stderr, + )? + .into_value(head); + } + } x => { stack.add_var(var_id, x); eval_block( &engine_state, - &mut stack, + stack, &block, PipelineData::new(head), redirect_stdout, redirect_stderr, - ) + )? + .into_value(head); } } + Ok(PipelineData::new(head)) } fn examples(&self) -> Vec { - let span = Span::test_data(); vec![ Example { description: "Echo the square of each integer", - example: "for x in [1 2 3] { $x * $x }", - result: Some(Value::List { - vals: vec![ - Value::Int { val: 1, span }, - Value::Int { val: 4, span }, - Value::Int { val: 9, span }, - ], - span, - }), + example: "for x in [1 2 3] { print ($x * $x) }", + result: None, }, Example { description: "Work with elements of a range", - example: "for $x in 1..3 { $x }", - result: Some(Value::List { - vals: vec![ - Value::Int { val: 1, span }, - Value::Int { val: 2, span }, - Value::Int { val: 3, span }, - ], - span, - }), + example: "for $x in 1..3 { print $x }", + result: None, }, Example { description: "Number each item and echo a message", - example: "for $it in ['bob' 'fred'] --numbered { $\"($it.index) is ($it.item)\" }", - result: Some(Value::List { - vals: vec![ - Value::String { - val: "0 is bob".into(), - span, - }, - Value::String { - val: "1 is fred".into(), - span, - }, - ], - span, - }), + example: + "for $it in ['bob' 'fred'] --numbered { print $\"($it.index) is ($it.item)\" }", + result: None, }, ] } diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 6d946aa486..c67b07e017 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -4732,6 +4732,16 @@ pub fn parse_expression( spans[0], )), ), + b"for" => ( + parse_call( + working_set, + &spans[pos..], + spans[0], + expand_aliases_denylist, + ) + .0, + Some(ParseError::BuiltinCommandInPipeline("for".into(), spans[0])), + ), b"let" => ( parse_call( working_set, @@ -4866,7 +4876,6 @@ pub fn parse_expression( )), ), - b"for" => parse_for(working_set, spans, expand_aliases_denylist), _ => parse_call( working_set, &spans[pos..], diff --git a/src/tests/test_engine.rs b/src/tests/test_engine.rs index 9056ef4562..8e0796c4e4 100644 --- a/src/tests/test_engine.rs +++ b/src/tests/test_engine.rs @@ -128,11 +128,6 @@ fn proper_variable_captures_with_nesting() -> TestResult { ) } -#[test] -fn proper_variable_for() -> TestResult { - run_test(r#"for x in 1..3 { if $x == 2 { "bob" } } | get 0"#, "bob") -} - #[test] fn divide_duration() -> TestResult { run_test(r#"4ms / 4ms"#, "1") diff --git a/src/tests/test_iteration.rs b/src/tests/test_iteration.rs index da815b78d9..645105857d 100644 --- a/src/tests/test_iteration.rs +++ b/src/tests/test_iteration.rs @@ -32,11 +32,6 @@ fn row_condition2() -> TestResult { ) } -#[test] -fn for_loops() -> TestResult { - run_test(r#"(for x in [1, 2, 3] { $x + 10 }).1"#, "12") -} - #[test] fn par_each() -> TestResult { run_test( diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs index 62135ea90e..5c072e47ac 100644 --- a/tests/shell/pipeline/commands/internal.rs +++ b/tests/shell/pipeline/commands/internal.rs @@ -83,6 +83,18 @@ fn argument_subexpression() { assert_eq!(actual.out, "foo"); } +#[test] +fn for_loop() { + let actual = nu!( + cwd: ".", + r#" + for i in 1..3 { print $i } + "# + ); + + assert_eq!(actual.out, "123"); +} + #[test] fn subexpression_handles_dot() { Playground::setup("subexpression_handles_dot", |dirs, sandbox| {