Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(debugger): Add support for debugging tests (#5208) #7

Open
wants to merge 31 commits into
base: feat/5208-debug-tests-repl-dap
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
37283ac
wip
anaPerezGhiglia Jul 3, 2024
21f0739
Compile for debug & run debugger on `nargo test --debug`
anaPerezGhiglia Jul 3, 2024
1589176
wip: extract common behavior
anaPerezGhiglia Jul 12, 2024
1580110
wip: evaluate test status from debugger result
anaPerezGhiglia Jul 12, 2024
e3f10d6
wip: register last seen error in debugger context
anaPerezGhiglia Jul 12, 2024
d9aaa1d
[skip-ci] failing compilation
anaPerezGhiglia Jul 16, 2024
6d16905
Expose errors when the debugger execution finishes
anaPerezGhiglia Jul 16, 2024
aebba26
remove unnecessary mut
anaPerezGhiglia Jul 16, 2024
8b96a17
Ignore --debug mode for tests with arguments
anaPerezGhiglia Jul 23, 2024
7b3961a
return brillig solver ownership
anaPerezGhiglia Jul 24, 2024
536a89f
Map opcode resulution erros in debugger as in executor
anaPerezGhiglia Jul 24, 2024
54fd555
format code
anaPerezGhiglia Jul 24, 2024
6b1cc78
Remove debug println!
anaPerezGhiglia Jul 24, 2024
16a76a1
Add 'Debug test' codelens request
anaPerezGhiglia Jul 26, 2024
873d516
add test_name arg to dap interface
anaPerezGhiglia Jul 26, 2024
92e551d
Compile test function for debugging if test_name is present
anaPerezGhiglia Jul 29, 2024
8da3eef
extract common behavior for building FileManager and ParsedFiles
anaPerezGhiglia Jul 29, 2024
5a1625e
Remove unused imports
anaPerezGhiglia Jul 29, 2024
9eacfe6
Notify if test passed or failed
anaPerezGhiglia Aug 7, 2024
c36f131
fix compile error after rebase
anaPerezGhiglia Aug 7, 2024
c8f8b40
Fix clippy warnings
anaPerezGhiglia Aug 7, 2024
ac3aa22
ignore clippy warnings since functions will be modified
anaPerezGhiglia Aug 7, 2024
7914726
run cargo fmt
anaPerezGhiglia Aug 7, 2024
19bf1e9
Move 'debug test' REPL responsibility from test_cmd to debug_cmd
anaPerezGhiglia Aug 8, 2024
2dc687c
simplify debu_test function
anaPerezGhiglia Aug 8, 2024
b70cd79
remove execution_helpers auxiliary file
anaPerezGhiglia Aug 8, 2024
8d76cac
Expand todo comment
anaPerezGhiglia Aug 8, 2024
2dbda88
Improve error message on unexpected compile error
anaPerezGhiglia Aug 9, 2024
4364c48
Re-shape file_manager_and_file_from to build_workspace_file_manager
anaPerezGhiglia Aug 9, 2024
5ef8e84
Document new "debug test" feature
anaPerezGhiglia Aug 9, 2024
38b58e7
Remove stale TODO
anaPerezGhiglia Aug 9, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions tooling/debugger/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ noirc_printable_type.workspace = true
noirc_errors.workspace = true
noirc_driver.workspace = true
noirc_artifacts.workspace = true
noirc_abi.workspace = true
thiserror.workspace = true
codespan-reporting.workspace = true
dap.workspace = true
Expand Down
35 changes: 29 additions & 6 deletions tooling/debugger/src/context.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::foreign_calls::DebugForeignCallExecutor;
use acvm::acir::brillig::BitSize;
use acvm::acir::circuit::brillig::BrilligBytecode;
use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation};
use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation, ResolvedOpcodeLocation};
use acvm::acir::native_types::{Witness, WitnessMap, WitnessStack};
use acvm::brillig_vm::MemoryValue;
use acvm::pwg::{
Expand All @@ -12,7 +12,7 @@

use codespan_reporting::files::{Files, SimpleFile};
use fm::FileId;
use nargo::errors::{ExecutionError, Location};
use nargo::errors::{map_execution_error, ExecutionError, Location};
use nargo::NargoError;
use noirc_artifacts::debug::{DebugArtifact, StackFrame};
use noirc_driver::DebugFile;
Expand Down Expand Up @@ -183,9 +183,14 @@

#[derive(Debug)]
pub(super) enum DebugCommandResult {
// TODO: validate comments
// The debugging session is over successfully
Done,
// The session is active and we should continue with the execution
Ok,
// Execution should be paused since we reached a Breakpoint
BreakpointReached(DebugLocation),
// Session is over with an error
Comment on lines +186 to +193
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all of these, it's probably desirable to use /// to use the comments as documentation.

Error(NargoError<FieldElement>),
}

Expand Down Expand Up @@ -316,6 +321,20 @@
frames
}

fn get_resolved_call_stack(&self) -> Vec<ResolvedOpcodeLocation> {
self.get_call_stack()
.iter()
.map(|debug_loc| {
// usize should be at least u32 for supported platforms
let acir_function_index = usize::try_from(debug_loc.circuit_id).unwrap();
acvm::acir::circuit::ResolvedOpcodeLocation {
acir_function_index,
opcode_location: debug_loc.opcode_location,
}
})
Comment on lines +327 to +334
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be better to implement the From trait (or Into) for ResolvedOpcodeLocation to convert from DebugLocation since both structs are very much equivalent. Then this would become something like:

self.get_call_stack().iter().map(|debug_loc| debug_loc.into()).collect()

.collect()
}

pub(super) fn is_source_location_in_debug_module(&self, location: &Location) -> bool {
self.debug_artifact
.file_map
Expand Down Expand Up @@ -472,9 +491,13 @@
self.brillig_solver = Some(solver);
self.handle_foreign_call(foreign_call)
}
Err(err) => DebugCommandResult::Error(NargoError::ExecutionError(
ExecutionError::SolvingError(err, None),
)),
Err(err) => {
self.brillig_solver = Some(solver);
DebugCommandResult::Error(NargoError::ExecutionError(map_execution_error(
err,
&self.get_resolved_call_stack(),
)))
}
}
}

Expand Down Expand Up @@ -574,7 +597,7 @@
}
}
ACVMStatus::Failure(error) => DebugCommandResult::Error(NargoError::ExecutionError(
ExecutionError::SolvingError(error, None),
map_execution_error(error, &self.get_resolved_call_stack()),
)),
ACVMStatus::RequiresForeignCall(foreign_call) => self.handle_foreign_call(foreign_call),
ACVMStatus::RequiresAcirCall(call_info) => self.handle_acir_call(call_info),
Expand Down Expand Up @@ -904,7 +927,7 @@
outputs: vec![],
predicate: None,
}];
let brillig_funcs = &vec![brillig_bytecode];

Check warning on line 930 in tooling/debugger/src/context.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (funcs)
let current_witness_index = 2;
let circuit = Circuit { current_witness_index, opcodes, ..Circuit::default() };
let circuits = &vec![circuit];
Expand All @@ -923,7 +946,7 @@
debug_artifact,
initial_witness,
foreign_call_executor,
brillig_funcs,

Check warning on line 949 in tooling/debugger/src/context.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (funcs)
);

assert_eq!(
Expand Down Expand Up @@ -1042,14 +1065,14 @@

let foreign_call_executor =
Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact));
let brillig_funcs = &vec![brillig_bytecode];

Check warning on line 1068 in tooling/debugger/src/context.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (funcs)
let mut context = DebugContext::new(
&StubbedBlackBoxSolver,
circuits,
debug_artifact,
initial_witness,
foreign_call_executor,
brillig_funcs,

Check warning on line 1075 in tooling/debugger/src/context.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (funcs)
);

// set breakpoint
Expand Down Expand Up @@ -1116,7 +1139,7 @@
};
let circuits = vec![circuit_one, circuit_two];
let debug_artifact = DebugArtifact { debug_symbols: vec![], file_map: BTreeMap::new() };
let brillig_funcs = &vec![brillig_one, brillig_two];

Check warning on line 1142 in tooling/debugger/src/context.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (funcs)

let context = DebugContext::new(
&StubbedBlackBoxSolver,
Expand Down
95 changes: 76 additions & 19 deletions tooling/debugger/src/dap.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use std::collections::BTreeMap;
use std::io::{Read, Write};

use acvm::acir::circuit::brillig::BrilligBytecode;
use acvm::acir::circuit::Circuit;
use acvm::acir::native_types::WitnessMap;
use acvm::{BlackBoxFunctionSolver, FieldElement};
use nargo::errors::try_to_diagnose_runtime_error;
use nargo::ops::check_expected_failure_message;
use noirc_abi::Abi;
use noirc_frontend::hir::def_map::TestFunction;

use crate::context::DebugContext;
use crate::context::{DebugCommandResult, DebugLocation};
Expand All @@ -21,8 +23,8 @@ use dap::responses::{
};
use dap::server::Server;
use dap::types::{
Breakpoint, DisassembledInstruction, Scope, Source, StackFrame, SteppingGranularity,
StoppedEventReason, Thread, Variable,
Breakpoint, DisassembledInstruction, OutputEventCategory, Scope, Source, StackFrame,
SteppingGranularity, StoppedEventReason, Thread, Variable,
};
use noirc_artifacts::debug::DebugArtifact;

Expand All @@ -39,6 +41,8 @@ pub struct DapSession<'a, R: Read, W: Write, B: BlackBoxFunctionSolver<FieldElem
next_breakpoint_id: BreakpointId,
instruction_breakpoints: Vec<(DebugLocation, BreakpointId)>,
source_breakpoints: BTreeMap<FileId, Vec<(DebugLocation, BreakpointId)>>,
test_function: Option<TestFunction>,
abi: &'a Abi,
}

enum ScopeReferences {
Expand All @@ -61,18 +65,18 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver<FieldElement>> DapSession<
pub fn new(
server: Server<R, W>,
solver: &'a B,
circuits: &'a [Circuit<FieldElement>],
program: &'a CompiledProgram,
debug_artifact: &'a DebugArtifact,
initial_witness: WitnessMap<FieldElement>,
unconstrained_functions: &'a [BrilligBytecode<FieldElement>],
test_function: Option<TestFunction>,
) -> Self {
let context = DebugContext::new(
solver,
circuits,
&program.program.functions,
debug_artifact,
initial_witness,
Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact)),
unconstrained_functions,
&program.program.unconstrained_functions,
);
Self {
server,
Expand All @@ -82,6 +86,8 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver<FieldElement>> DapSession<
next_breakpoint_id: 1,
instruction_breakpoints: vec![],
source_breakpoints: BTreeMap::new(),
test_function,
abi: &program.abi,
}
}

Expand Down Expand Up @@ -341,6 +347,14 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver<FieldElement>> DapSession<
match result {
DebugCommandResult::Done => {
self.running = false;
if let Some(test_function) = self.test_function.as_ref() {
let test_result = if test_function.should_fail() {
"x Test failed: Test passed when it should have failed"
} else {
"✓ Test passed"
};
self.send_debug_output_message(String::from(test_result))?;
}
}
DebugCommandResult::Ok => {
self.server.send_event(Event::Stopped(StoppedEventBody {
Expand Down Expand Up @@ -368,18 +382,65 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver<FieldElement>> DapSession<
DebugCommandResult::Error(err) => {
self.server.send_event(Event::Stopped(StoppedEventBody {
reason: StoppedEventReason::Exception,
description: Some(format!("{err:?}")),
description: Some(String::from("Paused on exception")), // This is shown in callstack section of the debug panel
thread_id: Some(0),
preserve_focus_hint: Some(false),
text: None,
text: Some(format!("{err}\n{err:?}")), // This is shown in the exception panel
all_threads_stopped: Some(false),
hit_breakpoint_ids: None,
}))?;
if let Some(test_function) = self.test_function.as_ref() {
let test_should_have_passed = !test_function.should_fail();
if test_should_have_passed {
// Do not finish the execution of the debugger
// Since the test shouldn't have failed, so that user can
// - see the error on the exception panel
// - restart the debug session
let message = format!("x Text failed: {}", err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let message = format!("x Text failed: {}", err);
let message = format!("x Test failed: {err}");

Nit and typo

self.send_debug_output_message(message)?;
} else {
// Finish the execution of the debugger since the test reached
// the expected state (failed)
self.running = false;
let diagnostic = try_to_diagnose_runtime_error(
&err,
self.abi,
&self.debug_artifact.debug_symbols,
);
let message = match check_expected_failure_message(
test_function,
err.user_defined_failure_message(&self.abi.error_types),
diagnostic,
) {
nargo::ops::TestStatus::Pass => String::from("✓ Test passed"),
nargo::ops::TestStatus::Fail { message, .. } => {
format!("x Test failed: {}", message)
}
nargo::ops::TestStatus::CompileError(..) => {
String::from("x Test failed: Something went wrong")
anaPerezGhiglia marked this conversation as resolved.
Show resolved Hide resolved
} // TODO: this shouldn't happen. Should we panic?
};
self.send_debug_output_message(message)?;
};
}
}
}
Ok(())
}

fn send_debug_output_message(&mut self, message: String) -> Result<(), ServerError> {
self.server.send_event(Event::Output(dap::events::OutputEventBody {
category: Some(OutputEventCategory::Console),
output: message,
group: None,
variables_reference: None,
source: None,
line: None,
column: None,
data: None,
}))
}

fn get_next_breakpoint_id(&mut self) -> BreakpointId {
let id = self.next_breakpoint_id;
self.next_breakpoint_id += 1;
Expand Down Expand Up @@ -607,16 +668,12 @@ pub fn run_session<R: Read, W: Write, B: BlackBoxFunctionSolver<FieldElement>>(
solver: &B,
program: CompiledProgram,
initial_witness: WitnessMap<FieldElement>,
test_function: Option<TestFunction>,
) -> Result<(), ServerError> {
let debug_artifact = DebugArtifact { debug_symbols: program.debug, file_map: program.file_map };
let mut session = DapSession::new(
server,
solver,
&program.program.functions,
&debug_artifact,
initial_witness,
&program.program.unconstrained_functions,
);
let debug_artifact =
DebugArtifact { debug_symbols: program.debug.clone(), file_map: program.file_map.clone() };
let mut session =
DapSession::new(server, solver, &program, &debug_artifact, initial_witness, test_function);

session.run_loop()
}
4 changes: 3 additions & 1 deletion tooling/debugger/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use acvm::{BlackBoxFunctionSolver, FieldElement};

use nargo::NargoError;
use noirc_driver::CompiledProgram;
use noirc_frontend::hir::def_map::TestFunction;

pub fn run_repl_session<B: BlackBoxFunctionSolver<FieldElement>>(
solver: &B,
Expand All @@ -28,6 +29,7 @@ pub fn run_dap_loop<R: Read, W: Write, B: BlackBoxFunctionSolver<FieldElement>>(
solver: &B,
program: CompiledProgram,
initial_witness: WitnessMap<FieldElement>,
test_function: Option<TestFunction>,
) -> Result<(), ServerError> {
dap::run_session(server, solver, program, initial_witness)
dap::run_session(server, solver, program, initial_witness, test_function)
}
13 changes: 12 additions & 1 deletion tooling/debugger/src/repl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,13 @@ impl<'a, B: BlackBoxFunctionSolver<FieldElement>> ReplDebugger<'a, B> {
self.context.is_solved()
}

fn last_error(self) -> Option<NargoError<FieldElement>> {
match self.last_result {
DebugCommandResult::Error(error) => Some(error),
_ => None,
}
}

fn finalize(self) -> WitnessStack<FieldElement> {
self.context.finalize()
}
Expand Down Expand Up @@ -615,6 +622,10 @@ pub fn run<B: BlackBoxFunctionSolver<FieldElement>>(
let solved_witness_stack = context.into_inner().finalize();
Ok(Some(solved_witness_stack))
} else {
Ok(None)
match context.into_inner().last_error() {
// Expose the last known error
Some(error) => Err(error),
None => Ok(None),
}
}
}
20 changes: 19 additions & 1 deletion tooling/lsp/src/requests/code_lens_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ use crate::{
const ARROW: &str = "▶\u{fe0e}";
const TEST_COMMAND: &str = "nargo.test";
const TEST_CODELENS_TITLE: &str = "Run Test";
const DEBUG_TEST_COMMAND: &str = "nargo.debug.test";
const DEBUG_TEST_CODELENS_TITLE: &str = "Debug test";
const COMPILE_COMMAND: &str = "nargo.compile";
const COMPILE_CODELENS_TITLE: &str = "Compile";
const INFO_COMMAND: &str = "nargo.info";
Expand Down Expand Up @@ -120,7 +122,7 @@ pub(crate) fn collect_lenses_for_package(
arguments: Some(
[
package_selection_args(workspace, package),
vec!["--exact".into(), func_name.into()],
vec!["--exact".into(), serde_json::Value::from(String::from(&func_name))],
Comment on lines -123 to +125
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed?

]
.concat(),
),
Expand All @@ -129,6 +131,22 @@ pub(crate) fn collect_lenses_for_package(
let test_lens = CodeLens { range, command: Some(test_command), data: None };

lenses.push(test_lens);

let debug_test_command = Command {
title: DEBUG_TEST_CODELENS_TITLE.to_string(),
command: DEBUG_TEST_COMMAND.into(),
arguments: Some(
[
package_selection_args(workspace, package),
vec!["--exact".into(), serde_json::Value::from(String::from(&func_name))],
]
.concat(),
),
};

let debug_test_lens = CodeLens { range, command: Some(debug_test_command), data: None };

lenses.push(debug_test_lens);
}

if package.is_binary() {
Expand Down
26 changes: 26 additions & 0 deletions tooling/nargo/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,3 +218,29 @@ pub fn try_to_diagnose_runtime_error(
.with_call_stack(source_locations),
)
}

pub fn map_execution_error<F: AcirField>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fan of the name... maybe build_execution_error or similar?

error: OpcodeResolutionError<F>,
call_stack: &[ResolvedOpcodeLocation],
) -> ExecutionError<F> {
let call_stack = match &error {
OpcodeResolutionError::UnsatisfiedConstrain { .. }
| OpcodeResolutionError::IndexOutOfBounds { .. }
| OpcodeResolutionError::BrilligFunctionFailed { .. } => Some(call_stack.to_vec()),
_ => None,
};

let assertion_payload: Option<ResolvedAssertionPayload<F>> = match &error {
OpcodeResolutionError::BrilligFunctionFailed { payload, .. }
| OpcodeResolutionError::UnsatisfiedConstrain { payload, .. } => payload.clone(),
_ => None,
};

match assertion_payload {
Some(payload) => ExecutionError::AssertionFailed(
payload,
call_stack.expect("Should have call stack for an assertion failure"),
),
None => ExecutionError::SolvingError(error, call_stack),
}
Comment on lines +226 to +245
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be simplified to

Suggested change
let call_stack = match &error {
OpcodeResolutionError::UnsatisfiedConstrain { .. }
| OpcodeResolutionError::IndexOutOfBounds { .. }
| OpcodeResolutionError::BrilligFunctionFailed { .. } => Some(call_stack.to_vec()),
_ => None,
};
let assertion_payload: Option<ResolvedAssertionPayload<F>> = match &error {
OpcodeResolutionError::BrilligFunctionFailed { payload, .. }
| OpcodeResolutionError::UnsatisfiedConstrain { payload, .. } => payload.clone(),
_ => None,
};
match assertion_payload {
Some(payload) => ExecutionError::AssertionFailed(
payload,
call_stack.expect("Should have call stack for an assertion failure"),
),
None => ExecutionError::SolvingError(error, call_stack),
}
match &error {
OpcodeResolutionError::BrilligFunctionFailed { payload: Some(payload), .. }
| OpcodeResolutionError::UnsatisfiedConstrain { payload: Some(payload), .. } => {
ExecutionError::AssertionFailed(payload.clone(), call_stack.to_vec())
}
_ => ExecutionError::SolvingError(error, Some(call_stack.to_vec())),
}

}
Loading
Loading