feat: Refactor Logging to use Brillig foreign calls (#1917)

* initial stdlib methods to start refactoring logign

* foreign call enum

* working println and println_format w/ brillig oracles

* fix up brillig_oracle test

* uncomment regression test for slice return from foreign calls in brillig

* cargo clippy

* got structs serialized correctly without aos_to_soa

* remove dbg

* working println_format

* cargo clippy

* rename enable_slices to experimental_ssa

* remove dbg and fix format_field_string

* pr comments, and remove format work to move into separate PR

* add comment about removing old println

* remove old comment

* have println return nothing

* Update crates/noirc_frontend/src/hir/def_map/mod.rs

Co-authored-by: jfecher <jake@aztecprotocol.com>

* Update crates/noirc_frontend/src/hir/def_map/mod.rs

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

* add comment to append_abi_arg call

* include println oracle comment in stdlib

---------

Co-authored-by: jfecher <jake@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

Cargo.toml

@@ -37,7 +37,6 @@ noirc_errors = { path = "crates/noirc_errors" }
 noirc_evaluator = { path = "crates/noirc_evaluator" }
 noirc_frontend = { path = "crates/noirc_frontend" }
 noir_wasm = { path = "crates/wasm" }
-
 cfg-if = "1.0.0"
 clap = { version = "4.1.4", features = ["derive"] }
 codespan = { version = "0.11.1", features = ["serialization"] }
@@ -58,4 +57,4 @@ wasm-bindgen-test = "0.3.33"
 base64 = "0.21.2"
 
 [patch.crates-io]
-async-lsp = { git = "https://github.com/oxalica/async-lsp", rev = "09dbcc11046f7a188a80137f8d36484d86c78c78" }
+async-lsp = { git = "https://github.com/oxalica/async-lsp", rev = "09dbcc11046f7a188a80137f8d36484d86c78c78" }

crates/nargo/Cargo.toml

@@ -17,6 +17,7 @@ noirc_driver.workspace = true
 iter-extended.workspace = true
 toml.workspace = true
 serde.workspace = true
+serde_json.workspace = true
 thiserror.workspace = true
 noirc_errors.workspace = true
 base64.workspace = true

crates/nargo/src/errors.rs

@@ -1,4 +1,5 @@
 use acvm::pwg::OpcodeResolutionError;
+use noirc_abi::errors::{AbiError, InputParserError};
 use thiserror::Error;
 
 #[derive(Debug, Error)]
@@ -10,4 +11,21 @@ pub enum NargoError {
     /// ACIR circuit solving error
     #[error(transparent)]
     SolvingError(#[from] OpcodeResolutionError),
+
+    #[error(transparent)]
+    ForeignCallError(#[from] ForeignCallError),
+}
+
+#[derive(Debug, Error)]
+pub enum ForeignCallError {
+    #[error("Foreign call inputs needed for execution are missing")]
+    MissingForeignCallInputs,
+
+    /// ABI encoding/decoding error
+    #[error(transparent)]
+    AbiError(#[from] AbiError),
+
+    /// Input parsing error
+    #[error(transparent)]
+    InputParserError(#[from] InputParserError),
 }

crates/nargo/src/ops/execute.rs

@@ -1,11 +1,11 @@
-use acvm::acir::brillig::{ForeignCallResult, Value};
-use acvm::pwg::{ACVMStatus, ForeignCallWaitInfo, ACVM};
+use acvm::pwg::{ACVMStatus, ACVM};
 use acvm::BlackBoxFunctionSolver;
 use acvm::{acir::circuit::Circuit, acir::native_types::WitnessMap};
-use iter_extended::vecmap;
 
 use crate::NargoError;
 
+use super::foreign_calls::ForeignCall;
+
 pub fn execute_circuit<B: BlackBoxFunctionSolver + Default>(
     _backend: &B,
     circuit: Circuit,
@@ -24,7 +24,7 @@ pub fn execute_circuit<B: BlackBoxFunctionSolver + Default>(
             ACVMStatus::Failure(error) => return Err(error.into()),
             ACVMStatus::RequiresForeignCall => {
                 while let Some(foreign_call) = acvm.get_pending_foreign_call() {
-                    let foreign_call_result = execute_foreign_call(foreign_call);
+                    let foreign_call_result = ForeignCall::execute(foreign_call)?;
                     acvm.resolve_pending_foreign_call(foreign_call_result);
                 }
             }
@@ -34,41 +34,3 @@ pub fn execute_circuit<B: BlackBoxFunctionSolver + Default>(
     let solved_witness = acvm.finalize();
     Ok(solved_witness)
 }
-
-fn execute_foreign_call(foreign_call: &ForeignCallWaitInfo) -> ForeignCallResult {
-    // TODO(#1615): Nargo only supports "oracle_print_**_impl" functions  that print a singular value or an array and nothing else
-    // This should be expanded in a general logging refactor
-    match foreign_call.function.as_str() {
-        // TODO(#1910): Move to an enum and don't match directly on these strings
-        "oracle_print_impl" => {
-            let values = &foreign_call.inputs[0];
-            println!("{:?}", values[0].to_field().to_hex());
-            values[0].into()
-        }
-        "oracle_print_array_impl" => {
-            let mut outputs_hex = Vec::new();
-            for values in &foreign_call.inputs {
-                for value in values {
-                    outputs_hex.push(value.to_field().to_hex());
-                }
-            }
-            // Join all of the hex strings using a comma
-            let comma_separated_elements = outputs_hex.join(", ");
-            let output_witnesses_string = "[".to_owned() + &comma_separated_elements + "]";
-            println!("{output_witnesses_string}");
-
-            foreign_call.inputs[0][0].into()
-        }
-        "get_number_sequence" => {
-            let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128();
-
-            vecmap(0..sequence_length, Value::from).into()
-        }
-        "get_reverse_number_sequence" => {
-            let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128();
-
-            vecmap((0..sequence_length).rev(), Value::from).into()
-        }
-        _ => panic!("unexpected foreign call type"),
-    }
-}

crates/nargo/src/ops/foreign_calls.rs

@@ -0,0 +1,93 @@
+use acvm::{
+    acir::brillig::{ForeignCallResult, Value},
+    pwg::ForeignCallWaitInfo,
+};
+use iter_extended::vecmap;
+use noirc_abi::{decode_string_value, decode_value, input_parser::json::JsonTypes, AbiType};
+
+use crate::errors::ForeignCallError;
+
+/// This enumeration represents the Brillig foreign calls that are natively supported by nargo.
+/// After resolution of a foreign call, nargo will restart execution of the ACVM
+pub(crate) enum ForeignCall {
+    Println,
+    Sequence,
+    ReverseSequence,
+}
+
+impl std::fmt::Display for ForeignCall {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.name())
+    }
+}
+
+impl ForeignCall {
+    pub(crate) fn name(&self) -> &'static str {
+        match self {
+            ForeignCall::Println => "println",
+            ForeignCall::Sequence => "get_number_sequence",
+            ForeignCall::ReverseSequence => "get_reverse_number_sequence",
+        }
+    }
+
+    pub(crate) fn lookup(op_name: &str) -> Option<ForeignCall> {
+        match op_name {
+            "println" => Some(ForeignCall::Println),
+            "get_number_sequence" => Some(ForeignCall::Sequence),
+            "get_reverse_number_sequence" => Some(ForeignCall::ReverseSequence),
+            _ => None,
+        }
+    }
+
+    pub(crate) fn execute(
+        foreign_call: &ForeignCallWaitInfo,
+    ) -> Result<ForeignCallResult, ForeignCallError> {
+        let foreign_call_name = foreign_call.function.as_str();
+        match Self::lookup(foreign_call_name) {
+            Some(ForeignCall::Println) => {
+                Self::execute_println(&foreign_call.inputs)?;
+                Ok(ForeignCallResult { values: vec![] })
+            }
+            Some(ForeignCall::Sequence) => {
+                let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128();
+
+                Ok(vecmap(0..sequence_length, Value::from).into())
+            }
+            Some(ForeignCall::ReverseSequence) => {
+                let sequence_length: u128 = foreign_call.inputs[0][0].to_field().to_u128();
+
+                Ok(vecmap((0..sequence_length).rev(), Value::from).into())
+            }
+            None => panic!("unexpected foreign call {:?}", foreign_call_name),
+        }
+    }
+
+    fn execute_println(foreign_call_inputs: &[Vec<Value>]) -> Result<(), ForeignCallError> {
+        let (abi_type, input_values) = fetch_abi_type(foreign_call_inputs)?;
+
+        // We must use a flat map here as each value in a struct will be in a separate input value
+        let mut input_values_as_fields =
+            input_values.iter().flat_map(|values| values.iter().map(|value| value.to_field()));
+        let decoded_value = decode_value(&mut input_values_as_fields, &abi_type)?;
+
+        let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type)?;
+
+        println!("{json_value}");
+        Ok(())
+    }
+}
+
+/// Fetch the abi type from the foreign call input
+/// The remaining input values should hold the values to be printed
+fn fetch_abi_type(
+    foreign_call_inputs: &[Vec<Value>],
+) -> Result<(AbiType, &[Vec<Value>]), ForeignCallError> {
+    let (abi_type_as_values, input_values) =
+        foreign_call_inputs.split_last().ok_or(ForeignCallError::MissingForeignCallInputs)?;
+    let abi_type_as_fields = vecmap(abi_type_as_values, |value| value.to_field());
+    let abi_type_as_string = decode_string_value(&abi_type_as_fields);
+    let abi_type: AbiType = serde_json::from_str(&abi_type_as_string)
+        .map_err(|err| ForeignCallError::InputParserError(err.into()))?;
+
+    Ok((abi_type, input_values))
+}

crates/nargo/src/ops/mod.rs

@@ -6,6 +6,7 @@ pub use self::verify::verify_proof;
 
 mod codegen_verifier;
 mod execute;
+mod foreign_calls;
 mod preprocess;
 mod prove;
 mod verify;

crates/nargo_cli/src/cli/check_cmd.rs

@@ -220,9 +220,9 @@ pub(crate) fn check_crate_and_report_errors(
     context: &mut Context,
     crate_id: CrateId,
     deny_warnings: bool,
-    enable_slices: bool,
+    experimental_ssa: bool,
 ) -> Result<(), ReportedErrors> {
-    let result =
-        check_crate(context, crate_id, deny_warnings, enable_slices).map(|warnings| ((), warnings));
+    let result = check_crate(context, crate_id, deny_warnings, experimental_ssa)
+        .map(|warnings| ((), warnings));
     super::compile_cmd::report_errors(result, context, deny_warnings)
 }

crates/nargo_cli/tests/test_data_ssa_refactor/brillig_oracle/src/main.nr

@@ -2,31 +2,9 @@ use dep::std::slice;
 
 // Tests oracle usage in brillig/unconstrained functions
 fn main(x: Field)  {
-    // call through a brillig wrapper
-    oracle_print_array_wrapper([x, x]);
-
-    // TODO(#1615) Nargo currently only supports resolving one foreign call
-    // oracle_print_wrapper(x);
-
     get_number_sequence_wrapper(20);
 }
 
-// TODO(#1911)
-#[oracle(oracle_print_impl)]
-unconstrained fn oracle_print(_x : Field) -> Field {}
-
-unconstrained fn oracle_print_wrapper(x: Field) {
-    oracle_print(x);
-}
-
-// TODO(#1911)
-#[oracle(oracle_print_array_impl)]
-unconstrained fn oracle_print_array(_arr : [Field; 2]) -> Field {}
-
-unconstrained fn oracle_print_array_wrapper(arr: [Field; 2]) {
-    oracle_print_array(arr);
-}
-
 // TODO(#1911): This function does not need to be an oracle but acts 
 // as a useful test while we finalize code generation for slices in Brillig
 #[oracle(get_number_sequence)]

crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/Nargo.toml

@@ -0,0 +1,5 @@
+[package]
+authors = [""]
+compiler_version = "0.8.0"
+
+[dependencies]

crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/Prover.toml

@@ -0,0 +1,2 @@
+x = "5"
+y = "10"

crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr

@@ -0,0 +1,22 @@
+use dep::std;
+
+fn main(x : Field, y : pub Field) {
+
+    std::println("*** println ***");
+    std::println(x);
+    std::println([x, y]);
+
+    let s = myStruct { y: x, x: y };
+    let foo = fooStruct { my_struct: s, foo: 15 };
+    std::println(foo);
+}
+
+struct myStruct {
+    y: Field,
+    x: Field,
+}
+
+struct fooStruct {
+    my_struct: myStruct,
+    foo: Field,
+}

crates/noirc_abi/src/input_parser/json.rs

@@ -59,7 +59,7 @@ pub(crate) fn serialize_to_json(
 
 #[derive(Debug, Deserialize, Serialize, Clone)]
 #[serde(untagged)]
-enum JsonTypes {
+pub enum JsonTypes {
     // This is most likely going to be a hex string
     // But it is possible to support UTF-8
     String(String),
@@ -78,14 +78,13 @@ enum JsonTypes {
 }
 
 impl JsonTypes {
-    fn try_from_input_value(
+    pub fn try_from_input_value(
         value: &InputValue,
         abi_type: &AbiType,
     ) -> Result<JsonTypes, InputParserError> {
         let json_value = match (value, abi_type) {
             (InputValue::Field(f), AbiType::Field | AbiType::Integer { .. }) => {
-                let f_str = format!("0x{}", f.to_hex());
-                JsonTypes::String(f_str)
+                JsonTypes::String(Self::format_field_string(*f))
             }
             (InputValue::Field(f), AbiType::Boolean) => JsonTypes::Bool(f.is_one()),
 
@@ -109,6 +108,21 @@ impl JsonTypes {
         };
         Ok(json_value)
     }
+
+    /// This trims any leading zeroes.
+    /// A singular '0' will be prepended as well if the trimmed string has an odd length.
+    /// A hex string's length needs to be even to decode into bytes, as two digits correspond to
+    /// one byte.
+    fn format_field_string(field: FieldElement) -> String {
+        if field.is_zero() {
+            return "0x00".to_owned();
+        }
+        let mut trimmed_field = field.to_hex().trim_start_matches('0').to_owned();
+        if trimmed_field.len() % 2 != 0 {
+            trimmed_field = "0".to_owned() + &trimmed_field;
+        }
+        "0x".to_owned() + &trimmed_field
+    }
 }
 
 impl InputValue {
@@ -161,3 +175,13 @@ impl InputValue {
         Ok(input_value)
     }
 }
+
+impl std::fmt::Display for JsonTypes {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        // From the docs: https://doc.rust-lang.org/std/fmt/struct.Error.html
+        // This type does not support transmission of an error other than that an error
+        // occurred. Any extra information must be arranged to be transmitted through
+        // some other means.
+        write!(f, "{}", serde_json::to_string(&self).map_err(|_| std::fmt::Error)?)
+    }
+}

crates/noirc_abi/src/input_parser/mod.rs

@@ -1,4 +1,4 @@
-mod json;
+pub mod json;
 mod toml;
 
 use std::collections::BTreeMap;