From 0bb7b53a5797553e8e652be81bd8203f44c4a49f Mon Sep 17 00:00:00 2001 From: ludamad Date: Wed, 5 Jul 2023 18:15:02 +0200 Subject: [PATCH 1/9] fix: workaround for #1838 This is currently not ideal in form but should inform a larger fix --- Cargo.lock | 1 + crates/fm/src/lib.rs | 2 +- crates/lsp/Cargo.toml | 1 + crates/lsp/src/lib.rs | 112 ++++++++++++++++++++++++++++++++++-------- 4 files changed, 95 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1cc8bc4a8c5..7fb07512d23 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2008,6 +2008,7 @@ dependencies = [ "noirc_frontend", "serde_json", "tokio", + "toml", "tower", ] diff --git a/crates/fm/src/lib.rs b/crates/fm/src/lib.rs index b79a2b2c700..efc0ba51030 100644 --- a/crates/fm/src/lib.rs +++ b/crates/fm/src/lib.rs @@ -70,7 +70,7 @@ impl FileManager { // Unwrap as we ensure that all file_id's map to a corresponding file in the file map self.file_map.get_file(file_id).unwrap() } - fn path(&mut self, file_id: FileId) -> &Path { + pub fn path(&self, file_id: FileId) -> &Path { // Unwrap as we ensure that all file_ids are created by the file manager // So all file_ids will points to a corresponding path self.id_to_path.get(&file_id).unwrap().0.as_path() diff --git a/crates/lsp/Cargo.toml b/crates/lsp/Cargo.toml index a1d4a295b9b..47278ab5291 100644 --- a/crates/lsp/Cargo.toml +++ b/crates/lsp/Cargo.toml @@ -16,6 +16,7 @@ noirc_driver.workspace = true noirc_errors.workspace = true noirc_frontend.workspace = true serde_json.workspace = true +toml.workspace = true tower.workspace = true async-lsp = { version = "0.0.4", default-features = false, features = ["omni-trait"] } diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index 7b99e5ac02b..08d74551623 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -1,6 +1,9 @@ use std::{ + collections::HashMap, + fs, future::Future, ops::{self, ControlFlow}, + path::{Path, PathBuf}, pin::Pin, task::{Context, Poll}, }; @@ -20,7 +23,7 @@ use lsp_types::{ }; use noirc_driver::Driver; use noirc_errors::{DiagnosticKind, FileDiagnostic}; -use noirc_frontend::graph::CrateType; +use noirc_frontend::graph::{CrateName, CrateType}; use serde_json::Value as JsonValue; use tower::Service; @@ -134,13 +137,8 @@ fn on_code_lens_request( params: CodeLensParams, ) -> impl Future>, ResponseError>> { async move { - // TODO: Requiring `Language` and `is_opcode_supported` to construct a driver makes for some real stinky code - // The driver should not require knowledge of the backend; instead should be implemented as an independent pass (in nargo?) - let mut driver = Driver::new(&Language::R1CS, Box::new(|_op| false)); - - let file_path = ¶ms.text_document.uri.to_file_path().unwrap(); - - driver.create_local_crate(file_path, CrateType::Binary); + let actual_path = params.text_document.uri.to_file_path().unwrap(); + let mut driver = create_driver_at_path(actual_path); // We ignore the warnings and errors produced by compilation for producing codelenses // because we can still get the test functions even if compilation fails @@ -218,33 +216,76 @@ fn on_did_close_text_document( ControlFlow::Continue(()) } +/// Find the nearest parent file with given names. +fn find_nearest_parent_file(path: &Path, filenames: &[&str]) -> Option { + let mut current_path = path; + + while let Some(parent_path) = current_path.parent() { + for filename in filenames { + let mut possible_file_path = parent_path.to_path_buf(); + possible_file_path.push(filename); + if possible_file_path.is_file() { + return Some(possible_file_path); + } + } + current_path = parent_path; + } + + None +} + +fn read_dependencies( + nargo_toml_path: &Path, +) -> Result, Box> { + let content: String = fs::read_to_string(nargo_toml_path)?; + let value: toml::Value = toml::from_str(&content)?; + + let mut dependencies = HashMap::new(); + + if let Some(toml::Value::Table(table)) = value.get("dependencies") { + for (key, value) in table { + if let toml::Value::Table(inner_table) = value { + if let Some(toml::Value::String(path)) = inner_table.get("path") { + dependencies.insert(key.clone(), path.clone()); + } + } + } + } + + Ok(dependencies) +} + fn on_did_save_text_document( state: &mut LspState, params: DidSaveTextDocumentParams, ) -> ControlFlow> { - // TODO: Requiring `Language` and `is_opcode_supported` to construct a driver makes for some real stinky code - // The driver should not require knowledge of the backend; instead should be implemented as an independent pass (in nargo?) - let mut driver = Driver::new(&Language::R1CS, Box::new(|_op| false)); - - let file_path = ¶ms.text_document.uri.to_file_path().unwrap(); - - driver.create_local_crate(file_path, CrateType::Binary); - - let mut diagnostics = Vec::new(); + let actual_path = params.text_document.uri.to_file_path().unwrap(); + let mut driver = create_driver_at_path(actual_path.clone()); let file_diagnostics = match driver.check_crate(false) { Ok(warnings) => warnings, Err(errors_and_warnings) => errors_and_warnings, }; + let mut diagnostics = Vec::new(); if !file_diagnostics.is_empty() { let fm = driver.file_manager(); let files = fm.as_simple_files(); for FileDiagnostic { file_id, diagnostic } in file_diagnostics { - // TODO(#1681): This file_id never be 0 because the "path" where it maps is the directory, not a file - if file_id.as_usize() != 0 { - continue; + // TODO(AD): HACK, undo these total hacks once we have a proper approach + if file_id.as_usize() == 0 { + // main.nr case + if actual_path.file_name().unwrap().to_str() != Some("main.nr") + && actual_path.file_name().unwrap().to_str() != Some("lib.nr") + { + continue; + } + } else if fm.path(file_id).file_name().unwrap().to_str().unwrap() + != actual_path.file_name().unwrap().to_str().unwrap().replace(".nr", "") + { + // every other file case + continue; // TODO(AD): HACK, we list all errors, filter by hacky final path component } let mut range = Range::default(); @@ -278,6 +319,37 @@ fn on_did_save_text_document( ControlFlow::Continue(()) } +fn create_driver_at_path(actual_path: PathBuf) -> Driver { + // TODO: Requiring `Language` and `is_opcode_supported` to construct a driver makes for some real stinky code + // The driver should not require knowledge of the backend; instead should be implemented as an independent pass (in nargo?) + let mut driver = Driver::new(&Language::R1CS, Box::new(|_op| false)); + + let mut file_path: PathBuf = actual_path; + // TODO better naming/unhacking + if let Some(new_path) = find_nearest_parent_file(&file_path, &["lib.nr", "main.nr"]) { + file_path = new_path.clone(); // TODO unhack + } + let nargo_toml_path = find_nearest_parent_file(&file_path, &["Nargo.toml"]); + + driver.create_local_crate(file_path, CrateType::Binary); + + // TODO(AD): undo hacky dependency resolution + if let Some(nargo_toml_path) = nargo_toml_path { + let dependencies = read_dependencies(&nargo_toml_path); + if let Ok(dependencies) = dependencies { + for (crate_name, dependency_path) in dependencies.iter() { + let path_to_lib = nargo_toml_path + .parent() + .unwrap() // TODO + .join(PathBuf::from(&dependency_path).join("src").join("lib.nr")); + let library_crate = driver.create_non_local_crate(path_to_lib, CrateType::Library); + driver.propagate_dep(library_crate, &CrateName::new(crate_name).unwrap()); + } + } + } + driver +} + fn on_exit(_state: &mut LspState, _params: ()) -> ControlFlow> { ControlFlow::Continue(()) } From a71dfd3b92c51155d895ca23723f55da9457bd61 Mon Sep 17 00:00:00 2001 From: ludamad Date: Wed, 5 Jul 2023 18:22:55 +0200 Subject: [PATCH 2/9] fix: cargo check --- crates/lsp/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index 08d74551623..6a58f4488f8 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -327,7 +327,7 @@ fn create_driver_at_path(actual_path: PathBuf) -> Driver { let mut file_path: PathBuf = actual_path; // TODO better naming/unhacking if let Some(new_path) = find_nearest_parent_file(&file_path, &["lib.nr", "main.nr"]) { - file_path = new_path.clone(); // TODO unhack + file_path = new_path; // TODO unhack } let nargo_toml_path = find_nearest_parent_file(&file_path, &["Nargo.toml"]); From 158f88f1dbc08b7b77b42dc15926d8e7ca50dc29 Mon Sep 17 00:00:00 2001 From: ludamad Date: Thu, 6 Jul 2023 14:35:48 +0200 Subject: [PATCH 3/9] Fix hacked path --- crates/lsp/src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index 6a58f4488f8..0036733e5ff 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -284,6 +284,11 @@ fn on_did_save_text_document( } else if fm.path(file_id).file_name().unwrap().to_str().unwrap() != actual_path.file_name().unwrap().to_str().unwrap().replace(".nr", "") { + if actual_path.file_name().unwrap().to_str() == Some("main.nr") + || actual_path.file_name().unwrap().to_str() == Some("lib.nr") + { + continue; + } // every other file case continue; // TODO(AD): HACK, we list all errors, filter by hacky final path component } From 45d1abe4bc15896f5302926a57e997af47bc17f1 Mon Sep 17 00:00:00 2001 From: ludamad Date: Thu, 6 Jul 2023 14:37:13 +0200 Subject: [PATCH 4/9] Fix hacked path --- crates/lsp/src/lib.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index 0036733e5ff..ecd97838766 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -281,14 +281,11 @@ fn on_did_save_text_document( { continue; } - } else if fm.path(file_id).file_name().unwrap().to_str().unwrap() - != actual_path.file_name().unwrap().to_str().unwrap().replace(".nr", "") + } else if (fm.path(file_id).file_name().unwrap().to_str().unwrap() + != actual_path.file_name().unwrap().to_str().unwrap().replace(".nr", "")) + || actual_path.file_name().unwrap().to_str() == Some("main.nr") + || actual_path.file_name().unwrap().to_str() == Some("lib.nr") { - if actual_path.file_name().unwrap().to_str() == Some("main.nr") - || actual_path.file_name().unwrap().to_str() == Some("lib.nr") - { - continue; - } // every other file case continue; // TODO(AD): HACK, we list all errors, filter by hacky final path component } From b0421a9828d740c5301b2955b83e7e12c488c5ab Mon Sep 17 00:00:00 2001 From: ludamad Date: Thu, 6 Jul 2023 14:43:19 +0200 Subject: [PATCH 5/9] Fix hacked path --- crates/lsp/src/lib.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index ecd97838766..6a58f4488f8 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -281,10 +281,8 @@ fn on_did_save_text_document( { continue; } - } else if (fm.path(file_id).file_name().unwrap().to_str().unwrap() - != actual_path.file_name().unwrap().to_str().unwrap().replace(".nr", "")) - || actual_path.file_name().unwrap().to_str() == Some("main.nr") - || actual_path.file_name().unwrap().to_str() == Some("lib.nr") + } else if fm.path(file_id).file_name().unwrap().to_str().unwrap() + != actual_path.file_name().unwrap().to_str().unwrap().replace(".nr", "") { // every other file case continue; // TODO(AD): HACK, we list all errors, filter by hacky final path component From 87baebf119ee41701bcfff2265d81902e512d066 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Thu, 6 Jul 2023 17:32:23 +0200 Subject: [PATCH 6/9] feat: make array indexes polymophic integers (#1877) * feat: make array indexes poly int * Update crates/noirc_frontend/src/hir/type_check/expr.rs * Update crates/noirc_frontend/src/hir/type_check/stmt.rs --------- Co-authored-by: jfecher --- .../integer_array_indexing/Nargo.toml | 5 +++++ .../integer_array_indexing/Prover.toml | 2 ++ .../integer_array_indexing/src/main.nr | 14 ++++++++++++++ crates/noirc_frontend/src/hir/type_check/expr.rs | 4 ++-- crates/noirc_frontend/src/hir/type_check/stmt.rs | 6 +++--- 5 files changed, 26 insertions(+), 5 deletions(-) create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/Nargo.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/Prover.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/src/main.nr diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/Nargo.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/Nargo.toml new file mode 100644 index 00000000000..e0b467ce5da --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/Nargo.toml @@ -0,0 +1,5 @@ +[package] +authors = [""] +compiler_version = "0.1" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/Prover.toml new file mode 100644 index 00000000000..1496028f60a --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/Prover.toml @@ -0,0 +1,2 @@ +arr = [1, 2, 3] +x = 2 diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/src/main.nr new file mode 100644 index 00000000000..e89e2ffbb80 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/integer_array_indexing/src/main.nr @@ -0,0 +1,14 @@ +use dep::std; + +global ARRAY_LEN: u32 = 3; + +fn main(arr: [Field; ARRAY_LEN], x: u32) -> pub Field { + + let mut value = arr[ARRAY_LEN - 1]; + + value += arr[0 as u32]; + value += arr[1 as Field]; + + value + (x as Field) + +} diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 03f4ef09a92..b310215a2f4 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -307,9 +307,9 @@ impl<'interner> TypeChecker<'interner> { let index_type = self.check_expression(&index_expr.index); let span = self.interner.expr_span(&index_expr.index); - index_type.make_subtype_of(&Type::field(Some(span)), span, &mut self.errors, || { + index_type.unify(&Type::polymorphic_integer(self.interner), span, &mut self.errors, || { TypeCheckError::TypeMismatch { - expected_typ: "Field".to_owned(), + expected_typ: "an integer".to_owned(), expr_typ: index_type.to_string(), expr_span: span, } diff --git a/crates/noirc_frontend/src/hir/type_check/stmt.rs b/crates/noirc_frontend/src/hir/type_check/stmt.rs index 5606547a568..a6d7442d98a 100644 --- a/crates/noirc_frontend/src/hir/type_check/stmt.rs +++ b/crates/noirc_frontend/src/hir/type_check/stmt.rs @@ -177,12 +177,12 @@ impl<'interner> TypeChecker<'interner> { let index_type = self.check_expression(&index); let expr_span = self.interner.expr_span(&index); - index_type.make_subtype_of( - &Type::field(Some(expr_span)), + index_type.unify( + &Type::polymorphic_integer(self.interner), expr_span, &mut self.errors, || TypeCheckError::TypeMismatch { - expected_typ: "Field".to_owned(), + expected_typ: "an integer".to_owned(), expr_typ: index_type.to_string(), expr_span, }, From f1bbdc2660efa0064c95a7de0c60fd1471a6e348 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Thu, 6 Jul 2023 16:03:31 +0000 Subject: [PATCH 7/9] Make proving and verification keys optional --- crates/nargo/src/artifacts/contract.rs | 4 ++-- crates/nargo/src/artifacts/program.rs | 4 ++-- crates/nargo/src/ops/preprocess.rs | 21 +++++++++++++++---- .../nargo_cli/src/cli/codegen_verifier_cmd.rs | 7 +++++-- crates/nargo_cli/src/cli/compile_cmd.rs | 18 ++++++++++++---- crates/nargo_cli/src/cli/mod.rs | 7 +++++-- crates/nargo_cli/src/cli/prove_cmd.rs | 7 ++++++- crates/nargo_cli/src/cli/verify_cmd.rs | 4 +++- 8 files changed, 54 insertions(+), 18 deletions(-) diff --git a/crates/nargo/src/artifacts/contract.rs b/crates/nargo/src/artifacts/contract.rs index 95f1ce9576d..4174207692f 100644 --- a/crates/nargo/src/artifacts/contract.rs +++ b/crates/nargo/src/artifacts/contract.rs @@ -36,6 +36,6 @@ pub struct PreprocessedContractFunction { )] pub bytecode: Circuit, - pub proving_key: Vec, - pub verification_key: Vec, + pub proving_key: Option>, + pub verification_key: Option>, } diff --git a/crates/nargo/src/artifacts/program.rs b/crates/nargo/src/artifacts/program.rs index 288a5dba99b..6ca49b35dd9 100644 --- a/crates/nargo/src/artifacts/program.rs +++ b/crates/nargo/src/artifacts/program.rs @@ -18,6 +18,6 @@ pub struct PreprocessedProgram { )] pub bytecode: Circuit, - pub proving_key: Vec, - pub verification_key: Vec, + pub proving_key: Option>, + pub verification_key: Option>, } diff --git a/crates/nargo/src/ops/preprocess.rs b/crates/nargo/src/ops/preprocess.rs index b545980b963..90364bec603 100644 --- a/crates/nargo/src/ops/preprocess.rs +++ b/crates/nargo/src/ops/preprocess.rs @@ -8,14 +8,21 @@ const BACKEND_IDENTIFIER: &str = "acvm-backend-barretenberg"; pub fn preprocess_program( backend: &B, + include_keys: bool, common_reference_string: &[u8], compiled_program: CompiledProgram, ) -> Result { // TODO: currently `compiled_program`'s bytecode is already optimized for the backend. // In future we'll need to apply those optimizations here. let optimized_bytecode = compiled_program.circuit; - let (proving_key, verification_key) = - backend.preprocess(common_reference_string, &optimized_bytecode)?; + + let (proving_key, verification_key) = if include_keys { + let (proving_key, verification_key) = + backend.preprocess(common_reference_string, &optimized_bytecode)?; + (Some(proving_key), Some(verification_key)) + } else { + (None, None) + }; Ok(PreprocessedProgram { backend: String::from(BACKEND_IDENTIFIER), @@ -28,14 +35,20 @@ pub fn preprocess_program( pub fn preprocess_contract_function( backend: &B, + include_keys: bool, common_reference_string: &[u8], func: ContractFunction, ) -> Result { // TODO: currently `func`'s bytecode is already optimized for the backend. // In future we'll need to apply those optimizations here. let optimized_bytecode = func.bytecode; - let (proving_key, verification_key) = - backend.preprocess(common_reference_string, &optimized_bytecode)?; + let (proving_key, verification_key) = if include_keys { + let (proving_key, verification_key) = + backend.preprocess(common_reference_string, &optimized_bytecode)?; + (Some(proving_key), Some(verification_key)) + } else { + (None, None) + }; Ok(PreprocessedContractFunction { name: func.name, diff --git a/crates/nargo_cli/src/cli/codegen_verifier_cmd.rs b/crates/nargo_cli/src/cli/codegen_verifier_cmd.rs index b65d64bb917..ae6fa411c7c 100644 --- a/crates/nargo_cli/src/cli/codegen_verifier_cmd.rs +++ b/crates/nargo_cli/src/cli/codegen_verifier_cmd.rs @@ -56,14 +56,17 @@ pub(crate) fn run( let common_reference_string = update_common_reference_string(backend, &common_reference_string, &program.circuit) .map_err(CliError::CommonReferenceStringError)?; - let program = preprocess_program(backend, &common_reference_string, program) + let program = preprocess_program(backend, true, &common_reference_string, program) .map_err(CliError::ProofSystemCompilerError)?; (common_reference_string, program) } }; + let verification_key = preprocessed_program + .verification_key + .expect("Verification key should exist as `true` is passed to `preprocess_program`"); let smart_contract_string = - codegen_verifier(backend, &common_reference_string, &preprocessed_program.verification_key) + codegen_verifier(backend, &common_reference_string, &verification_key) .map_err(CliError::SmartContractError)?; write_cached_common_reference_string(&common_reference_string); diff --git a/crates/nargo_cli/src/cli/compile_cmd.rs b/crates/nargo_cli/src/cli/compile_cmd.rs index 6d8fd8a2288..8c17cbe7866 100644 --- a/crates/nargo_cli/src/cli/compile_cmd.rs +++ b/crates/nargo_cli/src/cli/compile_cmd.rs @@ -30,6 +30,10 @@ pub(crate) struct CompileCommand { /// The name of the ACIR file circuit_name: String, + /// Include Proving and Verification keys in the build artifacts. + #[arg(long)] + include_keys: bool, + /// Compile each contract function used within the program #[arg(short, long)] contracts: bool, @@ -68,8 +72,13 @@ pub(crate) fn run( ) .map_err(CliError::CommonReferenceStringError)?; - preprocess_contract_function(backend, &common_reference_string, func) - .map_err(CliError::ProofSystemCompilerError) + preprocess_contract_function( + backend, + args.include_keys, + &common_reference_string, + func, + ) + .map_err(CliError::ProofSystemCompilerError) })?; Ok(PreprocessedContract { @@ -91,8 +100,9 @@ pub(crate) fn run( update_common_reference_string(backend, &common_reference_string, &program.circuit) .map_err(CliError::CommonReferenceStringError)?; - let preprocessed_program = preprocess_program(backend, &common_reference_string, program) - .map_err(CliError::ProofSystemCompilerError)?; + let preprocessed_program = + preprocess_program(backend, args.include_keys, &common_reference_string, program) + .map_err(CliError::ProofSystemCompilerError)?; save_program_to_file(&preprocessed_program, &args.circuit_name, circuit_dir); } diff --git a/crates/nargo_cli/src/cli/mod.rs b/crates/nargo_cli/src/cli/mod.rs index 0066d7c9135..3c290800801 100644 --- a/crates/nargo_cli/src/cli/mod.rs +++ b/crates/nargo_cli/src/cli/mod.rs @@ -110,8 +110,9 @@ pub fn prove_and_verify(program_dir: &Path, experimental_ssa: bool) -> bool { &program.circuit, ) .expect("Should fetch CRS"); - let preprocessed_program = preprocess_program(&backend, &common_reference_string, program) - .expect("Preprocess should succeed"); + let preprocessed_program = + preprocess_program(&backend, true, &common_reference_string, program) + .expect("Preprocess should succeed"); let nargo::artifacts::program::PreprocessedProgram { abi, @@ -120,6 +121,8 @@ pub fn prove_and_verify(program_dir: &Path, experimental_ssa: bool) -> bool { verification_key, .. } = preprocessed_program; + let proving_key = proving_key.unwrap(); + let verification_key = verification_key.unwrap(); // Parse the initial witness values from Prover.toml let (inputs_map, _) = fs::inputs::read_inputs_from_file( diff --git a/crates/nargo_cli/src/cli/prove_cmd.rs b/crates/nargo_cli/src/cli/prove_cmd.rs index 44f3bf62484..0200b417396 100644 --- a/crates/nargo_cli/src/cli/prove_cmd.rs +++ b/crates/nargo_cli/src/cli/prove_cmd.rs @@ -107,7 +107,7 @@ pub(crate) fn prove_with_path>( let common_reference_string = update_common_reference_string(backend, &common_reference_string, &program.circuit) .map_err(CliError::CommonReferenceStringError)?; - let program = preprocess_program(backend, &common_reference_string, program) + let program = preprocess_program(backend, true, &common_reference_string, program) .map_err(CliError::ProofSystemCompilerError)?; (common_reference_string, program) } @@ -137,12 +137,17 @@ pub(crate) fn prove_with_path>( Format::Toml, )?; + let proving_key = + proving_key.expect("Proving key should exist as `true` is passed to `preprocess_program`"); + let proof = prove_execution(backend, &common_reference_string, &bytecode, solved_witness, &proving_key) .map_err(CliError::ProofSystemCompilerError)?; if check_proof { let public_inputs = public_abi.encode(&public_inputs, return_value)?; + let verification_key = verification_key + .expect("Verification key should exist as `true` is passed to `preprocess_program`"); let valid_proof = verify_proof( backend, &common_reference_string, diff --git a/crates/nargo_cli/src/cli/verify_cmd.rs b/crates/nargo_cli/src/cli/verify_cmd.rs index e326aafbc52..c962e9fd081 100644 --- a/crates/nargo_cli/src/cli/verify_cmd.rs +++ b/crates/nargo_cli/src/cli/verify_cmd.rs @@ -87,7 +87,7 @@ fn verify_with_path>( let common_reference_string = update_common_reference_string(backend, &common_reference_string, &program.circuit) .map_err(CliError::CommonReferenceStringError)?; - let program = preprocess_program(backend, &common_reference_string, program) + let program = preprocess_program(backend, true, &common_reference_string, program) .map_err(CliError::ProofSystemCompilerError)?; (common_reference_string, program) } @@ -105,6 +105,8 @@ fn verify_with_path>( let public_inputs = public_abi.encode(&public_inputs_map, return_value)?; let proof = load_hex_data(&proof_path)?; + let verification_key = verification_key + .expect("Verification key should exist as `true` is passed to `preprocess_program`"); let valid_proof = verify_proof( backend, &common_reference_string, From b0cb5c0299278f0a59d8521c9da0c938f6c0b4d3 Mon Sep 17 00:00:00 2001 From: ludamad Date: Thu, 13 Jul 2023 10:45:40 -0400 Subject: [PATCH 8/9] More merge progress --- crates/lsp/src/lib.rs | 23 ++--------------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index 5f6a3c9122b..db04b004517 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -134,17 +134,8 @@ fn on_code_lens_request( _state: &mut LspState, params: CodeLensParams, ) -> impl Future>, ResponseError>> { -<<<<<<< HEAD - async move { - let actual_path = params.text_document.uri.to_file_path().unwrap(); - let mut driver = create_driver_at_path(actual_path); -======= - let file_path = ¶ms.text_document.uri.to_file_path().unwrap(); - - let mut context = Context::default(); - - let crate_id = create_local_crate(&mut context, file_path, CrateType::Binary); ->>>>>>> origin/master + let actual_path = params.text_document.uri.to_file_path().unwrap(); + let mut driver = create_driver_at_path(actual_path); // We ignore the warnings and errors produced by compilation for producing codelenses // because we can still get the test functions even if compilation fails @@ -266,18 +257,8 @@ fn on_did_save_text_document( state: &mut LspState, params: DidSaveTextDocumentParams, ) -> ControlFlow> { -<<<<<<< HEAD let actual_path = params.text_document.uri.to_file_path().unwrap(); let mut driver = create_driver_at_path(actual_path.clone()); -======= - let file_path = ¶ms.text_document.uri.to_file_path().unwrap(); - - let mut context = Context::default(); - - create_local_crate(&mut context, file_path, CrateType::Binary); - - let mut diagnostics = Vec::new(); ->>>>>>> origin/master let file_diagnostics = match check_crate(&mut context, false, false) { Ok(warnings) => warnings, From bb72cdeaa06af45f332025305f3dd1f368585574 Mon Sep 17 00:00:00 2001 From: ludamad Date: Thu, 13 Jul 2023 11:10:19 -0400 Subject: [PATCH 9/9] chore: introduce lib_hacky segregation --- crates/lsp/src/lib.rs | 140 ++++++---------- crates/lsp/src/lib_hacky.rs | 318 ++++++++++++++++++++++++++++++++++++ 2 files changed, 366 insertions(+), 92 deletions(-) create mode 100644 crates/lsp/src/lib_hacky.rs diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index db04b004517..95aae571fbb 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -1,9 +1,9 @@ +mod lib_hacky; +use std::env; + use std::{ - collections::HashMap, - fs, future::Future, ops::{self, ControlFlow}, - path::{Path, PathBuf}, pin::Pin, task::{self, Poll}, }; @@ -30,7 +30,7 @@ const TEST_COMMAND: &str = "nargo.test"; const TEST_CODELENS_TITLE: &str = "▶\u{fe0e} Run Test"; // State for the LSP gets implemented on this struct and is internal to the implementation -struct LspState { +pub struct LspState { client: ClientSocket, } @@ -46,6 +46,35 @@ pub struct NargoLspService { impl NargoLspService { pub fn new(client: &ClientSocket) -> Self { + // Using conditional running with lib_hacky to prevent non-hacky code from being identified as dead code + // Secondarily, provides a runtime way to stress the non-hacky code. + if env::var("NOIR_LSP_NO_HACK").is_err() { + let state = LspState::new(client); + let mut router = Router::new(state); + router + .request::(lib_hacky::on_initialize) + .request::(lib_hacky::on_shutdown) + .request::(lib_hacky::on_code_lens_request) + .notification::(lib_hacky::on_initialized) + .notification::( + lib_hacky::on_did_change_configuration, + ) + .notification::( + lib_hacky::on_did_open_text_document, + ) + .notification::( + lib_hacky::on_did_change_text_document, + ) + .notification::( + lib_hacky::on_did_close_text_document, + ) + .notification::( + lib_hacky::on_did_save_text_document, + ) + .notification::(lib_hacky::on_exit); + return Self { router }; + } + let state = LspState::new(client); let mut router = Router::new(state); router @@ -134,8 +163,11 @@ fn on_code_lens_request( _state: &mut LspState, params: CodeLensParams, ) -> impl Future>, ResponseError>> { - let actual_path = params.text_document.uri.to_file_path().unwrap(); - let mut driver = create_driver_at_path(actual_path); + let file_path = ¶ms.text_document.uri.to_file_path().unwrap(); + + let mut context = Context::default(); + + let crate_id = create_local_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 @@ -214,76 +246,31 @@ fn on_did_close_text_document( ControlFlow::Continue(()) } -/// Find the nearest parent file with given names. -fn find_nearest_parent_file(path: &Path, filenames: &[&str]) -> Option { - let mut current_path = path; - - while let Some(parent_path) = current_path.parent() { - for filename in filenames { - let mut possible_file_path = parent_path.to_path_buf(); - possible_file_path.push(filename); - if possible_file_path.is_file() { - return Some(possible_file_path); - } - } - current_path = parent_path; - } - - None -} - -fn read_dependencies( - nargo_toml_path: &Path, -) -> Result, Box> { - let content: String = fs::read_to_string(nargo_toml_path)?; - let value: toml::Value = toml::from_str(&content)?; - - let mut dependencies = HashMap::new(); - - if let Some(toml::Value::Table(table)) = value.get("dependencies") { - for (key, value) in table { - if let toml::Value::Table(inner_table) = value { - if let Some(toml::Value::String(path)) = inner_table.get("path") { - dependencies.insert(key.clone(), path.clone()); - } - } - } - } - - Ok(dependencies) -} - fn on_did_save_text_document( state: &mut LspState, params: DidSaveTextDocumentParams, ) -> ControlFlow> { - let actual_path = params.text_document.uri.to_file_path().unwrap(); - let mut driver = create_driver_at_path(actual_path.clone()); + let file_path = ¶ms.text_document.uri.to_file_path().unwrap(); + + let mut context = Context::default(); + + create_local_crate(&mut context, file_path, CrateType::Binary); + + let mut diagnostics = Vec::new(); let file_diagnostics = match check_crate(&mut context, false, false) { Ok(warnings) => warnings, Err(errors_and_warnings) => errors_and_warnings, }; - let mut diagnostics = Vec::new(); if !file_diagnostics.is_empty() { let fm = &context.file_manager; let files = fm.as_simple_files(); for FileDiagnostic { file_id, diagnostic } in file_diagnostics { - // TODO(AD): HACK, undo these total hacks once we have a proper approach - if file_id.as_usize() == 0 { - // main.nr case - if actual_path.file_name().unwrap().to_str() != Some("main.nr") - && actual_path.file_name().unwrap().to_str() != Some("lib.nr") - { - continue; - } - } else if fm.path(file_id).file_name().unwrap().to_str().unwrap() - != actual_path.file_name().unwrap().to_str().unwrap().replace(".nr", "") - { - // every other file case - continue; // TODO(AD): HACK, we list all errors, filter by hacky final path component + // TODO(#1681): This file_id never be 0 because the "path" where it maps is the directory, not a file + if file_id.as_usize() != 0 { + continue; } let mut range = Range::default(); @@ -317,37 +304,6 @@ fn on_did_save_text_document( ControlFlow::Continue(()) } -fn create_driver_at_path(actual_path: PathBuf) -> Driver { - // TODO: Requiring `Language` and `is_opcode_supported` to construct a driver makes for some real stinky code - // The driver should not require knowledge of the backend; instead should be implemented as an independent pass (in nargo?) - let mut driver = Driver::new(&Language::R1CS, Box::new(|_op| false)); - - let mut file_path: PathBuf = actual_path; - // TODO better naming/unhacking - if let Some(new_path) = find_nearest_parent_file(&file_path, &["lib.nr", "main.nr"]) { - file_path = new_path; // TODO unhack - } - let nargo_toml_path = find_nearest_parent_file(&file_path, &["Nargo.toml"]); - - driver.create_local_crate(file_path, CrateType::Binary); - - // TODO(AD): undo hacky dependency resolution - if let Some(nargo_toml_path) = nargo_toml_path { - let dependencies = read_dependencies(&nargo_toml_path); - if let Ok(dependencies) = dependencies { - for (crate_name, dependency_path) in dependencies.iter() { - let path_to_lib = nargo_toml_path - .parent() - .unwrap() // TODO - .join(PathBuf::from(&dependency_path).join("src").join("lib.nr")); - let library_crate = driver.create_non_local_crate(path_to_lib, CrateType::Library); - driver.propagate_dep(library_crate, &CrateName::new(crate_name).unwrap()); - } - } - } - driver -} - fn on_exit(_state: &mut LspState, _params: ()) -> ControlFlow> { ControlFlow::Continue(()) } diff --git a/crates/lsp/src/lib_hacky.rs b/crates/lsp/src/lib_hacky.rs new file mode 100644 index 00000000000..9d0e4c0c2dc --- /dev/null +++ b/crates/lsp/src/lib_hacky.rs @@ -0,0 +1,318 @@ +//! NOTE: This is a temporary module until https://github.com/noir-lang/noir/issues/1838 is fixed. +//! This is sectioned off, and currently the default implementation, unless the environment variable NOIR_LSP_NO_HACKS is set. +//! This is mainly so that non-hacky code is not considered dead. +use std::{ + collections::HashMap, + fs, + future::Future, + ops::{self, ControlFlow}, + path::{Path, PathBuf}, +}; + +use async_lsp::{LanguageClient, ResponseError}; +use codespan_reporting::files; +use lsp_types::{ + CodeLens, CodeLensOptions, CodeLensParams, Command, Diagnostic, DiagnosticSeverity, + DidChangeConfigurationParams, DidChangeTextDocumentParams, DidCloseTextDocumentParams, + DidOpenTextDocumentParams, DidSaveTextDocumentParams, InitializeParams, InitializeResult, + InitializedParams, Position, PublishDiagnosticsParams, Range, ServerCapabilities, + TextDocumentSyncOptions, +}; +use noirc_driver::{check_crate, create_local_crate, create_non_local_crate, propagate_dep}; +use noirc_errors::{DiagnosticKind, FileDiagnostic}; +use noirc_frontend::{ + graph::{CrateId, CrateName, CrateType}, + hir::Context, +}; + +use crate::LspState; + +const TEST_COMMAND: &str = "nargo.test"; +const TEST_CODELENS_TITLE: &str = "▶\u{fe0e} Run Test"; + +// Handlers +// The handlers for `request` are not `async` because it compiles down to lifetimes that can't be added to +// the router. To return a future that fits the trait, it is easiest wrap your implementations in an `async {}` +// block but you can also use `std::future::ready`. +// +// Additionally, the handlers for `notification` aren't async at all. +// +// They are not attached to the `NargoLspService` struct so they can be unit tested with only `LspState` +// and params passed in. + +pub fn on_initialize( + _state: &mut LspState, + _params: InitializeParams, +) -> impl Future> { + async { + let text_document_sync = + TextDocumentSyncOptions { save: Some(true.into()), ..Default::default() }; + + let code_lens = CodeLensOptions { resolve_provider: Some(false) }; + + Ok(InitializeResult { + capabilities: ServerCapabilities { + text_document_sync: Some(text_document_sync.into()), + code_lens_provider: Some(code_lens), + // Add capabilities before this spread when adding support for one + ..Default::default() + }, + server_info: None, + }) + } +} + +pub fn on_shutdown( + _state: &mut LspState, + _params: (), +) -> impl Future> { + async { Ok(()) } +} + +pub fn on_code_lens_request( + _state: &mut LspState, + params: CodeLensParams, +) -> impl Future>, ResponseError>> { + let actual_path = params.text_document.uri.to_file_path().unwrap(); + let (mut driver, current_crate_id) = create_context_at_path(actual_path); + + // We ignore the warnings and errors produced by compilation for producing codelenses + // because we can still get the test functions even if compilation fails + let _ = check_crate(&mut driver, false, false); + + let fm = &driver.file_manager; + let files = fm.as_simple_files(); + let tests = driver.get_all_test_functions_in_crate_matching(¤t_crate_id, ""); + + let mut lenses: Vec = vec![]; + for func_id in tests { + let location = driver.function_meta(&func_id).name.location; + let file_id = location.file; + // TODO(#1681): This file_id never be 0 because the "path" where it maps is the directory, not a file + if file_id.as_usize() != 0 { + continue; + } + + let func_name = driver.function_name(&func_id); + + let range = + byte_span_to_range(files, file_id.as_usize(), location.span.into()).unwrap_or_default(); + + let command = Command { + title: TEST_CODELENS_TITLE.into(), + command: TEST_COMMAND.into(), + arguments: Some(vec![func_name.into()]), + }; + + let lens = CodeLens { range, command: command.into(), data: None }; + + lenses.push(lens); + } + + async move { + if lenses.is_empty() { + Ok(None) + } else { + Ok(Some(lenses)) + } + } +} + +pub fn on_initialized( + _state: &mut LspState, + _params: InitializedParams, +) -> ControlFlow> { + ControlFlow::Continue(()) +} + +pub fn on_did_change_configuration( + _state: &mut LspState, + _params: DidChangeConfigurationParams, +) -> ControlFlow> { + ControlFlow::Continue(()) +} + +pub fn on_did_open_text_document( + _state: &mut LspState, + _params: DidOpenTextDocumentParams, +) -> ControlFlow> { + ControlFlow::Continue(()) +} + +pub fn on_did_change_text_document( + _state: &mut LspState, + _params: DidChangeTextDocumentParams, +) -> ControlFlow> { + ControlFlow::Continue(()) +} + +pub fn on_did_close_text_document( + _state: &mut LspState, + _params: DidCloseTextDocumentParams, +) -> ControlFlow> { + ControlFlow::Continue(()) +} + +/// Find the nearest parent file with given names. +fn find_nearest_parent_file(path: &Path, filenames: &[&str]) -> Option { + let mut current_path = path; + + while let Some(parent_path) = current_path.parent() { + for filename in filenames { + let mut possible_file_path = parent_path.to_path_buf(); + possible_file_path.push(filename); + if possible_file_path.is_file() { + return Some(possible_file_path); + } + } + current_path = parent_path; + } + + None +} + +fn read_dependencies( + nargo_toml_path: &Path, +) -> Result, Box> { + let content: String = fs::read_to_string(nargo_toml_path)?; + let value: toml::Value = toml::from_str(&content)?; + + let mut dependencies = HashMap::new(); + + if let Some(toml::Value::Table(table)) = value.get("dependencies") { + for (key, value) in table { + if let toml::Value::Table(inner_table) = value { + if let Some(toml::Value::String(path)) = inner_table.get("path") { + dependencies.insert(key.clone(), path.clone()); + } + } + } + } + + Ok(dependencies) +} + +pub fn on_did_save_text_document( + state: &mut LspState, + params: DidSaveTextDocumentParams, +) -> ControlFlow> { + let actual_path = params.text_document.uri.to_file_path().unwrap(); + let (mut context, _) = create_context_at_path(actual_path.clone()); + + let file_diagnostics = match check_crate(&mut context, false, false) { + Ok(warnings) => warnings, + Err(errors_and_warnings) => errors_and_warnings, + }; + let mut diagnostics = Vec::new(); + + if !file_diagnostics.is_empty() { + let fm = &context.file_manager; + let files = fm.as_simple_files(); + + for FileDiagnostic { file_id, diagnostic } in file_diagnostics { + // TODO(AD): HACK, undo these total hacks once we have a proper approach + if file_id.as_usize() == 0 { + // main.nr case + if actual_path.file_name().unwrap().to_str() != Some("main.nr") + && actual_path.file_name().unwrap().to_str() != Some("lib.nr") + { + continue; + } + } else if fm.path(file_id).file_name().unwrap().to_str().unwrap() + != actual_path.file_name().unwrap().to_str().unwrap().replace(".nr", "") + { + // every other file case + continue; // TODO(AD): HACK, we list all errors, filter by hacky final path component + } + + let mut range = Range::default(); + + // TODO: Should this be the first item in secondaries? Should we bail when we find a range? + for sec in diagnostic.secondaries { + // Not using `unwrap_or_default` here because we don't want to overwrite a valid range with a default range + if let Some(r) = byte_span_to_range(files, file_id.as_usize(), sec.span.into()) { + range = r + } + } + let severity = match diagnostic.kind { + DiagnosticKind::Error => Some(DiagnosticSeverity::ERROR), + DiagnosticKind::Warning => Some(DiagnosticSeverity::WARNING), + }; + diagnostics.push(Diagnostic { + range, + severity, + message: diagnostic.message, + ..Diagnostic::default() + }) + } + } + + let _ = state.client.publish_diagnostics(PublishDiagnosticsParams { + uri: params.text_document.uri, + version: None, + diagnostics, + }); + + ControlFlow::Continue(()) +} + +fn create_context_at_path(actual_path: PathBuf) -> (Context, CrateId) { + // TODO: Requiring `Language` and `is_opcode_supported` to construct a driver makes for some real stinky code + // The driver should not require knowledge of the backend; instead should be implemented as an independent pass (in nargo?) + let mut context = Context::default(); + + let mut file_path: PathBuf = actual_path; + // TODO better naming/unhacking + if let Some(new_path) = find_nearest_parent_file(&file_path, &["lib.nr", "main.nr"]) { + file_path = new_path; // TODO unhack + } + 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); + + // TODO(AD): undo hacky dependency resolution + if let Some(nargo_toml_path) = nargo_toml_path { + let dependencies = read_dependencies(&nargo_toml_path); + if let Ok(dependencies) = dependencies { + for (crate_name, dependency_path) in dependencies.iter() { + let path_to_lib = nargo_toml_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); + propagate_dep(&mut context, library_crate, &CrateName::new(crate_name).unwrap()); + } + } + } + (context, current_crate_id) +} + +pub fn on_exit(_state: &mut LspState, _params: ()) -> ControlFlow> { + ControlFlow::Continue(()) +} + +fn byte_span_to_range<'a, F: files::Files<'a> + ?Sized>( + files: &'a F, + file_id: F::FileId, + span: ops::Range, +) -> Option { + // TODO(#1683): Codespan ranges are often (always?) off by some amount of characters + if let Ok(codespan_range) = codespan_lsp::byte_span_to_range(files, file_id, span) { + // We have to manually construct a Range because the codespan_lsp restricts lsp-types to the wrong version range + // TODO: codespan is unmaintained and we should probably subsume it. Ref https://github.com/brendanzab/codespan/issues/345 + let range = Range { + start: Position { + line: codespan_range.start.line, + character: codespan_range.start.character, + }, + end: Position { + line: codespan_range.end.line, + character: codespan_range.end.character, + }, + }; + Some(range) + } else { + None + } +}