Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat!: Disable mocks in execute #6869

Merged
merged 12 commits into from
Jan 10, 2025
Merged
7 changes: 7 additions & 0 deletions test_programs/execution_failure/mocks_in_execution/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "mocks_in_execution"
type = "bin"
authors = [""]
compiler_version = ">=0.23.0"

[dependencies]
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
fn main() {
// Trying to use a mock in `nargo execute` should fail.
let mock = unsafe { std::test::OracleMock::mock("foo") };
assert_eq(mock.id, 0);
}
2 changes: 0 additions & 2 deletions test_programs/execution_success/brillig_oracle/Prover.toml

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
use std::slice;
use std::test::OracleMock;

#[test]
fn test_main() {
main(10);
}

// Tests oracle usage in brillig/unconstrained functions
fn main(_x: Field) {
/// Safety: testing context
Expand Down Expand Up @@ -46,4 +51,3 @@ unconstrained fn get_number_sequence_wrapper(size: Field) {
assert(slice[i] == reversed_slice[19 - i]);
}
}

8 changes: 6 additions & 2 deletions tooling/acvm_cli/src/cli/execute_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,12 @@ pub(crate) fn execute_program_from_witness(
&program,
inputs_map,
&Bn254BlackBoxSolver(pedantic_solving),
&mut DefaultForeignCallBuilder { output: PrintOutput::Stdout, ..Default::default() }
.build(),
&mut DefaultForeignCallBuilder {
output: PrintOutput::Stdout,
enable_mocks: false,
..Default::default()
}
.build(),
)
.map_err(CliError::CircuitExecutionError)
}
1 change: 1 addition & 0 deletions tooling/lsp/src/requests/test_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ fn on_test_run_request_inner(
|output, base| {
DefaultForeignCallBuilder {
output,
enable_mocks: true,
resolver_url: None, // NB without this the root and package don't do anything.
root_path: Some(workspace.root_dir.clone()),
package_name: Some(package.name.to_string()),
Expand Down
51 changes: 44 additions & 7 deletions tooling/nargo/src/foreign_calls/default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ use serde::{Deserialize, Serialize};
use crate::PrintOutput;

use super::{
layers::{self, Layer, Layering},
mocker::MockForeignCallExecutor,
layers::{self, Either, Layer, Layering},
mocker::{DisabledMockForeignCallExecutor, MockForeignCallExecutor},
print::PrintForeignCallExecutor,
ForeignCallExecutor,
};
Expand All @@ -15,24 +15,51 @@ use super::rpc::RPCForeignCallExecutor;

/// A builder for [DefaultForeignCallLayers] where we can enable fields based on feature flags,
/// which is easier than providing different overrides for a `new` method.
#[derive(Default)]
pub struct DefaultForeignCallBuilder<'a> {
pub output: PrintOutput<'a>,
pub enable_mocks: bool,

#[cfg(feature = "rpc")]
pub resolver_url: Option<String>,

#[cfg(feature = "rpc")]
pub root_path: Option<std::path::PathBuf>,

#[cfg(feature = "rpc")]
pub package_name: Option<String>,
}

impl<'a> Default for DefaultForeignCallBuilder<'a> {
fn default() -> Self {
Self {
output: PrintOutput::default(),
enable_mocks: true,

#[cfg(feature = "rpc")]
resolver_url: None,

#[cfg(feature = "rpc")]
root_path: None,

#[cfg(feature = "rpc")]
package_name: None,
}
}
}

impl<'a> DefaultForeignCallBuilder<'a> {
/// Override the output.
pub fn with_output(mut self, output: PrintOutput<'a>) -> Self {
self.output = output;
self
}

/// Enable or disable mocks.
pub fn with_mocks(mut self, enabled: bool) -> Self {
michaeljklein marked this conversation as resolved.
Show resolved Hide resolved
self.enable_mocks = enabled;
self
}

/// Compose the executor layers with [layers::Empty] as the default handler.
pub fn build<F>(self) -> DefaultForeignCallLayers<'a, layers::Empty, F>
where
Expand Down Expand Up @@ -69,7 +96,11 @@ impl<'a> DefaultForeignCallBuilder<'a> {
};

executor
.add_layer(MockForeignCallExecutor::default())
.add_layer(if self.enable_mocks {
Either::Left(MockForeignCallExecutor::default())
} else {
Either::Right(DisabledMockForeignCallExecutor)
})
.add_layer(PrintForeignCallExecutor::new(self.output))
}
}
Expand All @@ -78,11 +109,16 @@ impl<'a> DefaultForeignCallBuilder<'a> {
#[cfg(feature = "rpc")]
pub type DefaultForeignCallLayers<'a, B, F> = Layer<
PrintForeignCallExecutor<'a>,
Layer<MockForeignCallExecutor<F>, Layer<Option<RPCForeignCallExecutor>, B>>,
Layer<
Either<MockForeignCallExecutor<F>, DisabledMockForeignCallExecutor>,
Layer<Option<RPCForeignCallExecutor>, B>,
>,
>;
#[cfg(not(feature = "rpc"))]
pub type DefaultForeignCallLayers<'a, B, F> =
Layer<PrintForeignCallExecutor<'a>, Layer<MockForeignCallExecutor<F>, B>>;
pub type DefaultForeignCallLayers<'a, B, F> = Layer<
PrintForeignCallExecutor<'a>,
Layer<Either<MockForeignCallExecutor<F>, DisabledMockForeignCallExecutor>, B>,
>;

/// Convenience constructor for code that used to create the executor this way.
#[cfg(feature = "rpc")]
Expand All @@ -106,6 +142,7 @@ impl DefaultForeignCallExecutor {
{
DefaultForeignCallBuilder {
output,
enable_mocks: true,
resolver_url: resolver_url.map(|s| s.to_string()),
root_path,
package_name,
Expand Down
22 changes: 22 additions & 0 deletions tooling/nargo/src/foreign_calls/layers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,28 @@ impl<T> Layering for T {
}
}

/// A case where we can have either this or that type of handler.
pub enum Either<L, R> {
aakoshh marked this conversation as resolved.
Show resolved Hide resolved
Left(L),
Right(R),
}

impl<L, R, F> ForeignCallExecutor<F> for Either<L, R>
where
L: ForeignCallExecutor<F>,
R: ForeignCallExecutor<F>,
{
fn execute(
&mut self,
foreign_call: &ForeignCallWaitInfo<F>,
) -> Result<ForeignCallResult<F>, ForeignCallError> {
match self {
Either::Left(left) => left.execute(foreign_call),
Either::Right(right) => right.execute(foreign_call),
}
}
}

/// Support disabling a layer by making it optional.
/// This way we can still have a known static type for a composition,
/// because layers are always added, potentially wrapped in an `Option`.
Expand Down
12 changes: 4 additions & 8 deletions tooling/nargo/src/foreign_calls/mocker.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::marker::PhantomData;

use acvm::{
acir::brillig::{ForeignCallParam, ForeignCallResult},
pwg::ForeignCallWaitInfo,
Expand Down Expand Up @@ -178,12 +176,9 @@ where
}

/// Handler that panics if any of the mock functions are called.
#[allow(dead_code)] // TODO: Make the mocker optional
pub(crate) struct DisabledMockForeignCallExecutor<F> {
_field: PhantomData<F>,
}
pub struct DisabledMockForeignCallExecutor;

impl<F> ForeignCallExecutor<F> for DisabledMockForeignCallExecutor<F> {
impl<F> ForeignCallExecutor<F> for DisabledMockForeignCallExecutor {
fn execute(
&mut self,
foreign_call: &ForeignCallWaitInfo<F>,
Expand All @@ -198,7 +193,8 @@ impl<F> ForeignCallExecutor<F> for DisabledMockForeignCallExecutor<F> {
| ForeignCall::ClearMock,
) = ForeignCall::lookup(foreign_call_name)
{
panic!("unexpected mock call: {}", foreign_call.function)
// Returning an error instead of panicking so this can be tested.
return Err(ForeignCallError::Disabled(foreign_call.function.to_string()));
}
Err(ForeignCallError::NoHandler(foreign_call.function.clone()))
}
Expand Down
3 changes: 3 additions & 0 deletions tooling/nargo/src/foreign_calls/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ impl ForeignCall {

#[derive(Debug, Error)]
pub enum ForeignCallError {
#[error("Attempted to call disabled foreign call `{0}`")]
Disabled(String),

#[error("No handler could be found for foreign call `{0}`")]
NoHandler(String),

Expand Down
12 changes: 7 additions & 5 deletions tooling/nargo_cli/src/cli/execute_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use clap::Args;

use nargo::constants::PROVER_INPUT_FILE;
use nargo::errors::try_to_diagnose_runtime_error;
use nargo::foreign_calls::DefaultForeignCallExecutor;
use nargo::foreign_calls::DefaultForeignCallBuilder;
use nargo::package::Package;
use nargo::PrintOutput;
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml};
Expand Down Expand Up @@ -150,12 +150,14 @@ pub(crate) fn execute_program(
&compiled_program.program,
initial_witness,
&Bn254BlackBoxSolver(pedantic_solving),
&mut DefaultForeignCallExecutor::new(
PrintOutput::Stdout,
foreign_call_resolver_url,
&mut DefaultForeignCallBuilder {
output: PrintOutput::Stdout,
enable_mocks: false,
resolver_url: foreign_call_resolver_url.map(|s| s.to_string()),
root_path,
package_name,
),
}
.build(),
);
match solved_witness_stack_err {
Ok(solved_witness_stack) => Ok(solved_witness_stack),
Expand Down
1 change: 1 addition & 0 deletions tooling/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,7 @@ impl<'a> TestRunner<'a> {
|output, base| {
DefaultForeignCallBuilder {
output,
enable_mocks: true,
resolver_url: foreign_call_resolver_url.map(|s| s.to_string()),
root_path: root_path.clone(),
package_name: Some(package_name.clone()),
Expand Down
2 changes: 1 addition & 1 deletion tooling/nargo_cli/tests/stdlib-tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ fn run_stdlib_tests(force_brillig: bool, inliner_aggressiveness: i64) {
PrintOutput::Stdout,
&CompileOptions { force_brillig, inliner_aggressiveness, ..Default::default() },
|output, base| {
DefaultForeignCallBuilder { output, ..Default::default() }.build_with_base(base)
DefaultForeignCallBuilder::default().with_output(output).build_with_base(base)
},
);
(test_name, status)
Expand Down
Loading