Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

chore(driver): Remove backend dependence from driver construction #1846

Merged
merged 3 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions crates/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use std::{
task::{Context, Poll},
};

use acvm::Language;
use async_lsp::{
router::Router, AnyEvent, AnyNotification, AnyRequest, ClientSocket, Error, LanguageClient,
LspService, ResponseError,
Expand Down Expand Up @@ -134,9 +133,7 @@ fn on_code_lens_request(
params: CodeLensParams,
) -> impl Future<Output = Result<Option<Vec<CodeLens>>, 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 mut driver = Driver::new();

let file_path = &params.text_document.uri.to_file_path().unwrap();

Expand Down Expand Up @@ -222,9 +219,7 @@ fn on_did_save_text_document(
state: &mut LspState,
params: DidSaveTextDocumentParams,
) -> ControlFlow<Result<(), async_lsp::Error>> {
// 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 driver = Driver::new();

let file_path = &params.text_document.uri.to_file_path().unwrap();

Expand Down
24 changes: 13 additions & 11 deletions crates/nargo_cli/src/cli/check_cmd.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::errors::CliError;
use crate::{errors::CliError, resolver::Resolver};
use acvm::Backend;
use clap::Args;
use iter_extended::btree_map;
Expand All @@ -7,8 +7,8 @@ use noirc_driver::CompileOptions;
use noirc_errors::reporter::ReportedErrors;
use std::path::{Path, PathBuf};

use super::fs::write_to_file;
use super::NargoConfig;
use super::{compile_cmd::setup_driver, fs::write_to_file};
use crate::constants::{PROVER_INPUT_FILE, VERIFIER_INPUT_FILE};

/// Checks the constraint system for errors
Expand All @@ -23,17 +23,19 @@ pub(crate) fn run<B: Backend>(
args: CheckCommand,
config: NargoConfig,
) -> Result<(), CliError<B>> {
check_from_path(backend, config.program_dir, &args.compile_options)?;
check_from_path(backend, &config.program_dir, &args.compile_options)?;
println!("Constraint system successfully built!");
Ok(())
}

fn check_from_path<B: Backend, P: AsRef<Path>>(
backend: &B,
program_dir: P,
fn check_from_path<B: Backend>(
// Backend isn't used but keeping it in the signature allows for better type inference
// TODO: This function doesn't need to exist but requires a little more refactoring
_backend: &B,
program_dir: &Path,
compile_options: &CompileOptions,
) -> Result<(), CliError<B>> {
let mut driver = setup_driver(backend, program_dir.as_ref())?;
let mut driver = Resolver::resolve_root_manifest(program_dir)?;
check_crate_and_report_errors(&mut driver, compile_options.deny_warnings)?;

// XXX: We can have a --overwrite flag to determine if you want to overwrite the Prover/Verifier.toml files
Expand All @@ -42,7 +44,7 @@ fn check_from_path<B: Backend, P: AsRef<Path>>(
// For now it is hard-coded to be toml.
//
// Check for input.toml and verifier.toml
let path_to_root = PathBuf::from(program_dir.as_ref());
let path_to_root = PathBuf::from(program_dir);
let path_to_prover_input = path_to_root.join(format!("{PROVER_INPUT_FILE}.toml"));
let path_to_verifier_input = path_to_root.join(format!("{VERIFIER_INPUT_FILE}.toml"));

Expand Down Expand Up @@ -160,7 +162,7 @@ d2 = ["", "", ""]
for path in paths.flatten() {
let path = path.path();
assert!(
super::check_from_path(&backend, path.clone(), &config).is_ok(),
super::check_from_path(&backend, &path, &config).is_ok(),
"path: {}",
path.display()
);
Expand All @@ -179,7 +181,7 @@ d2 = ["", "", ""]
for path in paths.flatten() {
let path = path.path();
assert!(
super::check_from_path(&backend, path.clone(), &config).is_err(),
super::check_from_path(&backend, &path, &config).is_err(),
"path: {}",
path.display()
);
Expand All @@ -198,7 +200,7 @@ d2 = ["", "", ""]
for path in paths.flatten() {
let path = path.path();
assert!(
super::check_from_path(&backend, path.clone(), &config).is_ok(),
super::check_from_path(&backend, &path, &config).is_ok(),
"path: {}",
path.display()
);
Expand Down
29 changes: 12 additions & 17 deletions crates/nargo_cli/src/cli/compile_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use clap::Args;

use nargo::ops::{preprocess_contract_function, preprocess_program};

use crate::resolver::DependencyResolutionError;
use crate::{constants::TARGET_DIR, errors::CliError, resolver::Resolver};

use super::fs::{
Expand Down Expand Up @@ -49,9 +48,13 @@ pub(crate) fn run<B: Backend>(

// If contracts is set we're compiling every function in a 'contract' rather than just 'main'.
if args.contracts {
let mut driver = setup_driver(backend, &config.program_dir)?;
let mut driver = Resolver::resolve_root_manifest(&config.program_dir)?;

let result = driver.compile_contracts(&args.compile_options);
let result = driver.compile_contracts(
backend.np_language(),
&|op| backend.supports_opcode(op),
&args.compile_options,
);
let contracts = report_errors(result, &driver, args.compile_options.deny_warnings)?;

// TODO(#1389): I wonder if it is incorrect for nargo-core to know anything about contracts.
Expand Down Expand Up @@ -101,25 +104,17 @@ pub(crate) fn run<B: Backend>(
Ok(())
}

pub(super) fn setup_driver<B: Backend>(
backend: &B,
program_dir: &Path,
) -> Result<Driver, DependencyResolutionError> {
Resolver::resolve_root_manifest(
program_dir,
backend.np_language(),
// TODO(#1102): Remove need for driver to be aware of backend.
Box::new(|op| B::default().supports_opcode(op)),
)
}

pub(crate) fn compile_circuit<B: Backend>(
backend: &B,
program_dir: &Path,
compile_options: &CompileOptions,
) -> Result<CompiledProgram, CliError<B>> {
let mut driver = setup_driver(backend, program_dir)?;
let result = driver.compile_main(compile_options);
let mut driver = Resolver::resolve_root_manifest(program_dir)?;
let result = driver.compile_main(
backend.np_language(),
&|op| backend.supports_opcode(op),
compile_options,
);
report_errors(result, &driver, compile_options.deny_warnings).map_err(Into::into)
}

Expand Down
6 changes: 1 addition & 5 deletions crates/nargo_cli/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,11 +178,7 @@ mod tests {
///
/// This is used for tests.
fn file_compiles<P: AsRef<Path>>(root_file: P) -> bool {
let mut driver = Driver::new(
&acvm::Language::R1CS,
#[allow(deprecated)]
Box::new(acvm::pwg::default_is_opcode_supported(acvm::Language::R1CS)),
);
let mut driver = Driver::new();
driver.create_local_crate(&root_file, CrateType::Binary);
crate::resolver::add_std_lib(&mut driver);

Expand Down
9 changes: 3 additions & 6 deletions crates/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,7 @@ use noirc_driver::{CompileOptions, Driver};
use noirc_frontend::node_interner::FuncId;
use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};

use crate::{
cli::{check_cmd::check_crate_and_report_errors, compile_cmd::setup_driver},
errors::CliError,
};
use crate::{cli::check_cmd::check_crate_and_report_errors, errors::CliError, resolver::Resolver};

use super::NargoConfig;

Expand Down Expand Up @@ -40,7 +37,7 @@ fn run_tests<B: Backend>(
test_name: &str,
compile_options: &CompileOptions,
) -> Result<(), CliError<B>> {
let mut driver = setup_driver(backend, program_dir)?;
let mut driver = Resolver::resolve_root_manifest(program_dir)?;
check_crate_and_report_errors(&mut driver, compile_options.deny_warnings)?;

let test_functions = driver.get_all_test_functions_in_crate_matching(test_name);
Expand Down Expand Up @@ -86,7 +83,7 @@ fn run_test<B: Backend>(
config: &CompileOptions,
) -> Result<(), CliError<B>> {
let program = driver
.compile_no_check(config, main)
.compile_no_check(config, main, backend.np_language(), &|op| backend.supports_opcode(op))
.map_err(|_| CliError::Generic(format!("Test '{test_name}' failed to compile")))?;

// Run the backend to ensure the PWG evaluates functions like std::hash::pedersen,
Expand Down
5 changes: 1 addition & 4 deletions crates/nargo_cli/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use std::{
path::{Path, PathBuf},
};

use acvm::{acir::circuit::Opcode, Language};
use nargo::manifest::{Dependency, PackageManifest};
use noirc_driver::Driver;
use noirc_frontend::graph::{CrateId, CrateName, CrateType};
Expand Down Expand Up @@ -70,10 +69,8 @@ impl<'a> Resolver<'a> {
/// XXX: Need to handle when a local package changes!
pub(crate) fn resolve_root_manifest(
dir_path: &std::path::Path,
np_language: Language,
is_opcode_supported: Box<dyn Fn(&Opcode) -> bool>,
) -> Result<Driver, DependencyResolutionError> {
let mut driver = Driver::new(&np_language, is_opcode_supported);
let mut driver = Driver::new();
let (entry_path, crate_type) = super::lib_or_bin(dir_path)?;

let manifest_path = super::find_package_manifest(dir_path)?;
Expand Down
Loading