fix(frontend): Allow indirect calls to oracles from constrained functions

compiler/noirc_evaluator/src/brillig/brillig_check.rs

@@ -126,9 +126,7 @@ pub(crate) fn opcode_advisories<F: AcirField>(
         // Going backwards, so reads at the end are recorded before earlier writes.
         for loc in opcode_range.rev() {
             let opcode = &brillig_context.artifact().byte_code[loc];
-            if advisory_collector.should_visit_opcode(opcode) {
-                advisory_collector.visit_opcode(opcode, loc);
-            }
+            advisory_collector.visit_opcode(opcode, loc);
         }
 
         advisories.extend(advisory_collector.into_advisories());
@@ -432,8 +430,9 @@ trait OpcodeAddressVisitor {
     /// Called with all addresses written by the opcode.
     fn write(&mut self, addr: &MemoryAddress, location: OpcodeLocation);
 
-    /// Expected to be called with each opcode, traversing blocks in Post Order,
-    /// feeding the opcodes back to front.
+    /// Call with each opcode to record reads and writes.
+    ///
+    /// The expected order of visit depends on the purpose of the visitor implementation.
     fn visit_opcode<F: AcirField>(&mut self, opcode: &Opcode<F>, location: OpcodeLocation) {
         if !self.should_visit_opcode(opcode) {
             return;

compiler/noirc_evaluator/src/errors.rs

@@ -13,7 +13,7 @@ use noirc_errors::{CustomDiagnostic, Location, call_stack::CallStack};
 use noirc_frontend::signed_field::SignedField;
 use thiserror::Error;
 
-use crate::ssa::ir::types::NumericType;
+use crate::ssa::{ir::types::NumericType, ssa_gen::SHOW_INVALID_SSA_ENV_KEY};
 use serde::{Deserialize, Serialize};
 
 pub type RtResult<T> = Result<T, RuntimeError>;
@@ -240,11 +240,15 @@ impl RuntimeError {
                     call_stack.last().cloned().unwrap_or_else(Location::dummy);
 
                 let mut diagnostic = CustomDiagnostic::simple_error(
-                    message,
+                    format!("SSA validation error: {message}"),
                     String::new(),
                     location,
                 );
 
+                if std::env::var(SHOW_INVALID_SSA_ENV_KEY).is_err() {
+                    diagnostic.notes.push(format!("Set the {SHOW_INVALID_SSA_ENV_KEY} env var to see the SSA."));
+                }
+
                 if call_stack.is_empty() {
                     // Clear it otherwise it points to the top of the file.
                     diagnostic.secondaries.clear();

compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs

@@ -2156,13 +2156,13 @@ mod test {
         // The arguments are not meant to make sense, just pass SSA validation and not be simplified out.
         let call_target = match test_call {
             TestCall::Function(_) => "f1".to_string(),
-            TestCall::ForeignFunction => "print".to_string(), // The ony foreign function the SSA parser allows.
+            TestCall::ForeignFunction => "print".to_string(),
             TestCall::Intrinsic(intrinsic) => format!("{intrinsic}"),
         };
 
         let src = format!(
             r#"
-        acir(inline) fn main f0 {{
+        brillig(inline) fn main f0 {{
           b0(v0: [u64; 25]):
             jmp b1(u32 0)
           b1(v1: u32):
@@ -2176,7 +2176,7 @@ mod test {
             return
         }}
 
-        acir(inline) {dummy_purity} fn dummy f1 {{
+        brillig(inline) {dummy_purity} fn dummy f1 {{
           b0(v0: [u64; 25]):
             return v0
         }}

compiler/noirc_evaluator/src/ssa/opt/unrolling.rs

@@ -1846,8 +1846,6 @@ mod tests {
             v3 = eq v0, u128 {0}
             jmpif v3 then: b3, else: b2
           b2():
-            v2 = make_array b""
-            call print(u1 1, v0, v2, u1 0)
             v6 = unchecked_add v0, u128 1
             jmp b1(v6)
           b3():

compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs

@@ -606,8 +606,9 @@ impl Translator {
             return Ok(var_id);
         }
 
-        // We allow calls to the built-in print function
-        if &function.name == "print" {
+        // We allow calls to the built-in print function, or a function that is named as some kind of "oracle",
+        // which is a common pattern in the codebase and allows us to write tests with foreign functions in the SSA.
+        if &function.name == "print" || function.name.contains("oracle") {
             return Ok(self.builder.import_foreign_function(&function.name));
         }
 

compiler/noirc_evaluator/src/ssa/parser/tests.rs

@@ -808,6 +808,18 @@ fn test_parses_print() {
     assert_ssa_roundtrip(src);
 }
 
+#[test]
+fn test_parses_oracle() {
+    let src = "
+        brillig(inline) impure fn main f0 {
+          b0():
+            call oracle_call()
+            return
+        }
+        ";
+    assert_ssa_roundtrip(src);
+}
+
 #[test]
 fn parses_variable_from_a_syntactically_following_block_but_logically_preceding_block_with_jmp() {
     let src = "

compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs

@@ -42,6 +42,8 @@ use super::{
     },
 };
 
+pub(crate) const SHOW_INVALID_SSA_ENV_KEY: &str = "NOIR_SHOW_INVALID_SSA";
+
 pub(crate) const SSA_WORD_SIZE: u32 = 32;
 
 /// Generates SSA for the given monomorphized program.
@@ -147,7 +149,7 @@ fn validate_ssa_or_err(ssa: Ssa) -> Result<Ssa, RuntimeError> {
     if let Err(payload) = result {
         // Print the SSA, but it's potentially massive, and if we resume the unwind it might be displayed
         // under the panic message, which makes it difficult to see what went wrong.
-        if std::env::var("RUST_BACKTRACE").is_ok() {
+        if std::env::var(SHOW_INVALID_SSA_ENV_KEY).is_ok() {
             eprintln!("--- The SSA failed to validate:\n{ssa}\n");
         }
 

compiler/noirc_evaluator/src/ssa/validation/mod.rs

@@ -680,19 +680,19 @@
                 //     message_hash: [u8; N],
                 //     predicate: bool,
                 // ) -> bool
                 assert_arguments_length(arguments, 5, "ecdsa_secp_256");
 
                 let public_key_x = arguments[0];
                 let public_key_x_type = dfg.type_of_value(public_key_x);
                 let public_key_x_length =
                     assert_u8_array(&public_key_x_type, "ecdsa_secp256 public_key_x");
                 assert_array_length(public_key_x_length, 32, "ecdsa_secp256 public_key_x");
 
                 let public_key_y = arguments[1];
                 let public_key_y_type = dfg.type_of_value(public_key_y);
                 let public_key_y_length =
                     assert_u8_array(&public_key_y_type, "ecdsa_secp256 public_key_y");
                 assert_array_length(public_key_y_length, 32, "ecdsa_secp256 public_key_y");
 
                 let signature = arguments[2];
                 let signature_type = dfg.type_of_value(signature);
@@ -929,17 +929,26 @@
     /// Check the inputs and outputs of function calls going from ACIR to Brillig:
     /// * cannot pass references from constrained to unconstrained code
     /// * cannot return functions
+    /// * cannot call oracles directly
     fn check_calls_in_constrained(&self, instruction: InstructionId) {
         if !self.function.runtime().is_acir() {
             return;
         }
         let Instruction::Call { func, arguments } = &self.function.dfg[instruction] else {
             return;
         };
-        let Value::Function(func_id) = &self.function.dfg[*func] else {
-            return;
+        let callee_id = match &self.function.dfg[*func] {
+            Value::Function(func_id) => func_id,
+            Value::ForeignFunction(oracle) => {
+                panic!(
+                    "Trying to call foreign function '{oracle}' from ACIR function '{} {}'",
+                    self.function.name(),
+                    self.function.id()
+                );
+            }
+            _ => return,
         };
-        let called_function = &self.ssa.functions[func_id];
+        let called_function = &self.ssa.functions[callee_id];
         if called_function.runtime().is_acir() {
             return;
         }
@@ -1721,6 +1730,21 @@
         let _ = Ssa::from_str(src).unwrap();
     }
 
+    #[test]
+    #[should_panic(
+        expected = "Trying to call foreign function 'oracle_call' from ACIR function 'main f0'"
+    )]
+    fn disallows_calling_an_oracle_from_acir() {
+        let src = "
+        acir(inline) fn main f0 {
+          b0():
+            call oracle_call()
+            return
+        }
+        ";
+        let _ = Ssa::from_str(src).unwrap();
+    }
+
     #[test]
     #[should_panic(
         expected = "Trying to pass a reference from ACIR function 'main f0' to unconstrained 'foo f1' in argument v1: &mut u32"

compiler/noirc_frontend/src/elaborator/lints.rs

@@ -175,27 +175,6 @@ pub(super) fn oracle_returns_multiple_slices(
     }
 }
 
-/// Oracle functions may not be called by constrained functions directly.
-///
-/// In order for a constrained function to call an oracle it must first call through an unconstrained function.
-pub(super) fn oracle_called_from_constrained_function(
-    interner: &NodeInterner,
-    called_func: &FuncId,
-    calling_from_constrained_runtime: bool,
-    location: Location,
-) -> Option<ResolverError> {
-    if !calling_from_constrained_runtime {
-        return None;
-    }
-
-    let function_attributes = interner.function_attributes(called_func);
-    if function_attributes.function()?.kind.is_oracle() {
-        Some(ResolverError::UnconstrainedOracleReturnToConstrained { location })
-    } else {
-        None
-    }
-}
-
 /// `pub` is required on return types for entry point functions
 pub(super) fn missing_pub(func: &FuncMeta, modifiers: &FunctionModifiers) -> Option<ResolverError> {
     if func.is_entry_point

compiler/noirc_frontend/src/elaborator/types.rs

@@ -2321,6 +2321,7 @@ impl Elaborator<'_> {
         let is_unconstrained_call =
             func_type_is_unconstrained || self.is_unconstrained_call(call.func);
         let crossing_runtime_boundary = is_current_func_constrained && is_unconstrained_call;
+
         if crossing_runtime_boundary {
             match self.unsafe_block_status {
                 UnsafeBlockStatus::NotInUnsafeBlock => {
@@ -2333,18 +2334,6 @@ impl Elaborator<'_> {
                 UnsafeBlockStatus::InUnsafeBlockWithUnconstrainedCalls => (),
             }
 
-            if let Some(called_func_id) = self.interner.lookup_function_from_expr(&call.func) {
-                self.run_lint(|elaborator| {
-                    lints::oracle_called_from_constrained_function(
-                        elaborator.interner,
-                        &called_func_id,
-                        is_current_func_constrained,
-                        location,
-                    )
-                    .map(Into::into)
-                });
-            }
-
             let errors = lints::unconstrained_function_args(&args);
             self.push_errors(errors);
         }
@@ -2360,6 +2349,7 @@ impl Elaborator<'_> {
         return_type
     }
 
+    /// Check if the callee is an unconstrained function, or a variable referring to one.
     fn is_unconstrained_call(&self, expr: ExprId) -> bool {
         if let Some(func_id) = self.interner.lookup_function_from_expr(&expr) {
             let modifiers = self.interner.function_modifiers(&func_id);

compiler/noirc_frontend/src/hir/resolution/errors.rs

@@ -74,8 +74,6 @@ pub enum ResolverError {
     OracleMarkedAsConstrained { ident: Ident, location: Location },
     #[error("Oracle functions cannot return multiple slices")]
     OracleReturnsMultipleSlices { location: Location },
-    #[error("Oracle functions cannot be called directly from constrained functions")]
-    UnconstrainedOracleReturnToConstrained { location: Location },
     #[error("Dependency cycle found, '{item}' recursively depends on itself: {cycle} ")]
     DependencyCycle { location: Location, item: String, cycle: String },
     #[error("break/continue are only allowed in unconstrained functions")]
@@ -224,7 +222,6 @@ impl ResolverError {
             | ResolverError::InvalidClosureEnvironment { location, .. }
             | ResolverError::NestedSlices { location }
             | ResolverError::AbiAttributeOutsideContract { location }
-            | ResolverError::UnconstrainedOracleReturnToConstrained { location }
             | ResolverError::DependencyCycle { location, .. }
             | ResolverError::JumpInConstrainedFn { location, .. }
             | ResolverError::LoopInConstrainedFn { location }
@@ -487,11 +484,6 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
                     *location,
                 )
             },
-            ResolverError::UnconstrainedOracleReturnToConstrained { location } => Diagnostic::simple_error(
-                error.to_string(),
-                "This oracle call must be wrapped in a call to another unconstrained function before being returned to a constrained runtime".into(),
-                *location,
-            ),
             ResolverError::DependencyCycle { location, item, cycle } => {
                 Diagnostic::simple_error(
                     "Dependency cycle found".into(),