fix!: several format string fixes and improvements

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

@@ -2,6 +2,7 @@
 mod program;
 mod value;
 
+use noirc_frontend::token::FmtStrFragment;
 pub(crate) use program::Ssa;
 
 use context::SharedContext;
@@ -232,10 +233,26 @@
                 Ok(self.builder.numeric_constant(*value as u128, Type::bool()).into())
             }
             ast::Literal::Str(string) => Ok(self.codegen_string(string)),
-            ast::Literal::FmtStr(string, number_of_fields, fields) => {
+            ast::Literal::FmtStr(fragments, number_of_fields, fields) => {
+                let mut string = String::new();
+                for fragment in fragments {
+                    match fragment {
+                        FmtStrFragment::String(value) => {
+                            // Escape curly braces in non-interpolations
+                            let value = value.replace('{', "{{").replace('}', "}}");
+                            string.push_str(&value);
+                        }
+                        FmtStrFragment::Interpolation(value, _span) => {
+                            string.push('{');
+                            string.push_str(value);
+                            string.push('}');
+                        }
+                    }
+                }
+
                 // A caller needs multiple pieces of information to make use of a format string
                 // The message string, the number of fields to be formatted, and the fields themselves
-                let string = self.codegen_string(string);
+                let string = self.codegen_string(&string);
                 let field_count = self.builder.length_constant(*number_of_fields as u128);
                 let fields = self.codegen_expression(fields)?;
 
@@ -492,7 +509,7 @@
         // Remember the blocks and variable used in case there are break/continue instructions
         // within the loop which need to jump to them.
         self.enter_loop(loop_entry, loop_index, loop_end);
 
         self.builder.set_location(for_expr.start_range_location);
         let start_index = self.codegen_non_tuple_expression(&for_expr.start_range)?;
 
@@ -553,7 +570,7 @@
     ///   v1 = ... codegen a ...
     ///   br end_if()
     /// end_if:  // No block parameter is needed. Without an else, the unit value is always returned.
     ///   ... This is the current insert point after codegen_if finishes ...
     /// ```
     fn codegen_if(&mut self, if_expr: &ast::If) -> Result<Values, RuntimeError> {
         let condition = self.codegen_non_tuple_expression(&if_expr.condition)?;
@@ -568,7 +585,7 @@
 
         let mut result = Self::unit_value();
 
         if let Some(alternative) = &if_expr.alternative {
             let end_block = self.builder.insert_block();
             let then_values = then_value.into_value_list(self);
             self.builder.terminate_with_jmp(end_block, then_values);

compiler/noirc_frontend/Cargo.toml

@@ -25,7 +25,6 @@ num-bigint.workspace = true
 num-traits.workspace = true
 rustc-hash = "1.1.0"
 small-ord-set = "0.1.3"
-regex = "1.9.1"
 cfg-if.workspace = true
 tracing.workspace = true
 petgraph = "0.6"

compiler/noirc_frontend/src/ast/expression.rs

@@ -10,7 +10,7 @@ use crate::ast::{
 use crate::node_interner::{
     ExprId, InternedExpressionKind, InternedStatementKind, QuotedTypeId, StructId,
 };
-use crate::token::{Attributes, FunctionAttribute, Token, Tokens};
+use crate::token::{Attributes, FmtStrFragment, FunctionAttribute, Token, Tokens};
 use crate::{Kind, Type};
 use acvm::{acir::AcirField, FieldElement};
 use iter_extended::vecmap;
@@ -210,8 +210,8 @@ impl ExpressionKind {
         ExpressionKind::Literal(Literal::RawStr(contents, hashes))
     }
 
-    pub fn format_string(contents: String) -> ExpressionKind {
-        ExpressionKind::Literal(Literal::FmtStr(contents))
+    pub fn format_string(fragments: Vec<FmtStrFragment>, length: u32) -> ExpressionKind {
+        ExpressionKind::Literal(Literal::FmtStr(fragments, length))
     }
 
     pub fn constructor(
@@ -434,7 +434,7 @@ pub enum Literal {
     Integer(FieldElement, /*sign*/ bool), // false for positive integer and true for negative
     Str(String),
     RawStr(String, u8),
-    FmtStr(String),
+    FmtStr(Vec<FmtStrFragment>, u32 /* length */),
     Unit,
 }
 
@@ -669,7 +669,13 @@ impl Display for Literal {
                     std::iter::once('#').cycle().take(*num_hashes as usize).collect();
                 write!(f, "r{hashes}\"{string}\"{hashes}")
             }
-            Literal::FmtStr(string) => write!(f, "f\"{string}\""),
+            Literal::FmtStr(fragments, _length) => {
+                write!(f, "f\"")?;
+                for fragment in fragments {
+                    fragment.fmt(f)?;
+                }
+                write!(f, "\"")
+            }
             Literal::Unit => write!(f, "()"),
         }
     }

compiler/noirc_frontend/src/ast/visitor.rs

@@ -16,7 +16,7 @@
         InternedUnresolvedTypeData, QuotedTypeId,
     },
     parser::{Item, ItemKind, ParsedSubModule},
-    token::{MetaAttribute, SecondaryAttribute, Tokens},
+    token::{FmtStrFragment, MetaAttribute, SecondaryAttribute, Tokens},
     ParsedModule, QuotedType,
 };
 
@@ -172,7 +172,7 @@
 
     fn visit_literal_raw_str(&mut self, _: &str, _: u8) {}
 
-    fn visit_literal_fmt_str(&mut self, _: &str) {}
+    fn visit_literal_fmt_str(&mut self, _: &[FmtStrFragment], _length: u32) {}
 
     fn visit_literal_unit(&mut self) {}
 
@@ -900,7 +900,7 @@
             Literal::Integer(value, negative) => visitor.visit_literal_integer(*value, *negative),
             Literal::Str(str) => visitor.visit_literal_str(str),
             Literal::RawStr(str, length) => visitor.visit_literal_raw_str(str, *length),
-            Literal::FmtStr(str) => visitor.visit_literal_fmt_str(str),
+            Literal::FmtStr(fragments, length) => visitor.visit_literal_fmt_str(fragments, *length),
             Literal::Unit => visitor.visit_literal_unit(),
         }
     }
@@ -1322,8 +1322,8 @@
             UnresolvedTypeData::Unspecified => visitor.visit_unspecified_type(self.span),
             UnresolvedTypeData::Quoted(typ) => visitor.visit_quoted_type(typ, self.span),
             UnresolvedTypeData::FieldElement => visitor.visit_field_element_type(self.span),
             UnresolvedTypeData::Integer(signdness, size) => {
                 visitor.visit_integer_type(*signdness, *size, self.span);
             }
             UnresolvedTypeData::Bool => visitor.visit_bool_type(self.span),
             UnresolvedTypeData::Unit => visitor.visit_unit_type(self.span),

compiler/noirc_frontend/src/elaborator/expressions.rs

@@ -1,7 +1,6 @@
 use acvm::{AcirField, FieldElement};
 use iter_extended::vecmap;
 use noirc_errors::{Location, Span};
-use regex::Regex;
 use rustc_hash::FxHashSet as HashSet;
 
 use crate::{
@@ -29,7 +28,7 @@ use crate::{
         traits::{ResolvedTraitBound, TraitConstraint},
     },
     node_interner::{DefinitionKind, ExprId, FuncId, InternedStatementKind, TraitMethodId},
-    token::Tokens,
+    token::{FmtStrFragment, Tokens},
     Kind, QuotedType, Shared, StructType, Type,
 };
 
@@ -167,7 +166,7 @@ impl<'context> Elaborator<'context> {
                 let len = Type::Constant(str.len().into(), Kind::u32());
                 (Lit(HirLiteral::Str(str)), Type::String(Box::new(len)))
             }
-            Literal::FmtStr(str) => self.elaborate_fmt_string(str, span),
+            Literal::FmtStr(fragments, length) => self.elaborate_fmt_string(fragments, length),
             Literal::Array(array_literal) => {
                 self.elaborate_array_literal(array_literal, span, true)
             }
@@ -234,53 +233,50 @@ impl<'context> Elaborator<'context> {
         (HirExpression::Literal(constructor(expr)), typ)
     }
 
-    fn elaborate_fmt_string(&mut self, str: String, call_expr_span: Span) -> (HirExpression, Type) {
-        let re = Regex::new(r"\{([a-zA-Z0-9_]+)\}")
-            .expect("ICE: an invalid regex pattern was used for checking format strings");
-
+    fn elaborate_fmt_string(
+        &mut self,
+        fragments: Vec<FmtStrFragment>,
+        length: u32,
+    ) -> (HirExpression, Type) {
         let mut fmt_str_idents = Vec::new();
         let mut capture_types = Vec::new();
 
-        for field in re.find_iter(&str) {
-            let matched_str = field.as_str();
-            let ident_name = &matched_str[1..(matched_str.len() - 1)];
-
-            let scope_tree = self.scopes.current_scope_tree();
-            let variable = scope_tree.find(ident_name);
-
-            let hir_ident = if let Some((old_value, _)) = variable {
-                old_value.num_times_used += 1;
-                old_value.ident.clone()
-            } else if let Ok((definition_id, _)) =
-                self.lookup_global(Path::from_single(ident_name.to_string(), call_expr_span))
-            {
-                HirIdent::non_trait_method(definition_id, Location::new(call_expr_span, self.file))
-            } else if ident_name.parse::<usize>().is_ok() {
-                self.push_err(ResolverError::NumericConstantInFormatString {
-                    name: ident_name.to_owned(),
-                    span: call_expr_span,
-                });
-                continue;
-            } else {
-                self.push_err(ResolverError::VariableNotDeclared {
-                    name: ident_name.to_owned(),
-                    span: call_expr_span,
-                });
-                continue;
-            };
+        for fragment in &fragments {
+            if let FmtStrFragment::Interpolation(ident_name, string_span) = fragment {
+                let scope_tree = self.scopes.current_scope_tree();
+                let variable = scope_tree.find(ident_name);
+
+                let hir_ident = if let Some((old_value, _)) = variable {
+                    old_value.num_times_used += 1;
+                    old_value.ident.clone()
+                } else if let Ok((definition_id, _)) =
+                    self.lookup_global(Path::from_single(ident_name.to_string(), *string_span))
+                {
+                    HirIdent::non_trait_method(
+                        definition_id,
+                        Location::new(*string_span, self.file),
+                    )
+                } else {
+                    self.push_err(ResolverError::VariableNotDeclared {
+                        name: ident_name.to_owned(),
+                        span: *string_span,
+                    });
+                    continue;
+                };
 
-            let hir_expr = HirExpression::Ident(hir_ident.clone(), None);
-            let expr_id = self.interner.push_expr(hir_expr);
-            self.interner.push_expr_location(expr_id, call_expr_span, self.file);
-            let typ = self.type_check_variable(hir_ident, expr_id, None);
-            self.interner.push_expr_type(expr_id, typ.clone());
-            capture_types.push(typ);
-            fmt_str_idents.push(expr_id);
+                let hir_expr = HirExpression::Ident(hir_ident.clone(), None);
+                let expr_id = self.interner.push_expr(hir_expr);
+                self.interner.push_expr_location(expr_id, *string_span, self.file);
+                let typ = self.type_check_variable(hir_ident, expr_id, None);
+                self.interner.push_expr_type(expr_id, typ.clone());
+                capture_types.push(typ);
+                fmt_str_idents.push(expr_id);
+            }
         }
 
-        let len = Type::Constant(str.len().into(), Kind::u32());
+        let len = Type::Constant(length.into(), Kind::u32());
         let typ = Type::FmtString(Box::new(len), Box::new(Type::Tuple(capture_types)));
-        (HirExpression::Literal(HirLiteral::FmtStr(str, fmt_str_idents)), typ)
+        (HirExpression::Literal(HirLiteral::FmtStr(fragments, fmt_str_idents, length)), typ)
     }
 
     fn elaborate_prefix(&mut self, prefix: PrefixExpression, span: Span) -> (ExprId, Type) {

compiler/noirc_frontend/src/hir/comptime/display.rs

@@ -661,7 +661,7 @@ fn remove_interned_in_literal(interner: &NodeInterner, literal: Literal) -> Lite
         | Literal::Integer(_, _)
         | Literal::Str(_)
         | Literal::RawStr(_, _)
-        | Literal::FmtStr(_)
+        | Literal::FmtStr(_, _)
         | Literal::Unit => literal,
     }
 }

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

@@ -240,6 +240,9 @@
         err: Box<TypeCheckError>,
         location: Location,
     },
+    CannotInterpretFormatStringWithErrors {
+        location: Location,
+    },
 
     // These cases are not errors, they are just used to prevent us from running more code
     // until the loop can be resumed properly. These cases will never be displayed to users.
@@ -315,7 +318,8 @@
             | InterpreterError::TypeAnnotationsNeededForMethodCall { location }
             | InterpreterError::CannotResolveExpression { location, .. }
             | InterpreterError::CannotSetFunctionBody { location, .. }
-            | InterpreterError::UnknownArrayLength { location, .. } => *location,
+            | InterpreterError::UnknownArrayLength { location, .. }
+            | InterpreterError::CannotInterpretFormatStringWithErrors { location } => *location,
 
             InterpreterError::FailedToParseMacro { error, file, .. } => {
                 Location::new(error.span(), *file)
@@ -627,7 +631,7 @@
             }
             InterpreterError::GenericNameShouldBeAnIdent { name, location } => {
                 let msg =
                     "Generic name needs to be a valid identifer (one word beginning with a letter)"
                         .to_string();
                 let secondary = format!("`{name}` is not a valid identifier");
                 CustomDiagnostic::simple_error(msg, secondary, location.span)
@@ -664,6 +668,12 @@
                 let secondary = format!("Evaluating the length failed with: `{err}`");
                 CustomDiagnostic::simple_error(msg, secondary, location.span)
             }
+            InterpreterError::CannotInterpretFormatStringWithErrors { location } => {
+                let msg = "Cannot interpret format string with errors".to_string();
+                let secondary =
+                    "Some of the variables to interpolate could not be evaluated".to_string();
+                CustomDiagnostic::simple_error(msg, secondary, location.span)
+            }
         }
     }
 }

compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs

@@ -121,9 +121,9 @@ impl HirExpression {
             HirExpression::Literal(HirLiteral::Str(string)) => {
                 ExpressionKind::Literal(Literal::Str(string.clone()))
             }
-            HirExpression::Literal(HirLiteral::FmtStr(string, _exprs)) => {
+            HirExpression::Literal(HirLiteral::FmtStr(fragments, _exprs, length)) => {
                 // TODO: Is throwing away the exprs here valid?
-                ExpressionKind::Literal(Literal::FmtStr(string.clone()))
+                ExpressionKind::Literal(Literal::FmtStr(fragments.clone(), *length))
             }
             HirExpression::Literal(HirLiteral::Unit) => ExpressionKind::Literal(Literal::Unit),
             HirExpression::Block(expr) => ExpressionKind::Block(expr.to_display_ast(interner)),

compiler/noirc_frontend/src/hir/comptime/interpreter.rs

@@ -20,7 +20,7 @@
     perform_impl_bindings, perform_instantiation_bindings, resolve_trait_method,
     undo_instantiation_bindings,
 };
-use crate::token::Tokens;
+use crate::token::{FmtStrFragment, Tokens};
 use crate::TypeVariable;
 use crate::{
     hir_def::{
@@ -251,7 +251,7 @@
             }
         } else {
             let name = self.elaborator.interner.function_name(&function);
             unreachable!("Non-builtin, lowlevel or oracle builtin fn '{name}'")
         }
     }
 
@@ -623,8 +623,8 @@
                 self.evaluate_integer(value, is_negative, id)
             }
             HirLiteral::Str(string) => Ok(Value::String(Rc::new(string))),
-            HirLiteral::FmtStr(string, captures) => {
-                self.evaluate_format_string(string, captures, id)
+            HirLiteral::FmtStr(fragments, captures, _length) => {
+                self.evaluate_format_string(fragments, captures, id)
             }
             HirLiteral::Array(array) => self.evaluate_array(array, id),
             HirLiteral::Slice(array) => self.evaluate_slice(array, id),
@@ -633,7 +633,7 @@
 
     fn evaluate_format_string(
         &mut self,
-        string: String,
+        fragments: Vec<FmtStrFragment>,
         captures: Vec<ExprId>,
         id: ExprId,
     ) -> IResult<Value> {
@@ -644,13 +644,12 @@
         let mut values: VecDeque<_> =
             captures.into_iter().map(|capture| self.evaluate(capture)).collect::<Result<_, _>>()?;
 
-        for character in string.chars() {
-            match character {
-                '\\' => escaped = true,
-                '{' if !escaped => consuming = true,
-                '}' if !escaped && consuming => {
-                    consuming = false;
-
+        for fragment in fragments {
+            match fragment {
+                FmtStrFragment::String(string) => {
+                    result.push_str(&string);
+                }
+                FmtStrFragment::Interpolation(_, span) => {
                     if let Some(value) = values.pop_front() {
                         // When interpolating a quoted value inside a format string, we don't include the
                         // surrounding `quote {` ... `}` as if we are unquoting the quoted value inside the string.
@@ -665,13 +664,15 @@
                         } else {
                             result.push_str(&value.display(self.elaborator.interner).to_string());
                         }
+                    } else {
+                        // If we can't find a value for this fragment it means the interpolated value was not
+                        // found or it errored. In this case we error here as well.
+                        let location = self.elaborator.interner.expr_location(&id);
+                        return Err(InterpreterError::CannotInterpretFormatStringWithErrors {
+                            location,
+                        });
                     }
                 }
-                other if !consuming => {
-                    escaped = false;
-                    result.push(other);
-                }
-                _ => (),
             }
         }
 
@@ -1031,7 +1032,7 @@
         }
 
         /// Generate matches for bit shifting, which in Noir only accepts `u8` for RHS.
         macro_rules! match_bitshift {
             (($lhs_value:ident as $lhs:ident $op:literal $rhs_value:ident as $rhs:ident) => $expr:expr) => {
                 match_values! {
                     ($lhs_value as $lhs $op $rhs_value as $rhs) {
@@ -1101,10 +1102,10 @@
             BinaryOpKind::Xor => match_bitwise! {
                 (lhs_value as lhs "^" rhs_value as rhs) => lhs ^ rhs
             },
             BinaryOpKind::ShiftRight => match_bitshift! {
                 (lhs_value as lhs ">>" rhs_value as rhs) => lhs.checked_shr(rhs.into())
             },
             BinaryOpKind::ShiftLeft => match_bitshift! {
                 (lhs_value as lhs "<<" rhs_value as rhs) => lhs.checked_shl(rhs.into())
             },
             BinaryOpKind::Modulo => match_integer! {

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

@@ -77,8 +77,6 @@ pub enum ResolverError {
     MutableReferenceToImmutableVariable { variable: String, span: Span },
     #[error("Mutable references to array indices are unsupported")]
     MutableReferenceToArrayElement { span: Span },
-    #[error("Numeric constants should be printed without formatting braces")]
-    NumericConstantInFormatString { name: String, span: Span },
     #[error("Closure environment must be a tuple or unit type")]
     InvalidClosureEnvironment { typ: Type, span: Span },
     #[error("Nested slices, i.e. slices within an array or slice, are not supported")]
@@ -378,11 +376,6 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
             ResolverError::MutableReferenceToArrayElement { span } => {
                 Diagnostic::simple_error("Mutable references to array elements are currently unsupported".into(), "Try storing the element in a fresh variable first".into(), *span)
             },
-            ResolverError::NumericConstantInFormatString { name, span } => Diagnostic::simple_error(
-                format!("cannot find `{name}` in this scope "),
-                "Numeric constants should be printed without formatting braces".to_string(),
-                *span,
-            ),
             ResolverError::InvalidClosureEnvironment { span, typ } => Diagnostic::simple_error(
                 format!("{typ} is not a valid closure environment type"),
                 "Closure environment must be a tuple or unit type".to_string(), *span),