From 7d2e8875e0d9c19d74501cc69bde0103a040dd2e Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Tue, 10 Dec 2024 07:08:53 -0800 Subject: [PATCH] Make `timeit` take only closures as an argument (#14483) # Description Fixes #14401 where expressions passed to `timeit` will execute twice. This PR removes the expression support for `timeit`, as this behavior is almost exclusive to `timeit` and can hinder migration to the IR evaluator in the future. Additionally, `timeit` used to be able to take a `block` as an argument. Blocks should probably only be allowed for parser keywords, so this PR changes `timeit` to instead only take closures as an argument. This also fixes an issue where environment updates inside the `timeit` block would affect the parent scope and all commands later in the pipeline. ```nu > timeit { $env.FOO = 'bar' }; print $env.FOO bar ``` # User-Facing Changes `timeit` now only takes a closure as the first argument. # After Submitting Update examples in the book/docs if necessary. --- crates/nu-command/src/debug/timeit.rs | 60 ++++++++----------- .../nu-command/tests/commands/debug/timeit.rs | 2 +- crates/nu-engine/src/closure_eval.rs | 39 ++++++++++++ 3 files changed, 65 insertions(+), 36 deletions(-) diff --git a/crates/nu-command/src/debug/timeit.rs b/crates/nu-command/src/debug/timeit.rs index 7bb492769c..182cab87d7 100644 --- a/crates/nu-command/src/debug/timeit.rs +++ b/crates/nu-command/src/debug/timeit.rs @@ -1,4 +1,5 @@ -use nu_engine::{command_prelude::*, get_eval_block, get_eval_expression_with_input}; +use nu_engine::{command_prelude::*, ClosureEvalOnce}; +use nu_protocol::engine::Closure; use std::time::Instant; #[derive(Clone)] @@ -10,16 +11,18 @@ impl Command for TimeIt { } fn description(&self) -> &str { - "Time the running time of a block." + "Time how long it takes a closure to run." + } + + fn extra_description(&self) -> &str { + "Any pipeline input given to this command is passed to the closure. Note that streaming inputs may affect timing results, and it is recommended to add a `collect` command before this if the input is a stream. + +This command will bubble up any errors encountered when running the closure. The return pipeline of the closure is collected into a value and then discarded." } fn signature(&self) -> nu_protocol::Signature { Signature::build("timeit") - .required( - "command", - SyntaxShape::OneOf(vec![SyntaxShape::Block, SyntaxShape::Expression]), - "The command or block to run.", - ) + .required("command", SyntaxShape::Closure(None), "The closure to run.") .input_output_types(vec![ (Type::Any, Type::Duration), (Type::Nothing, Type::Duration), @@ -46,51 +49,38 @@ impl Command for TimeIt { // reset outdest, so the command can write to stdout and stderr. let stack = &mut stack.push_redirection(None, None); - let command_to_run = call.positional_nth(stack, 0); + let closure: Closure = call.req(engine_state, stack, 0)?; + let closure = ClosureEvalOnce::new_preserve_out_dest(engine_state, stack, closure); // Get the start time after all other computation has been done. let start_time = Instant::now(); + closure.run_with_input(input)?.into_value(call.head)?; + let time = start_time.elapsed(); - if let Some(command_to_run) = command_to_run { - if let Some(block_id) = command_to_run.as_block() { - let eval_block = get_eval_block(engine_state); - let block = engine_state.get_block(block_id); - eval_block(engine_state, stack, block, input)? - } else { - let eval_expression_with_input = get_eval_expression_with_input(engine_state); - let expression = &command_to_run.clone(); - eval_expression_with_input(engine_state, stack, expression, input)? - } - } else { - PipelineData::empty() - } - .into_value(call.head)?; - - let end_time = Instant::now(); - - let output = Value::duration( - end_time.saturating_duration_since(start_time).as_nanos() as i64, - call.head, - ); - + let output = Value::duration(time.as_nanos() as i64, call.head); Ok(output.into_pipeline_data()) } fn examples(&self) -> Vec { vec![ Example { - description: "Times a command within a closure", + description: "Time a closure containing one command", example: "timeit { sleep 500ms }", result: None, }, Example { - description: "Times a command using an existing input", - example: "http get https://www.nushell.sh/book/ | timeit { split chars }", + description: "Time a closure with an input value", + example: "'A really long string' | timeit { split chars }", result: None, }, Example { - description: "Times a command invocation", - example: "timeit ls -la", + description: "Time a closure with an input stream", + example: "open some_file.txt | collect | timeit { split chars }", + result: None, + }, + Example { + description: "Time a closure containing a pipeline", + example: "timeit { open some_file.txt | split chars }", result: None, }, ] diff --git a/crates/nu-command/tests/commands/debug/timeit.rs b/crates/nu-command/tests/commands/debug/timeit.rs index a59f67d26a..509f5bc4e7 100644 --- a/crates/nu-command/tests/commands/debug/timeit.rs +++ b/crates/nu-command/tests/commands/debug/timeit.rs @@ -2,7 +2,7 @@ use nu_test_support::nu; #[test] fn timeit_show_stdout() { - let actual = nu!("let t = timeit nu --testbin cococo abcdefg"); + let actual = nu!("let t = timeit { nu --testbin cococo abcdefg }"); assert_eq!(actual.out, "abcdefg"); } diff --git a/crates/nu-engine/src/closure_eval.rs b/crates/nu-engine/src/closure_eval.rs index b271d90cbe..66c6287cca 100644 --- a/crates/nu-engine/src/closure_eval.rs +++ b/crates/nu-engine/src/closure_eval.rs @@ -88,6 +88,29 @@ impl ClosureEval { } } + pub fn new_preserve_out_dest( + engine_state: &EngineState, + stack: &Stack, + closure: Closure, + ) -> Self { + let engine_state = engine_state.clone(); + let stack = stack.captures_to_stack_preserve_out_dest(closure.captures); + let block = engine_state.get_block(closure.block_id).clone(); + let env_vars = stack.env_vars.clone(); + let env_hidden = stack.env_hidden.clone(); + let eval = get_eval_block_with_early_return(&engine_state); + + Self { + engine_state, + stack, + block, + arg_index: 0, + env_vars, + env_hidden, + eval, + } + } + /// Sets whether to enable debugging when evaluating the closure. /// /// By default, this is controlled by the [`EngineState`] used to create this [`ClosureEval`]. @@ -189,6 +212,22 @@ impl<'a> ClosureEvalOnce<'a> { } } + pub fn new_preserve_out_dest( + engine_state: &'a EngineState, + stack: &Stack, + closure: Closure, + ) -> Self { + let block = engine_state.get_block(closure.block_id); + let eval = get_eval_block_with_early_return(engine_state); + Self { + engine_state, + stack: stack.captures_to_stack_preserve_out_dest(closure.captures), + block, + arg_index: 0, + eval, + } + } + /// Sets whether to enable debugging when evaluating the closure. /// /// By default, this is controlled by the [`EngineState`] used to create this [`ClosureEvalOnce`].