From a2d034505ebaa8097877cbcd0619ca930b251d6c Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Fri, 18 Aug 2023 14:54:36 -0700 Subject: [PATCH 1/9] chore: Decouple `noirc_abi` from frontend by introducing `PrintableType` --- Cargo.lock | 14 +- Cargo.toml | 2 + crates/nargo/Cargo.toml | 1 + crates/nargo/src/ops/foreign_calls.rs | 53 +++-- crates/noirc_abi/Cargo.toml | 1 + crates/noirc_abi/src/input_parser/mod.rs | 33 +-- crates/noirc_abi/src/lib.rs | 75 ++++++- crates/noirc_driver/src/lib.rs | 2 +- crates/noirc_evaluator/src/ssa/abi_gen/mod.rs | 6 +- crates/noirc_frontend/Cargo.toml | 3 +- crates/noirc_frontend/src/ast/expression.rs | 3 +- crates/noirc_frontend/src/ast/mod.rs | 21 -- crates/noirc_frontend/src/hir_def/types.rs | 107 +++++----- .../src/monomorphization/mod.rs | 24 +-- crates/noirc_printable_type/Cargo.toml | 14 ++ crates/noirc_printable_type/src/lib.rs | 199 ++++++++++++++++++ 16 files changed, 407 insertions(+), 151 deletions(-) create mode 100644 crates/noirc_printable_type/Cargo.toml create mode 100644 crates/noirc_printable_type/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index be1270f4fa2..d032e2074eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2234,6 +2234,7 @@ dependencies = [ "noirc_driver", "noirc_errors", "noirc_frontend", + "noirc_printable_type", "regex", "rustc_version", "serde", @@ -2354,6 +2355,7 @@ version = "0.10.3" dependencies = [ "acvm", "iter-extended", + "noirc_frontend", "serde", "serde_json", "strum", @@ -2413,11 +2415,10 @@ dependencies = [ "chumsky", "fm", "iter-extended", - "noirc_abi", "noirc_errors", + "noirc_printable_type", "regex", "rustc-hash", - "serde", "serde_json", "small-ord-set", "smol_str", @@ -2426,6 +2427,15 @@ dependencies = [ "thiserror", ] +[[package]] +name = "noirc_printable_type" +version = "0.10.3" +dependencies = [ + "acvm", + "iter-extended", + "serde", +] + [[package]] name = "nom" version = "7.1.3" diff --git a/Cargo.toml b/Cargo.toml index 76ec9edfa0d..998eb2bb65b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,6 +5,7 @@ members = [ "crates/noirc_frontend", "crates/noirc_errors", "crates/noirc_driver", + "crates/noirc_printable_type", "crates/nargo", "crates/nargo_cli", "crates/nargo_toml", @@ -38,6 +39,7 @@ noirc_driver = { path = "crates/noirc_driver" } noirc_errors = { path = "crates/noirc_errors" } noirc_evaluator = { path = "crates/noirc_evaluator" } noirc_frontend = { path = "crates/noirc_frontend" } +noirc_printable_type = { path = "crates/noirc_printable_type" } noir_wasm = { path = "crates/wasm" } cfg-if = "1.0.0" clap = { version = "4.3.19", features = ["derive"] } diff --git a/crates/nargo/Cargo.toml b/crates/nargo/Cargo.toml index 32ca04ad34f..cb72ba7b072 100644 --- a/crates/nargo/Cargo.toml +++ b/crates/nargo/Cargo.toml @@ -16,6 +16,7 @@ fm.workspace = true noirc_abi.workspace = true noirc_driver.workspace = true noirc_frontend.workspace = true +noirc_printable_type.workspace = true iter-extended.workspace = true serde.workspace = true serde_json.workspace = true diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index e1768d71c44..b94220110d4 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -3,7 +3,9 @@ use acvm::{ pwg::ForeignCallWaitInfo, }; use iter_extended::vecmap; -use noirc_abi::{decode_string_value, input_parser::InputValueDisplay, AbiType}; +use noirc_printable_type::{ + decode_string_value, decode_value, PrintableType, PrintableValueDisplay, +}; use regex::{Captures, Regex}; use crate::errors::ForeignCallError; @@ -93,18 +95,18 @@ impl ForeignCall { } fn convert_string_inputs(foreign_call_inputs: &[Vec]) -> Result { - // Fetch the abi type from the foreign call input + // Fetch the PrintableType from the foreign call input // The remaining input values should hold what is to be printed - let (abi_type_as_values, input_values) = + let (printable_type_as_values, input_values) = foreign_call_inputs.split_last().ok_or(ForeignCallError::MissingForeignCallInputs)?; - let abi_type = fetch_abi_type(abi_type_as_values)?; + let printable_type = fetch_printable_type(printable_type_as_values)?; // 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| vecmap(values, |value| value.to_field())); - let input_value_display = - InputValueDisplay::try_from_fields(&mut input_values_as_fields, abi_type)?; + let value = decode_value(&mut input_values_as_fields, &printable_type); + let input_value_display = PrintableValueDisplay::new(&value, &printable_type); Ok(input_value_display.to_string()) } @@ -112,34 +114,37 @@ fn convert_string_inputs(foreign_call_inputs: &[Vec]) -> Result], ) -> Result { - let (message_as_values, input_and_abi_values) = + let (message_as_values, input_and_printable_values) = foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; let message_as_fields = vecmap(message_as_values, |value| value.to_field()); let message_as_string = decode_string_value(&message_as_fields); - let (num_values, input_and_abi_values) = - input_and_abi_values.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; + let (num_values, input_and_printable_values) = input_and_printable_values + .split_first() + .ok_or(ForeignCallError::MissingForeignCallInputs)?; let mut output_strings = Vec::new(); let num_values = num_values[0].to_field().to_u128() as usize; - let mut abi_types = Vec::new(); - for abi_values in input_and_abi_values.iter().skip(input_and_abi_values.len() - num_values) { - let abi_type = fetch_abi_type(abi_values)?; - abi_types.push(abi_type); + let mut printable_types = Vec::new(); + for printable_value in + input_and_printable_values.iter().skip(input_and_printable_values.len() - num_values) + { + let printable_type = fetch_printable_type(printable_value)?; + printable_types.push(printable_type); } for i in 0..num_values { - let abi_type = &abi_types[i]; - let type_size = abi_type.field_count() as usize; + let printable_type = &printable_types[i]; + let type_size = printable_type.field_count() as usize; - let mut input_values_as_fields = input_and_abi_values[i..(i + type_size)] + let mut input_values_as_fields = input_and_printable_values[i..(i + type_size)] .iter() .flat_map(|values| vecmap(values, |value| value.to_field())); - let input_value_display = - InputValueDisplay::try_from_fields(&mut input_values_as_fields, abi_type.clone())?; + let value = decode_value(&mut input_values_as_fields, printable_type); + let input_value_display = PrintableValueDisplay::new(&value, printable_type); output_strings.push(input_value_display.to_string()); } @@ -157,11 +162,13 @@ fn convert_fmt_string_inputs( Ok(formatted_str.into_owned()) } -fn fetch_abi_type(abi_type_as_values: &[Value]) -> Result { - 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) +fn fetch_printable_type( + printable_type_as_values: &[Value], +) -> Result { + let printable_type_as_fields = vecmap(printable_type_as_values, |value| value.to_field()); + let printable_type_as_string = decode_string_value(&printable_type_as_fields); + let printable_type: PrintableType = serde_json::from_str(&printable_type_as_string) .map_err(|err| ForeignCallError::InputParserError(err.into()))?; - Ok(abi_type) + Ok(printable_type) } diff --git a/crates/noirc_abi/Cargo.toml b/crates/noirc_abi/Cargo.toml index 6af0cfe78b3..45466061fde 100644 --- a/crates/noirc_abi/Cargo.toml +++ b/crates/noirc_abi/Cargo.toml @@ -9,6 +9,7 @@ edition.workspace = true [dependencies] acvm.workspace = true iter-extended.workspace = true +noirc_frontend.workspace = true toml.workspace = true serde_json = "1.0" serde.workspace = true diff --git a/crates/noirc_abi/src/input_parser/mod.rs b/crates/noirc_abi/src/input_parser/mod.rs index 3a317697534..11d40f338d5 100644 --- a/crates/noirc_abi/src/input_parser/mod.rs +++ b/crates/noirc_abi/src/input_parser/mod.rs @@ -6,8 +6,8 @@ use std::collections::BTreeMap; use acvm::FieldElement; use serde::Serialize; -use crate::errors::{AbiError, InputParserError}; -use crate::{decode_value, Abi, AbiType}; +use crate::errors::InputParserError; +use crate::{Abi, AbiType}; /// This is what all formats eventually transform into /// For example, a toml file will parse into TomlTypes /// and those TomlTypes will be mapped to Value @@ -67,35 +67,6 @@ impl InputValue { } } -/// In order to display an `InputValue` we need an `AbiType` to accurately -/// convert the value into a human-readable format. -pub struct InputValueDisplay { - input_value: InputValue, - abi_type: AbiType, -} - -impl InputValueDisplay { - pub fn try_from_fields( - field_iterator: &mut impl Iterator, - abi_type: AbiType, - ) -> Result { - let input_value = decode_value(field_iterator, &abi_type)?; - Ok(InputValueDisplay { input_value, abi_type }) - } -} - -impl std::fmt::Display for InputValueDisplay { - 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. - let json_value = json::JsonTypes::try_from_input_value(&self.input_value, &self.abi_type) - .map_err(|_| std::fmt::Error)?; - write!(f, "{}", serde_json::to_string(&json_value).map_err(|_| std::fmt::Error)?) - } -} - /// The different formats that are supported when parsing /// the initial witness values #[cfg_attr(test, derive(strum_macros::EnumIter))] diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index b68a03f270d..cc5144df00a 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -12,6 +12,7 @@ use acvm::{ use errors::AbiError; use input_parser::InputValue; use iter_extended::{try_btree_map, try_vecmap, vecmap}; +use noirc_frontend::{Signedness, Type, TypeBinding, TypeVariableKind, Visibility}; use serde::{Deserialize, Serialize}; // This is the ABI used to bridge the different TOML formats for the initial // witness, the partial witness generator and the interpreter. @@ -74,6 +75,24 @@ pub enum AbiVisibility { Private, } +impl From for AbiVisibility { + fn from(value: Visibility) -> Self { + match value { + Visibility::Public => AbiVisibility::Public, + Visibility::Private => AbiVisibility::Private, + } + } +} + +impl From<&Visibility> for AbiVisibility { + fn from(value: &Visibility) -> Self { + match value { + Visibility::Public => AbiVisibility::Public, + Visibility::Private => AbiVisibility::Private, + } + } +} + #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] /// Represents whether the return value should compromise of unique witness indices such that no @@ -97,6 +116,60 @@ pub enum Sign { } impl AbiType { + // TODO: Add `Context` argument for resolving fully qualified struct paths + pub fn from_type(typ: &Type) -> Self { + // Note; use strict_eq instead of partial_eq when comparing field types + // in this method, you most likely want to distinguish between public and private + match typ { + Type::FieldElement => Self::Field, + Type::Array(size, typ) => { + let length = size + .evaluate_to_u64() + .expect("Cannot have variable sized arrays as a parameter to main"); + let typ = typ.as_ref(); + Self::Array { length, typ: Box::new(Self::from_type(typ)) } + } + Type::Integer(sign, bit_width) => { + let sign = match sign { + Signedness::Unsigned => Sign::Unsigned, + Signedness::Signed => Sign::Signed, + }; + + Self::Integer { sign, width: *bit_width } + } + Type::TypeVariable(binding, TypeVariableKind::IntegerOrField) => { + match &*binding.borrow() { + TypeBinding::Bound(typ) => Self::from_type(typ), + TypeBinding::Unbound(_) => Self::from_type(&Type::default_int_type()), + } + } + Type::Bool => Self::Boolean, + Type::String(size) => { + let size = size + .evaluate_to_u64() + .expect("Cannot have variable sized strings as a parameter to main"); + Self::String { length: size } + } + Type::FmtString(_, _) => unreachable!("format strings cannot be used in the abi"), + Type::Error => unreachable!(), + Type::Unit => unreachable!(), + Type::Constant(_) => unreachable!(), + Type::Struct(def, ref args) => { + let struct_type = def.borrow(); + let fields = struct_type.get_fields(args); + let fields = vecmap(fields, |(name, typ)| (name, Self::from_type(&typ))); + Self::Struct { fields, name: struct_type.name.to_string() } + } + Type::Tuple(_) => todo!("as_abi_type not yet implemented for tuple types"), + Type::TypeVariable(_, _) => unreachable!(), + Type::NamedGeneric(..) => unreachable!(), + Type::Forall(..) => unreachable!(), + Type::Function(_, _, _) => unreachable!(), + Type::MutableReference(_) => unreachable!("&mut cannot be used in the abi"), + Type::NotConstant => unreachable!(), + } + } + /// Returns the number of field elements required to represent the type once encoded. pub fn field_count(&self) -> u32 { match self { @@ -390,7 +463,7 @@ fn decode_value( Ok(value) } -pub fn decode_string_value(field_elements: &[FieldElement]) -> String { +fn decode_string_value(field_elements: &[FieldElement]) -> String { let string_as_slice = vecmap(field_elements, |e| { let mut field_as_bytes = e.to_be_bytes(); let char_byte = field_as_bytes.pop().unwrap(); // A character in a string is represented by a u8, thus we just want the last byte of the element diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index 1192416b98f..9fcc412d5a4 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -135,7 +135,7 @@ pub fn compute_function_abi( let (parameters, return_type) = func_meta.into_function_signature(); let parameters = into_abi_params(parameters, &context.def_interner); - let return_type = return_type.map(|typ| typ.as_abi_type()); + let return_type = return_type.map(|typ| AbiType::from_type(&typ)); Some((parameters, return_type)) } diff --git a/crates/noirc_evaluator/src/ssa/abi_gen/mod.rs b/crates/noirc_evaluator/src/ssa/abi_gen/mod.rs index cfefb3c9f73..f2c61715e37 100644 --- a/crates/noirc_evaluator/src/ssa/abi_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/abi_gen/mod.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use acvm::acir::native_types::Witness; use iter_extended::{btree_map, vecmap}; -use noirc_abi::{Abi, AbiParameter}; +use noirc_abi::{Abi, AbiParameter, AbiType}; use noirc_frontend::{ hir_def::{ function::{FunctionSignature, Param}, @@ -27,7 +27,7 @@ pub fn into_abi_params(params: Vec, interner: &NodeInterner) -> Vec Abi { let (parameters, return_type) = func_sig; let parameters = into_abi_params(parameters, interner); - let return_type = return_type.map(|typ| typ.as_abi_type()); + let return_type = return_type.map(|typ| AbiType::from_type(&typ)); let param_witnesses = param_witnesses_from_abi_param(¶meters, input_witnesses); Abi { parameters, return_type, param_witnesses, return_witnesses } } diff --git a/crates/noirc_frontend/Cargo.toml b/crates/noirc_frontend/Cargo.toml index 1f902d2d399..636f2e74b2a 100644 --- a/crates/noirc_frontend/Cargo.toml +++ b/crates/noirc_frontend/Cargo.toml @@ -8,15 +8,14 @@ edition.workspace = true [dependencies] acvm.workspace = true -noirc_abi.workspace = true noirc_errors.workspace = true +noirc_printable_type.workspace = true fm.workspace = true arena.workspace = true iter-extended.workspace = true chumsky.workspace = true thiserror.workspace = true smol_str.workspace = true -serde.workspace = true serde_json.workspace = true rustc-hash = "1.1.0" small-ord-set = "0.1.3" diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index b1ef26a7cd8..8b15f6e3b9d 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -382,8 +382,7 @@ pub enum FunctionReturnType { /// Describes the types of smart contract functions that are allowed. /// - All Noir programs in the non-contract context can be seen as `Secret`. -#[derive(serde::Serialize, serde::Deserialize, Debug, Copy, Clone, PartialEq, Eq)] -#[serde(rename_all = "snake_case")] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum ContractFunctionType { /// This function will be executed in a private /// context. diff --git a/crates/noirc_frontend/src/ast/mod.rs b/crates/noirc_frontend/src/ast/mod.rs index 19e0d5d2e62..0a4e69aa55f 100644 --- a/crates/noirc_frontend/src/ast/mod.rs +++ b/crates/noirc_frontend/src/ast/mod.rs @@ -14,7 +14,6 @@ mod type_alias; pub use expression::*; pub use function::*; -use noirc_abi::AbiVisibility; use noirc_errors::Span; pub use statement::*; pub use structure::*; @@ -248,26 +247,6 @@ impl std::fmt::Display for Visibility { } } -// TODO: Move this into noirc_abi when it depends upon noirc_frontend (instead of other way around) -impl From for AbiVisibility { - fn from(value: Visibility) -> Self { - match value { - Visibility::Public => AbiVisibility::Public, - Visibility::Private => AbiVisibility::Private, - } - } -} - -// TODO: Move this into noirc_abi when it depends upon noirc_frontend (instead of other way around) -impl From<&Visibility> for AbiVisibility { - fn from(value: &Visibility) -> Self { - match value { - Visibility::Public => AbiVisibility::Public, - Visibility::Private => AbiVisibility::Private, - } - } -} - #[derive(Clone, Copy, Debug, PartialEq, Eq)] /// Represents whether the return value should compromise of unique witness indices such that no /// index occurs within the program's abi more than once. diff --git a/crates/noirc_frontend/src/hir_def/types.rs b/crates/noirc_frontend/src/hir_def/types.rs index 1f2b9e8be74..7988c20d244 100644 --- a/crates/noirc_frontend/src/hir_def/types.rs +++ b/crates/noirc_frontend/src/hir_def/types.rs @@ -9,8 +9,8 @@ use crate::{ node_interner::{ExprId, NodeInterner, TypeAliasId}, }; use iter_extended::vecmap; -use noirc_abi::AbiType; use noirc_errors::Span; +use noirc_printable_type::PrintableType; use crate::{node_interner::StructId, node_interner::TraitId, Ident, Signedness}; @@ -1012,58 +1012,6 @@ impl Type { } } - // Note; use strict_eq instead of partial_eq when comparing field types - // in this method, you most likely want to distinguish between public and private - pub fn as_abi_type(&self) -> AbiType { - match self { - Type::FieldElement => AbiType::Field, - Type::Array(size, typ) => { - let length = size - .evaluate_to_u64() - .expect("Cannot have variable sized arrays as a parameter to main"); - AbiType::Array { length, typ: Box::new(typ.as_abi_type()) } - } - Type::Integer(sign, bit_width) => { - let sign = match sign { - Signedness::Unsigned => noirc_abi::Sign::Unsigned, - Signedness::Signed => noirc_abi::Sign::Signed, - }; - - AbiType::Integer { sign, width: *bit_width } - } - Type::TypeVariable(binding, TypeVariableKind::IntegerOrField) => { - match &*binding.borrow() { - TypeBinding::Bound(typ) => typ.as_abi_type(), - TypeBinding::Unbound(_) => Type::default_int_type().as_abi_type(), - } - } - Type::Bool => AbiType::Boolean, - Type::String(size) => { - let size = size - .evaluate_to_u64() - .expect("Cannot have variable sized strings as a parameter to main"); - AbiType::String { length: size } - } - Type::FmtString(_, _) => unreachable!("format strings cannot be used in the abi"), - Type::Error => unreachable!(), - Type::Unit => unreachable!(), - Type::Constant(_) => unreachable!(), - Type::Struct(def, args) => { - let struct_type = def.borrow(); - let fields = struct_type.get_fields(args); - let fields = vecmap(fields, |(name, typ)| (name, typ.as_abi_type())); - AbiType::Struct { fields, name: struct_type.name.to_string() } - } - Type::Tuple(_) => todo!("as_abi_type not yet implemented for tuple types"), - Type::TypeVariable(_, _) => unreachable!(), - Type::NamedGeneric(..) => unreachable!(), - Type::Forall(..) => unreachable!(), - Type::Function(_, _, _) => unreachable!(), - Type::MutableReference(_) => unreachable!("&mut cannot be used in the abi"), - Type::NotConstant => unreachable!(), - } - } - /// Iterate over the fields of this type. /// Panics if the type is not a struct or tuple. pub fn iter_fields(&self) -> impl Iterator { @@ -1334,3 +1282,56 @@ impl TypeVariableKind { } } } + +impl From for PrintableType { + fn from(value: Type) -> Self { + Self::from(&value) + } +} + +impl From<&Type> for PrintableType { + fn from(value: &Type) -> Self { + // Note; use strict_eq instead of partial_eq when comparing field types + // in this method, you most likely want to distinguish between public and private + match value { + Type::FieldElement => PrintableType::Field, + Type::Array(size, typ) => { + let length = size.evaluate_to_u64().expect("Cannot print variable sized arrays"); + let typ = typ.as_ref(); + PrintableType::Array { length, typ: Box::new(typ.into()) } + } + Type::Integer(sign, bit_width) => match sign { + Signedness::Unsigned => PrintableType::UnsignedInteger { width: *bit_width }, + Signedness::Signed => PrintableType::SignedInteger { width: *bit_width }, + }, + Type::TypeVariable(binding, TypeVariableKind::IntegerOrField) => { + match &*binding.borrow() { + TypeBinding::Bound(typ) => typ.into(), + TypeBinding::Unbound(_) => Type::default_int_type().into(), + } + } + Type::Bool => PrintableType::Boolean, + Type::String(size) => { + let size = size.evaluate_to_u64().expect("Cannot print variable sized strings"); + PrintableType::String { length: size } + } + Type::FmtString(_, _) => unreachable!("format strings cannot be printed"), + Type::Error => unreachable!(), + Type::Unit => unreachable!(), + Type::Constant(_) => unreachable!(), + Type::Struct(def, ref args) => { + let struct_type = def.borrow(); + let fields = struct_type.get_fields(args); + let fields = vecmap(fields, |(name, typ)| (name, typ.into())); + PrintableType::Struct { fields, name: struct_type.name.to_string() } + } + Type::Tuple(_) => todo!("printing tuple types is not yet implemented"), + Type::TypeVariable(_, _) => unreachable!(), + Type::NamedGeneric(..) => unreachable!(), + Type::Forall(..) => unreachable!(), + Type::Function(_, _, _) => unreachable!(), + Type::MutableReference(_) => unreachable!("cannot print &mut"), + Type::NotConstant => unreachable!(), + } + } +} diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 0d3297bf8a4..477c923c4e8 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -10,6 +10,7 @@ //! function, will monomorphize the entire reachable program. use acvm::FieldElement; use iter_extended::{btree_map, vecmap}; +use noirc_printable_type::PrintableType; use std::collections::{BTreeMap, HashMap, VecDeque}; use crate::{ @@ -775,7 +776,7 @@ impl<'interner> Monomorphizer<'interner> { if name.as_str() == "println" { // Oracle calls are required to be wrapped in an unconstrained function // Thus, the only argument to the `println` oracle is expected to always be an ident - self.append_abi_arg(&hir_arguments[0], &mut arguments); + self.append_printable_type_info(&hir_arguments[0], &mut arguments); } } } @@ -828,17 +829,16 @@ impl<'interner> Monomorphizer<'interner> { } /// Adds a function argument that contains type metadata that is required to tell - /// a caller (such as nargo) how to convert values passed to an foreign call - /// back to a human-readable string. + /// `println` how to convert values passed to an foreign call back to a human-readable string. /// The values passed to an foreign call will be a simple list of field elements, /// thus requiring extra metadata to correctly decode this list of elements. /// - /// The Noir compiler has an `AbiType` that handles encoding/decoding a list + /// The Noir compiler has a `PrintableType` that handles encoding/decoding a list /// of field elements to/from JSON. The type metadata attached in this method - /// is the serialized `AbiType` for the argument passed to the function. - /// The caller that is running a Noir program should then deserialize the `AbiType`, + /// is the serialized `PrintableType` for the argument passed to the function. + /// The caller that is running a Noir program should then deserialize the `PrintableType`, /// and accurately decode the list of field elements passed to the foreign call. - fn append_abi_arg( + fn append_printable_type_info( &mut self, hir_argument: &HirExpression, arguments: &mut Vec, @@ -854,7 +854,7 @@ impl<'interner> Monomorphizer<'interner> { match *elements { Type::Tuple(element_types) => { for typ in element_types { - Self::append_abi_arg_inner(&typ, arguments); + Self::append_printable_type_info_inner(&typ, arguments); } } _ => unreachable!( @@ -864,7 +864,7 @@ impl<'interner> Monomorphizer<'interner> { true } _ => { - Self::append_abi_arg_inner(&typ, arguments); + Self::append_printable_type_info_inner(&typ, arguments); false } }; @@ -875,15 +875,15 @@ impl<'interner> Monomorphizer<'interner> { } } - fn append_abi_arg_inner(typ: &Type, arguments: &mut Vec) { + fn append_printable_type_info_inner(typ: &Type, arguments: &mut Vec) { if let HirType::Array(size, _) = typ { if let HirType::NotConstant = **size { unreachable!("println does not support slices. Convert the slice to an array before passing it to println"); } } - let abi_type = typ.as_abi_type(); + let printable_type: PrintableType = typ.into(); let abi_as_string = - serde_json::to_string(&abi_type).expect("ICE: expected Abi type to serialize"); + serde_json::to_string(&printable_type).expect("ICE: expected Abi type to serialize"); arguments.push(ast::Expression::Literal(ast::Literal::Str(abi_as_string))); } diff --git a/crates/noirc_printable_type/Cargo.toml b/crates/noirc_printable_type/Cargo.toml new file mode 100644 index 00000000000..3ef4944ca12 --- /dev/null +++ b/crates/noirc_printable_type/Cargo.toml @@ -0,0 +1,14 @@ +[package] +name = "noirc_printable_type" +version.workspace = true +authors.workspace = true +edition.workspace = true + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] +acvm.workspace = true +iter-extended.workspace = true +serde.workspace = true + +[dev-dependencies] diff --git a/crates/noirc_printable_type/src/lib.rs b/crates/noirc_printable_type/src/lib.rs new file mode 100644 index 00000000000..fff09954bab --- /dev/null +++ b/crates/noirc_printable_type/src/lib.rs @@ -0,0 +1,199 @@ +use std::{collections::BTreeMap, str}; + +use acvm::FieldElement; +use iter_extended::vecmap; +use serde::{Deserialize, Serialize}; + +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "kind", rename_all = "lowercase")] +pub enum PrintableType { + Field, + Array { + length: u64, + #[serde(rename = "type")] + typ: Box, + }, + SignedInteger { + width: u32, + }, + UnsignedInteger { + width: u32, + }, + Boolean, + Struct { + name: String, + fields: Vec<(String, PrintableType)>, + }, + String { + length: u64, + }, +} + +impl PrintableType { + /// Returns the number of field elements required to represent the type once encoded. + pub fn field_count(&self) -> u32 { + match self { + Self::Field + | Self::SignedInteger { .. } + | Self::UnsignedInteger { .. } + | Self::Boolean => 1, + Self::Array { length, typ } => typ.field_count() * (*length as u32), + Self::Struct { fields, .. } => { + fields.iter().fold(0, |acc, (_, field_type)| acc + field_type.field_count()) + } + Self::String { length } => *length as u32, + } + } +} + +/// This is what all formats eventually transform into +/// For example, a toml file will parse into TomlTypes +/// and those TomlTypes will be mapped to Value +#[derive(Debug, Clone, Serialize, PartialEq)] +pub enum PrintableValue { + Field(FieldElement), + String(String), + Vec(Vec), + Struct(BTreeMap), +} + +/// In order to display an `PrintableValue` we need an `PrintableType` to accurately +/// convert the value into a human-readable format. +pub struct PrintableValueDisplay<'a> { + value: &'a PrintableValue, + typ: &'a PrintableType, +} + +impl<'a> PrintableValueDisplay<'a> { + #[must_use] + pub fn new(value: &'a PrintableValue, typ: &'a PrintableType) -> Self { + Self { value, typ } + } +} + +impl<'a> std::fmt::Display for PrintableValueDisplay<'a> { + fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match (&self.value, &self.typ) { + ( + PrintableValue::Field(f), + PrintableType::Field + // TODO: We should print the sign for these and probably print normal integers instead of field strings + | PrintableType::SignedInteger { .. } + | PrintableType::UnsignedInteger { .. }, + ) => { + write!(fmt, "{}", format_field_string(*f))?; + } + (PrintableValue::Field(f), PrintableType::Boolean) => { + if f.is_one() { + write!(fmt, "true")?; + } else { + write!(fmt, "false")?; + } + } + (PrintableValue::Vec(vector), PrintableType::Array { typ, .. }) => { + write!(fmt, "[")?; + let mut values = vector.iter().peekable(); + while let Some(value) = values.next() { + write!(fmt, "{}", PrintableValueDisplay::new(value, typ))?; + if values.peek().is_some() { + write!(fmt, ", ")?; + } + } + write!(fmt, "]")?; + } + + (PrintableValue::String(s), PrintableType::String { .. }) => { + write!(fmt, r#""{s}""#)?; + } + + (PrintableValue::Struct(map), PrintableType::Struct { name, fields, .. }) => { + write!(fmt, "{name} {{ ")?; + + let mut fields = fields.iter().peekable(); + while let Some((key, field_type)) = fields.next() { + let value = &map[key]; + write!(fmt, "{key}: {}", PrintableValueDisplay::new(value, field_type))?; + if fields.peek().is_some() { + write!(fmt, ", ")?; + } + } + + write!(fmt, " }}")?; + } + + _ => return Err(std::fmt::Error), + }; + Ok(()) + } +} + +/// 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 +} + +// TODO: Figure out a better API for this to avoid exporting the function +/// Assumes that `field_iterator` contains enough [FieldElement] in order to decode the [PrintableType] +pub fn decode_value( + field_iterator: &mut impl Iterator, + typ: &PrintableType, +) -> PrintableValue { + match typ { + PrintableType::Field + | PrintableType::SignedInteger { .. } + | PrintableType::UnsignedInteger { .. } + | PrintableType::Boolean => { + let field_element = field_iterator.next().unwrap(); + + PrintableValue::Field(field_element) + } + PrintableType::Array { length, typ } => { + let length = *length as usize; + let mut array_elements = Vec::with_capacity(length); + for _ in 0..length { + array_elements.push(decode_value(field_iterator, typ)); + } + + PrintableValue::Vec(array_elements) + } + PrintableType::String { length } => { + let field_elements: Vec = field_iterator.take(*length as usize).collect(); + + PrintableValue::String(decode_string_value(&field_elements)) + } + PrintableType::Struct { fields, .. } => { + let mut struct_map = BTreeMap::new(); + + for (field_key, param_type) in fields { + let field_value = decode_value(field_iterator, param_type); + + struct_map.insert(field_key.to_owned(), field_value); + } + + PrintableValue::Struct(struct_map) + } + } +} + +// TODO: Figure out a better API for this to avoid exporting the function +pub fn decode_string_value(field_elements: &[FieldElement]) -> String { + let string_as_slice = vecmap(field_elements, |e| { + let mut field_as_bytes = e.to_be_bytes(); + let char_byte = field_as_bytes.pop().unwrap(); // A character in a string is represented by a u8, thus we just want the last byte of the element + assert!(field_as_bytes.into_iter().all(|b| b == 0)); // Assert that the rest of the field element's bytes are empty + char_byte + }); + + let final_string = str::from_utf8(&string_as_slice).unwrap(); + final_string.to_owned() +} From 1f76e232eb07313f583d448bdb189d24e3888619 Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Fri, 18 Aug 2023 14:54:49 -0700 Subject: [PATCH 2/9] chore: Cleanup some unused error variants --- crates/noirc_abi/src/errors.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/noirc_abi/src/errors.rs b/crates/noirc_abi/src/errors.rs index 80f9d665dff..687fecfcc1d 100644 --- a/crates/noirc_abi/src/errors.rs +++ b/crates/noirc_abi/src/errors.rs @@ -10,8 +10,6 @@ pub enum InputParserError { ParseStr(String), #[error("Could not parse hex value {0}")] ParseHexStr(String), - #[error("duplicate variable name {0}")] - DuplicateVariableName(String), #[error("cannot parse value into {0:?}")] AbiTypeMismatch(AbiType), #[error("Expected argument `{0}`, but none was found")] @@ -38,8 +36,6 @@ impl From for InputParserError { #[derive(Debug, Error)] pub enum AbiError { - #[error("{0}")] - Generic(String), #[error("Received parameters not expected by ABI: {0:?}")] UnexpectedParams(Vec), #[error("The parameter {} is expected to be a {:?} but found incompatible value {value:?}", .param.name, .param.typ)] From 22a01b194a3cb52f508890b18933e0b63e68e0dc Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Mon, 21 Aug 2023 10:31:50 -0700 Subject: [PATCH 3/9] undo the full removal of serde, will do in followup PR --- Cargo.lock | 1 + crates/noirc_frontend/Cargo.toml | 1 + crates/noirc_frontend/src/ast/expression.rs | 3 ++- 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index d032e2074eb..840dc6d379f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2419,6 +2419,7 @@ dependencies = [ "noirc_printable_type", "regex", "rustc-hash", + "serde", "serde_json", "small-ord-set", "smol_str", diff --git a/crates/noirc_frontend/Cargo.toml b/crates/noirc_frontend/Cargo.toml index 636f2e74b2a..8d195b7ca7c 100644 --- a/crates/noirc_frontend/Cargo.toml +++ b/crates/noirc_frontend/Cargo.toml @@ -16,6 +16,7 @@ iter-extended.workspace = true chumsky.workspace = true thiserror.workspace = true smol_str.workspace = true +serde.workspace = true serde_json.workspace = true rustc-hash = "1.1.0" small-ord-set = "0.1.3" diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index 8b15f6e3b9d..b1ef26a7cd8 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -382,7 +382,8 @@ pub enum FunctionReturnType { /// Describes the types of smart contract functions that are allowed. /// - All Noir programs in the non-contract context can be seen as `Secret`. -#[derive(Debug, Copy, Clone, PartialEq, Eq)] +#[derive(serde::Serialize, serde::Deserialize, Debug, Copy, Clone, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] pub enum ContractFunctionType { /// This function will be executed in a private /// context. From 2bc6ce727edaf741c99d0514c076dde767f31c27 Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Mon, 21 Aug 2023 15:00:58 -0700 Subject: [PATCH 4/9] remove the display impl on abi now --- crates/noirc_abi/src/input_parser/json.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/crates/noirc_abi/src/input_parser/json.rs b/crates/noirc_abi/src/input_parser/json.rs index c8a421c8353..c6f1e304728 100644 --- a/crates/noirc_abi/src/input_parser/json.rs +++ b/crates/noirc_abi/src/input_parser/json.rs @@ -175,13 +175,3 @@ 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)?) - } -} From 47aa682eb5743c557bb2d67f49408e661c30b994 Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Tue, 22 Aug 2023 08:23:47 -0700 Subject: [PATCH 5/9] Apply suggestions from code review Co-authored-by: Maxim Vezenov --- crates/noirc_abi/src/lib.rs | 2 +- crates/noirc_frontend/src/monomorphization/mod.rs | 2 +- crates/noirc_printable_type/src/lib.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/noirc_abi/src/lib.rs b/crates/noirc_abi/src/lib.rs index cc5144df00a..76ecba9bff2 100644 --- a/crates/noirc_abi/src/lib.rs +++ b/crates/noirc_abi/src/lib.rs @@ -160,7 +160,7 @@ impl AbiType { let fields = vecmap(fields, |(name, typ)| (name, Self::from_type(&typ))); Self::Struct { fields, name: struct_type.name.to_string() } } - Type::Tuple(_) => todo!("as_abi_type not yet implemented for tuple types"), + Type::Tuple(_) => todo!("AbiType::from_type not yet implemented for tuple types"), Type::TypeVariable(_, _) => unreachable!(), Type::NamedGeneric(..) => unreachable!(), Type::Forall(..) => unreachable!(), diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 477c923c4e8..2ef980176d3 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -883,7 +883,7 @@ impl<'interner> Monomorphizer<'interner> { } let printable_type: PrintableType = typ.into(); let abi_as_string = - serde_json::to_string(&printable_type).expect("ICE: expected Abi type to serialize"); + serde_json::to_string(&printable_type).expect("ICE: expected PrintableType to serialize"); arguments.push(ast::Expression::Literal(ast::Literal::Str(abi_as_string))); } diff --git a/crates/noirc_printable_type/src/lib.rs b/crates/noirc_printable_type/src/lib.rs index fff09954bab..562cf1aed46 100644 --- a/crates/noirc_printable_type/src/lib.rs +++ b/crates/noirc_printable_type/src/lib.rs @@ -57,7 +57,7 @@ pub enum PrintableValue { Struct(BTreeMap), } -/// In order to display an `PrintableValue` we need an `PrintableType` to accurately +/// In order to display a `PrintableValue` we need a `PrintableType` to accurately /// convert the value into a human-readable format. pub struct PrintableValueDisplay<'a> { value: &'a PrintableValue, From 2b0697dcdcaeceb51a785a6481406bab53b575c5 Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Tue, 22 Aug 2023 08:27:10 -0700 Subject: [PATCH 6/9] Update crates/noirc_printable_type/src/lib.rs --- crates/noirc_printable_type/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_printable_type/src/lib.rs b/crates/noirc_printable_type/src/lib.rs index 562cf1aed46..8c7c543983f 100644 --- a/crates/noirc_printable_type/src/lib.rs +++ b/crates/noirc_printable_type/src/lib.rs @@ -77,7 +77,7 @@ impl<'a> std::fmt::Display for PrintableValueDisplay<'a> { ( PrintableValue::Field(f), PrintableType::Field - // TODO: We should print the sign for these and probably print normal integers instead of field strings + // TODO(#2401): We should print the sign for these and probably print normal integers instead of field strings | PrintableType::SignedInteger { .. } | PrintableType::UnsignedInteger { .. }, ) => { From 595040d55681342ad6672e64160ead34ecb07ab0 Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Tue, 22 Aug 2023 12:19:04 -0700 Subject: [PATCH 7/9] implement TryFrom and rework printing to avoid leaking internal functions --- Cargo.lock | 5 +- crates/nargo/Cargo.toml | 2 - crates/nargo/src/errors.rs | 16 +- crates/nargo/src/ops/foreign_calls.rs | 101 +----------- crates/noirc_printable_type/Cargo.toml | 3 + crates/noirc_printable_type/src/lib.rs | 217 ++++++++++++++++++------- 6 files changed, 175 insertions(+), 169 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 840dc6d379f..4deba52afa3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2235,10 +2235,8 @@ dependencies = [ "noirc_errors", "noirc_frontend", "noirc_printable_type", - "regex", "rustc_version", "serde", - "serde_json", "thiserror", ] @@ -2434,7 +2432,10 @@ version = "0.10.3" dependencies = [ "acvm", "iter-extended", + "regex", "serde", + "serde_json", + "thiserror", ] [[package]] diff --git a/crates/nargo/Cargo.toml b/crates/nargo/Cargo.toml index cb72ba7b072..25c73a3c025 100644 --- a/crates/nargo/Cargo.toml +++ b/crates/nargo/Cargo.toml @@ -19,8 +19,6 @@ noirc_frontend.workspace = true noirc_printable_type.workspace = true iter-extended.workspace = true serde.workspace = true -serde_json.workspace = true thiserror.workspace = true noirc_errors.workspace = true base64.workspace = true -regex = "1.9.1" diff --git a/crates/nargo/src/errors.rs b/crates/nargo/src/errors.rs index 2878d9f2db7..1c99d27ae42 100644 --- a/crates/nargo/src/errors.rs +++ b/crates/nargo/src/errors.rs @@ -1,5 +1,5 @@ use acvm::pwg::OpcodeResolutionError; -use noirc_abi::errors::{AbiError, InputParserError}; +use noirc_printable_type::ForeignCallError; use thiserror::Error; #[derive(Debug, Error)] @@ -15,17 +15,3 @@ pub enum NargoError { #[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), -} diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index b94220110d4..8eac516a7e9 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -3,12 +3,9 @@ use acvm::{ pwg::ForeignCallWaitInfo, }; use iter_extended::vecmap; -use noirc_printable_type::{ - decode_string_value, decode_value, PrintableType, PrintableValueDisplay, -}; -use regex::{Captures, Regex}; +use noirc_printable_type::PrintableValueDisplay; -use crate::errors::ForeignCallError; +use crate::NargoError; /// 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 @@ -45,7 +42,7 @@ impl ForeignCall { pub(crate) fn execute( foreign_call: &ForeignCallWaitInfo, show_output: bool, - ) -> Result { + ) -> Result { let foreign_call_name = foreign_call.function.as_str(); match Self::lookup(foreign_call_name) { Some(ForeignCall::Println) => { @@ -80,95 +77,9 @@ impl ForeignCall { } } - fn execute_println(foreign_call_inputs: &[Vec]) -> Result<(), ForeignCallError> { - let (is_fmt_str, foreign_call_inputs) = - foreign_call_inputs.split_last().ok_or(ForeignCallError::MissingForeignCallInputs)?; - - let output_string = if is_fmt_str[0].to_field().is_one() { - convert_fmt_string_inputs(foreign_call_inputs)? - } else { - convert_string_inputs(foreign_call_inputs)? - }; - println!("{output_string}"); + fn execute_println(foreign_call_inputs: &[Vec]) -> Result<(), NargoError> { + let display_values: PrintableValueDisplay = foreign_call_inputs.try_into()?; + println!("{display_values}"); Ok(()) } } - -fn convert_string_inputs(foreign_call_inputs: &[Vec]) -> Result { - // Fetch the PrintableType from the foreign call input - // The remaining input values should hold what is to be printed - let (printable_type_as_values, input_values) = - foreign_call_inputs.split_last().ok_or(ForeignCallError::MissingForeignCallInputs)?; - let printable_type = fetch_printable_type(printable_type_as_values)?; - - // 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| vecmap(values, |value| value.to_field())); - - let value = decode_value(&mut input_values_as_fields, &printable_type); - let input_value_display = PrintableValueDisplay::new(&value, &printable_type); - - Ok(input_value_display.to_string()) -} - -fn convert_fmt_string_inputs( - foreign_call_inputs: &[Vec], -) -> Result { - let (message_as_values, input_and_printable_values) = - foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; - - let message_as_fields = vecmap(message_as_values, |value| value.to_field()); - let message_as_string = decode_string_value(&message_as_fields); - - let (num_values, input_and_printable_values) = input_and_printable_values - .split_first() - .ok_or(ForeignCallError::MissingForeignCallInputs)?; - - let mut output_strings = Vec::new(); - let num_values = num_values[0].to_field().to_u128() as usize; - - let mut printable_types = Vec::new(); - for printable_value in - input_and_printable_values.iter().skip(input_and_printable_values.len() - num_values) - { - let printable_type = fetch_printable_type(printable_value)?; - printable_types.push(printable_type); - } - - for i in 0..num_values { - let printable_type = &printable_types[i]; - let type_size = printable_type.field_count() as usize; - - let mut input_values_as_fields = input_and_printable_values[i..(i + type_size)] - .iter() - .flat_map(|values| vecmap(values, |value| value.to_field())); - - let value = decode_value(&mut input_values_as_fields, printable_type); - let input_value_display = PrintableValueDisplay::new(&value, printable_type); - - output_strings.push(input_value_display.to_string()); - } - - let mut output_strings_iter = output_strings.into_iter(); - let re = Regex::new(r"\{([a-zA-Z0-9_]+)\}") - .expect("ICE: an invalid regex pattern was used for checking format strings"); - - let formatted_str = re.replace_all(&message_as_string, |_: &Captures| { - output_strings_iter - .next() - .expect("ICE: there are more regex matches than fields supplied to the format string") - }); - - Ok(formatted_str.into_owned()) -} - -fn fetch_printable_type( - printable_type_as_values: &[Value], -) -> Result { - let printable_type_as_fields = vecmap(printable_type_as_values, |value| value.to_field()); - let printable_type_as_string = decode_string_value(&printable_type_as_fields); - let printable_type: PrintableType = serde_json::from_str(&printable_type_as_string) - .map_err(|err| ForeignCallError::InputParserError(err.into()))?; - - Ok(printable_type) -} diff --git a/crates/noirc_printable_type/Cargo.toml b/crates/noirc_printable_type/Cargo.toml index 3ef4944ca12..b3de1a63dec 100644 --- a/crates/noirc_printable_type/Cargo.toml +++ b/crates/noirc_printable_type/Cargo.toml @@ -9,6 +9,9 @@ edition.workspace = true [dependencies] acvm.workspace = true iter-extended.workspace = true +regex = "1.9.1" serde.workspace = true +serde_json.workspace = true +thiserror.workspace = true [dev-dependencies] diff --git a/crates/noirc_printable_type/src/lib.rs b/crates/noirc_printable_type/src/lib.rs index 8c7c543983f..1915030041a 100644 --- a/crates/noirc_printable_type/src/lib.rs +++ b/crates/noirc_printable_type/src/lib.rs @@ -1,8 +1,10 @@ use std::{collections::BTreeMap, str}; -use acvm::FieldElement; +use acvm::{brillig_vm::brillig::Value, FieldElement}; use iter_extended::vecmap; +use regex::{Captures, Regex}; use serde::{Deserialize, Serialize}; +use thiserror::Error; #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "lowercase")] @@ -31,7 +33,7 @@ pub enum PrintableType { impl PrintableType { /// Returns the number of field elements required to represent the type once encoded. - pub fn field_count(&self) -> u32 { + fn field_count(&self) -> u32 { match self { Self::Field | Self::SignedInteger { .. } @@ -59,71 +61,177 @@ pub enum PrintableValue { /// In order to display a `PrintableValue` we need a `PrintableType` to accurately /// convert the value into a human-readable format. -pub struct PrintableValueDisplay<'a> { - value: &'a PrintableValue, - typ: &'a PrintableType, +pub enum PrintableValueDisplay { + Plain(PrintableValue, PrintableType), + FmtString(String, Vec<(PrintableValue, PrintableType)>), } -impl<'a> PrintableValueDisplay<'a> { - #[must_use] - pub fn new(value: &'a PrintableValue, typ: &'a PrintableType) -> Self { - Self { value, typ } +#[derive(Debug, Error)] +pub enum ForeignCallError { + #[error("Foreign call inputs needed for execution are missing")] + MissingForeignCallInputs, + + #[error("Could not parse PrintableType argumen. {0}")] + ParsingError(#[from] serde_json::Error), +} + +impl TryFrom<&[Vec]> for PrintableValueDisplay { + type Error = ForeignCallError; + + fn try_from(foreign_call_inputs: &[Vec]) -> Result { + let (is_fmt_str, foreign_call_inputs) = + foreign_call_inputs.split_last().ok_or(ForeignCallError::MissingForeignCallInputs)?; + + if is_fmt_str[0].to_field().is_one() { + convert_fmt_string_inputs(foreign_call_inputs) + } else { + convert_string_inputs(foreign_call_inputs) + } } } -impl<'a> std::fmt::Display for PrintableValueDisplay<'a> { - fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match (&self.value, &self.typ) { - ( - PrintableValue::Field(f), - PrintableType::Field - // TODO(#2401): We should print the sign for these and probably print normal integers instead of field strings - | PrintableType::SignedInteger { .. } - | PrintableType::UnsignedInteger { .. }, - ) => { - write!(fmt, "{}", format_field_string(*f))?; +fn convert_string_inputs( + foreign_call_inputs: &[Vec], +) -> Result { + // Fetch the PrintableType from the foreign call input + // The remaining input values should hold what is to be printed + let (printable_type_as_values, input_values) = + foreign_call_inputs.split_last().ok_or(ForeignCallError::MissingForeignCallInputs)?; + let printable_type = fetch_printable_type(printable_type_as_values)?; + + // 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| vecmap(values, |value| value.to_field())); + + let value = decode_value(&mut input_values_as_fields, &printable_type); + + Ok(PrintableValueDisplay::Plain(value, printable_type)) +} + +fn convert_fmt_string_inputs( + foreign_call_inputs: &[Vec], +) -> Result { + let (message_as_values, input_and_printable_values) = + foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; + + let message_as_fields = vecmap(message_as_values, |value| value.to_field()); + let message_as_string = decode_string_value(&message_as_fields); + + let (num_values, input_and_printable_values) = input_and_printable_values + .split_first() + .ok_or(ForeignCallError::MissingForeignCallInputs)?; + + let mut output = Vec::new(); + let num_values = num_values[0].to_field().to_u128() as usize; + + for (i, printable_value) in input_and_printable_values + .iter() + .skip(input_and_printable_values.len() - num_values) + .enumerate() + { + let printable_type = fetch_printable_type(printable_value)?; + let type_size = printable_type.field_count() as usize; + + let mut input_values_as_fields = input_and_printable_values[i..(i + type_size)] + .iter() + .flat_map(|values| vecmap(values, |value| value.to_field())); + + let value = decode_value(&mut input_values_as_fields, &printable_type); + + output.push((value, printable_type)); + } + + Ok(PrintableValueDisplay::FmtString(message_as_string, output)) +} + +fn fetch_printable_type( + printable_type_as_values: &[Value], +) -> Result { + let printable_type_as_fields = vecmap(printable_type_as_values, |value| value.to_field()); + let printable_type_as_string = decode_string_value(&printable_type_as_fields); + let printable_type: PrintableType = serde_json::from_str(&printable_type_as_string)?; + + Ok(printable_type) +} + +fn to_string(value: &PrintableValue, typ: &PrintableType) -> Option { + let mut output = String::new(); + match (value, typ) { + ( + PrintableValue::Field(f), + PrintableType::Field + // TODO(#2401): We should print the sign for these and probably print normal integers instead of field strings + | PrintableType::SignedInteger { .. } + | PrintableType::UnsignedInteger { .. }, + ) => { + output.push_str(&format_field_string(*f)); + } + (PrintableValue::Field(f), PrintableType::Boolean) => { + if f.is_one() { + output.push_str("true"); + } else { + output.push_str("false"); } - (PrintableValue::Field(f), PrintableType::Boolean) => { - if f.is_one() { - write!(fmt, "true")?; - } else { - write!(fmt, "false")?; + } + (PrintableValue::Vec(vector), PrintableType::Array { typ, .. }) => { + output.push('['); + let mut values = vector.iter().peekable(); + while let Some(value) = values.next() { + output.push_str(&format!("{}", PrintableValueDisplay::Plain(value.clone(), *typ.clone()))); + if values.peek().is_some() { + output.push_str(", "); } } - (PrintableValue::Vec(vector), PrintableType::Array { typ, .. }) => { - write!(fmt, "[")?; - let mut values = vector.iter().peekable(); - while let Some(value) = values.next() { - write!(fmt, "{}", PrintableValueDisplay::new(value, typ))?; - if values.peek().is_some() { - write!(fmt, ", ")?; - } + output.push(']'); + } + + (PrintableValue::String(s), PrintableType::String { .. }) => { + output.push_str(&format!(r#""{s}""#)); + } + + (PrintableValue::Struct(map), PrintableType::Struct { name, fields, .. }) => { + output.push_str(&format!("{name} {{ ")); + + let mut fields = fields.iter().peekable(); + while let Some((key, field_type)) = fields.next() { + let value = &map[key]; + output.push_str(&format!("{key}: {}", PrintableValueDisplay::Plain(value.clone(), field_type.clone()))); + if fields.peek().is_some() { + output.push_str(", "); } - write!(fmt, "]")?; } - (PrintableValue::String(s), PrintableType::String { .. }) => { - write!(fmt, r#""{s}""#)?; - } + output.push_str(" }"); + } - (PrintableValue::Struct(map), PrintableType::Struct { name, fields, .. }) => { - write!(fmt, "{name} {{ ")?; + _ => return None + }; - let mut fields = fields.iter().peekable(); - while let Some((key, field_type)) = fields.next() { - let value = &map[key]; - write!(fmt, "{key}: {}", PrintableValueDisplay::new(value, field_type))?; - if fields.peek().is_some() { - write!(fmt, ", ")?; - } - } + Some(output) +} - write!(fmt, " }}")?; +impl std::fmt::Display for PrintableValueDisplay { + fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Plain(value, typ) => { + let output_string = to_string(value, typ).ok_or(std::fmt::Error)?; + write!(fmt, "{output_string}") } + Self::FmtString(template, values) => { + let mut display_iter = values.iter(); + let re = Regex::new(r"\{([a-zA-Z0-9_]+)\}") + .expect("ICE: an invalid regex pattern was used for checking format strings"); + + let formatted_str = re.replace_all(template, |_: &Captures| { + let (value, typ) = display_iter.next().expect( + "ICE: there are more regex matches than fields supplied to the format string", + ); + to_string(value, typ).expect("ICE: Cannot convert display value into string") + }); - _ => return Err(std::fmt::Error), - }; - Ok(()) + write!(fmt, "{formatted_str}") + } + } } } @@ -142,9 +250,8 @@ fn format_field_string(field: FieldElement) -> String { "0x".to_owned() + &trimmed_field } -// TODO: Figure out a better API for this to avoid exporting the function /// Assumes that `field_iterator` contains enough [FieldElement] in order to decode the [PrintableType] -pub fn decode_value( +fn decode_value( field_iterator: &mut impl Iterator, typ: &PrintableType, ) -> PrintableValue { @@ -185,8 +292,8 @@ pub fn decode_value( } } -// TODO: Figure out a better API for this to avoid exporting the function -pub fn decode_string_value(field_elements: &[FieldElement]) -> String { +fn decode_string_value(field_elements: &[FieldElement]) -> String { + // TODO: Replace with `into` when Char is supported let string_as_slice = vecmap(field_elements, |e| { let mut field_as_bytes = e.to_be_bytes(); let char_byte = field_as_bytes.pop().unwrap(); // A character in a string is represented by a u8, thus we just want the last byte of the element From 214562a15c0b675b81c1a15aa2229689f1ffc9b7 Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Tue, 22 Aug 2023 14:13:42 -0700 Subject: [PATCH 8/9] cspell --- crates/noirc_printable_type/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_printable_type/src/lib.rs b/crates/noirc_printable_type/src/lib.rs index 1915030041a..633cf93a508 100644 --- a/crates/noirc_printable_type/src/lib.rs +++ b/crates/noirc_printable_type/src/lib.rs @@ -71,7 +71,7 @@ pub enum ForeignCallError { #[error("Foreign call inputs needed for execution are missing")] MissingForeignCallInputs, - #[error("Could not parse PrintableType argumen. {0}")] + #[error("Could not parse PrintableType argument. {0}")] ParsingError(#[from] serde_json::Error), } From 5e1fa6acadf2d2ae82a7d3d031c449bdcfea0c9c Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Tue, 22 Aug 2023 14:18:56 -0700 Subject: [PATCH 9/9] avoid unwrapping in display --- crates/noirc_printable_type/src/lib.rs | 33 +++++++++++++++++++------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/crates/noirc_printable_type/src/lib.rs b/crates/noirc_printable_type/src/lib.rs index 633cf93a508..46f59f665a0 100644 --- a/crates/noirc_printable_type/src/lib.rs +++ b/crates/noirc_printable_type/src/lib.rs @@ -210,6 +210,24 @@ fn to_string(value: &PrintableValue, typ: &PrintableType) -> Option { Some(output) } +// Taken from Regex docs directly +fn replace_all( + re: &Regex, + haystack: &str, + mut replacement: impl FnMut(&Captures) -> Result, +) -> Result { + let mut new = String::with_capacity(haystack.len()); + let mut last_match = 0; + for caps in re.captures_iter(haystack) { + let m = caps.get(0).unwrap(); + new.push_str(&haystack[last_match..m.start()]); + new.push_str(&replacement(&caps)?); + last_match = m.end(); + } + new.push_str(&haystack[last_match..]); + Ok(new) +} + impl std::fmt::Display for PrintableValueDisplay { fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -219,15 +237,12 @@ impl std::fmt::Display for PrintableValueDisplay { } Self::FmtString(template, values) => { let mut display_iter = values.iter(); - let re = Regex::new(r"\{([a-zA-Z0-9_]+)\}") - .expect("ICE: an invalid regex pattern was used for checking format strings"); - - let formatted_str = re.replace_all(template, |_: &Captures| { - let (value, typ) = display_iter.next().expect( - "ICE: there are more regex matches than fields supplied to the format string", - ); - to_string(value, typ).expect("ICE: Cannot convert display value into string") - }); + let re = Regex::new(r"\{([a-zA-Z0-9_]+)\}").map_err(|_| std::fmt::Error)?; + + let formatted_str = replace_all(&re, template, |_: &Captures| { + let (value, typ) = display_iter.next().ok_or(std::fmt::Error)?; + to_string(value, typ).ok_or(std::fmt::Error) + })?; write!(fmt, "{formatted_str}") }