From 3e5a70a7f0cd8dac2194e3058a99beaa1cc11e91 Mon Sep 17 00:00:00 2001 From: Martin Verzilli Date: Wed, 18 Oct 2023 12:40:47 +0200 Subject: [PATCH 1/8] Make it work --- Cargo.lock | 1 + tooling/debugger/Cargo.toml | 3 +- tooling/debugger/src/lib.rs | 68 ++++++++++++++++++++++++++++++++----- 3 files changed, 62 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5601dbae53d..9499f8d9f29 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2542,6 +2542,7 @@ name = "noir_debugger" version = "0.16.0" dependencies = [ "acvm", + "codespan-reporting", "easy-repl", "nargo", "noirc_printable_type", diff --git a/tooling/debugger/Cargo.toml b/tooling/debugger/Cargo.toml index e01abd9ac61..1f9a54b4fe8 100644 --- a/tooling/debugger/Cargo.toml +++ b/tooling/debugger/Cargo.toml @@ -13,5 +13,6 @@ acvm.workspace = true nargo.workspace = true noirc_printable_type.workspace = true thiserror.workspace = true +codespan-reporting.workspace = true easy-repl = "0.2.1" -owo-colors = "3" \ No newline at end of file +owo-colors = "3" diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index f8e9db19234..08258ec55cc 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -14,6 +14,8 @@ use std::cell::{Cell, RefCell}; use owo_colors::OwoColorize; +use codespan_reporting::files::Files; + enum SolveResult { Done, Ok, @@ -74,25 +76,73 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { println!("Finished execution"); } else { println!("Stopped at opcode {}: {}", ip, opcodes[ip]); - Self::show_source_code_location(&OpcodeLocation::Acir(ip), &self.debug_artifact); + self.show_source_code_location(&OpcodeLocation::Acir(ip), &self.debug_artifact); } } - fn show_source_code_location(location: &OpcodeLocation, debug_artifact: &DebugArtifact) { + fn show_source_code_location(&self, location: &OpcodeLocation, debug_artifact: &DebugArtifact) { let locations = debug_artifact.debug_symbols[0].opcode_location(location); if let Some(locations) = locations { for loc in locations { let file = &debug_artifact.file_map[&loc.file]; let source = &file.source.as_str(); - let start = loc.span.start() as usize; - let end = loc.span.end() as usize; - println!("At {}:{start}-{end}", file.path.as_path().display()); + let loc_start = loc.span.start() as usize; + let loc_end = loc.span.end() as usize; + + let line_index = + Files::line_index(&self.debug_artifact, loc.file, loc_start).unwrap(); + let line_number = + Files::line_number(&self.debug_artifact, loc.file, line_index).unwrap(); + let column_number = + Files::column_number(&self.debug_artifact, loc.file, line_index, loc_start) + .unwrap(); + let line_span = + DebugArtifact::line_range(&self.debug_artifact, loc.file, line_index).unwrap(); + let last_line_index = + Files::line_index(&self.debug_artifact, loc.file, source.len()).unwrap(); + let first_line_to_print = if line_index < 5 { 0 } else { line_index - 5 }; + + let last_line_to_print = + if line_index + 5 > last_line_index { last_line_index } else { line_index + 5 }; + println!( - "\n{}{}{}\n", - &source[0..start].to_string().dimmed(), - &source[start..end], - &source[end..].to_string().dimmed(), + "At {}:{line_number}:{column_number}", + Files::name(&self.debug_artifact, loc.file).unwrap() ); + + for (current_line_index, line) in source.lines().enumerate() { + let number = current_line_index + 1; + + if current_line_index < first_line_to_print { + // Ignore lines before range starts + continue; + } else if current_line_index == first_line_to_print && current_line_index > 0 { + // Denote that there's more lines before but we're not showing them + println!("{:>3} {}", current_line_index.dimmed(), "...".dimmed()); + } + + if current_line_index > last_line_to_print { + // Denote that there's more lines after but we're not showing them, + // and stop printing + println!("{:>3} {}", number.dimmed(), "...".dimmed()); + break; + } + + if current_line_index == line_index { + let loc_start_in_line = loc_start - line_span.start; + let loc_end_in_line = loc_end - line_span.start; + println!( + "{:>3} {:2} {}{}{}", + number, + "->", + &line[0..loc_start_in_line].to_string().dimmed(), + &line[loc_start_in_line..loc_end_in_line], + &line[loc_end_in_line..].to_string().dimmed() + ); + } else { + println!("{:>3} {:2} {}", number.dimmed(), "".dimmed(), line.dimmed()); + } + } } } } From 77aa63f60264317fa894da3fec49ac547b047f7f Mon Sep 17 00:00:00 2001 From: Martin Verzilli Date: Wed, 18 Oct 2023 12:59:36 +0200 Subject: [PATCH 2/8] Add fn to retrieve the source of the file a loc is in --- tooling/debugger/src/lib.rs | 3 +-- tooling/nargo/src/artifacts/debug.rs | 6 +++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 08258ec55cc..97eca2e7bf4 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -84,8 +84,7 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { let locations = debug_artifact.debug_symbols[0].opcode_location(location); if let Some(locations) = locations { for loc in locations { - let file = &debug_artifact.file_map[&loc.file]; - let source = &file.source.as_str(); + let source = debug_artifact.location_source_code(loc); let loc_start = loc.span.start() as usize; let loc_end = loc.span.end() as usize; diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index 3c173f34876..e435096057e 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -1,6 +1,6 @@ use codespan_reporting::files::{Error, Files, SimpleFile}; use noirc_driver::DebugFile; -use noirc_errors::debug_info::DebugInfo; +use noirc_errors::{debug_info::DebugInfo, Location}; use serde::{Deserialize, Serialize}; use std::{ collections::{BTreeMap, BTreeSet}, @@ -45,6 +45,10 @@ impl DebugArtifact { Self { debug_symbols, file_map } } + + pub fn location_source_code(&self, location: Location) -> &str { + self.file_map[&location.file].source.as_str() + } } impl<'a> Files<'a> for DebugArtifact { From eec1f951f039c5d4fdfe2a3cc9001c6ada9ca198 Mon Sep 17 00:00:00 2001 From: Martin Verzilli Date: Wed, 18 Oct 2023 13:22:36 +0200 Subject: [PATCH 3/8] Add more fns to query debug artifact by location --- tooling/debugger/src/lib.rs | 11 ++++------- tooling/nargo/src/artifacts/debug.rs | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 97eca2e7bf4..038aa27882e 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -85,16 +85,13 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { if let Some(locations) = locations { for loc in locations { let source = debug_artifact.location_source_code(loc); + let line_index = debug_artifact.location_line_index(loc).unwrap(); + let line_number = debug_artifact.location_line_number(loc).unwrap(); + let column_number = debug_artifact.location_column_number(loc).unwrap(); + let loc_start = loc.span.start() as usize; let loc_end = loc.span.end() as usize; - let line_index = - Files::line_index(&self.debug_artifact, loc.file, loc_start).unwrap(); - let line_number = - Files::line_number(&self.debug_artifact, loc.file, line_index).unwrap(); - let column_number = - Files::column_number(&self.debug_artifact, loc.file, line_index, loc_start) - .unwrap(); let line_span = DebugArtifact::line_range(&self.debug_artifact, loc.file, line_index).unwrap(); let last_line_index = diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index e435096057e..25d795acab9 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -46,9 +46,30 @@ impl DebugArtifact { Self { debug_symbols, file_map } } + /// Given a location, returns its file's source code pub fn location_source_code(&self, location: Location) -> &str { self.file_map[&location.file].source.as_str() } + + /// Given a location, returns the index of the line it starts at + pub fn location_line_index(&self, location: Location) -> Result { + let location_start = location.span.start() as usize; + Files::line_index(self, location.file, location_start) + } + + /// Given a location, returns the line number it starts at + pub fn location_line_number(&self, location: Location) -> Result { + let location_start = location.span.start() as usize; + let line_index = Files::line_index(self, location.file, location_start); + Files::line_number(self, location.file, line_index.unwrap()) + } + + /// Given a location, returns the column number it starts at + pub fn location_column_number(&self, location: Location) -> Result { + let location_start = location.span.start() as usize; + let line_index = Files::line_index(self, location.file, location_start); + Files::column_number(self, location.file, line_index.unwrap(), location_start) + } } impl<'a> Files<'a> for DebugArtifact { From 2231480b7d6781db19afce7f16f3af69cb53302d Mon Sep 17 00:00:00 2001 From: Martin Verzilli Date: Wed, 18 Oct 2023 13:28:22 +0200 Subject: [PATCH 4/8] Make location_source_code safer --- tooling/debugger/src/lib.rs | 2 +- tooling/nargo/src/artifacts/debug.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 038aa27882e..8ce6c33ebc8 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -84,7 +84,7 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { let locations = debug_artifact.debug_symbols[0].opcode_location(location); if let Some(locations) = locations { for loc in locations { - let source = debug_artifact.location_source_code(loc); + let source = debug_artifact.location_source_code(loc).unwrap(); let line_index = debug_artifact.location_line_index(loc).unwrap(); let line_number = debug_artifact.location_line_number(loc).unwrap(); let column_number = debug_artifact.location_column_number(loc).unwrap(); diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index 25d795acab9..c90eb163cdf 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -47,8 +47,8 @@ impl DebugArtifact { } /// Given a location, returns its file's source code - pub fn location_source_code(&self, location: Location) -> &str { - self.file_map[&location.file].source.as_str() + pub fn location_source_code(&self, location: Location) -> Result<&str, Error> { + Files::source(self, location.file) } /// Given a location, returns the index of the line it starts at From d30d356bdfa6ef1e45581f955827969a599aafa3 Mon Sep 17 00:00:00 2001 From: Martin Verzilli Date: Wed, 18 Oct 2023 14:00:52 +0200 Subject: [PATCH 5/8] Add fn to get a loc's relative position in its line --- tooling/debugger/src/lib.rs | 19 ++++++++----------- tooling/nargo/src/artifacts/debug.rs | 23 +++++++++++++++++++---- 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 8ce6c33ebc8..622d755cd5f 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -10,7 +10,10 @@ use nargo::NargoError; use nargo::ops::ForeignCallExecutor; use easy_repl::{command, CommandStatus, Critical, Repl}; -use std::cell::{Cell, RefCell}; +use std::{ + cell::{Cell, RefCell}, + ops::Range +}; use owo_colors::OwoColorize; @@ -89,11 +92,6 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { let line_number = debug_artifact.location_line_number(loc).unwrap(); let column_number = debug_artifact.location_column_number(loc).unwrap(); - let loc_start = loc.span.start() as usize; - let loc_end = loc.span.end() as usize; - - let line_span = - DebugArtifact::line_range(&self.debug_artifact, loc.file, line_index).unwrap(); let last_line_index = Files::line_index(&self.debug_artifact, loc.file, source.len()).unwrap(); let first_line_to_print = if line_index < 5 { 0 } else { line_index - 5 }; @@ -125,15 +123,14 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { } if current_line_index == line_index { - let loc_start_in_line = loc_start - line_span.start; - let loc_end_in_line = loc_end - line_span.start; + let Range { start: loc_start, end: loc_end } = debug_artifact.location_in_line(loc).unwrap(); println!( "{:>3} {:2} {}{}{}", number, "->", - &line[0..loc_start_in_line].to_string().dimmed(), - &line[loc_start_in_line..loc_end_in_line], - &line[loc_end_in_line..].to_string().dimmed() + &line[0..loc_start].to_string().dimmed(), + &line[loc_start..loc_end], + &line[loc_end..].to_string().dimmed() ); } else { println!("{:>3} {:2} {}", number.dimmed(), "".dimmed(), line.dimmed()); diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index c90eb163cdf..03135057ff9 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -60,15 +60,30 @@ impl DebugArtifact { /// Given a location, returns the line number it starts at pub fn location_line_number(&self, location: Location) -> Result { let location_start = location.span.start() as usize; - let line_index = Files::line_index(self, location.file, location_start); - Files::line_number(self, location.file, line_index.unwrap()) + let line_index = Files::line_index(self, location.file, location_start).unwrap(); + Files::line_number(self, location.file, line_index) } /// Given a location, returns the column number it starts at pub fn location_column_number(&self, location: Location) -> Result { let location_start = location.span.start() as usize; - let line_index = Files::line_index(self, location.file, location_start); - Files::column_number(self, location.file, line_index.unwrap(), location_start) + let line_index = Files::line_index(self, location.file, location_start).unwrap(); + Files::column_number(self, location.file, line_index, location_start) + } + + /// Given a location, returns a Span relative to its line's + /// position in the file. This is useful when processing a file's + /// contents on a per-line-basis. + pub fn location_in_line(&self, location: Location) -> Result, Error> { + let location_start = location.span.start() as usize; + let location_end = location.span.end() as usize; + let line_index = Files::line_index(self, location.file, location_start).unwrap(); + let line_span = DebugArtifact::line_range(self, location.file, line_index).unwrap(); + + let start_in_line = location_start - line_span.start; + let end_in_line = location_end - line_span.start; + + Ok(Range { start: start_in_line, end: end_in_line }) } } From a73b9db399d917d87a80068b746ff2b139cb3bb7 Mon Sep 17 00:00:00 2001 From: Martin Verzilli Date: Wed, 18 Oct 2023 14:23:57 +0200 Subject: [PATCH 6/8] refactor show_source_code_location --- tooling/debugger/src/lib.rs | 60 ++++++++++++++++++---------- tooling/nargo/src/artifacts/debug.rs | 7 ++++ tooling/nargo/src/errors.rs | 5 ++- 3 files changed, 49 insertions(+), 23 deletions(-) diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 622d755cd5f..36ffbac6153 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -4,7 +4,7 @@ use acvm::BlackBoxFunctionSolver; use acvm::{acir::circuit::Circuit, acir::native_types::WitnessMap}; use nargo::artifacts::debug::DebugArtifact; -use nargo::errors::ExecutionError; +use nargo::errors::{ExecutionError, Location}; use nargo::NargoError; use nargo::ops::ForeignCallExecutor; @@ -12,7 +12,7 @@ use nargo::ops::ForeignCallExecutor; use easy_repl::{command, CommandStatus, Critical, Repl}; use std::{ cell::{Cell, RefCell}, - ops::Range + ops::Range, }; use owo_colors::OwoColorize; @@ -83,29 +83,39 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { } } + fn print_location_path(&self, loc: Location) { + let line_number = self.debug_artifact.location_line_number(loc).unwrap(); + let column_number = self.debug_artifact.location_column_number(loc).unwrap(); + + println!( + "At {}:{line_number}:{column_number}", + Files::name(&self.debug_artifact, loc.file).unwrap() + ); + } + fn show_source_code_location(&self, location: &OpcodeLocation, debug_artifact: &DebugArtifact) { let locations = debug_artifact.debug_symbols[0].opcode_location(location); if let Some(locations) = locations { for loc in locations { - let source = debug_artifact.location_source_code(loc).unwrap(); - let line_index = debug_artifact.location_line_index(loc).unwrap(); - let line_number = debug_artifact.location_line_number(loc).unwrap(); - let column_number = debug_artifact.location_column_number(loc).unwrap(); - - let last_line_index = - Files::line_index(&self.debug_artifact, loc.file, source.len()).unwrap(); - let first_line_to_print = if line_index < 5 { 0 } else { line_index - 5 }; - - let last_line_to_print = - if line_index + 5 > last_line_index { last_line_index } else { line_index + 5 }; + self.print_location_path(loc); - println!( - "At {}:{line_number}:{column_number}", - Files::name(&self.debug_artifact, loc.file).unwrap() - ); + let line_index = debug_artifact.location_line_index(loc).unwrap(); + let last_line_index = debug_artifact.last_line_index(loc).unwrap(); + let print_context_size = 5; + let first_line_to_print = if line_index < print_context_size { + 0 + } else { + line_index - print_context_size + }; + let last_line_to_print = if line_index + print_context_size > last_line_index { + last_line_index + } else { + line_index + print_context_size + }; + let source = debug_artifact.location_source_code(loc).unwrap(); for (current_line_index, line) in source.lines().enumerate() { - let number = current_line_index + 1; + let current_line_number = current_line_index + 1; if current_line_index < first_line_to_print { // Ignore lines before range starts @@ -118,22 +128,28 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { if current_line_index > last_line_to_print { // Denote that there's more lines after but we're not showing them, // and stop printing - println!("{:>3} {}", number.dimmed(), "...".dimmed()); + println!("{:>3} {}", current_line_number.dimmed(), "...".dimmed()); break; } if current_line_index == line_index { - let Range { start: loc_start, end: loc_end } = debug_artifact.location_in_line(loc).unwrap(); + let Range { start: loc_start, end: loc_end } = + debug_artifact.location_in_line(loc).unwrap(); println!( "{:>3} {:2} {}{}{}", - number, + current_line_number, "->", &line[0..loc_start].to_string().dimmed(), &line[loc_start..loc_end], &line[loc_end..].to_string().dimmed() ); } else { - println!("{:>3} {:2} {}", number.dimmed(), "".dimmed(), line.dimmed()); + println!( + "{:>3} {:2} {}", + current_line_number.dimmed(), + "".dimmed(), + line.dimmed() + ); } } } diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index 03135057ff9..10a53276a0b 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -85,6 +85,13 @@ impl DebugArtifact { Ok(Range { start: start_in_line, end: end_in_line }) } + + /// Given a location, returns the last line index + /// of its file + pub fn last_line_index(&self, location: Location) -> Result { + let source = self.source(location.file).unwrap(); + Files::line_index(self, location.file, source.len()) + } } impl<'a> Files<'a> for DebugArtifact { diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs index ea6e7fa8108..0c920716f2a 100644 --- a/tooling/nargo/src/errors.rs +++ b/tooling/nargo/src/errors.rs @@ -2,7 +2,10 @@ use acvm::{ acir::circuit::OpcodeLocation, pwg::{ErrorLocation, OpcodeResolutionError}, }; -use noirc_errors::{debug_info::DebugInfo, CustomDiagnostic, FileDiagnostic, Location}; +use noirc_errors::{debug_info::DebugInfo, CustomDiagnostic, FileDiagnostic}; + +pub use noirc_errors::Location; + use noirc_printable_type::ForeignCallError; use thiserror::Error; From 60a9431ca1a9b43bab8c235e0f2ef18ec0278d09 Mon Sep 17 00:00:00 2001 From: Martin Verzilli Date: Wed, 18 Oct 2023 14:48:36 +0200 Subject: [PATCH 7/8] Extract some auxiliaries for printing --- tooling/debugger/src/lib.rs | 42 +++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 36ffbac6153..206d72dc8de 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -99,18 +99,20 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { for loc in locations { self.print_location_path(loc); - let line_index = debug_artifact.location_line_index(loc).unwrap(); + let loc_line_index = debug_artifact.location_line_index(loc).unwrap(); + + // How many lines before or after the location's line we + // print + let context_lines = 5; + + let first_line_to_print = + if loc_line_index < context_lines { 0 } else { loc_line_index - context_lines }; + let last_line_index = debug_artifact.last_line_index(loc).unwrap(); - let print_context_size = 5; - let first_line_to_print = if line_index < print_context_size { - 0 - } else { - line_index - print_context_size - }; - let last_line_to_print = if line_index + print_context_size > last_line_index { + let last_line_to_print = if loc_line_index + context_lines > last_line_index { last_line_index } else { - line_index + print_context_size + loc_line_index + context_lines }; let source = debug_artifact.location_source_code(loc).unwrap(); @@ -122,17 +124,18 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { continue; } else if current_line_index == first_line_to_print && current_line_index > 0 { // Denote that there's more lines before but we're not showing them - println!("{:>3} {}", current_line_index.dimmed(), "...".dimmed()); + print_ellipsized_line(current_line_index); } if current_line_index > last_line_to_print { // Denote that there's more lines after but we're not showing them, // and stop printing - println!("{:>3} {}", current_line_number.dimmed(), "...".dimmed()); + print_ellipsized_line(current_line_number); break; } - if current_line_index == line_index { + if current_line_index == loc_line_index { + // Highlight current location let Range { start: loc_start, end: loc_end } = debug_artifact.location_in_line(loc).unwrap(); println!( @@ -144,12 +147,7 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { &line[loc_end..].to_string().dimmed() ); } else { - println!( - "{:>3} {:2} {}", - current_line_number.dimmed(), - "".dimmed(), - line.dimmed() - ); + print_dimmed_line(current_line_number, line); } } } @@ -171,6 +169,14 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { } } +fn print_ellipsized_line(line_number: usize) { + println!("{}", format!("{:>3} {}", line_number, "...").dimmed()); +} + +fn print_dimmed_line(line_number: usize, line: &str) { + println!("{}", format!("{:>3} {:2} {}", line_number, "", line).dimmed()); +} + fn map_command_status(result: SolveResult) -> CommandStatus { match result { SolveResult::Ok => CommandStatus::Done, From 08bc809e2dd5687dded6d0e4a7b1f9dfa127da72 Mon Sep 17 00:00:00 2001 From: Martin Verzilli Date: Tue, 24 Oct 2023 09:44:38 +0200 Subject: [PATCH 8/8] Apply code review suggestions --- tooling/debugger/src/lib.rs | 91 +++++++++++++--------------- tooling/nargo/src/artifacts/debug.rs | 20 +++--- 2 files changed, 53 insertions(+), 58 deletions(-) diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 206d72dc8de..3da9c5c7989 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -89,66 +89,61 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { println!( "At {}:{line_number}:{column_number}", - Files::name(&self.debug_artifact, loc.file).unwrap() + self.debug_artifact.name(loc.file).unwrap() ); } fn show_source_code_location(&self, location: &OpcodeLocation, debug_artifact: &DebugArtifact) { let locations = debug_artifact.debug_symbols[0].opcode_location(location); - if let Some(locations) = locations { - for loc in locations { - self.print_location_path(loc); + let Some(locations) = locations else { return }; + for loc in locations { + self.print_location_path(loc); - let loc_line_index = debug_artifact.location_line_index(loc).unwrap(); + let loc_line_index = debug_artifact.location_line_index(loc).unwrap(); - // How many lines before or after the location's line we - // print - let context_lines = 5; + // How many lines before or after the location's line we + // print + let context_lines = 5; - let first_line_to_print = - if loc_line_index < context_lines { 0 } else { loc_line_index - context_lines }; + let first_line_to_print = + if loc_line_index < context_lines { 0 } else { loc_line_index - context_lines }; - let last_line_index = debug_artifact.last_line_index(loc).unwrap(); - let last_line_to_print = if loc_line_index + context_lines > last_line_index { - last_line_index - } else { - loc_line_index + context_lines - }; + let last_line_index = debug_artifact.last_line_index(loc).unwrap(); + let last_line_to_print = std::cmp::min(loc_line_index + context_lines, last_line_index); - let source = debug_artifact.location_source_code(loc).unwrap(); - for (current_line_index, line) in source.lines().enumerate() { - let current_line_number = current_line_index + 1; + let source = debug_artifact.location_source_code(loc).unwrap(); + for (current_line_index, line) in source.lines().enumerate() { + let current_line_number = current_line_index + 1; - if current_line_index < first_line_to_print { - // Ignore lines before range starts - continue; - } else if current_line_index == first_line_to_print && current_line_index > 0 { - // Denote that there's more lines before but we're not showing them - print_ellipsized_line(current_line_index); - } + if current_line_index < first_line_to_print { + // Ignore lines before range starts + continue; + } else if current_line_index == first_line_to_print && current_line_index > 0 { + // Denote that there's more lines before but we're not showing them + print_line_of_ellipsis(current_line_index); + } - if current_line_index > last_line_to_print { - // Denote that there's more lines after but we're not showing them, - // and stop printing - print_ellipsized_line(current_line_number); - break; - } + if current_line_index > last_line_to_print { + // Denote that there's more lines after but we're not showing them, + // and stop printing + print_line_of_ellipsis(current_line_number); + break; + } - if current_line_index == loc_line_index { - // Highlight current location - let Range { start: loc_start, end: loc_end } = - debug_artifact.location_in_line(loc).unwrap(); - println!( - "{:>3} {:2} {}{}{}", - current_line_number, - "->", - &line[0..loc_start].to_string().dimmed(), - &line[loc_start..loc_end], - &line[loc_end..].to_string().dimmed() - ); - } else { - print_dimmed_line(current_line_number, line); - } + if current_line_index == loc_line_index { + // Highlight current location + let Range { start: loc_start, end: loc_end } = + debug_artifact.location_in_line(loc).unwrap(); + println!( + "{:>3} {:2} {}{}{}", + current_line_number, + "->", + &line[0..loc_start].to_string().dimmed(), + &line[loc_start..loc_end], + &line[loc_end..].to_string().dimmed() + ); + } else { + print_dimmed_line(current_line_number, line); } } } @@ -169,7 +164,7 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> { } } -fn print_ellipsized_line(line_number: usize) { +fn print_line_of_ellipsis(line_number: usize) { println!("{}", format!("{:>3} {}", line_number, "...").dimmed()); } diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs index 10a53276a0b..4e2e4166e9c 100644 --- a/tooling/nargo/src/artifacts/debug.rs +++ b/tooling/nargo/src/artifacts/debug.rs @@ -48,27 +48,27 @@ impl DebugArtifact { /// Given a location, returns its file's source code pub fn location_source_code(&self, location: Location) -> Result<&str, Error> { - Files::source(self, location.file) + self.source(location.file) } /// Given a location, returns the index of the line it starts at pub fn location_line_index(&self, location: Location) -> Result { let location_start = location.span.start() as usize; - Files::line_index(self, location.file, location_start) + self.line_index(location.file, location_start) } /// Given a location, returns the line number it starts at pub fn location_line_number(&self, location: Location) -> Result { let location_start = location.span.start() as usize; - let line_index = Files::line_index(self, location.file, location_start).unwrap(); - Files::line_number(self, location.file, line_index) + let line_index = self.line_index(location.file, location_start)?; + self.line_number(location.file, line_index) } /// Given a location, returns the column number it starts at pub fn location_column_number(&self, location: Location) -> Result { let location_start = location.span.start() as usize; - let line_index = Files::line_index(self, location.file, location_start).unwrap(); - Files::column_number(self, location.file, line_index, location_start) + let line_index = self.line_index(location.file, location_start)?; + self.column_number(location.file, line_index, location_start) } /// Given a location, returns a Span relative to its line's @@ -77,8 +77,8 @@ impl DebugArtifact { pub fn location_in_line(&self, location: Location) -> Result, Error> { let location_start = location.span.start() as usize; let location_end = location.span.end() as usize; - let line_index = Files::line_index(self, location.file, location_start).unwrap(); - let line_span = DebugArtifact::line_range(self, location.file, line_index).unwrap(); + let line_index = self.line_index(location.file, location_start)?; + let line_span = self.line_range(location.file, line_index)?; let start_in_line = location_start - line_span.start; let end_in_line = location_end - line_span.start; @@ -89,8 +89,8 @@ impl DebugArtifact { /// Given a location, returns the last line index /// of its file pub fn last_line_index(&self, location: Location) -> Result { - let source = self.source(location.file).unwrap(); - Files::line_index(self, location.file, source.len()) + let source = self.source(location.file)?; + self.line_index(location.file, source.len()) } }