From 39610af5b3cc8de7e3aa963a2cbff3083179cbf4 Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Wed, 2 Aug 2023 01:28:09 -0700 Subject: [PATCH 1/2] chore(noirc_driver): Unify crate preparation (#2119) --- crates/lsp/src/lib.rs | 6 +++--- crates/lsp/src/lib_hacky.rs | 7 +++---- crates/nargo_cli/src/cli/mod.rs | 4 ++-- crates/nargo_cli/src/lib.rs | 7 +++---- crates/noirc_driver/src/lib.rs | 29 +++-------------------------- crates/wasm/src/compile.rs | 8 ++++---- 6 files changed, 18 insertions(+), 43 deletions(-) diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index bd4112218e4..1c02c802808 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -22,7 +22,7 @@ use lsp_types::{ InitializeParams, InitializeResult, InitializedParams, Position, PublishDiagnosticsParams, Range, ServerCapabilities, TextDocumentSyncOptions, }; -use noirc_driver::{check_crate, create_local_crate}; +use noirc_driver::{check_crate, prepare_crate}; use noirc_errors::{DiagnosticKind, FileDiagnostic}; use noirc_frontend::{ graph::{CrateGraph, CrateType}, @@ -190,7 +190,7 @@ fn on_code_lens_request( } }; - let crate_id = create_local_crate(&mut context, file_path, CrateType::Binary); + let crate_id = prepare_crate(&mut context, file_path, CrateType::Binary); // We ignore the warnings and errors produced by compilation for producing codelenses // because we can still get the test functions even if compilation fails @@ -283,7 +283,7 @@ fn on_did_save_text_document( } }; - let crate_id = create_local_crate(&mut context, file_path, CrateType::Binary); + let crate_id = prepare_crate(&mut context, file_path, CrateType::Binary); let mut diagnostics = Vec::new(); diff --git a/crates/lsp/src/lib_hacky.rs b/crates/lsp/src/lib_hacky.rs index 72a2625fcac..13bb2b82847 100644 --- a/crates/lsp/src/lib_hacky.rs +++ b/crates/lsp/src/lib_hacky.rs @@ -19,7 +19,7 @@ use lsp_types::{ InitializedParams, Position, PublishDiagnosticsParams, Range, ServerCapabilities, TextDocumentSyncOptions, }; -use noirc_driver::{check_crate, create_local_crate, create_non_local_crate, propagate_dep}; +use noirc_driver::{check_crate, prepare_crate, propagate_dep}; use noirc_errors::{DiagnosticKind, FileDiagnostic}; use noirc_frontend::{ graph::{CrateGraph, CrateId, CrateType}, @@ -286,7 +286,7 @@ fn create_context_at_path( } let nargo_toml_path = find_nearest_parent_file(&file_path, &["Nargo.toml"]); - let current_crate_id = create_local_crate(&mut context, &file_path, CrateType::Binary); + let current_crate_id = prepare_crate(&mut context, &file_path, CrateType::Binary); // TODO(AD): undo hacky dependency resolution if let Some(nargo_toml_path) = nargo_toml_path { @@ -297,8 +297,7 @@ fn create_context_at_path( .parent() .unwrap() // TODO .join(PathBuf::from(&dependency_path).join("src").join("lib.nr")); - let library_crate = - create_non_local_crate(&mut context, &path_to_lib, CrateType::Library); + let library_crate = prepare_crate(&mut context, &path_to_lib, CrateType::Library); propagate_dep(&mut context, library_crate, &crate_name.parse().unwrap()); } } diff --git a/crates/nargo_cli/src/cli/mod.rs b/crates/nargo_cli/src/cli/mod.rs index 8ce66db1b7b..9d494b21e6a 100644 --- a/crates/nargo_cli/src/cli/mod.rs +++ b/crates/nargo_cli/src/cli/mod.rs @@ -92,7 +92,7 @@ pub fn start_cli() -> eyre::Result<()> { #[cfg(test)] mod tests { use fm::FileManager; - use noirc_driver::{check_crate, create_local_crate}; + use noirc_driver::{check_crate, prepare_crate}; use noirc_errors::reporter; use noirc_frontend::{ graph::{CrateGraph, CrateType}, @@ -110,7 +110,7 @@ mod tests { let fm = FileManager::new(root_dir); let graph = CrateGraph::default(); let mut context = Context::new(fm, graph); - let crate_id = create_local_crate(&mut context, root_file, CrateType::Binary); + let crate_id = prepare_crate(&mut context, root_file, CrateType::Binary); let result = check_crate(&mut context, crate_id, false); let success = result.is_ok(); diff --git a/crates/nargo_cli/src/lib.rs b/crates/nargo_cli/src/lib.rs index b456d31c0ca..05753f7f3d8 100644 --- a/crates/nargo_cli/src/lib.rs +++ b/crates/nargo_cli/src/lib.rs @@ -9,7 +9,7 @@ use fm::FileManager; use nargo::package::{Dependency, Package}; -use noirc_driver::{add_dep, create_local_crate, create_non_local_crate}; +use noirc_driver::{add_dep, prepare_crate}; use noirc_frontend::{ graph::{CrateGraph, CrateId, CrateName, CrateType}, hir::Context, @@ -107,8 +107,7 @@ fn prepare_dependencies( for (dep_name, dep) in dependencies.into_iter() { match dep { Dependency::Remote { package } | Dependency::Local { package } => { - let crate_id = - create_non_local_crate(context, &package.entry_path, package.crate_type); + let crate_id = prepare_crate(context, &package.entry_path, package.crate_type); add_dep(context, parent_crate, crate_id, dep_name); prepare_dependencies(context, crate_id, package.dependencies.to_owned()); } @@ -121,7 +120,7 @@ fn prepare_package(package: &Package) -> (Context, CrateId) { let graph = CrateGraph::default(); let mut context = Context::new(fm, graph); - let crate_id = create_local_crate(&mut context, &package.entry_path, package.crate_type); + let crate_id = prepare_crate(&mut context, &package.entry_path, package.crate_type); prepare_dependencies(&mut context, crate_id, package.dependencies.to_owned()); diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index c0957313f69..4d1b7fe2675 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -52,40 +52,17 @@ pub fn compile_file( context: &mut Context, root_file: &Path, ) -> Result<(CompiledProgram, Warnings), ErrorsAndWarnings> { - let crate_id = create_local_crate(context, root_file, CrateType::Binary); + let crate_id = prepare_crate(context, root_file, CrateType::Binary); compile_main(context, crate_id, &CompileOptions::default()) } -/// Adds the File with the local crate root to the file system -/// and adds the local crate to the graph -/// XXX: This may pose a problem with workspaces, where you can change the local crate and where -/// we have multiple binaries in one workspace -/// A Fix would be for the driver instance to store the local crate id. -// Granted that this is the only place which relies on the local crate being first -pub fn create_local_crate( - context: &mut Context, - file_name: &Path, - crate_type: CrateType, -) -> CrateId { +/// Adds the file from the file system at `Path` to the crate graph +pub fn prepare_crate(context: &mut Context, file_name: &Path, crate_type: CrateType) -> CrateId { let root_file_id = context.file_manager.add_file(file_name).unwrap(); context.crate_graph.add_crate_root(crate_type, root_file_id) } -/// Creates a Non Local Crate. A Non Local Crate is any crate which is the not the crate that -/// the compiler is compiling. -pub fn create_non_local_crate( - context: &mut Context, - file_name: &Path, - crate_type: CrateType, -) -> CrateId { - let root_file_id = context.file_manager.add_file(file_name).unwrap(); - - // You can add any crate type to the crate graph - // but you cannot depend on Binaries - context.crate_graph.add_crate_root(crate_type, root_file_id) -} - /// Adds a edge in the crate graph for two crates pub fn add_dep( context: &mut Context, diff --git a/crates/wasm/src/compile.rs b/crates/wasm/src/compile.rs index c940f0ce246..15d8d5107ea 100644 --- a/crates/wasm/src/compile.rs +++ b/crates/wasm/src/compile.rs @@ -3,8 +3,8 @@ use fm::FileManager; use gloo_utils::format::JsValueSerdeExt; use log::debug; use noirc_driver::{ - check_crate, compile_contracts, compile_no_check, create_local_crate, create_non_local_crate, - propagate_dep, CompileOptions, CompiledContract, + check_crate, compile_contracts, compile_no_check, prepare_crate, propagate_dep, CompileOptions, + CompiledContract, }; use noirc_frontend::{ graph::{CrateGraph, CrateType}, @@ -63,7 +63,7 @@ impl Default for WASMCompileOptions { fn add_noir_lib(context: &mut Context, crate_name: &str) { let path_to_lib = Path::new(&crate_name).join("lib.nr"); - let library_crate = create_non_local_crate(context, &path_to_lib, CrateType::Library); + let library_crate = prepare_crate(context, &path_to_lib, CrateType::Library); propagate_dep(context, library_crate, &crate_name.parse().unwrap()); } @@ -87,7 +87,7 @@ pub fn compile(args: JsValue) -> JsValue { let mut context = Context::new(fm, graph); let path = Path::new(&options.entry_point); - let crate_id = create_local_crate(&mut context, path, CrateType::Binary); + let crate_id = prepare_crate(&mut context, path, CrateType::Binary); for dependency in options.optional_dependencies_set { add_noir_lib(&mut context, dependency.as_str()); From f7742ab026092f129bd4ec4f122bcd3249100529 Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 2 Aug 2023 03:59:08 -0500 Subject: [PATCH 2/2] fix: flattening pass no longer overwrites previously mapped condition values (#2117) * Fix flattening pass overwriting previously mapped values * chore: add backticks to variable names in comment --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../test_data/regression_2099/Nargo.toml | 6 +++ .../test_data/regression_2099/src/main.nr | 37 +++++++++++++++++++ .../src/ssa_refactor/ir/function_inserter.rs | 1 - .../src/ssa_refactor/opt/flatten_cfg.rs | 5 ++- 4 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 crates/nargo_cli/tests/test_data/regression_2099/Nargo.toml create mode 100644 crates/nargo_cli/tests/test_data/regression_2099/src/main.nr diff --git a/crates/nargo_cli/tests/test_data/regression_2099/Nargo.toml b/crates/nargo_cli/tests/test_data/regression_2099/Nargo.toml new file mode 100644 index 00000000000..ca96e7164a5 --- /dev/null +++ b/crates/nargo_cli/tests/test_data/regression_2099/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "regression_2099" +authors = [""] +compiler_version = "0.9.0" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data/regression_2099/src/main.nr b/crates/nargo_cli/tests/test_data/regression_2099/src/main.nr new file mode 100644 index 00000000000..b96e664dedf --- /dev/null +++ b/crates/nargo_cli/tests/test_data/regression_2099/src/main.nr @@ -0,0 +1,37 @@ +use dep::std::ec::tecurve::affine::Curve as AffineCurve; +use dep::std::ec::tecurve::affine::Point as Gaffine; +use dep::std::ec::tecurve::curvegroup::Curve; +use dep::std::ec::tecurve::curvegroup::Point as G; + +use dep::std::ec::swcurve::affine::Point as SWGaffine; +use dep::std::ec::swcurve::curvegroup::Point as SWG; + +use dep::std::ec::montcurve::affine::Point as MGaffine; +use dep::std::ec::montcurve::curvegroup::Point as MG; + +fn main() { + // Define Baby Jubjub (ERC-2494) parameters in affine representation + let bjj_affine = AffineCurve::new(168700, 168696, Gaffine::new(995203441582195749578291179787384436505546430278305826713579947235728471134,5472060717959818805561601436314318772137091100104008585924551046643952123905)); + + // Test addition + let p1_affine = Gaffine::new(17777552123799933955779906779655732241715742912184938656739573121738514868268, 2626589144620713026669568689430873010625803728049924121243784502389097019475); + let p2_affine = Gaffine::new(16540640123574156134436876038791482806971768689494387082833631921987005038935, 20819045374670962167435360035096875258406992893633759881276124905556507972311); + let _p3_affine = bjj_affine.add(p1_affine, p2_affine); + + // Test SWCurve equivalents of the above + // First the affine representation + let bjj_swcurve_affine = bjj_affine.into_swcurve(); + + let p1_swcurve_affine = bjj_affine.map_into_swcurve(p1_affine); + let p2_swcurve_affine = bjj_affine.map_into_swcurve(p2_affine); + + let _p3_swcurve_affine_from_add = bjj_swcurve_affine.add( + p1_swcurve_affine, + p2_swcurve_affine + ); + + // Check that these points are on the curve + assert( + bjj_swcurve_affine.contains(p1_swcurve_affine) + ); +} diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/function_inserter.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/function_inserter.rs index 38dcfbbb168..15c755f40c2 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/function_inserter.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/function_inserter.rs @@ -124,7 +124,6 @@ impl<'f> FunctionInserter<'f> { let old_parameters = self.function.dfg.block_parameters(block); for (param, new_param) in old_parameters.iter().zip(new_values) { - // Don't overwrite any existing entries to avoid overwriting the induction variable self.values.entry(*param).or_insert(*new_param); } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs index 4ff857f942f..fdc4be085d7 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/flatten_cfg.rs @@ -274,7 +274,10 @@ impl<'f> Context<'f> { // end, in addition to resetting the value of old_condition since it is set to // known to be true/false within the then/else branch respectively. self.insert_current_side_effects_enabled(); - self.inserter.map_value(old_condition, old_condition); + + // We must map back to `then_condition` here. Mapping `old_condition` to itself would + // lose any previous mappings. + self.inserter.map_value(old_condition, then_condition); // While there is a condition on the stack we don't compile outside the condition // until it is popped. This ensures we inline the full then and else branches