From 75689ec98aa7037f1101844ea16f1934c51ff706 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Wed, 22 May 2024 19:56:51 +0300 Subject: [PATCH] Small improvements to `debug profile` (#12930) # Description 1. With the `-l` flag, `debug profile` now collects files and line numbers of profiled pipeline elements ![profiler_lines](https://github.com/nushell/nushell/assets/25571562/b400a956-d958-4aff-aa4c-7e65da3f78fa) 2. Error from the profiled closure will be reported instead of silently ignored. ![profiler_lines_error](https://github.com/nushell/nushell/assets/25571562/54f7ad7a-06a3-4d56-92c2-c3466917bee8) # User-Facing Changes New `--lines(-l)` flag to `debug profile`. The command will also fail if the profiled closure fails, so technically it is a breaking change. # Tests + Formatting # After Submitting --------- Co-authored-by: Ian Manske --- crates/nu-command/src/debug/profile.rs | 16 ++++----- crates/nu-protocol/src/debugger/profiler.rs | 40 +++++++++++++++++++++ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/crates/nu-command/src/debug/profile.rs b/crates/nu-command/src/debug/profile.rs index bd5de6041a..0cef979094 100644 --- a/crates/nu-command/src/debug/profile.rs +++ b/crates/nu-command/src/debug/profile.rs @@ -28,6 +28,7 @@ impl Command for DebugProfile { Some('v'), ) .switch("expr", "Collect expression types", Some('x')) + .switch("lines", "Collect line numbers", Some('l')) .named( "max-depth", SyntaxShape::Int, @@ -90,6 +91,7 @@ confusing the id/parent_id hierarchy. The --expr flag is helpful for investigati let collect_expanded_source = call.has_flag(engine_state, stack, "expanded-source")?; let collect_values = call.has_flag(engine_state, stack, "values")?; let collect_exprs = call.has_flag(engine_state, stack, "expr")?; + let collect_lines = call.has_flag(engine_state, stack, "lines")?; let max_depth = call .get_flag(engine_state, stack, "max-depth")? .unwrap_or(2); @@ -101,6 +103,7 @@ confusing the id/parent_id hierarchy. The --expr flag is helpful for investigati collect_expanded_source, collect_values, collect_exprs, + collect_lines, call.span(), ); @@ -118,14 +121,11 @@ confusing the id/parent_id hierarchy. The --expr flag is helpful for investigati let result = ClosureEvalOnce::new(engine_state, stack, closure).run_with_input(input); - // TODO: See eval_source() - match result { - Ok(pipeline_data) => { - let _ = pipeline_data.into_value(call.span()); - // pipeline_data.print(engine_state, caller_stack, true, false) - } - Err(_e) => (), // TODO: Report error - } + // Return potential errors + let pipeline_data = result?; + + // Collect the output + let _ = pipeline_data.into_value(call.span()); Ok(engine_state .deactivate_debugger() diff --git a/crates/nu-protocol/src/debugger/profiler.rs b/crates/nu-protocol/src/debugger/profiler.rs index 53b9d0555a..b95bf571b4 100644 --- a/crates/nu-protocol/src/debugger/profiler.rs +++ b/crates/nu-protocol/src/debugger/profiler.rs @@ -9,6 +9,7 @@ use crate::{ engine::EngineState, record, PipelineData, ShellError, Span, Value, }; +use std::io::BufRead; use std::time::Instant; #[derive(Debug, Clone, Copy)] @@ -50,11 +51,13 @@ pub struct Profiler { collect_expanded_source: bool, collect_values: bool, collect_exprs: bool, + collect_lines: bool, elements: Vec, element_stack: Vec, } impl Profiler { + #[allow(clippy::too_many_arguments)] pub fn new( max_depth: i64, collect_spans: bool, @@ -62,6 +65,7 @@ impl Profiler { collect_expanded_source: bool, collect_values: bool, collect_exprs: bool, + collect_lines: bool, span: Span, ) -> Self { let first = ElementInfo { @@ -82,6 +86,7 @@ impl Profiler { collect_expanded_source, collect_values, collect_exprs, + collect_lines, elements: vec![first], element_stack: vec![ElementId(0)], } @@ -262,6 +267,31 @@ fn expr_to_string(engine_state: &EngineState, expr: &Expr) -> String { } } +// Find a file name and a line number (indexed from 1) of a span +fn find_file_of_span(engine_state: &EngineState, span: Span) -> Option<(&str, usize)> { + for file in engine_state.files() { + if file.covered_span.contains_span(span) { + // count the number of lines between file start and the searched span start + let chunk = + engine_state.get_span_contents(Span::new(file.covered_span.start, span.start)); + let nlines = chunk.lines().count(); + // account for leading part of current line being counted as a separate line + let line_num = if chunk.last() == Some(&b'\n') { + nlines + 1 + } else { + nlines + }; + + // first line has no previous line, clamp up to `1` + let line_num = usize::max(line_num, 1); + + return Some((&file.name, line_num)); + } + } + + None +} + fn collect_data( engine_state: &EngineState, profiler: &Profiler, @@ -277,6 +307,16 @@ fn collect_data( "parent_id" => Value::int(parent_id.0 as i64, profiler_span), }; + if profiler.collect_lines { + if let Some((fname, line_num)) = find_file_of_span(engine_state, element.element_span) { + row.push("file", Value::string(fname, profiler_span)); + row.push("line_num", Value::int(line_num as i64, profiler_span)); + } else { + row.push("file", Value::nothing(profiler_span)); + row.push("line", Value::nothing(profiler_span)); + } + } + if profiler.collect_spans { let span_start = i64::try_from(element.element_span.start) .map_err(|_| profiler_error("error converting span start to i64", profiler_span))?;