From 0541568b4c209927a70778b895e8f1e50d9b6543 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Thu, 11 Jul 2024 20:28:34 -0400 Subject: [PATCH 1/4] feat: Handle ACIR calls in the debugger (#5051) # Description ## Problem\* Resolves #4824 With the introduction of ACIR calls to allow non-inlined ACIR functions, the debugger did not have proper support for programs that had them, neither when compiling in ACIR mode, nor in the default mode which compiles the full program to Brillig. ## Summary\* This PR depends on #4941 and is built on top of it. The changes allow compiling a program with `#[fold]` (and other non-inline) annotated functions in Brillig mode (the default when debugging) by forcing the selection of the Brillig runtime when compiling them. For inline functions, during SSA generation the runtime is _not changed_ in order to allow some optimizations in the stdlib to still work. For example, [some constant assertions](https://github.com/noir-lang/noir/blob/9c6de4b25d318c6d211361dd62a112a9d2432c56/noir_stdlib/src/field.nr#L6) don't work unless the function is inlined. When debugging in ACIR mode (with the `--acir-mode` CLI or `generateAcir: true` DAP options), the debugger now properly handles programs with multiple circuits and ACIR calls by: 1. keeping a stack of ACVMs and a record of the circuit being executed by each one, and 2. extending the debug location to also indicate in which circuit is the current opcode being executed ## Additional Context ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Martin Verzilli Co-authored-by: Maxim Vezenov Co-authored-by: Ana Perez Ghiglia --- compiler/noirc_evaluator/src/ssa.rs | 19 +- .../src/ssa/ssa_gen/context.rs | 11 +- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 2 +- tooling/debugger/Cargo.toml | 2 +- tooling/debugger/ignored-tests.txt | 8 - tooling/debugger/src/context.rs | 680 ++++++++++++------ tooling/debugger/src/dap.rs | 48 +- tooling/debugger/src/lib.rs | 17 +- tooling/debugger/src/repl.rs | 138 ++-- tooling/debugger/tests/debug.rs | 2 +- tooling/nargo_cli/src/cli/debug_cmd.rs | 52 +- 11 files changed, 633 insertions(+), 346 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 820374df9c1..81327cec013 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -190,12 +190,19 @@ pub fn create_program( let recursive = program.recursive; let ArtifactsAndWarnings((generated_acirs, generated_brillig, error_types), ssa_level_warnings) = optimize_into_acir(program, options)?; - assert_eq!( - generated_acirs.len(), - func_sigs.len(), - "The generated ACIRs should match the supplied function signatures" - ); - + if options.force_brillig_output { + assert_eq!( + generated_acirs.len(), + 1, + "Only the main ACIR is expected when forcing Brillig output" + ); + } else { + assert_eq!( + generated_acirs.len(), + func_sigs.len(), + "The generated ACIRs should match the supplied function signatures" + ); + } let mut program_artifact = SsaProgramArtifact::new(generated_brillig, error_types); // Add warnings collected at the Ssa stage diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index e013546f14a..8e55debec1d 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -5,7 +5,7 @@ use acvm::{acir::AcirField, FieldElement}; use iter_extended::vecmap; use noirc_errors::Location; use noirc_frontend::ast::{BinaryOpKind, Signedness}; -use noirc_frontend::monomorphization::ast::{self, LocalId, Parameters}; +use noirc_frontend::monomorphization::ast::{self, InlineType, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; use crate::errors::RuntimeError; @@ -121,9 +121,14 @@ impl<'a> FunctionContext<'a> { /// /// Note that the previous function cannot be resumed after calling this. Developers should /// avoid calling new_function until the previous function is completely finished with ssa-gen. - pub(super) fn new_function(&mut self, id: IrFunctionId, func: &ast::Function) { + pub(super) fn new_function( + &mut self, + id: IrFunctionId, + func: &ast::Function, + force_brillig_runtime: bool, + ) { self.definitions.clear(); - if func.unconstrained { + if func.unconstrained || (force_brillig_runtime && func.inline_type != InlineType::Inline) { self.builder.new_brillig_function(func.name.clone(), id); } else { self.builder.new_function(func.name.clone(), id, func.inline_type); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index afe44881830..abd251b008f 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -111,7 +111,7 @@ pub(crate) fn generate_ssa( // to generate SSA for each function used within the program. while let Some((src_function_id, dest_id)) = context.pop_next_function_in_queue() { let function = &context.program[src_function_id]; - function_context.new_function(dest_id, function); + function_context.new_function(dest_id, function, force_brillig_runtime); function_context.codegen_function_body(&function.body)?; } diff --git a/tooling/debugger/Cargo.toml b/tooling/debugger/Cargo.toml index 05b28f9d95a..540d6d11bc0 100644 --- a/tooling/debugger/Cargo.toml +++ b/tooling/debugger/Cargo.toml @@ -14,7 +14,7 @@ build-data.workspace = true acvm.workspace = true fm.workspace = true nargo.workspace = true -noirc_frontend.workspace = true +noirc_frontend = { workspace = true, features = ["bn254"] } noirc_printable_type.workspace = true noirc_errors.workspace = true noirc_driver.workspace = true diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index 2a6648b094b..745971d9b28 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -1,13 +1,5 @@ brillig_references debug_logs -fold_after_inlined_calls -fold_basic -fold_basic_nested_call -fold_call_witness_condition -fold_complex_outputs -fold_distinct_return -fold_fibonacci -fold_numeric_generic_poseidon is_unconstrained macros references diff --git a/tooling/debugger/src/context.rs b/tooling/debugger/src/context.rs index cb36988bf0b..d18ec5f0786 100644 --- a/tooling/debugger/src/context.rs +++ b/tooling/debugger/src/context.rs @@ -1,10 +1,11 @@ use crate::foreign_calls::DebugForeignCallExecutor; use acvm::acir::circuit::brillig::BrilligBytecode; use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation}; -use acvm::acir::native_types::{Witness, WitnessMap}; +use acvm::acir::native_types::{Witness, WitnessMap, WitnessStack}; use acvm::brillig_vm::MemoryValue; use acvm::pwg::{ - ACVMStatus, BrilligSolver, BrilligSolverStatus, ForeignCallWaitInfo, StepResult, ACVM, + ACVMStatus, AcirCallWaitInfo, BrilligSolver, BrilligSolverStatus, ForeignCallWaitInfo, + OpcodeNotSolvable, StepResult, ACVM, }; use acvm::{BlackBoxFunctionSolver, FieldElement}; @@ -15,56 +16,235 @@ use nargo::NargoError; use noirc_artifacts::debug::{DebugArtifact, StackFrame}; use noirc_driver::DebugFile; +use thiserror::Error; + use std::collections::BTreeMap; use std::collections::{hash_set::Iter, HashSet}; +/// A Noir program is composed by +/// `n` ACIR circuits +/// |_ `m` ACIR opcodes +/// |_ Acir call +/// |_ Acir Brillig function invocation +/// |_ `p` Brillig opcodes +/// +/// The purpose of this structure is to map the opcode locations in ACIR circuits into +/// a flat contiguous address space to be able to expose them to the DAP interface. +/// In this address space, the ACIR circuits are laid out one after the other, and +/// Brillig functions called from such circuits are expanded inline, replacing +/// the `BrilligCall` ACIR opcode. +/// +/// `addresses: Vec>` +/// * The outer vec is `n` sized - one element per ACIR circuit +/// * Each nested vec is `m` sized - one element per ACIR opcode in circuit +/// * Each element is the "virtual address" of such opcode +/// +/// For flattening we map each ACIR circuit and ACIR opcode with a sequential address number +/// We start by assigning 0 to the very first ACIR opcode and then start accumulating by +/// traversing by depth-first +/// +/// Even if the address space is continuous, the `addresses` tree only +/// keeps track of the ACIR opcodes, since the Brillig opcode addresses can be +/// calculated from the initial opcode address. +/// As a result the flattened indexed addresses list may have "holes". +/// +/// If between two consequent `addresses` nodes there is a "hole" (an address jump), +/// this means that the first one is actually a ACIR Brillig call +/// which has as many brillig opcodes as `second_address - first_address` +/// +#[derive(Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] +pub struct AddressMap { + addresses: Vec>, + + // virtual address of the last opcode of the program + last_valid_address: usize, +} + +impl AddressMap { + pub(super) fn new( + circuits: &[Circuit], + unconstrained_functions: &[BrilligBytecode], + ) -> Self { + let opcode_address_size = |opcode: &Opcode| { + if let Opcode::BrilligCall { id, .. } = opcode { + unconstrained_functions[*id as usize].bytecode.len() + } else { + 1 + } + }; + + let mut addresses = Vec::with_capacity(circuits.len()); + let mut next_address = 0usize; + + for circuit in circuits { + let mut circuit_addresses = Vec::with_capacity(circuit.opcodes.len()); + for opcode in &circuit.opcodes { + circuit_addresses.push(next_address); + next_address += opcode_address_size(opcode); + } + addresses.push(circuit_addresses); + } + + Self { addresses, last_valid_address: next_address - 1 } + } + + /// Returns the absolute address of the opcode at the given location. + /// Absolute here means accounting for nested Brillig opcodes in BrilligCall + /// opcodes. + pub fn debug_location_to_address(&self, location: &DebugLocation) -> usize { + let circuit_addresses = &self.addresses[location.circuit_id as usize]; + match &location.opcode_location { + OpcodeLocation::Acir(acir_index) => circuit_addresses[*acir_index], + OpcodeLocation::Brillig { acir_index, brillig_index } => { + circuit_addresses[*acir_index] + *brillig_index + } + } + } + + pub fn address_to_debug_location(&self, address: usize) -> Option { + if address > self.last_valid_address { + return None; + } + // We binary search if the given address is the first opcode address of each circuit id + // if is not, this means that the address itself is "contained" in the previous + // circuit indicated by `Err(insert_index)` + let circuit_id = + match self.addresses.binary_search_by(|addresses| addresses[0].cmp(&address)) { + Ok(found_index) => found_index, + // This means that the address is not in `insert_index` circuit + // because is an `Err`, so it must be included in previous circuit vec of opcodes + Err(insert_index) => insert_index - 1, + }; + + // We binary search among the selected `circuit_id`` list of opcodes + // If Err(insert_index) this means that the given address + // is a Brillig addresses that's contained in previous index ACIR opcode index + let opcode_location = match self.addresses[circuit_id].binary_search(&address) { + Ok(found_index) => OpcodeLocation::Acir(found_index), + Err(insert_index) => { + let acir_index = insert_index - 1; + let base_offset = self.addresses[circuit_id][acir_index]; + let brillig_index = address - base_offset; + OpcodeLocation::Brillig { acir_index, brillig_index } + } + }; + Some(DebugLocation { circuit_id: circuit_id as u32, opcode_location }) + } +} + +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] +pub struct DebugLocation { + pub circuit_id: u32, + pub opcode_location: OpcodeLocation, +} + +impl std::fmt::Display for DebugLocation { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let circuit_id = self.circuit_id; + match self.opcode_location { + OpcodeLocation::Acir(index) => write!(f, "{circuit_id}:{index}"), + OpcodeLocation::Brillig { acir_index, brillig_index } => { + write!(f, "{circuit_id}:{acir_index}.{brillig_index}") + } + } + } +} + +#[derive(Error, Debug)] +pub enum DebugLocationFromStrError { + #[error("Invalid debug location string: {0}")] + InvalidDebugLocationString(String), +} + +impl std::str::FromStr for DebugLocation { + type Err = DebugLocationFromStrError; + fn from_str(s: &str) -> Result { + let parts: Vec<_> = s.split(':').collect(); + let error = Err(DebugLocationFromStrError::InvalidDebugLocationString(s.to_string())); + + match parts.len() { + 1 => OpcodeLocation::from_str(parts[0]).map_or(error, |opcode_location| { + Ok(DebugLocation { circuit_id: 0, opcode_location }) + }), + 2 => { + let first_part = parts[0].parse().ok(); + let second_part = OpcodeLocation::from_str(parts[1]).ok(); + if let (Some(circuit_id), Some(opcode_location)) = (first_part, second_part) { + Ok(DebugLocation { circuit_id, opcode_location }) + } else { + error + } + } + _ => error, + } + } +} + #[derive(Debug)] pub(super) enum DebugCommandResult { Done, Ok, - BreakpointReached(OpcodeLocation), + BreakpointReached(DebugLocation), Error(NargoError), } +pub struct ExecutionFrame<'a, B: BlackBoxFunctionSolver> { + circuit_id: u32, + acvm: ACVM<'a, FieldElement, B>, +} + pub(super) struct DebugContext<'a, B: BlackBoxFunctionSolver> { acvm: ACVM<'a, FieldElement, B>, + current_circuit_id: u32, brillig_solver: Option>, + + witness_stack: WitnessStack, + acvm_stack: Vec>, + + backend: &'a B, foreign_call_executor: Box, + debug_artifact: &'a DebugArtifact, - breakpoints: HashSet, - source_to_opcodes: BTreeMap>, + breakpoints: HashSet, + source_to_locations: BTreeMap>, + + circuits: &'a [Circuit], unconstrained_functions: &'a [BrilligBytecode], - // Absolute (in terms of all the opcodes ACIR+Brillig) addresses of the ACIR - // opcodes with one additional entry for to indicate the last valid address. - acir_opcode_addresses: Vec, + acir_opcode_addresses: AddressMap, } impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { pub(super) fn new( blackbox_solver: &'a B, - circuit: &'a Circuit, + circuits: &'a [Circuit], debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, foreign_call_executor: Box, unconstrained_functions: &'a [BrilligBytecode], ) -> Self { let source_to_opcodes = build_source_to_opcode_debug_mappings(debug_artifact); - let acir_opcode_addresses = build_acir_opcode_offsets(circuit, unconstrained_functions); + let current_circuit_id: u32 = 0; + let initial_circuit = &circuits[current_circuit_id as usize]; + let acir_opcode_addresses = AddressMap::new(circuits, unconstrained_functions); Self { - // TODO: need to handle brillig pointer in the debugger acvm: ACVM::new( blackbox_solver, - &circuit.opcodes, + &initial_circuit.opcodes, initial_witness, unconstrained_functions, - &circuit.assert_messages, + &initial_circuit.assert_messages, ), + current_circuit_id, brillig_solver: None, + witness_stack: WitnessStack::default(), + acvm_stack: vec![], + backend: blackbox_solver, foreign_call_executor, debug_artifact, breakpoints: HashSet::new(), - source_to_opcodes, + source_to_locations: source_to_opcodes, + circuits, unconstrained_functions, acir_opcode_addresses, } @@ -74,6 +254,10 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.acvm.opcodes() } + pub(super) fn get_opcodes_of_circuit(&self, circuit_id: u32) -> &[Opcode] { + &self.circuits[circuit_id as usize].opcodes + } + pub(super) fn get_witness_map(&self) -> &WitnessMap { self.acvm.witness_map() } @@ -86,36 +270,49 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.acvm.overwrite_witness(witness, value) } - pub(super) fn get_current_opcode_location(&self) -> Option { + pub(super) fn get_current_debug_location(&self) -> Option { let ip = self.acvm.instruction_pointer(); if ip >= self.get_opcodes().len() { None - } else if let Some(ref solver) = self.brillig_solver { - Some(OpcodeLocation::Brillig { - acir_index: ip, - brillig_index: solver.program_counter(), - }) } else { - Some(OpcodeLocation::Acir(ip)) + let opcode_location = if let Some(ref solver) = self.brillig_solver { + OpcodeLocation::Brillig { acir_index: ip, brillig_index: solver.program_counter() } + } else { + OpcodeLocation::Acir(ip) + }; + Some(DebugLocation { circuit_id: self.current_circuit_id, opcode_location }) } } - pub(super) fn get_call_stack(&self) -> Vec { + pub(super) fn get_call_stack(&self) -> Vec { + // Build the frames from parent ACIR calls + let mut frames: Vec<_> = self + .acvm_stack + .iter() + .map(|ExecutionFrame { circuit_id, acvm }| DebugLocation { + circuit_id: *circuit_id, + opcode_location: OpcodeLocation::Acir(acvm.instruction_pointer()), + }) + .collect(); + + // Now add the frame(s) for the currently executing ACVM let instruction_pointer = self.acvm.instruction_pointer(); - if instruction_pointer >= self.get_opcodes().len() { - vec![] - } else if let Some(ref solver) = self.brillig_solver { - solver - .get_call_stack() - .iter() - .map(|program_counter| OpcodeLocation::Brillig { + let circuit_id = self.current_circuit_id; + if let Some(ref solver) = self.brillig_solver { + frames.extend(solver.get_call_stack().iter().map(|program_counter| DebugLocation { + circuit_id, + opcode_location: OpcodeLocation::Brillig { acir_index: instruction_pointer, brillig_index: *program_counter, - }) - .collect() - } else { - vec![OpcodeLocation::Acir(instruction_pointer)] + }, + })); + } else if instruction_pointer < self.get_opcodes().len() { + frames.push(DebugLocation { + circuit_id, + opcode_location: OpcodeLocation::Acir(instruction_pointer), + }); } + frames } pub(super) fn is_source_location_in_debug_module(&self, location: &Location) -> bool { @@ -142,10 +339,10 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { &self, file_id: &FileId, line: i64, - ) -> Option { + ) -> Option { let line = line as usize; - let line_to_opcodes = self.source_to_opcodes.get(file_id)?; - let found_index = match line_to_opcodes.binary_search_by(|x| x.0.cmp(&line)) { + let line_to_opcodes = self.source_to_locations.get(file_id)?; + let found_location = match line_to_opcodes.binary_search_by(|x| x.0.cmp(&line)) { Ok(index) => { // move backwards to find the first opcode which matches the line let mut index = index; @@ -161,7 +358,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { line_to_opcodes[index].1 } }; - Some(found_index) + Some(found_location) } /// Returns the callstack in source code locations for the currently @@ -172,9 +369,9 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { /// This function also filters source locations that are determined to be in /// the internal debug module. pub(super) fn get_current_source_location(&self) -> Option> { - self.get_current_opcode_location() + self.get_current_debug_location() .as_ref() - .map(|opcode_location| self.get_source_location_for_opcode_location(opcode_location)) + .map(|debug_location| self.get_source_location_for_debug_location(debug_location)) .filter(|v: &Vec| !v.is_empty()) } @@ -184,15 +381,12 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { /// the given opcode location cannot be mapped back to a source location /// (eg. it may be pure debug instrumentation code or other synthetically /// produced opcode by the compiler) - pub(super) fn get_source_location_for_opcode_location( + pub(super) fn get_source_location_for_debug_location( &self, - opcode_location: &OpcodeLocation, + debug_location: &DebugLocation, ) -> Vec { - // TODO: this assumes we're debugging a program (ie. the DebugArtifact - // will contain a single DebugInfo), but this assumption doesn't hold - // for contracts - self.debug_artifact.debug_symbols[0] - .opcode_location(opcode_location) + self.debug_artifact.debug_symbols[debug_location.circuit_id as usize] + .opcode_location(&debug_location.opcode_location) .map(|source_locations| { source_locations .into_iter() @@ -208,48 +402,30 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { /// general, the matching between opcode location and source location is 1 /// to 1, but due to the compiler inlining functions a single opcode /// location may expand to multiple source locations. - pub(super) fn get_source_call_stack(&self) -> Vec<(OpcodeLocation, Location)> { + pub(super) fn get_source_call_stack(&self) -> Vec<(DebugLocation, Location)> { self.get_call_stack() .iter() - .flat_map(|opcode_location| { - self.get_source_location_for_opcode_location(opcode_location) + .flat_map(|debug_location| { + self.get_source_location_for_debug_location(debug_location) .into_iter() - .map(|source_location| (*opcode_location, source_location)) + .map(|source_location| (*debug_location, source_location)) }) .collect() } /// Returns the absolute address of the opcode at the given location. - /// Absolute here means accounting for nested Brillig opcodes in BrilligCall - /// opcodes. - pub fn opcode_location_to_address(&self, location: &OpcodeLocation) -> usize { - match location { - OpcodeLocation::Acir(acir_index) => self.acir_opcode_addresses[*acir_index], - OpcodeLocation::Brillig { acir_index, brillig_index } => { - self.acir_opcode_addresses[*acir_index] + *brillig_index - } - } + pub fn debug_location_to_address(&self, location: &DebugLocation) -> usize { + self.acir_opcode_addresses.debug_location_to_address(location) } - pub fn address_to_opcode_location(&self, address: usize) -> Option { - if address >= *self.acir_opcode_addresses.last().unwrap_or(&0) { - return None; - } - let location = match self.acir_opcode_addresses.binary_search(&address) { - Ok(found_index) => OpcodeLocation::Acir(found_index), - Err(insert_index) => { - let acir_index = insert_index - 1; - let base_offset = self.acir_opcode_addresses[acir_index]; - let brillig_index = address - base_offset; - OpcodeLocation::Brillig { acir_index, brillig_index } - } - }; - Some(location) + // Returns the DebugLocation associated to the given address + pub fn address_to_debug_location(&self, address: usize) -> Option { + self.acir_opcode_addresses.address_to_debug_location(address) } - pub(super) fn render_opcode_at_location(&self, location: &OpcodeLocation) -> String { - let opcodes = self.get_opcodes(); - match location { + pub(super) fn render_opcode_at_location(&self, location: &DebugLocation) -> String { + let opcodes = self.get_opcodes_of_circuit(location.circuit_id); + match &location.opcode_location { OpcodeLocation::Acir(acir_index) => { let opcode = &opcodes[*acir_index]; match opcode { @@ -280,7 +456,7 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.brillig_solver = Some(solver); if self.breakpoint_reached() { DebugCommandResult::BreakpointReached( - self.get_current_opcode_location() + self.get_current_debug_location() .expect("Breakpoint reached but we have no location"), ) } else { @@ -296,7 +472,6 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { self.handle_foreign_call(foreign_call) } Err(err) => DebugCommandResult::Error(NargoError::ExecutionError( - // TODO: debugger does not handle multiple acir calls ExecutionError::SolvingError(err, None), )), } @@ -315,24 +490,82 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } else { self.acvm.resolve_pending_foreign_call(foreign_call_result); } - // TODO: should we retry executing the opcode somehow in this case? + // TODO: should we retry executing the opcode somehow in this + // case? Otherwise, executing a foreign call takes two debugging + // steps. DebugCommandResult::Ok } Err(error) => DebugCommandResult::Error(error.into()), } } - fn handle_acvm_status(&mut self, status: ACVMStatus) -> DebugCommandResult { - if let ACVMStatus::RequiresForeignCall(foreign_call) = status { - return self.handle_foreign_call(foreign_call); + fn handle_acir_call( + &mut self, + call_info: AcirCallWaitInfo, + ) -> DebugCommandResult { + let callee_circuit = &self.circuits[call_info.id as usize]; + let callee_witness_map = call_info.initial_witness; + let callee_acvm = ACVM::new( + self.backend, + &callee_circuit.opcodes, + callee_witness_map, + self.unconstrained_functions, + &callee_circuit.assert_messages, + ); + let caller_acvm = std::mem::replace(&mut self.acvm, callee_acvm); + self.acvm_stack + .push(ExecutionFrame { circuit_id: self.current_circuit_id, acvm: caller_acvm }); + self.current_circuit_id = call_info.id; + + // Explicitly handling the new ACVM status here handles two edge cases: + // 1. there is a breakpoint set at the beginning of a circuit + // 2. the called circuit has no opcodes + self.handle_acvm_status(self.acvm.get_status().clone()) + } + + fn handle_acir_call_finished(&mut self) -> DebugCommandResult { + let caller_frame = self.acvm_stack.pop().expect("Execution stack should not be empty"); + let caller_acvm = caller_frame.acvm; + let callee_acvm = std::mem::replace(&mut self.acvm, caller_acvm); + self.current_circuit_id = caller_frame.circuit_id; + let call_solved_witness = callee_acvm.finalize(); + + let ACVMStatus::RequiresAcirCall(call_info) = self.acvm.get_status() else { + unreachable!("Resolving an ACIR call, the caller is in an invalid state"); + }; + let acir_to_call = &self.circuits[call_info.id as usize]; + + let mut call_resolved_outputs = Vec::new(); + for return_witness_index in acir_to_call.return_values.indices() { + if let Some(return_value) = call_solved_witness.get_index(return_witness_index) { + call_resolved_outputs.push(*return_value); + } else { + return DebugCommandResult::Error( + ExecutionError::SolvingError( + OpcodeNotSolvable::MissingAssignment(return_witness_index).into(), + None, // Missing assignment errors do not supply user-facing diagnostics so we do not need to attach a call stack + ) + .into(), + ); + } } + self.acvm.resolve_pending_acir_call(call_resolved_outputs); + + DebugCommandResult::Ok + } + fn handle_acvm_status(&mut self, status: ACVMStatus) -> DebugCommandResult { match status { - ACVMStatus::Solved => DebugCommandResult::Done, + ACVMStatus::Solved => { + if self.acvm_stack.is_empty() { + return DebugCommandResult::Done; + } + self.handle_acir_call_finished() + } ACVMStatus::InProgress => { if self.breakpoint_reached() { DebugCommandResult::BreakpointReached( - self.get_current_opcode_location() + self.get_current_debug_location() .expect("Breakpoint reached but we have no location"), ) } else { @@ -340,15 +573,10 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } } ACVMStatus::Failure(error) => DebugCommandResult::Error(NargoError::ExecutionError( - // TODO: debugger does not handle multiple acir calls ExecutionError::SolvingError(error, None), )), - ACVMStatus::RequiresForeignCall(_) => { - unreachable!("Unexpected pending foreign call resolution"); - } - ACVMStatus::RequiresAcirCall(_) => { - todo!("Multiple ACIR calls are not supported"); - } + ACVMStatus::RequiresForeignCall(foreign_call) => self.handle_foreign_call(foreign_call), + ACVMStatus::RequiresAcirCall(call_info) => self.handle_acir_call(call_info), } } @@ -367,9 +595,11 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } fn get_current_acir_index(&self) -> Option { - self.get_current_opcode_location().map(|opcode_location| match opcode_location { - OpcodeLocation::Acir(acir_index) => acir_index, - OpcodeLocation::Brillig { acir_index, .. } => acir_index, + self.get_current_debug_location().map(|debug_location| { + match debug_location.opcode_location { + OpcodeLocation::Acir(acir_index) => acir_index, + OpcodeLocation::Brillig { acir_index, .. } => acir_index, + } }) } @@ -394,10 +624,16 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { return true; } - match self.get_current_opcode_location() { - Some(OpcodeLocation::Brillig { .. }) => true, - Some(OpcodeLocation::Acir(acir_index)) => { - matches!(self.get_opcodes()[acir_index], Opcode::BrilligCall { .. }) + match self.get_current_debug_location() { + Some(DebugLocation { opcode_location: OpcodeLocation::Brillig { .. }, .. }) => true, + Some(DebugLocation { + circuit_id, + opcode_location: OpcodeLocation::Acir(acir_index), + }) => { + matches!( + self.get_opcodes_of_circuit(circuit_id)[acir_index], + Opcode::BrilligCall { .. } + ) } _ => false, } @@ -491,16 +727,19 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } fn breakpoint_reached(&self) -> bool { - if let Some(location) = self.get_current_opcode_location() { + if let Some(location) = self.get_current_debug_location() { self.breakpoints.contains(&location) } else { false } } - pub(super) fn is_valid_opcode_location(&self, location: &OpcodeLocation) -> bool { - let opcodes = self.get_opcodes(); - match *location { + pub(super) fn is_valid_debug_location(&self, location: &DebugLocation) -> bool { + if location.circuit_id as usize >= self.circuits.len() { + return false; + } + let opcodes = self.get_opcodes_of_circuit(location.circuit_id); + match location.opcode_location { OpcodeLocation::Acir(acir_index) => acir_index < opcodes.len(), OpcodeLocation::Brillig { acir_index, brillig_index } => { if acir_index < opcodes.len() { @@ -518,19 +757,19 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { } } - pub(super) fn is_breakpoint_set(&self, location: &OpcodeLocation) -> bool { + pub(super) fn is_breakpoint_set(&self, location: &DebugLocation) -> bool { self.breakpoints.contains(location) } - pub(super) fn add_breakpoint(&mut self, location: OpcodeLocation) -> bool { + pub(super) fn add_breakpoint(&mut self, location: DebugLocation) -> bool { self.breakpoints.insert(location) } - pub(super) fn delete_breakpoint(&mut self, location: &OpcodeLocation) -> bool { + pub(super) fn delete_breakpoint(&mut self, location: &DebugLocation) -> bool { self.breakpoints.remove(location) } - pub(super) fn iterate_breakpoints(&self) -> Iter<'_, OpcodeLocation> { + pub(super) fn iterate_breakpoints(&self) -> Iter<'_, DebugLocation> { self.breakpoints.iter() } @@ -542,8 +781,10 @@ impl<'a, B: BlackBoxFunctionSolver> DebugContext<'a, B> { matches!(self.acvm.get_status(), ACVMStatus::Solved) } - pub fn finalize(self) -> WitnessMap { - self.acvm.finalize() + pub fn finalize(mut self) -> WitnessStack { + let last_witness_map = self.acvm.finalize(); + self.witness_stack.push(0, last_witness_map); + self.witness_stack } } @@ -555,11 +796,10 @@ fn is_debug_file_in_debug_crate(debug_file: &DebugFile) -> bool { /// numbers and opcode locations corresponding to those line numbers fn build_source_to_opcode_debug_mappings( debug_artifact: &DebugArtifact, -) -> BTreeMap> { +) -> BTreeMap> { if debug_artifact.debug_symbols.is_empty() { return BTreeMap::new(); } - let locations = &debug_artifact.debug_symbols[0].locations; let simple_files: BTreeMap<_, _> = debug_artifact .file_map .iter() @@ -572,50 +812,34 @@ fn build_source_to_opcode_debug_mappings( }) .collect(); - let mut result: BTreeMap> = BTreeMap::new(); - locations.iter().for_each(|(opcode_location, source_locations)| { - source_locations.iter().for_each(|source_location| { - let span = source_location.span; - let file_id = source_location.file; - let Some(file) = simple_files.get(&file_id) else { - return; - }; - let Ok(line_index) = file.line_index((), span.start() as usize) else { - return; - }; - let line_number = line_index + 1; - - result.entry(file_id).or_default().push((line_number, *opcode_location)); - }); - }); + let mut result: BTreeMap> = BTreeMap::new(); + + for (circuit_id, debug_symbols) in debug_artifact.debug_symbols.iter().enumerate() { + for (opcode_location, source_locations) in &debug_symbols.locations { + source_locations.iter().for_each(|source_location| { + let span = source_location.span; + let file_id = source_location.file; + let Some(file) = simple_files.get(&file_id) else { + return; + }; + let Ok(line_index) = file.line_index((), span.start() as usize) else { + return; + }; + let line_number = line_index + 1; + + let debug_location = DebugLocation { + circuit_id: circuit_id as u32, + opcode_location: *opcode_location, + }; + result.entry(file_id).or_default().push((line_number, debug_location)); + }); + } + } result.iter_mut().for_each(|(_, file_locations)| file_locations.sort_by_key(|x| (x.0, x.1))); result } -fn build_acir_opcode_offsets( - circuit: &Circuit, - unconstrained_functions: &[BrilligBytecode], -) -> Vec { - let mut result = Vec::with_capacity(circuit.opcodes.len() + 1); - // address of the first opcode is always 0 - result.push(0); - circuit.opcodes.iter().fold(0, |acc, opcode| { - let acc = acc - + match opcode { - Opcode::BrilligCall { id, .. } => { - unconstrained_functions[*id as usize].bytecode.len() - } - _ => 1, - }; - // push the starting address of the next opcode - result.push(acc); - acc - }); - result -} - -// TODO: update all debugger tests to use unconstrained brillig pointers #[cfg(test)] mod tests { use super::*; @@ -625,7 +849,7 @@ mod tests { acir::{ circuit::{ brillig::{BrilligInputs, BrilligOutputs}, - opcodes::BlockId, + opcodes::{BlockId, BlockType}, }, native_types::Expression, AcirField, @@ -675,7 +899,8 @@ mod tests { }]; let brillig_funcs = &vec![brillig_bytecode]; let current_witness_index = 2; - let circuit = &Circuit { current_witness_index, opcodes, ..Circuit::default() }; + let circuit = Circuit { current_witness_index, opcodes, ..Circuit::default() }; + let circuits = &vec![circuit]; let debug_symbols = vec![]; let file_map = BTreeMap::new(); @@ -687,51 +912,66 @@ mod tests { Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact)); let mut context = DebugContext::new( &StubbedBlackBoxSolver, - circuit, + circuits, debug_artifact, initial_witness, foreign_call_executor, brillig_funcs, ); - assert_eq!(context.get_current_opcode_location(), Some(OpcodeLocation::Acir(0))); + assert_eq!( + context.get_current_debug_location(), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(0) }) + ); // Execute the first Brillig opcode (calldata copy) let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( - context.get_current_opcode_location(), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }) + context.get_current_debug_location(), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 } + }) ); // execute the second Brillig opcode (const) let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( - context.get_current_opcode_location(), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }) + context.get_current_debug_location(), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 } + }) ); // try to execute the third Brillig opcode (and resolve the foreign call) let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( - context.get_current_opcode_location(), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }) + context.get_current_debug_location(), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 } + }) ); // retry the third Brillig opcode (foreign call should be finished) let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( - context.get_current_opcode_location(), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }) + context.get_current_debug_location(), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 } + }) ); // last Brillig opcode let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Done)); - assert_eq!(context.get_current_opcode_location(), None); + assert_eq!(context.get_current_debug_location(), None); } #[test] @@ -784,7 +1024,8 @@ mod tests { }), ]; let current_witness_index = 3; - let circuit = &Circuit { current_witness_index, opcodes, ..Circuit::default() }; + let circuit = Circuit { current_witness_index, opcodes, ..Circuit::default() }; + let circuits = &vec![circuit]; let debug_symbols = vec![]; let file_map = BTreeMap::new(); @@ -797,7 +1038,7 @@ mod tests { let brillig_funcs = &vec![brillig_bytecode]; let mut context = DebugContext::new( &StubbedBlackBoxSolver, - circuit, + circuits, debug_artifact, initial_witness, foreign_call_executor, @@ -805,28 +1046,40 @@ mod tests { ); // set breakpoint - let breakpoint_location = OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }; + let breakpoint_location = DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }, + }; assert!(context.add_breakpoint(breakpoint_location)); // execute the first ACIR opcode (Brillig block) -> should reach the breakpoint instead let result = context.step_acir_opcode(); assert!(matches!(result, DebugCommandResult::BreakpointReached(_))); - assert_eq!(context.get_current_opcode_location(), Some(breakpoint_location)); + assert_eq!(context.get_current_debug_location(), Some(breakpoint_location)); // continue execution to the next ACIR opcode let result = context.step_acir_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); - assert_eq!(context.get_current_opcode_location(), Some(OpcodeLocation::Acir(1))); + assert_eq!( + context.get_current_debug_location(), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(1) }) + ); // last ACIR opcode let result = context.step_acir_opcode(); assert!(matches!(result, DebugCommandResult::Done)); - assert_eq!(context.get_current_opcode_location(), None); + assert_eq!(context.get_current_debug_location(), None); } #[test] - fn test_address_opcode_location_mapping() { - let brillig_bytecode = BrilligBytecode { + fn test_address_debug_location_mapping() { + let brillig_one = BrilligBytecode { + bytecode: vec![ + BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 }, + BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 }, + ], + }; + let brillig_two = BrilligBytecode { bytecode: vec![ BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 }, BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 0 }, @@ -834,22 +1087,33 @@ mod tests { ], }; - let opcodes = vec![ - Opcode::BrilligCall { id: 0, inputs: vec![], outputs: vec![], predicate: None }, - Opcode::MemoryInit { - block_id: BlockId(0), - init: vec![], - block_type: acvm::acir::circuit::opcodes::BlockType::Memory, - }, - Opcode::BrilligCall { id: 0, inputs: vec![], outputs: vec![], predicate: None }, - Opcode::AssertZero(Expression::default()), - ]; - let circuit = Circuit { opcodes, ..Circuit::default() }; + let circuit_one = Circuit { + opcodes: vec![ + Opcode::MemoryInit { + block_id: BlockId(0), + init: vec![], + block_type: BlockType::Memory, + }, + Opcode::BrilligCall { id: 0, inputs: vec![], outputs: vec![], predicate: None }, + Opcode::Call { id: 1, inputs: vec![], outputs: vec![], predicate: None }, + Opcode::AssertZero(Expression::default()), + ], + ..Circuit::default() + }; + let circuit_two = Circuit { + opcodes: vec![ + Opcode::BrilligCall { id: 1, inputs: vec![], outputs: vec![], predicate: None }, + Opcode::AssertZero(Expression::default()), + ], + ..Circuit::default() + }; + let circuits = vec![circuit_one, circuit_two]; let debug_artifact = DebugArtifact { debug_symbols: vec![], file_map: BTreeMap::new() }; - let brillig_funcs = &vec![brillig_bytecode]; + let brillig_funcs = &vec![brillig_one, brillig_two]; + let context = DebugContext::new( &StubbedBlackBoxSolver, - &circuit, + &circuits, &debug_artifact, WitnessMap::new(), Box::new(DefaultDebugForeignCallExecutor::new(true)), @@ -857,46 +1121,56 @@ mod tests { ); let locations = - (0..=7).map(|address| context.address_to_opcode_location(address)).collect::>(); + (0..=8).map(|address| context.address_to_debug_location(address)).collect::>(); // mapping from addresses to opcode locations assert_eq!( locations, vec![ - Some(OpcodeLocation::Acir(0)), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 }), - Some(OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }), - Some(OpcodeLocation::Acir(1)), - Some(OpcodeLocation::Acir(2)), - Some(OpcodeLocation::Brillig { acir_index: 2, brillig_index: 1 }), - Some(OpcodeLocation::Brillig { acir_index: 2, brillig_index: 2 }), - Some(OpcodeLocation::Acir(3)), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(0) }), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(1) }), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 1 } + }), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(2) }), + Some(DebugLocation { circuit_id: 0, opcode_location: OpcodeLocation::Acir(3) }), + Some(DebugLocation { circuit_id: 1, opcode_location: OpcodeLocation::Acir(0) }), + Some(DebugLocation { + circuit_id: 1, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 1 } + }), + Some(DebugLocation { + circuit_id: 1, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 } + }), + Some(DebugLocation { circuit_id: 1, opcode_location: OpcodeLocation::Acir(1) }), ] ); let addresses = locations .iter() .flatten() - .map(|location| context.opcode_location_to_address(location)) + .map(|location| context.debug_location_to_address(location)) .collect::>(); // and vice-versa - assert_eq!(addresses, (0..=7).collect::>()); + assert_eq!(addresses, (0..=8).collect::>()); // check edge cases - assert_eq!(None, context.address_to_opcode_location(8)); + assert_eq!(None, context.address_to_debug_location(9)); assert_eq!( - 0, - context.opcode_location_to_address(&OpcodeLocation::Brillig { - acir_index: 0, - brillig_index: 0 + 1, + context.debug_location_to_address(&DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 1, brillig_index: 0 } }) ); assert_eq!( - 4, - context.opcode_location_to_address(&OpcodeLocation::Brillig { - acir_index: 2, - brillig_index: 0 + 5, + context.debug_location_to_address(&DebugLocation { + circuit_id: 1, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 0 } }) ); } diff --git a/tooling/debugger/src/dap.rs b/tooling/debugger/src/dap.rs index 77abf3093cd..cfe33a61cb5 100644 --- a/tooling/debugger/src/dap.rs +++ b/tooling/debugger/src/dap.rs @@ -2,12 +2,12 @@ use std::collections::BTreeMap; use std::io::{Read, Write}; use acvm::acir::circuit::brillig::BrilligBytecode; -use acvm::acir::circuit::{Circuit, OpcodeLocation}; +use acvm::acir::circuit::Circuit; use acvm::acir::native_types::WitnessMap; use acvm::{BlackBoxFunctionSolver, FieldElement}; -use crate::context::DebugCommandResult; use crate::context::DebugContext; +use crate::context::{DebugCommandResult, DebugLocation}; use crate::foreign_calls::DefaultDebugForeignCallExecutor; use dap::errors::ServerError; @@ -37,8 +37,8 @@ pub struct DapSession<'a, R: Read, W: Write, B: BlackBoxFunctionSolver, - source_breakpoints: BTreeMap>, + instruction_breakpoints: Vec<(DebugLocation, BreakpointId)>, + source_breakpoints: BTreeMap>, } enum ScopeReferences { @@ -61,14 +61,14 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< pub fn new( server: Server, solver: &'a B, - circuit: &'a Circuit, + circuits: &'a [Circuit], debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, unconstrained_functions: &'a [BrilligBytecode], ) -> Self { let context = DebugContext::new( solver, - circuit, + circuits, debug_artifact, initial_witness, Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact)), @@ -100,7 +100,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< } pub fn run_loop(&mut self) -> Result<(), ServerError> { - self.running = self.context.get_current_opcode_location().is_some(); + self.running = self.context.get_current_debug_location().is_some(); if self.running && self.context.get_current_source_location().is_none() { // TODO: remove this? This is to ensure that the tool has a proper @@ -194,7 +194,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< .get_source_call_stack() .iter() .enumerate() - .map(|(index, (opcode_location, source_location))| { + .map(|(index, (debug_location, source_location))| { let line_number = self.debug_artifact.location_line_number(*source_location).unwrap(); let column_number = @@ -204,7 +204,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< Some(frame) => format!("{} {}", frame.function_name, index), None => format!("frame #{index}"), }; - let address = self.context.opcode_location_to_address(opcode_location); + let address = self.context.debug_location_to_address(debug_location); StackFrame { id: index as i64, @@ -251,18 +251,18 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< let mut instructions: Vec = vec![]; while count > 0 { - let opcode_location = if address >= 0 { - self.context.address_to_opcode_location(address as usize) + let debug_location = if address >= 0 { + self.context.address_to_debug_location(address as usize) } else { None }; - if let Some(opcode_location) = opcode_location { + if let Some(debug_location) = debug_location { instructions.push(DisassembledInstruction { address: address.to_string(), // we'll use the instruction_bytes field to render the OpcodeLocation - instruction_bytes: Some(opcode_location.to_string()), - instruction: self.context.render_opcode_at_location(&opcode_location), + instruction_bytes: Some(debug_location.to_string()), + instruction: self.context.render_opcode_at_location(&debug_location), ..DisassembledInstruction::default() }); } else { @@ -320,16 +320,16 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< self.handle_execution_result(result) } - fn find_breakpoints_at_location(&self, opcode_location: &OpcodeLocation) -> Vec { + fn find_breakpoints_at_location(&self, debug_location: &DebugLocation) -> Vec { let mut result = vec![]; for (location, id) in &self.instruction_breakpoints { - if opcode_location == location { + if debug_location == location { result.push(*id); } } for breakpoints in self.source_breakpoints.values() { for (location, id) in breakpoints { - if opcode_location == location { + if debug_location == location { result.push(*id); } } @@ -404,7 +404,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< }; // compute breakpoints to set and return - let mut breakpoints_to_set: Vec<(OpcodeLocation, i64)> = vec![]; + let mut breakpoints_to_set: Vec<(DebugLocation, i64)> = vec![]; let breakpoints: Vec = args .breakpoints .iter() @@ -420,8 +420,8 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< }; let Some(location) = self .context - .address_to_opcode_location(address) - .filter(|location| self.context.is_valid_opcode_location(location)) + .address_to_debug_location(address) + .filter(|location| self.context.is_valid_debug_location(location)) else { return Breakpoint { verified: false, @@ -472,7 +472,7 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< let Some(ref breakpoints) = &args.breakpoints else { return vec![]; }; - let mut breakpoints_to_set: Vec<(OpcodeLocation, i64)> = vec![]; + let mut breakpoints_to_set: Vec<(DebugLocation, i64)> = vec![]; let breakpoints = breakpoints .iter() .map(|breakpoint| { @@ -490,14 +490,14 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver> DapSession< // TODO: line will not necessarily be the one requested; we // should do the reverse mapping and retrieve the actual source // code line number - if !self.context.is_valid_opcode_location(&location) { + if !self.context.is_valid_debug_location(&location) { return Breakpoint { verified: false, message: Some(String::from("Invalid opcode location")), ..Breakpoint::default() }; } - let breakpoint_address = self.context.opcode_location_to_address(&location); + let breakpoint_address = self.context.debug_location_to_address(&location); let instruction_reference = format!("{}", breakpoint_address); let breakpoint_id = self.get_next_breakpoint_id(); breakpoints_to_set.push((location, breakpoint_id)); @@ -612,7 +612,7 @@ pub fn run_session>( let mut session = DapSession::new( server, solver, - &program.program.functions[0], + &program.program.functions, &debug_artifact, initial_witness, &program.program.unconstrained_functions, diff --git a/tooling/debugger/src/lib.rs b/tooling/debugger/src/lib.rs index 9d0059ee495..37ac088ca35 100644 --- a/tooling/debugger/src/lib.rs +++ b/tooling/debugger/src/lib.rs @@ -9,23 +9,18 @@ use std::io::{Read, Write}; use ::dap::errors::ServerError; use ::dap::server::Server; -use acvm::acir::circuit::brillig::BrilligBytecode; -use acvm::{acir::circuit::Circuit, acir::native_types::WitnessMap}; +use acvm::acir::native_types::{WitnessMap, WitnessStack}; use acvm::{BlackBoxFunctionSolver, FieldElement}; -use noirc_artifacts::debug::DebugArtifact; - use nargo::NargoError; use noirc_driver::CompiledProgram; -pub fn debug_circuit>( - blackbox_solver: &B, - circuit: &Circuit, - debug_artifact: DebugArtifact, +pub fn run_repl_session>( + solver: &B, + program: CompiledProgram, initial_witness: WitnessMap, - unconstrained_functions: &[BrilligBytecode], -) -> Result>, NargoError> { - repl::run(blackbox_solver, circuit, &debug_artifact, initial_witness, unconstrained_functions) +) -> Result>, NargoError> { + repl::run(solver, program, initial_witness) } pub fn run_dap_loop>( diff --git a/tooling/debugger/src/repl.rs b/tooling/debugger/src/repl.rs index 7d8c6e0947d..d3462985642 100644 --- a/tooling/debugger/src/repl.rs +++ b/tooling/debugger/src/repl.rs @@ -1,11 +1,12 @@ -use crate::context::{DebugCommandResult, DebugContext}; +use crate::context::{DebugCommandResult, DebugContext, DebugLocation}; use acvm::acir::circuit::brillig::BrilligBytecode; use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation}; -use acvm::acir::native_types::{Witness, WitnessMap}; +use acvm::acir::native_types::{Witness, WitnessMap, WitnessStack}; use acvm::brillig_vm::brillig::Opcode as BrilligOpcode; use acvm::{BlackBoxFunctionSolver, FieldElement}; use nargo::NargoError; +use noirc_driver::CompiledProgram; use crate::foreign_calls::DefaultDebugForeignCallExecutor; use noirc_artifacts::debug::DebugArtifact; @@ -19,17 +20,21 @@ use crate::source_code_printer::print_source_code_location; pub struct ReplDebugger<'a, B: BlackBoxFunctionSolver> { context: DebugContext<'a, B>, blackbox_solver: &'a B, - circuit: &'a Circuit, debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, last_result: DebugCommandResult, + + // ACIR functions to debug + circuits: &'a [Circuit], + + // Brillig functions referenced from the ACIR circuits above unconstrained_functions: &'a [BrilligBytecode], } impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { pub fn new( blackbox_solver: &'a B, - circuit: &'a Circuit, + circuits: &'a [Circuit], debug_artifact: &'a DebugArtifact, initial_witness: WitnessMap, unconstrained_functions: &'a [BrilligBytecode], @@ -38,13 +43,13 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact)); let context = DebugContext::new( blackbox_solver, - circuit, + circuits, debug_artifact, initial_witness.clone(), foreign_call_executor, unconstrained_functions, ); - let last_result = if context.get_current_opcode_location().is_none() { + let last_result = if context.get_current_debug_location().is_none() { // handle circuit with no opcodes DebugCommandResult::Done } else { @@ -53,7 +58,7 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { Self { context, blackbox_solver, - circuit, + circuits, debug_artifact, initial_witness, last_result, @@ -62,42 +67,43 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { } pub fn show_current_vm_status(&self) { - let location = self.context.get_current_opcode_location(); - let opcodes = self.context.get_opcodes(); + let location = self.context.get_current_debug_location(); match location { None => println!("Finished execution"), Some(location) => { - match location { + let circuit_id = location.circuit_id; + let opcodes = self.context.get_opcodes_of_circuit(circuit_id); + match &location.opcode_location { OpcodeLocation::Acir(ip) => { - println!("At opcode {}: {}", ip, opcodes[ip]); + println!("At opcode {} :: {}", location, opcodes[*ip]); } OpcodeLocation::Brillig { acir_index, brillig_index } => { let brillig_bytecode = - if let Opcode::BrilligCall { id, .. } = opcodes[acir_index] { + if let Opcode::BrilligCall { id, .. } = opcodes[*acir_index] { &self.unconstrained_functions[id as usize].bytecode } else { unreachable!("Brillig location does not contain Brillig opcodes"); }; println!( - "At opcode {}.{}: {:?}", - acir_index, brillig_index, brillig_bytecode[brillig_index] + "At opcode {} :: {:?}", + location, brillig_bytecode[*brillig_index] ); } } - let locations = self.context.get_source_location_for_opcode_location(&location); + let locations = self.context.get_source_location_for_debug_location(&location); print_source_code_location(self.debug_artifact, &locations); } } } - fn show_stack_frame(&self, index: usize, location: &OpcodeLocation) { + fn show_stack_frame(&self, index: usize, debug_location: &DebugLocation) { let opcodes = self.context.get_opcodes(); - match location { + match &debug_location.opcode_location { OpcodeLocation::Acir(instruction_pointer) => { println!( - "Frame #{index}, opcode {}: {}", - instruction_pointer, opcodes[*instruction_pointer] + "Frame #{index}, opcode {} :: {}", + debug_location, opcodes[*instruction_pointer] ) } OpcodeLocation::Brillig { acir_index, brillig_index } => { @@ -108,12 +114,12 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { unreachable!("Brillig location does not contain Brillig opcodes"); }; println!( - "Frame #{index}, opcode {}.{}: {:?}", - acir_index, brillig_index, brillig_bytecode[*brillig_index] + "Frame #{index}, opcode {} :: {:?}", + debug_location, brillig_bytecode[*brillig_index] ); } } - let locations = self.context.get_source_location_for_opcode_location(location); + let locations = self.context.get_source_location_for_debug_location(debug_location); print_source_code_location(self.debug_artifact, &locations); } @@ -130,8 +136,21 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { } fn display_opcodes(&self) { - let opcodes = self.context.get_opcodes(); - let current_opcode_location = self.context.get_current_opcode_location(); + for i in 0..self.circuits.len() { + self.display_opcodes_of_circuit(i as u32); + } + } + + fn display_opcodes_of_circuit(&self, circuit_id: u32) { + let current_opcode_location = + self.context.get_current_debug_location().and_then(|debug_location| { + if debug_location.circuit_id == circuit_id { + Some(debug_location.opcode_location) + } else { + None + } + }); + let opcodes = self.context.get_opcodes_of_circuit(circuit_id); let current_acir_index = match current_opcode_location { Some(OpcodeLocation::Acir(ip)) => Some(ip), Some(OpcodeLocation::Brillig { acir_index, .. }) => Some(acir_index), @@ -144,7 +163,10 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { let outer_marker = |acir_index| { if current_acir_index == Some(acir_index) { "->" - } else if self.context.is_breakpoint_set(&OpcodeLocation::Acir(acir_index)) { + } else if self.context.is_breakpoint_set(&DebugLocation { + circuit_id, + opcode_location: OpcodeLocation::Acir(acir_index), + }) { " *" } else { "" @@ -153,10 +175,10 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { let brillig_marker = |acir_index, brillig_index| { if current_acir_index == Some(acir_index) && brillig_index == current_brillig_index { "->" - } else if self - .context - .is_breakpoint_set(&OpcodeLocation::Brillig { acir_index, brillig_index }) - { + } else if self.context.is_breakpoint_set(&DebugLocation { + circuit_id, + opcode_location: OpcodeLocation::Brillig { acir_index, brillig_index }, + }) { " *" } else { "" @@ -165,7 +187,8 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { let print_brillig_bytecode = |acir_index, bytecode: &[BrilligOpcode]| { for (brillig_index, brillig_opcode) in bytecode.iter().enumerate() { println!( - "{:>3}.{:<2} |{:2} {:?}", + "{:>2}:{:>3}.{:<2} |{:2} {:?}", + circuit_id, acir_index, brillig_index, brillig_marker(acir_index, brillig_index), @@ -178,33 +201,33 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { match &opcode { Opcode::BrilligCall { id, inputs, outputs, .. } => { println!( - "{:>3} {:2} BRILLIG CALL id={} inputs={:?}", - acir_index, marker, id, inputs + "{:>2}:{:>3} {:2} BRILLIG CALL id={} inputs={:?}", + circuit_id, acir_index, marker, id, inputs ); - println!(" | outputs={:?}", outputs); + println!(" | outputs={:?}", outputs); let bytecode = &self.unconstrained_functions[*id as usize].bytecode; print_brillig_bytecode(acir_index, bytecode); } - _ => println!("{:>3} {:2} {:?}", acir_index, marker, opcode), + _ => println!("{:>2}:{:>3} {:2} {:?}", circuit_id, acir_index, marker, opcode), } } } - fn add_breakpoint_at(&mut self, location: OpcodeLocation) { - if !self.context.is_valid_opcode_location(&location) { - println!("Invalid opcode location {location}"); + fn add_breakpoint_at(&mut self, location: DebugLocation) { + if !self.context.is_valid_debug_location(&location) { + println!("Invalid location {location}"); } else if self.context.add_breakpoint(location) { - println!("Added breakpoint at opcode {location}"); + println!("Added breakpoint at {location}"); } else { - println!("Breakpoint at opcode {location} already set"); + println!("Breakpoint at {location} already set"); } } - fn delete_breakpoint_at(&mut self, location: OpcodeLocation) { + fn delete_breakpoint_at(&mut self, location: DebugLocation) { if self.context.delete_breakpoint(&location) { - println!("Breakpoint at opcode {location} deleted"); + println!("Breakpoint at {location} deleted"); } else { - println!("Breakpoint at opcode {location} not set"); + println!("Breakpoint at {location} not set"); } } @@ -281,20 +304,19 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { } fn restart_session(&mut self) { - let breakpoints: Vec = - self.context.iterate_breakpoints().copied().collect(); + let breakpoints: Vec = self.context.iterate_breakpoints().copied().collect(); let foreign_call_executor = Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, self.debug_artifact)); self.context = DebugContext::new( self.blackbox_solver, - self.circuit, + self.circuits, self.debug_artifact, self.initial_witness.clone(), foreign_call_executor, self.unconstrained_functions, ); - for opcode_location in breakpoints { - self.context.add_breakpoint(opcode_location); + for debug_location in breakpoints { + self.context.add_breakpoint(debug_location); } self.last_result = DebugCommandResult::Ok; println!("Restarted debugging session."); @@ -372,21 +394,23 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { self.context.is_solved() } - fn finalize(self) -> WitnessMap { + fn finalize(self) -> WitnessStack { self.context.finalize() } } pub fn run>( blackbox_solver: &B, - circuit: &Circuit, - debug_artifact: &DebugArtifact, + program: CompiledProgram, initial_witness: WitnessMap, - unconstrained_functions: &[BrilligBytecode], -) -> Result>, NargoError> { +) -> Result>, NargoError> { + let circuits = &program.program.functions; + let debug_artifact = + &DebugArtifact { debug_symbols: program.debug, file_map: program.file_map }; + let unconstrained_functions = &program.program.unconstrained_functions; let context = RefCell::new(ReplDebugger::new( blackbox_solver, - circuit, + circuits, debug_artifact, initial_witness, unconstrained_functions, @@ -480,7 +504,7 @@ pub fn run>( "break", command! { "add a breakpoint at an opcode location", - (LOCATION:OpcodeLocation) => |location| { + (LOCATION:DebugLocation) => |location| { ref_context.borrow_mut().add_breakpoint_at(location); Ok(CommandStatus::Done) } @@ -490,7 +514,7 @@ pub fn run>( "delete", command! { "delete breakpoint at an opcode location", - (LOCATION:OpcodeLocation) => |location| { + (LOCATION:DebugLocation) => |location| { ref_context.borrow_mut().delete_breakpoint_at(location); Ok(CommandStatus::Done) } @@ -576,8 +600,8 @@ pub fn run>( drop(repl); if context.borrow().is_solved() { - let solved_witness = context.into_inner().finalize(); - Ok(Some(solved_witness)) + let solved_witness_stack = context.into_inner().finalize(); + Ok(Some(solved_witness_stack)) } else { Ok(None) } diff --git a/tooling/debugger/tests/debug.rs b/tooling/debugger/tests/debug.rs index 313b6b30591..2dca6b95f0e 100644 --- a/tooling/debugger/tests/debug.rs +++ b/tooling/debugger/tests/debug.rs @@ -12,7 +12,7 @@ mod tests { let nargo_bin = cargo_bin("nargo").into_os_string().into_string().expect("Cannot parse nargo path"); - let timeout_seconds = 20; + let timeout_seconds = 25; let mut dbg_session = spawn_bash(Some(timeout_seconds * 1000)).expect("Could not start bash session"); diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index 778009bf791..2014a3acaa4 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -1,6 +1,6 @@ use std::path::PathBuf; -use acvm::acir::native_types::{WitnessMap, WitnessStack}; +use acvm::acir::native_types::WitnessStack; use acvm::FieldElement; use bn254_blackbox_solver::Bn254BlackBoxSolver; use clap::Args; @@ -15,7 +15,6 @@ use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_abi::input_parser::{Format, InputValue}; use noirc_abi::InputMap; -use noirc_artifacts::debug::DebugArtifact; use noirc_driver::{ file_manager_with_stdlib, CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING, }; @@ -169,10 +168,10 @@ fn run_async( runtime.block_on(async { println!("[{}] Starting debugger", package.name); - let (return_value, solved_witness) = + let (return_value, witness_stack) = debug_program_and_decode(program, package, prover_name)?; - if let Some(solved_witness) = solved_witness { + if let Some(solved_witness_stack) = witness_stack { println!("[{}] Circuit witness successfully solved", package.name); if let Some(return_value) = return_value { @@ -180,11 +179,8 @@ fn run_async( } if let Some(witness_name) = witness_name { - let witness_path = save_witness_to_dir( - WitnessStack::from(solved_witness), - witness_name, - target_dir, - )?; + let witness_path = + save_witness_to_dir(solved_witness_stack, witness_name, target_dir)?; println!("[{}] Witness saved to {}", package.name, witness_path.display()); } @@ -200,38 +196,32 @@ fn debug_program_and_decode( program: CompiledProgram, package: &Package, prover_name: &str, -) -> Result<(Option, Option>), CliError> { +) -> Result<(Option, Option>), CliError> { // Parse the initial witness values from Prover.toml let (inputs_map, _) = read_inputs_from_file(&package.root_dir, prover_name, Format::Toml, &program.abi)?; - let solved_witness = debug_program(&program, &inputs_map)?; - - match solved_witness { - Some(witness) => { - let (_, return_value) = program.abi.decode(&witness)?; - Ok((return_value, Some(witness))) + let program_abi = program.abi.clone(); + let witness_stack = debug_program(program, &inputs_map)?; + + match witness_stack { + Some(witness_stack) => { + let main_witness = &witness_stack + .peek() + .expect("Should have at least one witness on the stack") + .witness; + let (_, return_value) = program_abi.decode(main_witness)?; + Ok((return_value, Some(witness_stack))) } None => Ok((None, None)), } } pub(crate) fn debug_program( - compiled_program: &CompiledProgram, + compiled_program: CompiledProgram, inputs_map: &InputMap, -) -> Result>, CliError> { +) -> Result>, CliError> { let initial_witness = compiled_program.abi.encode(inputs_map, None)?; - let debug_artifact = DebugArtifact { - debug_symbols: compiled_program.debug.clone(), - file_map: compiled_program.file_map.clone(), - }; - - noir_debugger::debug_circuit( - &Bn254BlackBoxSolver, - &compiled_program.program.functions[0], - debug_artifact, - initial_witness, - &compiled_program.program.unconstrained_functions, - ) - .map_err(CliError::from) + noir_debugger::run_repl_session(&Bn254BlackBoxSolver, compiled_program, initial_witness) + .map_err(CliError::from) } From f4592571a6403b5943adf553b5c04c26c611c142 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Fri, 12 Jul 2024 13:21:34 +0100 Subject: [PATCH 2/4] chore: update typo PR script (#5488) # Description ## Problem\* Resolves ## Summary\* This PR updates the typo PR script to squash the commits as multi-commit PRs weren't being processed properly. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- scripts/redo-typo-pr.sh | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/scripts/redo-typo-pr.sh b/scripts/redo-typo-pr.sh index 416be65a449..4b3b93b48ed 100755 --- a/scripts/redo-typo-pr.sh +++ b/scripts/redo-typo-pr.sh @@ -16,16 +16,21 @@ gh pr checkout $ORIGINAL_PR_NUMBER echo "Creating new local branch $NEW_BRANCH" git checkout -b $NEW_BRANCH -# Step 3: Push the new branch to GitHub +# Step 3: Squash commits +echo "Squashing new local branch $NEW_BRANCH" +git reset --soft master +git add . +git commit -m "chore: typo fixes" + +# Step 4: Push the new branch to GitHub echo "Pushing new branch $NEW_BRANCH to GitHub" -git commit --amend --no-edit git push origin $NEW_BRANCH -# Step 4: create a new pull request +# Step 5: create a new pull request echo "Creating a new pull request for $NEW_BRANCH" gh pr create --base master --head $NEW_BRANCH --title "chore: redo typo PR by $AUTHOR" --body "Thanks $AUTHOR for https://github.com/$REPO/pull/$ORIGINAL_PR_NUMBER. Our policy is to redo typo changes to dissuade metric farming. This is an automated script." -# Step 5: Close the original PR +# Step 6: Close the original PR echo "Closing original PR #$ORIGINAL_PR_NUMBER" gh pr close $ORIGINAL_PR_NUMBER From 010c835e4ebfdf49ea4e9326abafcdeb587153b6 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 12 Jul 2024 10:54:06 -0300 Subject: [PATCH 3/4] feat: LSP hover (#5491) # Description ## Problem Resolves #1575 ## Summary Implements LSP hover for: - modules - structs - struct members - traits - global variables - functions - aliases - local variables The popup content is similar to that of Rust Analyzer except that there's no link to go to that reference. The reason is that it seems the LSP type to do that has an "actions" property but I can't find that in the lsp-types library we are using. That said, once this PR is merged doing that should be relatively easy, and it might be better to keep PRs smaller. There's one case where the hover doesn't work: if you have `let x: i32 = Default::default();`, hovering over `default` won't show the impl function. It's doable, but doing it requires more changes in this PR and at one point I wanted to stop :-) This PR also includes several fixes that I found in "find-all-references" and "rename", but because hover and those are all kind of related, and testing things for hover is simpler, I decided to add tests for those things and fix them. Here it's in action: https://github.com/noir-lang/noir/assets/209371/bb64d368-2cf7-450c-99f3-e2aba52ca4e3 ## Additional Context None. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../src/elaborator/expressions.rs | 9 +- compiler/noirc_frontend/src/elaborator/mod.rs | 12 +- .../noirc_frontend/src/elaborator/scope.rs | 16 +- .../noirc_frontend/src/elaborator/types.rs | 18 +- .../src/hir/def_collector/dc_crate.rs | 27 +- .../src/hir/def_collector/dc_mod.rs | 29 +- .../src/hir/resolution/import.rs | 21 +- .../src/hir/resolution/path_resolver.rs | 9 +- .../src/hir/resolution/resolver.rs | 1 + .../noirc_frontend/src/hir/type_check/mod.rs | 3 +- .../noirc_frontend/src/hir_def/function.rs | 5 +- compiler/noirc_frontend/src/locations.rs | 49 +- compiler/noirc_frontend/src/node_interner.rs | 32 +- tooling/lsp/src/lib.rs | 5 +- tooling/lsp/src/requests/goto_declaration.rs | 6 +- tooling/lsp/src/requests/goto_definition.rs | 13 +- tooling/lsp/src/requests/hover.rs | 641 ++++++++++++++++++ tooling/lsp/src/requests/mod.rs | 35 +- tooling/lsp/src/requests/references.rs | 24 +- tooling/lsp/src/requests/rename.rs | 71 +- tooling/lsp/src/types.rs | 8 +- .../test_programs/rename_function/src/main.nr | 13 + .../test_programs/workspace/one/src/lib.nr | 26 +- .../test_programs/workspace/two/src/lib.nr | 48 ++ .../test_programs/workspace/two/src/other.nr | 2 + 25 files changed, 986 insertions(+), 137 deletions(-) create mode 100644 tooling/lsp/src/requests/hover.rs create mode 100644 tooling/lsp/test_programs/workspace/two/src/other.nr diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index e8098723692..72e2beea570 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -320,6 +320,7 @@ impl<'context> Elaborator<'context> { let (mut object, mut object_type) = self.elaborate_expression(method_call.object); object_type = object_type.follow_bindings(); + let method_name_span = method_call.method_name.span(); let method_name = method_call.method_name.0.contents.as_str(); match self.lookup_method(&object_type, method_name, span) { Some(method_ref) => { @@ -385,6 +386,9 @@ impl<'context> Elaborator<'context> { self.interner.push_expr_type(function_id, func_type.clone()); + self.interner + .add_function_reference(func_id, Location::new(method_name_span, self.file)); + // Type check the new call now that it has been changed from a method call // to a function call. This way we avoid duplicating code. let typ = self.type_check_call(&function_call, func_type, function_args, span); @@ -399,7 +403,8 @@ impl<'context> Elaborator<'context> { constructor: ConstructorExpression, ) -> (HirExpression, Type) { let span = constructor.type_name.span(); - let is_self_type = constructor.type_name.last_segment().is_self_type_name(); + let last_segment = constructor.type_name.last_segment(); + let is_self_type = last_segment.is_self_type_name(); let (r#type, struct_generics) = if let Some(struct_id) = constructor.struct_type { let typ = self.interner.get_struct(struct_id); @@ -430,7 +435,7 @@ impl<'context> Elaborator<'context> { }); let struct_id = struct_type.borrow().id; - let reference_location = Location::new(span, self.file); + let reference_location = Location::new(last_segment.span(), self.file); self.interner.add_struct_reference(struct_id, reference_location, is_self_type); (expr, Type::Struct(struct_type, generics)) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index f736ad76970..f0d3e3cae1f 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -720,6 +720,12 @@ impl<'context> Elaborator<'context> { let statements = std::mem::take(&mut func.def.body.statements); let body = BlockExpression { statements }; + let struct_id = if let Some(Type::Struct(struct_type, _)) = &self.self_type { + Some(struct_type.borrow().id) + } else { + None + }; + let meta = FuncMeta { name: name_ident, kind: func.kind, @@ -727,6 +733,7 @@ impl<'context> Elaborator<'context> { typ, direct_generics, all_generics: self.generics.clone(), + struct_id, trait_impl: self.current_trait_impl, parameters: parameters.into(), parameter_idents, @@ -1232,7 +1239,7 @@ impl<'context> Elaborator<'context> { for field_index in 0..fields_len { self.interner - .add_definition_location(ReferenceId::StructMember(type_id, field_index)); + .add_definition_location(ReferenceId::StructMember(type_id, field_index), None); } self.run_comptime_attributes_on_struct(attributes, type_id, span, &mut generated_items); @@ -1426,7 +1433,8 @@ impl<'context> Elaborator<'context> { self.elaborate_comptime_global(global_id); } - self.interner.add_definition_location(ReferenceId::Global(global_id)); + self.interner + .add_definition_location(ReferenceId::Global(global_id), Some(self.module_id())); self.local_module = old_module; self.file = old_file; diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index af6fc0e5d5e..e01c7e997d4 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -6,7 +6,6 @@ use crate::hir::resolution::path_resolver::{PathResolver, StandardPathResolver}; use crate::hir::resolution::resolver::SELF_TYPE_NAME; use crate::hir::scope::{Scope as GenericScope, ScopeTree as GenericScopeTree}; use crate::macros_api::Ident; -use crate::node_interner::ReferenceId; use crate::{ hir::{ def_map::{ModuleDefId, TryFromModuleDefId}, @@ -48,17 +47,30 @@ impl<'context> Elaborator<'context> { let path_resolution; if self.interner.track_references { - let mut references: Vec = Vec::new(); + let last_segment = path.last_segment(); + let location = Location::new(last_segment.span(), self.file); + let is_self_type_name = last_segment.is_self_type_name(); + + let mut references: Vec<_> = Vec::new(); path_resolution = resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut references))?; for (referenced, ident) in references.iter().zip(path.segments) { + let Some(referenced) = referenced else { + continue; + }; self.interner.add_reference( *referenced, Location::new(ident.span(), self.file), ident.is_self_type_name(), ); } + + self.interner.add_module_def_id_reference( + path_resolution.module_def_id, + location, + is_self_type_name, + ); } else { path_resolution = resolver.resolve(self.def_maps, path, &mut None)?; } diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 9134fc851b9..35ff421ed32 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -57,12 +57,16 @@ impl<'context> Elaborator<'context> { use crate::ast::UnresolvedTypeData::*; let span = typ.span; - let (is_self_type_name, is_synthetic) = if let Named(ref named_path, _, synthetic) = typ.typ - { - (named_path.last_segment().is_self_type_name(), synthetic) - } else { - (false, false) - }; + let (named_path_span, is_self_type_name, is_synthetic) = + if let Named(ref named_path, _, synthetic) = typ.typ { + ( + Some(named_path.last_segment().span()), + named_path.last_segment().is_self_type_name(), + synthetic, + ) + } else { + (None, false, false) + }; let resolved_type = match typ.typ { FieldElement => Type::FieldElement, @@ -153,7 +157,7 @@ impl<'context> Elaborator<'context> { }; if let Some(unresolved_span) = typ.span { - let location = Location::new(unresolved_span, self.file); + let location = Location::new(named_path_span.unwrap_or(unresolved_span), self.file); match resolved_type { Type::Struct(ref struct_type, _) => { diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 8b926c8107f..f42819dab1f 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -347,7 +347,7 @@ impl DefCollector { for collected_import in std::mem::take(&mut def_collector.imports) { let module_id = collected_import.module_id; let resolved_import = if context.def_interner.track_references { - let mut references: Vec = Vec::new(); + let mut references: Vec> = Vec::new(); let resolved_import = resolve_import( crate_id, &collected_import, @@ -359,6 +359,9 @@ impl DefCollector { let file_id = current_def_map.file_id(module_id); for (referenced, ident) in references.iter().zip(&collected_import.path.segments) { + let Some(referenced) = referenced else { + continue; + }; context.def_interner.add_reference( *referenced, Location::new(ident.span(), file_id), @@ -552,27 +555,7 @@ fn add_import_reference( } let location = Location::new(name.span(), file_id); - - match def_id { - crate::macros_api::ModuleDefId::ModuleId(module_id) => { - interner.add_module_reference(module_id, location); - } - crate::macros_api::ModuleDefId::FunctionId(func_id) => { - interner.add_function_reference(func_id, location); - } - crate::macros_api::ModuleDefId::TypeId(struct_id) => { - interner.add_struct_reference(struct_id, location, false); - } - crate::macros_api::ModuleDefId::TraitId(trait_id) => { - interner.add_trait_reference(trait_id, location, false); - } - crate::macros_api::ModuleDefId::TypeAliasId(type_alias_id) => { - interner.add_alias_reference(type_alias_id, location); - } - crate::macros_api::ModuleDefId::GlobalId(global_id) => { - interner.add_global_reference(global_id, location); - } - }; + interner.add_module_def_id_reference(def_id, location, false); } fn inject_prelude( diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index bcd24ca8ed3..1f30b4388b6 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -14,7 +14,7 @@ use crate::ast::{ TypeImpl, }; use crate::macros_api::NodeInterner; -use crate::node_interner::ReferenceId; +use crate::node_interner::{ModuleAttributes, ReferenceId}; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, @@ -90,7 +90,7 @@ pub fn collect_defs( errors.extend(collector.collect_structs(context, ast.types, crate_id)); - errors.extend(collector.collect_type_aliases(context, ast.type_aliases)); + errors.extend(collector.collect_type_aliases(context, ast.type_aliases, crate_id)); errors.extend(collector.collect_functions(context, ast.functions, crate_id)); @@ -318,7 +318,10 @@ impl<'a> ModCollector<'a> { // And store the TypeId -> StructType mapping somewhere it is reachable self.def_collector.items.types.insert(id, unresolved); - context.def_interner.add_definition_location(ReferenceId::Struct(id)); + context.def_interner.add_definition_location( + ReferenceId::Struct(id), + Some(ModuleId { krate, local_id: self.module_id }), + ); } definition_errors } @@ -329,6 +332,7 @@ impl<'a> ModCollector<'a> { &mut self, context: &mut Context, type_aliases: Vec, + krate: CrateId, ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; for type_alias in type_aliases { @@ -365,7 +369,10 @@ impl<'a> ModCollector<'a> { self.def_collector.items.type_aliases.insert(type_alias_id, unresolved); - context.def_interner.add_definition_location(ReferenceId::Alias(type_alias_id)); + context.def_interner.add_definition_location( + ReferenceId::Alias(type_alias_id), + Some(ModuleId { krate, local_id: self.module_id }), + ); } errors } @@ -532,7 +539,10 @@ impl<'a> ModCollector<'a> { }; context.def_interner.push_empty_trait(trait_id, &unresolved, resolved_generics); - context.def_interner.add_definition_location(ReferenceId::Trait(trait_id)); + context.def_interner.add_definition_location( + ReferenceId::Trait(trait_id), + Some(ModuleId { krate, local_id: self.module_id }), + ); self.def_collector.items.traits.insert(trait_id, unresolved); } @@ -720,7 +730,14 @@ impl<'a> ModCollector<'a> { return Err(err); } - context.def_interner.add_module_location(mod_id, mod_location); + context.def_interner.add_module_attributes( + mod_id, + ModuleAttributes { + name: mod_name.0.contents.clone(), + location: mod_location, + parent: self.module_id, + }, + ); } Ok(mod_id) diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 710c12a91bf..e0c87e2d9e4 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -81,7 +81,7 @@ pub fn resolve_import( crate_id: CrateId, import_directive: &ImportDirective, def_maps: &BTreeMap, - path_references: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>>, ) -> Result { let module_scope = import_directive.module_id; let NamespaceResolution { @@ -126,7 +126,7 @@ fn resolve_path_to_ns( crate_id: CrateId, importing_crate: CrateId, def_maps: &BTreeMap, - path_references: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>>, ) -> NamespaceResolutionResult { let import_path = &import_directive.path.segments; let def_map = &def_maps[&crate_id]; @@ -196,7 +196,7 @@ fn resolve_path_from_crate_root( import_path: &[Ident], def_maps: &BTreeMap, - path_references: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>>, ) -> NamespaceResolutionResult { resolve_name_in_module( crate_id, @@ -214,7 +214,7 @@ fn resolve_name_in_module( import_path: &[Ident], starting_mod: LocalModuleId, def_maps: &BTreeMap, - path_references: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>>, ) -> NamespaceResolutionResult { let def_map = &def_maps[&krate]; let mut current_mod_id = ModuleId { krate, local_id: starting_mod }; @@ -247,7 +247,7 @@ fn resolve_name_in_module( current_mod_id = match typ { ModuleDefId::ModuleId(id) => { if let Some(path_references) = path_references { - path_references.push(ReferenceId::Module(id)); + path_references.push(Some(ReferenceId::Module(id))); } id } @@ -255,14 +255,14 @@ fn resolve_name_in_module( // TODO: If impls are ever implemented, types can be used in a path ModuleDefId::TypeId(id) => { if let Some(path_references) = path_references { - path_references.push(ReferenceId::Struct(id)); + path_references.push(Some(ReferenceId::Struct(id))); } id.module_id() } ModuleDefId::TypeAliasId(_) => panic!("type aliases cannot be used in type namespace"), ModuleDefId::TraitId(id) => { if let Some(path_references) = path_references { - path_references.push(ReferenceId::Trait(id)); + path_references.push(Some(ReferenceId::Trait(id))); } id.0 } @@ -309,7 +309,7 @@ fn resolve_external_dep( current_def_map: &CrateDefMap, directive: &ImportDirective, def_maps: &BTreeMap, - path_references: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>>, importing_crate: CrateId, ) -> NamespaceResolutionResult { // Use extern_prelude to get the dep @@ -327,6 +327,11 @@ fn resolve_external_dep( // See `singleton_import.nr` test case for a check that such cases are handled elsewhere. let path_without_crate_name = &path[1..]; + // Given that we skipped the first segment, record that it doesn't refer to any module or type. + if let Some(path_references) = path_references { + path_references.push(None); + } + let path = Path { segments: path_without_crate_name.to_vec(), kind: PathKind::Plain, diff --git a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs index c3dc76b635f..7cd44a84018 100644 --- a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -9,12 +9,13 @@ use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; pub trait PathResolver { /// Resolve the given path returning the resolved ModuleDefId. /// If `path_references` is `Some`, a `ReferenceId` for each segment in `path` - /// will be resolved and pushed. + /// will be resolved and pushed (some entries will be None if they don't refer to + /// a module or type). fn resolve( &self, def_maps: &BTreeMap, path: Path, - path_references: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>>, ) -> PathResolutionResult; fn local_module_id(&self) -> LocalModuleId; @@ -38,7 +39,7 @@ impl PathResolver for StandardPathResolver { &self, def_maps: &BTreeMap, path: Path, - path_references: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>>, ) -> PathResolutionResult { resolve_path(def_maps, self.module_id, path, path_references) } @@ -58,7 +59,7 @@ pub fn resolve_path( def_maps: &BTreeMap, module_id: ModuleId, path: Path, - path_references: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>>, ) -> PathResolutionResult { // lets package up the path into an ImportDirective and resolve it using that let import = diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index 856a769c9dd..55cb2145ba8 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -1105,6 +1105,7 @@ impl<'a> Resolver<'a> { location, typ, direct_generics, + struct_id: None, trait_impl: self.current_trait_impl, parameters: parameters.into(), return_type: func.def.return_type.clone(), diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs index 1a70bade863..57125e1e2dd 100644 --- a/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/compiler/noirc_frontend/src/hir/type_check/mod.rs @@ -570,6 +570,7 @@ pub mod test { .into(), return_visibility: Visibility::Private, has_body: true, + struct_id: None, trait_impl: None, return_type: FunctionReturnType::Default(Span::default()), trait_constraints: Vec::new(), @@ -696,7 +697,7 @@ pub mod test { &self, _def_maps: &BTreeMap, path: Path, - _path_references: &mut Option<&mut Vec>, + _path_references: &mut Option<&mut Vec>>, ) -> PathResolutionResult { // Not here that foo::bar and hello::foo::bar would fetch the same thing let name = path.segments.last().unwrap(); diff --git a/compiler/noirc_frontend/src/hir_def/function.rs b/compiler/noirc_frontend/src/hir_def/function.rs index fa8bb55abee..e666f996231 100644 --- a/compiler/noirc_frontend/src/hir_def/function.rs +++ b/compiler/noirc_frontend/src/hir_def/function.rs @@ -6,7 +6,7 @@ use super::stmt::HirPattern; use super::traits::TraitConstraint; use crate::ast::{FunctionKind, FunctionReturnType, Visibility}; use crate::graph::CrateId; -use crate::macros_api::BlockExpression; +use crate::macros_api::{BlockExpression, StructId}; use crate::node_interner::{ExprId, NodeInterner, TraitImplId}; use crate::{ResolvedGeneric, Type}; @@ -126,6 +126,9 @@ pub struct FuncMeta { pub trait_constraints: Vec, + /// The struct this function belongs to, if any + pub struct_id: Option, + /// The trait impl this function belongs to, if any pub trait_impl: Option, diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 5ca8c1493eb..0ba74e22781 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -4,7 +4,7 @@ use rangemap::RangeMap; use rustc_hash::FxHashMap; use crate::{ - hir::def_map::ModuleId, + hir::def_map::{ModuleDefId, ModuleId}, macros_api::{NodeInterner, StructId}, node_interner::{DefinitionId, FuncId, GlobalId, ReferenceId, TraitId, TypeAliasId}, }; @@ -17,7 +17,7 @@ pub(crate) struct LocationIndices { impl LocationIndices { pub(crate) fn add_location(&mut self, location: Location, node_index: PetGraphIndex) { - // Some location spans are empty: maybe they are from ficticious nodes? + // Some location spans are empty: maybe they are from fictitious nodes? if location.span.start() == location.span.end() { return; } @@ -35,7 +35,7 @@ impl LocationIndices { impl NodeInterner { pub fn reference_location(&self, reference: ReferenceId) -> Location { match reference { - ReferenceId::Module(id) => self.module_location(&id), + ReferenceId::Module(id) => self.module_attributes(&id).location, ReferenceId::Function(id) => self.function_modifiers(&id).name_location, ReferenceId::Struct(id) => { let struct_type = self.get_struct(id); @@ -62,6 +62,38 @@ impl NodeInterner { } } + pub fn reference_module(&self, reference: ReferenceId) -> Option<&ModuleId> { + self.reference_modules.get(&reference) + } + + pub(crate) fn add_module_def_id_reference( + &mut self, + def_id: ModuleDefId, + location: Location, + is_self_type: bool, + ) { + match def_id { + ModuleDefId::ModuleId(module_id) => { + self.add_module_reference(module_id, location); + } + ModuleDefId::FunctionId(func_id) => { + self.add_function_reference(func_id, location); + } + ModuleDefId::TypeId(struct_id) => { + self.add_struct_reference(struct_id, location, is_self_type); + } + ModuleDefId::TraitId(trait_id) => { + self.add_trait_reference(trait_id, location, is_self_type); + } + ModuleDefId::TypeAliasId(type_alias_id) => { + self.add_alias_reference(type_alias_id, location); + } + ModuleDefId::GlobalId(global_id) => { + self.add_global_reference(global_id, location); + } + }; + } + pub(crate) fn add_module_reference(&mut self, id: ModuleId, location: Location) { self.add_reference(ReferenceId::Module(id), location, false); } @@ -129,7 +161,11 @@ impl NodeInterner { self.location_indices.add_location(reference_location, reference_index); } - pub(crate) fn add_definition_location(&mut self, referenced: ReferenceId) { + pub(crate) fn add_definition_location( + &mut self, + referenced: ReferenceId, + module_id: Option, + ) { if !self.track_references { return; } @@ -137,6 +173,9 @@ impl NodeInterner { let referenced_index = self.get_or_insert_reference(referenced); let referenced_location = self.reference_location(referenced); self.location_indices.add_location(referenced_location, referenced_index); + if let Some(module_id) = module_id { + self.reference_modules.insert(referenced, module_id); + } } #[tracing::instrument(skip(self), ret)] @@ -168,7 +207,7 @@ impl NodeInterner { // Starting at the given location, find the node referenced by it. Then, gather // all locations that reference that node, and return all of them - // (the references and optionally the referenced node if `include_referencedd` is true). + // (the references and optionally the referenced node if `include_referenced` is true). // If `include_self_type_name` is true, references where "Self" is written are returned, // otherwise they are not. // Returns `None` if the location is not known to this interner. diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index a009a42df53..e534f1bab29 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -44,6 +44,13 @@ use crate::{Shared, TypeAlias, TypeBindings, TypeVariable, TypeVariableId, TypeV /// This is needed to stop recursing for cases such as `impl Foo for T where T: Eq` const IMPL_SEARCH_RECURSION_LIMIT: u32 = 10; +#[derive(Debug)] +pub struct ModuleAttributes { + pub name: String, + pub location: Location, + pub parent: LocalModuleId, +} + type StructAttributes = Vec; /// The node interner is the central storage location of all nodes in Noir's Hir (the @@ -68,7 +75,7 @@ pub struct NodeInterner { function_modules: HashMap, // The location of each module - module_locations: HashMap, + module_attributes: HashMap, /// This graph tracks dependencies between different global definitions. /// This is used to ensure the absence of dependency cycles for globals and types. @@ -219,6 +226,10 @@ pub struct NodeInterner { /// Store the location of the references in the graph pub(crate) location_indices: LocationIndices, + + // The module where each reference is + // (ReferenceId::Reference and ReferenceId::Local aren't included here) + pub(crate) reference_modules: HashMap, } /// A dependency in the dependency graph may be a type or a definition. @@ -553,7 +564,7 @@ impl Default for NodeInterner { function_definition_ids: HashMap::new(), function_modifiers: HashMap::new(), function_modules: HashMap::new(), - module_locations: HashMap::new(), + module_attributes: HashMap::new(), func_id_to_trait: HashMap::new(), dependency_graph: petgraph::graph::DiGraph::new(), dependency_graph_indices: HashMap::new(), @@ -586,6 +597,7 @@ impl Default for NodeInterner { location_indices: LocationIndices::default(), reference_graph: petgraph::graph::DiGraph::new(), reference_graph_indices: HashMap::new(), + reference_modules: HashMap::new(), }; // An empty block expression is used often, we add this into the `node` on startup @@ -854,7 +866,7 @@ impl NodeInterner { self.definitions.push(DefinitionInfo { name, mutable, comptime, kind, location }); if is_local { - self.add_definition_location(ReferenceId::Local(id)); + self.add_definition_location(ReferenceId::Local(id), None); } id @@ -892,7 +904,7 @@ impl NodeInterner { // This needs to be done after pushing the definition since it will reference the // location that was stored - self.add_definition_location(ReferenceId::Function(id)); + self.add_definition_location(ReferenceId::Function(id), Some(module)); definition_id } @@ -993,12 +1005,16 @@ impl NodeInterner { &self.struct_attributes[struct_id] } - pub fn add_module_location(&mut self, module_id: ModuleId, location: Location) { - self.module_locations.insert(module_id, location); + pub fn add_module_attributes(&mut self, module_id: ModuleId, attributes: ModuleAttributes) { + self.module_attributes.insert(module_id, attributes); + } + + pub fn module_attributes(&self, module_id: &ModuleId) -> &ModuleAttributes { + &self.module_attributes[module_id] } - pub fn module_location(&self, module_id: &ModuleId) -> Location { - self.module_locations[module_id] + pub fn try_module_attributes(&self, module_id: &ModuleId) -> Option<&ModuleAttributes> { + self.module_attributes.get(module_id) } pub fn global_attributes(&self, global_id: &GlobalId) -> &[SecondaryAttribute] { diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index e91e0fb3325..28ba5425fe7 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -21,7 +21,7 @@ use async_lsp::{ use fm::{codespan_files as files, FileManager}; use fxhash::FxHashSet; use lsp_types::{ - request::{PrepareRenameRequest, References, Rename}, + request::{HoverRequest, PrepareRenameRequest, References, Rename}, CodeLens, }; use nargo::{ @@ -46,7 +46,7 @@ use notifications::{ }; use requests::{ on_code_lens_request, on_formatting, on_goto_declaration_request, on_goto_definition_request, - on_goto_type_definition_request, on_initialize, on_prepare_rename_request, + on_goto_type_definition_request, on_hover_request, on_initialize, on_prepare_rename_request, on_profile_run_request, on_references_request, on_rename_request, on_shutdown, on_test_run_request, on_tests_request, }; @@ -129,6 +129,7 @@ impl NargoLspService { .request::(on_references_request) .request::(on_prepare_rename_request) .request::(on_rename_request) + .request::(on_hover_request) .notification::(on_initialized) .notification::(on_did_change_configuration) .notification::(on_did_open_text_document) diff --git a/tooling/lsp/src/requests/goto_declaration.rs b/tooling/lsp/src/requests/goto_declaration.rs index 1f87f2c87d4..bd0f0afb827 100644 --- a/tooling/lsp/src/requests/goto_declaration.rs +++ b/tooling/lsp/src/requests/goto_declaration.rs @@ -20,10 +20,10 @@ fn on_goto_definition_inner( state: &mut LspState, params: GotoDeclarationParams, ) -> Result { - process_request(state, params.text_document_position_params, |location, interner, files, _| { - interner.get_declaration_location_from(location).and_then(|found_location| { + process_request(state, params.text_document_position_params, |args| { + args.interner.get_declaration_location_from(args.location).and_then(|found_location| { let file_id = found_location.file; - let definition_position = to_lsp_location(files, file_id, found_location.span)?; + let definition_position = to_lsp_location(args.files, file_id, found_location.span)?; let response = GotoDeclarationResponse::from(definition_position).to_owned(); Some(response) }) diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index 27d3503a2fd..72fac676a72 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -29,15 +29,16 @@ fn on_goto_definition_inner( params: GotoDefinitionParams, return_type_location_instead: bool, ) -> Result { - process_request(state, params.text_document_position_params, |location, interner, files, _| { - interner.get_definition_location_from(location, return_type_location_instead).and_then( - |found_location| { + process_request(state, params.text_document_position_params, |args| { + args.interner + .get_definition_location_from(args.location, return_type_location_instead) + .and_then(|found_location| { let file_id = found_location.file; - let definition_position = to_lsp_location(files, file_id, found_location.span)?; + let definition_position = + to_lsp_location(args.files, file_id, found_location.span)?; let response = GotoDefinitionResponse::from(definition_position).to_owned(); Some(response) - }, - ) + }) }) } diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs new file mode 100644 index 00000000000..161fd20f555 --- /dev/null +++ b/tooling/lsp/src/requests/hover.rs @@ -0,0 +1,641 @@ +use std::future::{self, Future}; + +use async_lsp::ResponseError; +use lsp_types::{Hover, HoverContents, HoverParams, MarkupContent, MarkupKind}; +use noirc_frontend::{ + ast::Visibility, + graph::CrateId, + hir::def_map::ModuleId, + hir_def::stmt::HirPattern, + macros_api::{NodeInterner, StructId}, + node_interner::{ + DefinitionId, DefinitionKind, FuncId, GlobalId, ReferenceId, TraitId, TypeAliasId, + }, + Generics, Type, +}; + +use crate::LspState; + +use super::{process_request, to_lsp_location, ProcessRequestCallbackArgs}; + +pub(crate) fn on_hover_request( + state: &mut LspState, + params: HoverParams, +) -> impl Future, ResponseError>> { + let result = process_request(state, params.text_document_position_params, |args| { + args.interner.reference_at_location(args.location).map(|reference| { + let location = args.interner.reference_location(reference); + let lsp_location = to_lsp_location(args.files, location.file, location.span); + Hover { + range: lsp_location.map(|location| location.range), + contents: HoverContents::Markup(MarkupContent { + kind: MarkupKind::Markdown, + value: format_reference(reference, &args), + }), + } + }) + }); + + future::ready(result) +} + +fn format_reference(reference: ReferenceId, args: &ProcessRequestCallbackArgs) -> String { + match reference { + ReferenceId::Module(id) => format_module(id, args), + ReferenceId::Struct(id) => format_struct(id, args), + ReferenceId::StructMember(id, field_index) => format_struct_member(id, field_index, args), + ReferenceId::Trait(id) => format_trait(id, args), + ReferenceId::Global(id) => format_global(id, args), + ReferenceId::Function(id) => format_function(id, args), + ReferenceId::Alias(id) => format_alias(id, args), + ReferenceId::Local(id) => format_local(id, args), + ReferenceId::Reference(location, _) => { + format_reference(args.interner.find_referenced(location).unwrap(), args) + } + } +} +fn format_module(id: ModuleId, args: &ProcessRequestCallbackArgs) -> String { + let module_attributes = args.interner.module_attributes(&id); + + let mut string = String::new(); + if format_parent_module_from_module_id( + &ModuleId { krate: id.krate, local_id: module_attributes.parent }, + args, + &mut string, + ) { + string.push('\n'); + } + string.push_str(" "); + string.push_str("mod "); + string.push_str(&module_attributes.name); + string +} + +fn format_struct(id: StructId, args: &ProcessRequestCallbackArgs) -> String { + let struct_type = args.interner.get_struct(id); + let struct_type = struct_type.borrow(); + + let mut string = String::new(); + if format_parent_module(ReferenceId::Struct(id), args, &mut string) { + string.push('\n'); + } + string.push_str(" "); + string.push_str("struct "); + string.push_str(&struct_type.name.0.contents); + format_generics(&struct_type.generics, &mut string); + string.push_str(" {\n"); + for (field_name, field_type) in struct_type.get_fields_as_written() { + string.push_str(" "); + string.push_str(&field_name); + string.push_str(": "); + string.push_str(&format!("{}", field_type)); + string.push_str(",\n"); + } + string.push_str(" }"); + string +} + +fn format_struct_member( + id: StructId, + field_index: usize, + args: &ProcessRequestCallbackArgs, +) -> String { + let struct_type = args.interner.get_struct(id); + let struct_type = struct_type.borrow(); + let (field_name, field_type) = struct_type.field_at(field_index); + + let mut string = String::new(); + if format_parent_module(ReferenceId::Struct(id), args, &mut string) { + string.push_str("::"); + } + string.push_str(&struct_type.name.0.contents); + string.push('\n'); + string.push_str(" "); + string.push_str(&field_name.0.contents); + string.push_str(": "); + string.push_str(&format!("{}", field_type)); + string +} + +fn format_trait(id: TraitId, args: &ProcessRequestCallbackArgs) -> String { + let a_trait = args.interner.get_trait(id); + + let mut string = String::new(); + if format_parent_module(ReferenceId::Trait(id), args, &mut string) { + string.push('\n'); + } + string.push_str(" "); + string.push_str("trait "); + string.push_str(&a_trait.name.0.contents); + format_generics(&a_trait.generics, &mut string); + string +} + +fn format_global(id: GlobalId, args: &ProcessRequestCallbackArgs) -> String { + let global_info = args.interner.get_global(id); + let definition_id = global_info.definition_id; + let typ = args.interner.definition_type(definition_id); + + let mut string = String::new(); + if format_parent_module(ReferenceId::Global(id), args, &mut string) { + string.push('\n'); + } + string.push_str(" "); + string.push_str("global "); + string.push_str(&global_info.ident.0.contents); + string.push_str(": "); + string.push_str(&format!("{}", typ)); + string +} + +fn format_function(id: FuncId, args: &ProcessRequestCallbackArgs) -> String { + let func_meta = args.interner.function_meta(&id); + let func_name_definition_id = args.interner.definition(func_meta.name.id); + + let mut string = String::new(); + let formatted_parent_module = + format_parent_module(ReferenceId::Function(id), args, &mut string); + let formatted_parent_struct = if let Some(struct_id) = func_meta.struct_id { + let struct_type = args.interner.get_struct(struct_id); + let struct_type = struct_type.borrow(); + if formatted_parent_module { + string.push_str("::"); + } + string.push_str(&struct_type.name.0.contents); + true + } else { + false + }; + if formatted_parent_module || formatted_parent_struct { + string.push('\n'); + } + string.push_str(" "); + string.push_str("fn "); + string.push_str(&func_name_definition_id.name); + format_generics(&func_meta.direct_generics, &mut string); + string.push('('); + let parameters = &func_meta.parameters; + for (index, (pattern, typ, visibility)) in parameters.iter().enumerate() { + format_pattern(pattern, args.interner, &mut string); + if !pattern_is_self(pattern, args.interner) { + string.push_str(": "); + if matches!(visibility, Visibility::Public) { + string.push_str("pub "); + } + string.push_str(&format!("{}", typ)); + } + if index != parameters.len() - 1 { + string.push_str(", "); + } + } + + string.push(')'); + + let return_type = func_meta.return_type(); + match return_type { + Type::Unit => (), + _ => { + string.push_str(" -> "); + string.push_str(&format!("{}", return_type)); + } + } + + string +} + +fn format_alias(id: TypeAliasId, args: &ProcessRequestCallbackArgs) -> String { + let type_alias = args.interner.get_type_alias(id); + let type_alias = type_alias.borrow(); + + let mut string = String::new(); + format_parent_module(ReferenceId::Alias(id), args, &mut string); + string.push('\n'); + string.push_str(" "); + string.push_str("type "); + string.push_str(&type_alias.name.0.contents); + string.push_str(" = "); + string.push_str(&format!("{}", &type_alias.typ)); + string +} + +fn format_local(id: DefinitionId, args: &ProcessRequestCallbackArgs) -> String { + let definition_info = args.interner.definition(id); + let DefinitionKind::Local(expr_id) = definition_info.kind else { + panic!("Expected a local reference to reference a local definition") + }; + let typ = args.interner.definition_type(id); + + let mut string = String::new(); + string.push_str(" "); + if definition_info.comptime { + string.push_str("comptime "); + } + if expr_id.is_some() { + string.push_str("let "); + } + if definition_info.mutable { + if expr_id.is_none() { + string.push_str("let "); + } + string.push_str("mut "); + } + string.push_str(&definition_info.name); + if !matches!(typ, Type::Error) { + string.push_str(": "); + string.push_str(&format!("{}", typ)); + } + string +} + +fn format_generics(generics: &Generics, string: &mut String) { + if generics.is_empty() { + return; + } + + string.push('<'); + for (index, generic) in generics.iter().enumerate() { + string.push_str(&generic.name); + if index != generics.len() - 1 { + string.push_str(", "); + } + } + string.push('>'); +} +fn format_pattern(pattern: &HirPattern, interner: &NodeInterner, string: &mut String) { + match pattern { + HirPattern::Identifier(ident) => { + let definition = interner.definition(ident.id); + string.push_str(&definition.name); + } + HirPattern::Mutable(pattern, _) => { + string.push_str("mut "); + format_pattern(pattern, interner, string); + } + HirPattern::Tuple(..) | HirPattern::Struct(..) => { + string.push('_'); + } + } +} + +fn pattern_is_self(pattern: &HirPattern, interner: &NodeInterner) -> bool { + match pattern { + HirPattern::Identifier(ident) => { + let definition = interner.definition(ident.id); + definition.name == "self" + } + HirPattern::Mutable(pattern, _) => pattern_is_self(pattern, interner), + HirPattern::Tuple(..) | HirPattern::Struct(..) => false, + } +} + +fn format_parent_module( + referenced: ReferenceId, + args: &ProcessRequestCallbackArgs, + string: &mut String, +) -> bool { + let Some(module) = args.interner.reference_module(referenced) else { + return false; + }; + + format_parent_module_from_module_id(module, args, string) +} + +fn format_parent_module_from_module_id( + module: &ModuleId, + args: &ProcessRequestCallbackArgs, + string: &mut String, +) -> bool { + let crate_id = module.krate; + let crate_name = match crate_id { + CrateId::Root(_) => Some(args.root_crate_name.clone()), + CrateId::Crate(_) => args + .root_crate_dependencies + .iter() + .find(|dep| dep.crate_id == crate_id) + .map(|dep| format!("{}", dep.name)), + CrateId::Stdlib(_) => Some("std".to_string()), + CrateId::Dummy => None, + }; + + let wrote_crate = if let Some(crate_name) = crate_name { + string.push_str(" "); + string.push_str(&crate_name); + true + } else { + false + }; + + let Some(module_attributes) = args.interner.try_module_attributes(module) else { + return wrote_crate; + }; + + if wrote_crate { + string.push_str("::"); + } else { + string.push_str(" "); + } + + let mut segments = Vec::new(); + let mut current_attributes = module_attributes; + while let Some(parent_attributes) = args.interner.try_module_attributes(&ModuleId { + krate: module.krate, + local_id: current_attributes.parent, + }) { + segments.push(&parent_attributes.name); + current_attributes = parent_attributes; + } + + for segment in segments.iter().rev() { + string.push_str(segment); + string.push_str("::"); + } + + string.push_str(&module_attributes.name); + + true +} + +#[cfg(test)] +mod hover_tests { + use crate::test_utils; + + use super::*; + use lsp_types::{ + Position, TextDocumentIdentifier, TextDocumentPositionParams, Url, WorkDoneProgressParams, + }; + use tokio::test; + + async fn assert_hover(directory: &str, file: &str, position: Position, expected_text: &str) { + let (mut state, noir_text_document) = test_utils::init_lsp_server(directory).await; + + // noir_text_document is always `src/main.nr` in the workspace directory, so let's go to the workspace dir + let noir_text_document = noir_text_document.to_file_path().unwrap(); + let workspace_dir = noir_text_document.parent().unwrap().parent().unwrap(); + + let file_uri = Url::from_file_path(workspace_dir.join(file)).unwrap(); + + let hover = on_hover_request( + &mut state, + HoverParams { + text_document_position_params: TextDocumentPositionParams { + text_document: TextDocumentIdentifier { uri: file_uri }, + position, + }, + work_done_progress_params: WorkDoneProgressParams { work_done_token: None }, + }, + ) + .await + .expect("Could not execute hover") + .unwrap(); + + let HoverContents::Markup(markup) = hover.contents else { + panic!("Expected hover contents to be Markup"); + }; + + assert_eq!(markup.value, expected_text); + } + + #[test] + async fn hover_on_module() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 6, character: 9 }, + r#" one + mod subone"#, + ) + .await; + } + + #[test] + async fn hover_on_struct() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 9, character: 20 }, + r#" one::subone + struct SubOneStruct { + some_field: i32, + some_other_field: Field, + }"#, + ) + .await; + } + + #[test] + async fn hover_on_generic_struct() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 46, character: 17 }, + r#" one::subone + struct GenericStruct { + }"#, + ) + .await; + } + + #[test] + async fn hover_on_struct_member() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 9, character: 35 }, + r#" one::subone::SubOneStruct + some_field: i32"#, + ) + .await; + } + + #[test] + async fn hover_on_trait() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 12, character: 17 }, + r#" one::subone + trait SomeTrait"#, + ) + .await; + } + + #[test] + async fn hover_on_global() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 15, character: 25 }, + r#" one::subone + global some_global: Field"#, + ) + .await; + } + + #[test] + async fn hover_on_function() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 3, character: 4 }, + r#" one + fn function_one()"#, + ) + .await; + } + + #[test] + async fn hover_on_local_function() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 2, character: 7 }, + r#" two + fn function_two()"#, + ) + .await; + } + + #[test] + async fn hover_on_struct_method() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 20, character: 6 }, + r#" one::subone::SubOneStruct + fn foo(self, x: i32, y: i32) -> Field"#, + ) + .await; + } + + #[test] + async fn hover_on_local_var() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 25, character: 12 }, + " let regular_var: Field", + ) + .await; + } + + #[test] + async fn hover_on_local_mut_var() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 27, character: 4 }, + " let mut mutable_var: Field", + ) + .await; + } + + #[test] + async fn hover_on_parameter() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 31, character: 12 }, + " some_param: i32", + ) + .await; + } + + #[test] + async fn hover_on_alias() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 34, character: 17 }, + r#" one::subone + type SomeAlias = i32"#, + ) + .await; + } + + #[test] + async fn hover_on_trait_on_call() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 39, character: 17 }, + r#" std::default + trait Default"#, + ) + .await; + } + + #[test] + async fn hover_on_std_module_in_use() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 36, character: 9 }, + r#" std + mod default"#, + ) + .await; + } + + #[test] + async fn hover_on_crate_module_in_call() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 15, character: 17 }, + r#" one + mod subone"#, + ) + .await; + } + + #[test] + async fn hover_on_module_without_crate_or_std_prefix() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 43, character: 4 }, + r#" two + mod other"#, + ) + .await; + } + + #[test] + async fn hover_on_module_with_crate_prefix() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 44, character: 11 }, + r#" two + mod other"#, + ) + .await; + } + + #[test] + async fn hover_on_module_on_struct_constructor() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 19, character: 12 }, + r#" one + mod subone"#, + ) + .await; + } + + #[test] + async fn hover_on_type_inside_generic_arguments() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 51, character: 30 }, + r#" one::subone + struct SubOneStruct { + some_field: i32, + some_other_field: Field, + }"#, + ) + .await; + } +} diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 9ece797a57a..e029750e589 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -14,7 +14,7 @@ use lsp_types::{ use nargo::insert_all_files_for_workspace_into_file_manager; use nargo_fmt::Config; use noirc_driver::file_manager_with_stdlib; -use noirc_frontend::macros_api::NodeInterner; +use noirc_frontend::{graph::Dependency, macros_api::NodeInterner}; use serde::{Deserialize, Serialize}; use crate::{ @@ -35,6 +35,7 @@ use crate::{ mod code_lens_request; mod goto_declaration; mod goto_definition; +mod hover; mod profile_run; mod references; mod rename; @@ -44,9 +45,10 @@ mod tests; pub(crate) use { code_lens_request::collect_lenses_for_package, code_lens_request::on_code_lens_request, goto_declaration::on_goto_declaration_request, goto_definition::on_goto_definition_request, - goto_definition::on_goto_type_definition_request, profile_run::on_profile_run_request, - references::on_references_request, rename::on_prepare_rename_request, - rename::on_rename_request, test_run::on_test_run_request, tests::on_tests_request, + goto_definition::on_goto_type_definition_request, hover::on_hover_request, + profile_run::on_profile_run_request, references::on_references_request, + rename::on_prepare_rename_request, rename::on_rename_request, test_run::on_test_run_request, + tests::on_tests_request, }; /// LSP client will send initialization request after the server has started. @@ -127,6 +129,11 @@ pub(crate) fn on_initialize( work_done_progress: None, }, })), + hover_provider: Some(lsp_types::OneOf::Right(lsp_types::HoverOptions { + work_done_progress_options: WorkDoneProgressOptions { + work_done_progress: None, + }, + })), }, server_info: None, }) @@ -264,13 +271,22 @@ pub(crate) fn on_shutdown( async { Ok(()) } } +pub(crate) struct ProcessRequestCallbackArgs<'a> { + location: noirc_errors::Location, + files: &'a FileMap, + interner: &'a NodeInterner, + interners: &'a HashMap, + root_crate_name: String, + root_crate_dependencies: &'a Vec, +} + pub(crate) fn process_request( state: &mut LspState, text_document_position_params: TextDocumentPositionParams, callback: F, ) -> Result where - F: FnOnce(noirc_errors::Location, &NodeInterner, &FileMap, &HashMap) -> T, + F: FnOnce(ProcessRequestCallbackArgs) -> T, { let file_path = text_document_position_params.text_document.uri.to_file_path().map_err(|_| { @@ -309,7 +325,14 @@ where &text_document_position_params.position, )?; - Ok(callback(location, interner, files, &state.cached_definitions)) + Ok(callback(ProcessRequestCallbackArgs { + location, + files, + interner, + interners: &state.cached_definitions, + root_crate_name: package.name.to_string(), + root_crate_dependencies: &context.crate_graph[context.root_crate_id()].dependencies, + })) } pub(crate) fn find_all_references_in_workspace( location: noirc_errors::Location, diff --git a/tooling/lsp/src/requests/references.rs b/tooling/lsp/src/requests/references.rs index 6c872656c28..badea8921b2 100644 --- a/tooling/lsp/src/requests/references.rs +++ b/tooling/lsp/src/requests/references.rs @@ -12,20 +12,16 @@ pub(crate) fn on_references_request( params: ReferenceParams, ) -> impl Future>, ResponseError>> { let include_declaration = params.context.include_declaration; - let result = process_request( - state, - params.text_document_position, - |location, interner, files, cached_interners| { - find_all_references_in_workspace( - location, - interner, - cached_interners, - files, - include_declaration, - true, - ) - }, - ); + let result = process_request(state, params.text_document_position, |args| { + find_all_references_in_workspace( + args.location, + args.interner, + args.interners, + args.files, + include_declaration, + true, + ) + }); future::ready(result) } diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index 245c2ed0882..84956681167 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -17,8 +17,8 @@ pub(crate) fn on_prepare_rename_request( state: &mut LspState, params: TextDocumentPositionParams, ) -> impl Future, ResponseError>> { - let result = process_request(state, params, |location, interner, _, _| { - let reference_id = interner.reference_at_location(location); + let result = process_request(state, params, |args| { + let reference_id = args.interner.reference_at_location(args.location); let rename_possible = match reference_id { // Rename shouldn't be possible when triggered on top of "Self" Some(ReferenceId::Reference(_, true /* is self type name */)) => false, @@ -34,40 +34,36 @@ pub(crate) fn on_rename_request( state: &mut LspState, params: RenameParams, ) -> impl Future, ResponseError>> { - let result = process_request( - state, - params.text_document_position, - |location, interner, files, cached_interners| { - let rename_changes = find_all_references_in_workspace( - location, - interner, - cached_interners, - files, - true, - false, - ) - .map(|locations| { - let rs = locations.iter().fold( - HashMap::new(), - |mut acc: HashMap>, location| { - let edit = - TextEdit { range: location.range, new_text: params.new_name.clone() }; - acc.entry(location.uri.clone()).or_default().push(edit); - acc - }, - ); - rs - }); - - let response = WorkspaceEdit { - changes: rename_changes, - document_changes: None, - change_annotations: None, - }; + let result = process_request(state, params.text_document_position, |args| { + let rename_changes = find_all_references_in_workspace( + args.location, + args.interner, + args.interners, + args.files, + true, + false, + ) + .map(|locations| { + let rs = locations.iter().fold( + HashMap::new(), + |mut acc: HashMap>, location| { + let edit = + TextEdit { range: location.range, new_text: params.new_name.clone() }; + acc.entry(location.uri.clone()).or_default().push(edit); + acc + }, + ); + rs + }); + + let response = WorkspaceEdit { + changes: rename_changes, + document_changes: None, + change_annotations: None, + }; - Some(response) - }, - ); + Some(response) + }); future::ready(result) } @@ -174,6 +170,11 @@ mod rename_tests { check_rename_succeeds("rename_function_use", "some_function").await; } + #[test] + async fn test_rename_method() { + check_rename_succeeds("rename_function", "some_method").await; + } + #[test] async fn test_rename_struct() { check_rename_succeeds("rename_struct", "Foo").await; diff --git a/tooling/lsp/src/types.rs b/tooling/lsp/src/types.rs index 57eb2dd3618..991773ce94f 100644 --- a/tooling/lsp/src/types.rs +++ b/tooling/lsp/src/types.rs @@ -1,7 +1,7 @@ use fm::FileId; use lsp_types::{ - DeclarationCapability, DefinitionOptions, OneOf, ReferencesOptions, RenameOptions, - TypeDefinitionProviderCapability, + DeclarationCapability, DefinitionOptions, HoverOptions, OneOf, ReferencesOptions, + RenameOptions, TypeDefinitionProviderCapability, }; use noirc_driver::DebugFile; use noirc_errors::{debug_info::OpCodesCount, Location}; @@ -144,6 +144,10 @@ pub(crate) struct ServerCapabilities { /// The server provides references support. #[serde(skip_serializing_if = "Option::is_none")] pub(crate) references_provider: Option>, + + /// The server provides hover support. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) hover_provider: Option>, } #[derive(Debug, PartialEq, Clone, Default, Deserialize, Serialize)] diff --git a/tooling/lsp/test_programs/rename_function/src/main.nr b/tooling/lsp/test_programs/rename_function/src/main.nr index 7a70084276e..e77b50c0b26 100644 --- a/tooling/lsp/test_programs/rename_function/src/main.nr +++ b/tooling/lsp/test_programs/rename_function/src/main.nr @@ -25,3 +25,16 @@ use foo::some_other_function as bar; fn x() { bar(); } + +struct SomeStruct { + +} + +impl SomeStruct { + fn some_method(self) {} +} + +fn y() { + let some_struct = SomeStruct {}; + some_struct.some_method(); +} diff --git a/tooling/lsp/test_programs/workspace/one/src/lib.nr b/tooling/lsp/test_programs/workspace/one/src/lib.nr index d4f660e35fb..61f282fa2a7 100644 --- a/tooling/lsp/test_programs/workspace/one/src/lib.nr +++ b/tooling/lsp/test_programs/workspace/one/src/lib.nr @@ -1 +1,25 @@ -pub fn function_one() {} +pub fn function_one() {} + +mod subone { + struct SubOneStruct { + some_field: i32, + some_other_field: Field, + } + + impl SubOneStruct { + fn foo(self, x: i32, y: i32) -> Field { + 0 + } + } + + trait SomeTrait { + } + + global some_global = 2; + + type SomeAlias = i32; + + struct GenericStruct { + + } +} diff --git a/tooling/lsp/test_programs/workspace/two/src/lib.nr b/tooling/lsp/test_programs/workspace/two/src/lib.nr index adf7079b4c9..3f0f0f117b7 100644 --- a/tooling/lsp/test_programs/workspace/two/src/lib.nr +++ b/tooling/lsp/test_programs/workspace/two/src/lib.nr @@ -3,3 +3,51 @@ use one::function_one; pub fn function_two() { function_one() } + +use one::subone; + +fn use_struct() { + let _ = subone::SubOneStruct { some_field: 0, some_other_field: 2 }; +} + +use one::subone::SomeTrait; + +fn use_global() { + let _ = one::subone::some_global; +} + +fn use_struct_method() { + let s = subone::SubOneStruct { some_field: 0, some_other_field: 2 }; + s.foo(0, 1); +} + +fn use_local_var() { + let regular_var = 0; + let _ = regular_var; + let mut mutable_var = 0; + mutable_var = 1; +} + +fn use_parameter(some_param: i32) { + let _ = some_param; +} + +use one::subone::SomeAlias; + +use std::default::Default; + +fn use_impl_method() { + let _: i32 = Default::default(); +} + +mod other; +use other::another_function; +use crate::other::other_function; + +use one::subone::GenericStruct; + +use std::collections::bounded_vec::BoundedVec; + +fn instantiate_generic() { + let x: BoundedVec = BoundedVec::new(); +} diff --git a/tooling/lsp/test_programs/workspace/two/src/other.nr b/tooling/lsp/test_programs/workspace/two/src/other.nr new file mode 100644 index 00000000000..4d2fffcee80 --- /dev/null +++ b/tooling/lsp/test_programs/workspace/two/src/other.nr @@ -0,0 +1,2 @@ +fn other_function() {} +fn another_function() {} From 299703cf4b87a84257f48f059eb58135ad36265d Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 12 Jul 2024 10:06:57 -0500 Subject: [PATCH 4/4] fix: Don't type error when calling certain trait impls in the interpreter (#5471) # Description ## Problem\* Resolves https://github.com/noir-lang/noir/issues/5428 ## Summary\* The interpreter was calling into `perform_impl_bindings` in more cases than the monomorphizer. Where the former calls into it for every trait impl method, the later only calls into it for methods called from a `TraitMethodExpr` or `ImplKind::TraitMethod` on an ident/operator. It seemed more correct to call into this function in all cases so I removed the bug by also replacing NamedGenerics in the trait itself with bindable type variables. ## Additional Context I've moved around some methods in `hir_def/types.rs` so that we don't have 3 separate `impl Type {`s in the one file. The only two changes in that file are the addition of `replace_named_generics_with_type_variables` and the formatting change for debugging NamedGenerics. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/hir_def/types.rs | 491 ++++++++++-------- .../src/monomorphization/mod.rs | 23 +- .../regression_5428/Nargo.toml | 6 + .../regression_5428/src/main.nr | 9 + tooling/nargo_cli/build.rs | 3 +- 5 files changed, 309 insertions(+), 223 deletions(-) create mode 100644 test_programs/compile_success_empty/regression_5428/Nargo.toml create mode 100644 test_programs/compile_success_empty/regression_5428/src/main.nr diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 8183911c845..6919f697d27 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -114,79 +114,6 @@ pub enum Type { Error, } -impl Type { - /// Returns the number of field elements required to represent the type once encoded. - pub fn field_count(&self) -> u32 { - match self { - Type::FieldElement | Type::Integer { .. } | Type::Bool => 1, - Type::Array(size, typ) => { - let length = size - .evaluate_to_u32() - .expect("Cannot have variable sized arrays as a parameter to main"); - let typ = typ.as_ref(); - length * typ.field_count() - } - Type::Struct(def, args) => { - let struct_type = def.borrow(); - let fields = struct_type.get_fields(args); - fields.iter().fold(0, |acc, (_, field_type)| acc + field_type.field_count()) - } - Type::Alias(def, generics) => def.borrow().get_type(generics).field_count(), - Type::Tuple(fields) => { - fields.iter().fold(0, |acc, field_typ| acc + field_typ.field_count()) - } - Type::String(size) => size - .evaluate_to_u32() - .expect("Cannot have variable sized strings as a parameter to main"), - Type::FmtString(_, _) - | Type::Unit - | Type::TypeVariable(_, _) - | Type::TraitAsType(..) - | Type::NamedGeneric(_, _, _) - | Type::Function(_, _, _) - | Type::MutableReference(_) - | Type::Forall(_, _) - | Type::Constant(_) - | Type::Quoted(_) - | Type::Slice(_) - | Type::Error => unreachable!("This type cannot exist as a parameter to main"), - } - } - - pub(crate) fn is_nested_slice(&self) -> bool { - match self { - Type::Slice(elem) => elem.as_ref().contains_slice(), - Type::Array(_, elem) => elem.as_ref().contains_slice(), - Type::Alias(alias, generics) => alias.borrow().get_type(generics).is_nested_slice(), - _ => false, - } - } - - pub(crate) fn contains_slice(&self) -> bool { - match self { - Type::Slice(_) => true, - Type::Struct(struct_typ, generics) => { - let fields = struct_typ.borrow().get_fields(generics); - for field in fields.iter() { - if field.1.contains_slice() { - return true; - } - } - false - } - Type::Tuple(types) => { - for typ in types.iter() { - if typ.contains_slice() { - return true; - } - } - false - } - _ => false, - } - } -} - /// A Kind is the type of a Type. These are used since only certain kinds of types are allowed in /// certain positions. /// @@ -594,7 +521,7 @@ impl TypeVariable { /// TypeBindings are the mutable insides of a TypeVariable. /// They are either bound to some type, or are unbound. -#[derive(Clone, PartialEq, Eq, Hash)] +#[derive(Clone, PartialEq, Eq, Hash, Debug)] pub enum TypeBinding { Bound(Type), Unbound(TypeVariableId), @@ -610,6 +537,152 @@ impl TypeBinding { #[derive(Copy, Clone, PartialEq, Eq, Hash)] pub struct TypeVariableId(pub usize); +impl std::fmt::Display for Type { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Type::FieldElement => { + write!(f, "Field") + } + Type::Array(len, typ) => { + write!(f, "[{typ}; {len}]") + } + Type::Slice(typ) => { + write!(f, "[{typ}]") + } + Type::Integer(sign, num_bits) => match sign { + Signedness::Signed => write!(f, "i{num_bits}"), + Signedness::Unsigned => write!(f, "u{num_bits}"), + }, + Type::TypeVariable(var, TypeVariableKind::Normal) => write!(f, "{}", var.borrow()), + Type::TypeVariable(binding, TypeVariableKind::Integer) => { + if let TypeBinding::Unbound(_) = &*binding.borrow() { + write!(f, "{}", Type::default_int_type()) + } else { + write!(f, "{}", binding.borrow()) + } + } + Type::TypeVariable(binding, TypeVariableKind::IntegerOrField) => { + if let TypeBinding::Unbound(_) = &*binding.borrow() { + // Show a Field by default if this TypeVariableKind::IntegerOrField is unbound, since that is + // what they bind to by default anyway. It is less confusing than displaying it + // as a generic. + write!(f, "Field") + } else { + write!(f, "{}", binding.borrow()) + } + } + Type::TypeVariable(binding, TypeVariableKind::Constant(n)) => { + if let TypeBinding::Unbound(_) = &*binding.borrow() { + // TypeVariableKind::Constant(n) binds to Type::Constant(n) by default, so just show that. + write!(f, "{n}") + } else { + write!(f, "{}", binding.borrow()) + } + } + Type::Struct(s, args) => { + let args = vecmap(args, |arg| arg.to_string()); + if args.is_empty() { + write!(f, "{}", s.borrow()) + } else { + write!(f, "{}<{}>", s.borrow(), args.join(", ")) + } + } + Type::Alias(alias, args) => { + let args = vecmap(args, |arg| arg.to_string()); + if args.is_empty() { + write!(f, "{}", alias.borrow()) + } else { + write!(f, "{}<{}>", alias.borrow(), args.join(", ")) + } + } + Type::TraitAsType(_id, name, generics) => { + write!(f, "impl {}", name)?; + if !generics.is_empty() { + let generics = vecmap(generics, ToString::to_string).join(", "); + write!(f, "<{generics}>")?; + } + Ok(()) + } + Type::Tuple(elements) => { + let elements = vecmap(elements, ToString::to_string); + write!(f, "({})", elements.join(", ")) + } + Type::Bool => write!(f, "bool"), + Type::String(len) => write!(f, "str<{len}>"), + Type::FmtString(len, elements) => { + write!(f, "fmtstr<{len}, {elements}>") + } + Type::Unit => write!(f, "()"), + Type::Error => write!(f, "error"), + Type::NamedGeneric(binding, name, _) => match &*binding.borrow() { + TypeBinding::Bound(binding) => binding.fmt(f), + TypeBinding::Unbound(_) if name.is_empty() => write!(f, "_"), + TypeBinding::Unbound(_) => write!(f, "{name}"), + }, + Type::Constant(x) => x.fmt(f), + Type::Forall(typevars, typ) => { + let typevars = vecmap(typevars, |var| var.id().to_string()); + write!(f, "forall {}. {}", typevars.join(" "), typ) + } + Type::Function(args, ret, env) => { + let closure_env_text = match **env { + Type::Unit => "".to_string(), + _ => format!(" with env {env}"), + }; + + let args = vecmap(args.iter(), ToString::to_string); + + write!(f, "fn({}) -> {ret}{closure_env_text}", args.join(", ")) + } + Type::MutableReference(element) => { + write!(f, "&mut {element}") + } + Type::Quoted(quoted) => write!(f, "{}", quoted), + } + } +} + +impl std::fmt::Display for BinaryTypeOperator { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + BinaryTypeOperator::Addition => write!(f, "+"), + BinaryTypeOperator::Subtraction => write!(f, "-"), + BinaryTypeOperator::Multiplication => write!(f, "*"), + BinaryTypeOperator::Division => write!(f, "/"), + BinaryTypeOperator::Modulo => write!(f, "%"), + } + } +} + +impl std::fmt::Display for TypeVariableId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "_") + } +} + +impl std::fmt::Display for TypeBinding { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + TypeBinding::Bound(typ) => typ.fmt(f), + TypeBinding::Unbound(id) => id.fmt(f), + } + } +} + +impl std::fmt::Display for QuotedType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + QuotedType::Expr => write!(f, "Expr"), + QuotedType::Quoted => write!(f, "Quoted"), + QuotedType::TopLevelItem => write!(f, "TopLevelItem"), + QuotedType::Type => write!(f, "Type"), + QuotedType::StructDefinition => write!(f, "StructDefinition"), + } + } +} + +pub struct UnificationError; + impl Type { pub fn default_int_or_field_type() -> Type { Type::FieldElement @@ -1044,155 +1117,78 @@ impl Type { // | Type::Error => Kind::Normal, // } // } -} -impl std::fmt::Display for Type { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + /// Returns the number of field elements required to represent the type once encoded. + pub fn field_count(&self) -> u32 { match self { - Type::FieldElement => { - write!(f, "Field") - } - Type::Array(len, typ) => { - write!(f, "[{typ}; {len}]") - } - Type::Slice(typ) => { - write!(f, "[{typ}]") - } - Type::Integer(sign, num_bits) => match sign { - Signedness::Signed => write!(f, "i{num_bits}"), - Signedness::Unsigned => write!(f, "u{num_bits}"), - }, - Type::TypeVariable(var, TypeVariableKind::Normal) => write!(f, "{}", var.borrow()), - Type::TypeVariable(binding, TypeVariableKind::Integer) => { - if let TypeBinding::Unbound(_) = &*binding.borrow() { - write!(f, "{}", Type::default_int_type()) - } else { - write!(f, "{}", binding.borrow()) - } - } - Type::TypeVariable(binding, TypeVariableKind::IntegerOrField) => { - if let TypeBinding::Unbound(_) = &*binding.borrow() { - // Show a Field by default if this TypeVariableKind::IntegerOrField is unbound, since that is - // what they bind to by default anyway. It is less confusing than displaying it - // as a generic. - write!(f, "Field") - } else { - write!(f, "{}", binding.borrow()) - } - } - Type::TypeVariable(binding, TypeVariableKind::Constant(n)) => { - if let TypeBinding::Unbound(_) = &*binding.borrow() { - // TypeVariableKind::Constant(n) binds to Type::Constant(n) by default, so just show that. - write!(f, "{n}") - } else { - write!(f, "{}", binding.borrow()) - } - } - Type::Struct(s, args) => { - let args = vecmap(args, |arg| arg.to_string()); - if args.is_empty() { - write!(f, "{}", s.borrow()) - } else { - write!(f, "{}<{}>", s.borrow(), args.join(", ")) - } - } - Type::Alias(alias, args) => { - let args = vecmap(args, |arg| arg.to_string()); - if args.is_empty() { - write!(f, "{}", alias.borrow()) - } else { - write!(f, "{}<{}>", alias.borrow(), args.join(", ")) - } - } - Type::TraitAsType(_id, name, generics) => { - write!(f, "impl {}", name)?; - if !generics.is_empty() { - let generics = vecmap(generics, ToString::to_string).join(", "); - write!(f, "<{generics}>")?; - } - Ok(()) - } - Type::Tuple(elements) => { - let elements = vecmap(elements, ToString::to_string); - write!(f, "({})", elements.join(", ")) - } - Type::Bool => write!(f, "bool"), - Type::String(len) => write!(f, "str<{len}>"), - Type::FmtString(len, elements) => { - write!(f, "fmtstr<{len}, {elements}>") - } - Type::Unit => write!(f, "()"), - Type::Error => write!(f, "error"), - Type::NamedGeneric(binding, name, _) => match &*binding.borrow() { - TypeBinding::Bound(binding) => binding.fmt(f), - TypeBinding::Unbound(_) if name.is_empty() => write!(f, "_"), - TypeBinding::Unbound(_) => write!(f, "{name}"), - }, - Type::Constant(x) => x.fmt(f), - Type::Forall(typevars, typ) => { - let typevars = vecmap(typevars, |var| var.id().to_string()); - write!(f, "forall {}. {}", typevars.join(" "), typ) + Type::FieldElement | Type::Integer { .. } | Type::Bool => 1, + Type::Array(size, typ) => { + let length = size + .evaluate_to_u32() + .expect("Cannot have variable sized arrays as a parameter to main"); + let typ = typ.as_ref(); + length * typ.field_count() } - Type::Function(args, ret, env) => { - let closure_env_text = match **env { - Type::Unit => "".to_string(), - _ => format!(" with env {env}"), - }; - - let args = vecmap(args.iter(), ToString::to_string); - - write!(f, "fn({}) -> {ret}{closure_env_text}", args.join(", ")) + Type::Struct(def, args) => { + let struct_type = def.borrow(); + let fields = struct_type.get_fields(args); + fields.iter().fold(0, |acc, (_, field_type)| acc + field_type.field_count()) } - Type::MutableReference(element) => { - write!(f, "&mut {element}") + Type::Alias(def, generics) => def.borrow().get_type(generics).field_count(), + Type::Tuple(fields) => { + fields.iter().fold(0, |acc, field_typ| acc + field_typ.field_count()) } - Type::Quoted(quoted) => write!(f, "{}", quoted), - } - } -} - -impl std::fmt::Display for BinaryTypeOperator { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - BinaryTypeOperator::Addition => write!(f, "+"), - BinaryTypeOperator::Subtraction => write!(f, "-"), - BinaryTypeOperator::Multiplication => write!(f, "*"), - BinaryTypeOperator::Division => write!(f, "/"), - BinaryTypeOperator::Modulo => write!(f, "%"), + Type::String(size) => size + .evaluate_to_u32() + .expect("Cannot have variable sized strings as a parameter to main"), + Type::FmtString(_, _) + | Type::Unit + | Type::TypeVariable(_, _) + | Type::TraitAsType(..) + | Type::NamedGeneric(_, _, _) + | Type::Function(_, _, _) + | Type::MutableReference(_) + | Type::Forall(_, _) + | Type::Constant(_) + | Type::Quoted(_) + | Type::Slice(_) + | Type::Error => unreachable!("This type cannot exist as a parameter to main"), } } -} - -impl std::fmt::Display for TypeVariableId { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "_") - } -} -impl std::fmt::Display for TypeBinding { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + pub(crate) fn is_nested_slice(&self) -> bool { match self { - TypeBinding::Bound(typ) => typ.fmt(f), - TypeBinding::Unbound(id) => id.fmt(f), + Type::Slice(elem) => elem.as_ref().contains_slice(), + Type::Array(_, elem) => elem.as_ref().contains_slice(), + Type::Alias(alias, generics) => alias.borrow().get_type(generics).is_nested_slice(), + _ => false, } } -} -impl std::fmt::Display for QuotedType { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + pub(crate) fn contains_slice(&self) -> bool { match self { - QuotedType::Expr => write!(f, "Expr"), - QuotedType::Quoted => write!(f, "Quoted"), - QuotedType::TopLevelItem => write!(f, "TopLevelItem"), - QuotedType::Type => write!(f, "Type"), - QuotedType::StructDefinition => write!(f, "StructDefinition"), + Type::Slice(_) => true, + Type::Struct(struct_typ, generics) => { + let fields = struct_typ.borrow().get_fields(generics); + for field in fields.iter() { + if field.1.contains_slice() { + return true; + } + } + false + } + Type::Tuple(types) => { + for typ in types.iter() { + if typ.contains_slice() { + return true; + } + } + false + } + _ => false, } } -} -pub struct UnificationError; - -impl Type { /// Try to bind a MaybeConstant variable to self, succeeding if self is a Constant, /// MaybeConstant, or type variable. If successful, the binding is placed in the /// given TypeBindings map rather than linked immediately. @@ -2007,6 +2003,83 @@ impl Type { pub fn from_generics(generics: &GenericTypeVars) -> Vec { vecmap(generics, |var| Type::TypeVariable(var.clone(), TypeVariableKind::Normal)) } + + /// Replace any `Type::NamedGeneric` in this type with a `Type::TypeVariable` + /// using to the same inner `TypeVariable`. This is used during monomorphization + /// to bind to named generics since they are unbindable during type checking. + pub fn replace_named_generics_with_type_variables(&mut self) { + match self { + Type::FieldElement + | Type::Constant(_) + | Type::Integer(_, _) + | Type::Bool + | Type::Unit + | Type::Error + | Type::Quoted(_) => (), + + Type::Array(len, elem) => { + len.replace_named_generics_with_type_variables(); + elem.replace_named_generics_with_type_variables(); + } + + Type::Slice(elem) => elem.replace_named_generics_with_type_variables(), + Type::String(len) => len.replace_named_generics_with_type_variables(), + Type::FmtString(len, captures) => { + len.replace_named_generics_with_type_variables(); + captures.replace_named_generics_with_type_variables(); + } + Type::Tuple(fields) => { + for field in fields { + field.replace_named_generics_with_type_variables(); + } + } + Type::Struct(_, generics) => { + for generic in generics { + generic.replace_named_generics_with_type_variables(); + } + } + Type::Alias(alias, generics) => { + let mut typ = alias.borrow().get_type(generics); + typ.replace_named_generics_with_type_variables(); + *self = typ; + } + Type::TypeVariable(var, _) => { + let var = var.borrow(); + if let TypeBinding::Bound(binding) = &*var { + let mut binding = binding.clone(); + drop(var); + binding.replace_named_generics_with_type_variables(); + *self = binding; + } + } + Type::TraitAsType(_, _, generics) => { + for generic in generics { + generic.replace_named_generics_with_type_variables(); + } + } + Type::NamedGeneric(var, _, _) => { + let type_binding = var.borrow(); + if let TypeBinding::Bound(binding) = &*type_binding { + let mut binding = binding.clone(); + drop(type_binding); + binding.replace_named_generics_with_type_variables(); + *self = binding; + } else { + drop(type_binding); + *self = Type::TypeVariable(var.clone(), TypeVariableKind::Normal); + } + } + Type::Function(args, ret, env) => { + for arg in args { + arg.replace_named_generics_with_type_variables(); + } + ret.replace_named_generics_with_type_variables(); + env.replace_named_generics_with_type_variables(); + } + Type::MutableReference(elem) => elem.replace_named_generics_with_type_variables(), + Type::Forall(_, typ) => typ.replace_named_generics_with_type_variables(), + } + } } /// Wraps a given `expression` in `expression.as_slice()` @@ -2203,10 +2276,10 @@ impl std::fmt::Debug for Type { Type::Error => write!(f, "error"), Type::NamedGeneric(binding, name, kind) => match kind { Kind::Normal => { - write!(f, "{} -> {:?}", name, binding) + write!(f, "{}{:?}", name, binding) } Kind::Numeric(typ) => { - write!(f, "({} : {}) -> {:?}", name, typ, binding) + write!(f, "({} : {}){:?}", name, typ, binding) } }, Type::Constant(x) => x.fmt(f), diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index be222cc4e35..a46f32e3094 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -21,7 +21,7 @@ use crate::{ types, }, node_interner::{self, DefinitionKind, NodeInterner, StmtId, TraitImplKind, TraitMethodId}, - Type, TypeBinding, TypeBindings, TypeVariable, TypeVariableKind, + Type, TypeBinding, TypeBindings, }; use acvm::{acir::AcirField, FieldElement}; use iter_extended::{btree_map, try_vecmap, vecmap}; @@ -1788,24 +1788,21 @@ pub fn perform_impl_bindings( if let Some(trait_method) = trait_method { let the_trait = interner.get_trait(trait_method.trait_id); - let trait_method_type = the_trait.methods[trait_method.method_index].typ.as_monotype(); + let mut trait_method_type = + the_trait.methods[trait_method.method_index].typ.as_monotype().clone(); + + let mut impl_method_type = + interner.function_meta(&impl_method).typ.unwrap_forall().1.clone(); // Make each NamedGeneric in this type bindable by replacing it with a TypeVariable // with the same internal id and binding. - let (generics, impl_method_type) = interner.function_meta(&impl_method).typ.unwrap_forall(); - - let replace_type_variable = |var: &TypeVariable| { - (var.id(), (var.clone(), Type::TypeVariable(var.clone(), TypeVariableKind::Normal))) - }; - - // Replace each NamedGeneric with a TypeVariable containing the same internal type variable - let type_bindings = generics.iter().map(replace_type_variable).collect(); - let impl_method_type = impl_method_type.force_substitute(&type_bindings); + trait_method_type.replace_named_generics_with_type_variables(); + impl_method_type.replace_named_generics_with_type_variables(); trait_method_type.try_unify(&impl_method_type, &mut bindings).map_err(|_| { InterpreterError::ImplMethodTypeMismatch { - expected: trait_method_type.clone(), - actual: impl_method_type, + expected: trait_method_type.follow_bindings(), + actual: impl_method_type.follow_bindings(), location, } })?; diff --git a/test_programs/compile_success_empty/regression_5428/Nargo.toml b/test_programs/compile_success_empty/regression_5428/Nargo.toml new file mode 100644 index 00000000000..7507b934d66 --- /dev/null +++ b/test_programs/compile_success_empty/regression_5428/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "regression_5428" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/compile_success_empty/regression_5428/src/main.nr b/test_programs/compile_success_empty/regression_5428/src/main.nr new file mode 100644 index 00000000000..f01b89cbea4 --- /dev/null +++ b/test_programs/compile_success_empty/regression_5428/src/main.nr @@ -0,0 +1,9 @@ +fn main() { + assert_true!(); +} + +comptime fn assert_true() -> Quoted { + let first = quote { assert( }; + let second = quote { true); }; + first.append(second) +} diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 2681f2501cd..5e255da135d 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -61,7 +61,7 @@ const IGNORED_BRILLIG_TESTS: [&str; 11] = [ /// Certain features are only available in the elaborator. /// We skip these tests for non-elaborator code since they are not /// expected to work there. This can be removed once the old code is removed. -const IGNORED_NEW_FEATURE_TESTS: [&str; 10] = [ +const IGNORED_NEW_FEATURE_TESTS: [&str; 11] = [ "macros", "wildcard_type", "type_definition_annotation", @@ -71,6 +71,7 @@ const IGNORED_NEW_FEATURE_TESTS: [&str; 10] = [ "comptime_slice_methods", "unary_operator_overloading", "unquote_multiple_items_from_annotation", + "regression_5428", "unquote", ];