From 5ddf9d01e51677f2f9a19d524cba488cd21fc659 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 30 Jun 2023 10:54:28 +0200 Subject: [PATCH 01/13] Explore work on references --- crates/noirc_evaluator/src/ssa/context.rs | 1 + crates/noirc_evaluator/src/ssa/function.rs | 3 +- crates/noirc_evaluator/src/ssa/value.rs | 1 + .../src/ssa_refactor/ssa_gen/context.rs | 23 ++++--- .../src/ssa_refactor/ssa_gen/mod.rs | 9 +-- .../src/ssa_refactor/ssa_gen/value.rs | 16 +++++ crates/noirc_frontend/src/ast/expression.rs | 21 +++++- crates/noirc_frontend/src/ast/statement.rs | 2 + .../src/hir/resolution/resolver.rs | 69 ++++++++++++++----- .../noirc_frontend/src/hir/type_check/stmt.rs | 5 +- crates/noirc_frontend/src/hir_def/function.rs | 2 + crates/noirc_frontend/src/hir_def/stmt.rs | 4 +- crates/noirc_frontend/src/hir_def/types.rs | 14 ++++ .../src/monomorphization/ast.rs | 10 +-- .../src/monomorphization/mod.rs | 55 +++++++++------ .../src/monomorphization/printer.rs | 4 +- crates/noirc_frontend/src/node_interner.rs | 26 +++++-- crates/noirc_frontend/src/parser/parser.rs | 36 +++++++--- 18 files changed, 225 insertions(+), 76 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/context.rs b/crates/noirc_evaluator/src/ssa/context.rs index 2efdd8ff30..64b44ebf02 100644 --- a/crates/noirc_evaluator/src/ssa/context.rs +++ b/crates/noirc_evaluator/src/ssa/context.rs @@ -1206,6 +1206,7 @@ impl SsaContext { Signedness::Unsigned => ObjectType::unsigned_integer(*bit_size), } } + Type::MutableReference(element) => self.convert_type(element), Type::Array(..) => panic!("Cannot convert an array type {t} into an ObjectType since it is unknown which array it refers to"), Type::Unit => ObjectType::NotAnObject, Type::Function(..) => ObjectType::Function, diff --git a/crates/noirc_evaluator/src/ssa/function.rs b/crates/noirc_evaluator/src/ssa/function.rs index 33957998e7..59e6f7ce38 100644 --- a/crates/noirc_evaluator/src/ssa/function.rs +++ b/crates/noirc_evaluator/src/ssa/function.rs @@ -10,6 +10,7 @@ use crate::ssa::{ }; use iter_extended::try_vecmap; use noirc_frontend::monomorphization::ast::{Call, Definition, FuncId, LocalId, Type}; +use noirc_frontend::node_interner::Mutability; use std::collections::{HashMap, VecDeque}; #[derive(Clone, Debug, PartialEq, Eq, Copy)] @@ -155,7 +156,7 @@ impl IrGenerator { //arguments: for (param_id, mutable, name, typ) in std::mem::take(&mut function.parameters) { let node_ids = self.create_function_parameter(param_id, &typ, &name); - func.arguments.extend(node_ids.into_iter().map(|id| (id, mutable))); + func.arguments.extend(node_ids.into_iter().map(|id| (id, mutable == Mutability::Mutable))); } // ensure return types are defined in case of recursion call cycle diff --git a/crates/noirc_evaluator/src/ssa/value.rs b/crates/noirc_evaluator/src/ssa/value.rs index 915effe480..83b73181b3 100644 --- a/crates/noirc_evaluator/src/ssa/value.rs +++ b/crates/noirc_evaluator/src/ssa/value.rs @@ -93,6 +93,7 @@ impl Value { let values = vecmap(tup, |v| Self::reshape(v, iter)); Value::Tuple(values) } + Type::MutableReference(element) => Self::reshape(element, iter), Type::Unit | Type::Function(..) | Type::Array(..) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index a154af45ec..5237a071a9 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -7,6 +7,7 @@ use iter_extended::vecmap; use noirc_frontend::monomorphization::ast::{self, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; use noirc_frontend::Signedness; +use noirc_frontend::node_interner::Mutability; use crate::ssa_refactor::ir::dfg::DataFlowGraph; use crate::ssa_refactor::ir::function::FunctionId as IrFunctionId; @@ -131,15 +132,16 @@ impl<'a> FunctionContext<'a> { &mut self, parameter_id: LocalId, parameter_type: &ast::Type, - mutable: bool, + mutability: Mutability, ) { // Add a separate parameter for each field type in 'parameter_type' let parameter_value = Self::map_type(parameter_type, |typ| { - let value = self.builder.add_parameter(typ); - if mutable { - self.new_mutable_variable(value) - } else { - value.into() + let value = self.builder.add_parameter(typ.clone()); + + match mutability { + Mutability::Immutable => value.into(), + Mutability::Mutable => self.new_mutable_variable(value, false), + Mutability::MutableReference => Value::MutableReference(value, typ), } }); @@ -148,11 +150,15 @@ impl<'a> FunctionContext<'a> { /// Allocate a single slot of memory and store into it the given initial value of the variable. /// Always returns a Value::Mutable wrapping the allocate instruction. - pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId) -> Value { + pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId, is_reference: bool) -> Value { let alloc = self.builder.insert_allocate(); self.builder.insert_store(alloc, value_to_store); let typ = self.builder.type_of_value(value_to_store); - Value::Mutable(alloc, typ) + if is_reference { + Value::MutableReference(alloc, typ) + } else { + Value::Mutable(alloc, typ) + } } /// Maps the given type to a Tree of the result type. @@ -201,6 +207,7 @@ impl<'a> FunctionContext<'a> { ast::Type::Unit => panic!("convert_non_tuple_type called on a unit type"), ast::Type::Tuple(_) => panic!("convert_non_tuple_type called on a tuple: {typ}"), ast::Type::Function(_, _) => Type::Function, + ast::Type::MutableReference(element) => Self::convert_non_tuple_type(element), // How should we represent Vecs? // Are they a struct of array + length + capacity? diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index 0dc003dd9d..c8b6cb17dc 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -9,7 +9,7 @@ pub(crate) use program::Ssa; use context::SharedContext; use iter_extended::vecmap; use noirc_errors::Location; -use noirc_frontend::monomorphization::ast::{self, Expression, Program}; +use noirc_frontend::{monomorphization::ast::{self, Expression, Program}, node_interner::Mutability}; use self::{ context::FunctionContext, @@ -342,7 +342,7 @@ impl<'a> FunctionContext<'a> { let arguments = call .arguments .iter() - .flat_map(|argument| self.codegen_expression(argument).into_value_list(self)) + .flat_map(|argument| self.codegen_expression(argument).into_argument_list(self)) .collect(); let function = self.codegen_non_tuple_expression(&call.func); @@ -356,10 +356,11 @@ impl<'a> FunctionContext<'a> { fn codegen_let(&mut self, let_expr: &ast::Let) -> Values { let mut values = self.codegen_expression(&let_expr.expression); - if let_expr.mutable { + if let_expr.mutability != Mutability::Immutable { values = values.map(|value| { let value = value.eval(self); - Tree::Leaf(self.new_mutable_variable(value)) + let is_mutable_reference = let_expr.mutability == Mutability::MutableReference; + Tree::Leaf(self.new_mutable_variable(value, is_mutable_reference)) }); } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs index c50abb9ca3..879e8b28f0 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs @@ -32,6 +32,8 @@ pub(super) enum Value { /// A mutable variable that must be loaded as the given type before being used Mutable(IrValueId, Type), + + MutableReference(IrValueId, Type), } impl Value { @@ -42,6 +44,7 @@ impl Value { match self { Value::Normal(value) => value, Value::Mutable(address, typ) => ctx.builder.insert_load(address, typ), + Value::MutableReference(address, typ) => ctx.builder.insert_load(address, typ), } } @@ -51,6 +54,15 @@ impl Value { match self { Value::Normal(value) => value, Value::Mutable(address, _) => address, + Value::MutableReference(address, _) => address, + } + } + + pub(super) fn eval_argument(self, ctx: &mut FunctionContext) -> IrValueId { + match self { + Value::Normal(value) => value, + Value::Mutable(address, typ) => ctx.builder.insert_load(address, typ), + Value::MutableReference(address, _) => address, } } } @@ -160,4 +172,8 @@ impl Tree { pub(super) fn into_value_list(self, ctx: &mut FunctionContext) -> Vec { vecmap(self.flatten(), |value| value.eval(ctx)) } + + pub(super) fn into_argument_list(self, ctx: &mut FunctionContext) -> Vec { + vecmap(self.flatten(), |value| value.eval_argument(ctx)) + } } diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index 9e9ff2f592..f6496e94a0 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -157,13 +157,19 @@ impl Expression { Expression::new(kind, span) } - pub fn call(lhs: Expression, arguments: Vec, span: Span) -> Expression { + pub fn call(lhs: Expression, arguments: Vec<(ArgumentMode, Expression)>, span: Span) -> Expression { let func = Box::new(lhs); let kind = ExpressionKind::Call(Box::new(CallExpression { func, arguments })); Expression::new(kind, span) } } +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +pub enum ArgumentMode { + PassByValue, + PassByReference, +} + #[derive(Debug, PartialEq, Eq, Clone)] pub struct ForExpression { pub identifier: Ident, @@ -356,7 +362,7 @@ pub enum ArrayLiteral { #[derive(Debug, PartialEq, Eq, Clone)] pub struct CallExpression { pub func: Box, - pub arguments: Vec, + pub arguments: Vec<(ArgumentMode, Expression)>, } #[derive(Debug, PartialEq, Eq, Clone)] @@ -487,11 +493,20 @@ impl Display for IndexExpression { impl Display for CallExpression { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let args = vecmap(&self.arguments, ToString::to_string); + let args = vecmap(&self.arguments, |(mode, arg)| format!("{}{}", mode, arg)); write!(f, "{}({})", self.func, args.join(", ")) } } +impl Display for ArgumentMode { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ArgumentMode::PassByValue => Ok(()), + ArgumentMode::PassByReference => write!(f, "&mut "), + } + } +} + impl Display for MethodCallExpression { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let args = vecmap(&self.arguments, ToString::to_string); diff --git a/crates/noirc_frontend/src/ast/statement.rs b/crates/noirc_frontend/src/ast/statement.rs index 7f77716c5e..2240b92806 100644 --- a/crates/noirc_frontend/src/ast/statement.rs +++ b/crates/noirc_frontend/src/ast/statement.rs @@ -412,6 +412,7 @@ pub struct ConstrainStatement(pub Expression); pub enum Pattern { Identifier(Ident), Mutable(Box, Span), + MutableReference(Box, Span), Tuple(Vec, Span), Struct(Path, Vec<(Ident, Pattern)>, Span), } @@ -523,6 +524,7 @@ impl Display for Pattern { match self { Pattern::Identifier(name) => name.fmt(f), Pattern::Mutable(name, _) => write!(f, "mut {name}"), + Pattern::MutableReference(name, _) => write!(f, "&mut {name}"), Pattern::Tuple(fields, _) => { let fields = vecmap(fields, ToString::to_string); write!(f, "({})", fields.join(", ")) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 883b323ce6..658eb5e9ca 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -25,7 +25,7 @@ use crate::graph::CrateId; use crate::hir::def_map::{ModuleDefId, TryFromModuleDefId, MAIN_FUNCTION}; use crate::hir_def::stmt::{HirAssignStatement, HirLValue, HirPattern}; use crate::node_interner::{ - DefinitionId, DefinitionKind, ExprId, FuncId, NodeInterner, StmtId, StructId, + DefinitionId, DefinitionKind, ExprId, FuncId, NodeInterner, StmtId, StructId, Mutability, }; use crate::{ hir::{def_map::CrateDefMap, resolution::path_resolver::PathResolver}, @@ -189,7 +189,7 @@ impl<'a> Resolver<'a> { fn add_variable_decl( &mut self, name: Ident, - mutable: bool, + mutable: Mutability, allow_shadowing: bool, definition: DefinitionKind, ) -> HirIdent { @@ -199,7 +199,7 @@ impl<'a> Resolver<'a> { fn add_variable_decl_inner( &mut self, name: Ident, - mutable: bool, + mutable: Mutability, allow_shadowing: bool, warn_if_unused: bool, definition: DefinitionKind, @@ -252,7 +252,7 @@ impl<'a> Resolver<'a> { ident = hir_let_stmt.ident(); resolver_meta = ResolverMeta { num_times_used: 0, ident, warn_if_unused: true }; } else { - let id = self.interner.push_definition(name.0.contents.clone(), false, definition); + let id = self.interner.push_definition(name.0.contents.clone(), Mutability::Immutable, definition); let location = Location::new(name.span(), self.file); ident = HirIdent { location, id }; resolver_meta = ResolverMeta { num_times_used: 0, ident, warn_if_unused: true }; @@ -616,8 +616,11 @@ impl<'a> Resolver<'a> { self.push_err(ResolverError::UnnecessaryPub { ident: func.name_ident().clone() }); } - let pattern = self.resolve_pattern(pattern, DefinitionKind::Local(None)); let typ = self.resolve_type_inner(typ, &mut generics); + let typ = self.add_mutable_reference_from_pattern_to_type(&pattern, typ); + + let pattern = self.resolve_pattern(pattern, DefinitionKind::Local(None)); + parameters.push(Param(pattern, typ.clone(), visibility)); parameter_types.push(typ); } @@ -721,7 +724,7 @@ impl<'a> Resolver<'a> { { let ident = Ident::new(name.to_string(), *span); let definition = DefinitionKind::GenericType(type_variable); - self.add_variable_decl_inner(ident, false, false, false, definition); + self.add_variable_decl_inner(ident, Mutability::Immutable, false, false, definition); } } } @@ -781,6 +784,7 @@ impl<'a> Resolver<'a> { } } Type::Vec(element) => Self::find_numeric_generics_in_type(element, found), + Type::MutableReference(element) => Self::find_numeric_generics_in_type(element, found), } } @@ -895,7 +899,7 @@ impl<'a> Resolver<'a> { ExpressionKind::Call(call_expr) => { // Get the span and name of path for error reporting let func = self.resolve_expression(*call_expr.func); - let arguments = vecmap(call_expr.arguments, |arg| self.resolve_expression(arg)); + let arguments = vecmap(call_expr.arguments, |(_, arg)| self.resolve_expression(arg)); let location = Location::new(expr.span, self.file); HirExpression::Call(HirCallExpression { func, arguments, location }) } @@ -925,7 +929,7 @@ impl<'a> Resolver<'a> { let (identifier, block_id) = self.in_new_scope(|this| { let decl = this.add_variable_decl( identifier, - false, + Mutability::Immutable, false, DefinitionKind::Local(None), ); @@ -1010,37 +1014,46 @@ impl<'a> Resolver<'a> { } fn resolve_pattern(&mut self, pattern: Pattern, definition: DefinitionKind) -> HirPattern { - self.resolve_pattern_mutable(pattern, None, definition) + self.resolve_pattern_mutable(pattern, Mutability::Immutable, None, definition) } fn resolve_pattern_mutable( &mut self, pattern: Pattern, - mutable: Option, + mutability: Mutability, + mutable_span: Option, definition: DefinitionKind, ) -> HirPattern { match pattern { Pattern::Identifier(name) => { // If this definition is mutable, do not store the rhs because it will // not always refer to the correct value of the variable - let definition = match (mutable, definition) { + let definition = match (mutable_span, definition) { (Some(_), DefinitionKind::Local(_)) => DefinitionKind::Local(None), (_, other) => other, }; - let id = self.add_variable_decl(name, mutable.is_some(), false, definition); + let id = self.add_variable_decl(name, mutability, false, definition); HirPattern::Identifier(id) } Pattern::Mutable(pattern, span) => { - if let Some(first_mut) = mutable { + if let Some(first_mut) = mutable_span { self.push_err(ResolverError::UnnecessaryMut { first_mut, second_mut: span }); } - let pattern = self.resolve_pattern_mutable(*pattern, Some(span), definition); + let pattern = self.resolve_pattern_mutable(*pattern, Mutability::Mutable, Some(span), definition); HirPattern::Mutable(Box::new(pattern), span) } + Pattern::MutableReference(pattern, span) => { + if let Some(first_mut) = mutable_span { + self.push_err(ResolverError::UnnecessaryMut { first_mut, second_mut: span }); + } + + let pattern = self.resolve_pattern_mutable(*pattern, Mutability::MutableReference, Some(span), definition); + HirPattern::MutableReference(Box::new(pattern), span) + } Pattern::Tuple(fields, span) => { let fields = vecmap(fields, |field| { - self.resolve_pattern_mutable(field, mutable, definition.clone()) + self.resolve_pattern_mutable(field, mutability, mutable_span, definition.clone()) }); HirPattern::Tuple(fields, span) } @@ -1050,7 +1063,7 @@ impl<'a> Resolver<'a> { // shadowing here lets us avoid further errors if we define ERROR_IDENT // multiple times. let name = ERROR_IDENT.into(); - let identifier = this.add_variable_decl(name, false, true, definition.clone()); + let identifier = this.add_variable_decl(name, Mutability::Immutable, true, definition.clone()); HirPattern::Identifier(identifier) }; @@ -1064,7 +1077,7 @@ impl<'a> Resolver<'a> { }; let resolve_field = |this: &mut Self, pattern| { - this.resolve_pattern_mutable(pattern, mutable, definition.clone()) + this.resolve_pattern_mutable(pattern, mutability, mutable_span, definition.clone()) }; let typ = struct_type.clone(); @@ -1244,6 +1257,28 @@ impl<'a> Resolver<'a> { let module_id = self.path_resolver.module_id(); module_id.module(self.def_maps).is_contract } + + fn add_mutable_reference_from_pattern_to_type(&self, pattern: &Pattern, typ: Type, wrap_in_mutref: bool) -> Type { + match (pattern, typ) { + (Pattern::Identifier(ident), typ) => { + if wrap_in_mutref { + Type::MutableReference(Box::new(typ)) + } else { + typ + } + }, + (Pattern::Mutable(pattern, _), typ) => self.add_mutable_reference_from_pattern_to_type(pattern, typ, wrap_in_mutref), + (Pattern::MutableReference(pattern, _), typ) => self.add_mutable_reference_from_pattern_to_type(pattern, typ, true), + (Pattern::Tuple(patterns, _), Type::Tuple(fields)) => { + Type::Tuple(vecmap(patterns.iter().zip(fields), |(pattern, field_type)| { + self.add_mutable_reference_from_pattern_to_type(pattern, field_type, wrap_in_mutref) + })) + }, + (Pattern::Struct(_, patterns, _), Type::Struct(struct_type, generics)) => { + todo!() + }, + } + } } // XXX: These tests repeat a lot of code diff --git a/crates/noirc_frontend/src/hir/type_check/stmt.rs b/crates/noirc_frontend/src/hir/type_check/stmt.rs index e6ec71bfc1..9c6908c564 100644 --- a/crates/noirc_frontend/src/hir/type_check/stmt.rs +++ b/crates/noirc_frontend/src/hir/type_check/stmt.rs @@ -4,7 +4,7 @@ use crate::hir_def::stmt::{ HirAssignStatement, HirConstrainStatement, HirLValue, HirLetStatement, HirPattern, HirStatement, }; use crate::hir_def::types::Type; -use crate::node_interner::{DefinitionId, ExprId, StmtId}; +use crate::node_interner::{DefinitionId, ExprId, StmtId, Mutability}; use crate::CompTime; use super::errors::TypeCheckError; @@ -58,6 +58,7 @@ impl<'interner> TypeChecker<'interner> { match pattern { HirPattern::Identifier(ident) => self.interner.push_definition_type(ident.id, typ), HirPattern::Mutable(pattern, _) => self.bind_pattern(pattern, typ), + HirPattern::MutableReference(pattern, _) => self.bind_pattern(pattern, typ), HirPattern::Tuple(fields, span) => match typ { Type::Tuple(field_types) if field_types.len() == fields.len() => { for (field, field_type) in fields.iter().zip(field_types) { @@ -124,7 +125,7 @@ impl<'interner> TypeChecker<'interner> { Type::Error } else { let definition = self.interner.definition(ident.id); - if !definition.mutable { + if definition.mutability == Mutability::Immutable { self.errors.push(TypeCheckError::Unstructured { msg: format!( "Variable {} must be mutable to be assigned to", diff --git a/crates/noirc_frontend/src/hir_def/function.rs b/crates/noirc_frontend/src/hir_def/function.rs index 60cb8ea4f8..889e889605 100644 --- a/crates/noirc_frontend/src/hir_def/function.rs +++ b/crates/noirc_frontend/src/hir_def/function.rs @@ -44,6 +44,7 @@ fn get_param_name<'a>(pattern: &HirPattern, interner: &'a NodeInterner) -> Optio match pattern { HirPattern::Identifier(ident) => Some(interner.definition_name(ident.id)), HirPattern::Mutable(pattern, _) => get_param_name(pattern, interner), + HirPattern::MutableReference(pattern, _) => get_param_name(pattern, interner), HirPattern::Tuple(_, _) => None, HirPattern::Struct(_, _, _) => None, } @@ -68,6 +69,7 @@ impl Parameters { let mut spans = vecmap(&self.0, |param| match ¶m.0 { HirPattern::Identifier(ident) => ident.location.span, HirPattern::Mutable(_, span) => *span, + HirPattern::MutableReference(_, span) => *span, HirPattern::Tuple(_, span) => *span, HirPattern::Struct(_, _, span) => *span, }); diff --git a/crates/noirc_frontend/src/hir_def/stmt.rs b/crates/noirc_frontend/src/hir_def/stmt.rs index 04a6d9770f..c68f9588cc 100644 --- a/crates/noirc_frontend/src/hir_def/stmt.rs +++ b/crates/noirc_frontend/src/hir_def/stmt.rs @@ -52,6 +52,7 @@ pub struct HirConstrainStatement(pub ExprId, pub FileId); pub enum HirPattern { Identifier(HirIdent), Mutable(Box, Span), + MutableReference(Box, Span), Tuple(Vec, Span), Struct(Type, Vec<(Ident, HirPattern)>, Span), } @@ -60,7 +61,8 @@ impl HirPattern { pub fn field_count(&self) -> usize { match self { HirPattern::Identifier(_) => 0, - HirPattern::Mutable(_, _) => 0, + HirPattern::Mutable(pattern, _) => pattern.field_count(), + HirPattern::MutableReference(pattern, _) => pattern.field_count(), HirPattern::Tuple(fields, _) => fields.len(), HirPattern::Struct(_, fields, _) => fields.len(), } diff --git a/crates/noirc_frontend/src/hir_def/types.rs b/crates/noirc_frontend/src/hir_def/types.rs index 133d8a7905..59df276935 100644 --- a/crates/noirc_frontend/src/hir_def/types.rs +++ b/crates/noirc_frontend/src/hir_def/types.rs @@ -44,6 +44,9 @@ pub enum Type { /// A tuple type with the given list of fields in the order they appear in source code. Tuple(Vec), + /// &mut T + MutableReference(Box), + /// TypeVariables are stand-in variables for some type which is not yet known. /// They are not to be confused with NamedGenerics. While the later mostly works /// as with normal types (ie. for two NamedGenerics T and U, T != U), TypeVariables @@ -578,6 +581,7 @@ impl Type { Type::Tuple(fields) => { fields.iter().any(|field| field.contains_numeric_typevar(target_id)) } + Type::MutableReference(typ) => typ.contains_numeric_typevar(target_id), Type::Function(parameters, return_type) => { parameters.iter().any(|parameter| parameter.contains_numeric_typevar(target_id)) || return_type.contains_numeric_typevar(target_id) @@ -629,6 +633,7 @@ impl std::fmt::Display for Type { let elements = vecmap(elements, ToString::to_string); write!(f, "({})", elements.join(", ")) } + Type::MutableReference(element) => write!(f, "&mut {element}"), Type::Bool(comp_time) => write!(f, "{comp_time}bool"), Type::String(len) => write!(f, "str<{len}>"), Type::Unit => write!(f, "()"), @@ -983,6 +988,8 @@ impl Type { (Vec(elem_a), Vec(elem_b)) => elem_a.try_unify(elem_b, span), + (MutableReference(elem_a), MutableReference(elem_b)) => elem_a.try_unify(elem_b, span), + (other_a, other_b) => { if other_a == other_b { Ok(()) @@ -1116,6 +1123,8 @@ impl Type { (Vec(elem_a), Vec(elem_b)) => elem_a.is_subtype_of(elem_b, span), + (MutableReference(elem_a), MutableReference(elem_b)) => elem_a.is_subtype_of(elem_b, span), + (other_a, other_b) => { if other_a == other_b { Ok(()) @@ -1186,6 +1195,7 @@ impl Type { Type::NamedGeneric(..) => unreachable!(), Type::Forall(..) => unreachable!(), Type::Function(_, _) => unreachable!(), + Type::MutableReference(_) => unreachable!("&mut cannot be used in the abi"), Type::Vec(_) => unreachable!("Vecs cannot be used in the abi"), } } @@ -1302,6 +1312,8 @@ impl Type { } Type::Vec(element) => Type::Vec(Box::new(element.substitute(type_bindings))), + Type::MutableReference(element) => Type::MutableReference(Box::new(element.substitute(type_bindings))), + Type::FieldElement(_) | Type::Integer(_, _, _) | Type::Bool(_) @@ -1331,6 +1343,7 @@ impl Type { args.iter().any(|arg| arg.occurs(target_id)) || ret.occurs(target_id) } Type::Vec(element) => element.occurs(target_id), + Type::MutableReference(element) => element.occurs(target_id), Type::FieldElement(_) | Type::Integer(_, _, _) @@ -1373,6 +1386,7 @@ impl Type { Function(args, ret) } Vec(element) => Vec(Box::new(element.follow_bindings())), + MutableReference(element) => MutableReference(Box::new(element.follow_bindings())), // Expect that this function should only be called on instantiated types Forall(..) => unreachable!(), diff --git a/crates/noirc_frontend/src/monomorphization/ast.rs b/crates/noirc_frontend/src/monomorphization/ast.rs index aaaf7c5bb2..ef60f20aaa 100644 --- a/crates/noirc_frontend/src/monomorphization/ast.rs +++ b/crates/noirc_frontend/src/monomorphization/ast.rs @@ -3,7 +3,7 @@ use iter_extended::vecmap; use noirc_abi::FunctionSignature; use noirc_errors::Location; -use crate::{BinaryOpKind, Signedness}; +use crate::{BinaryOpKind, Signedness, node_interner::Mutability}; /// The monomorphized AST is expression-based, all statements are also /// folded into this expression enum. Compared to the HIR, the monomorphized @@ -61,7 +61,7 @@ pub struct FuncId(pub u32); pub struct Ident { pub location: Option, pub definition: Definition, - pub mutable: bool, + pub mutability: Mutability, pub name: String, pub typ: Type, } @@ -152,7 +152,7 @@ pub struct Index { #[derive(Debug, Clone)] pub struct Let { pub id: LocalId, - pub mutable: bool, + pub mutability: Mutability, pub name: String, pub expression: Box, } @@ -178,7 +178,7 @@ pub enum LValue { MemberAccess { object: Box, field_index: usize }, } -pub type Parameters = Vec<(LocalId, /*mutable:*/ bool, /*name:*/ String, Type)>; +pub type Parameters = Vec<(LocalId, Mutability, /*name:*/ String, Type)>; #[derive(Debug, Clone)] pub struct Function { @@ -208,6 +208,7 @@ pub enum Type { Unit, Tuple(Vec), Vec(Box), + MutableReference(Box), Function(/*args:*/ Vec, /*ret:*/ Box), } @@ -321,6 +322,7 @@ impl std::fmt::Display for Type { write!(f, "fn({}) -> {}", args.join(", "), ret) } Type::Vec(element) => write!(f, "Vec<{element}>"), + Type::MutableReference(element) => write!(f, "&mut {element}"), } } } diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 8c588acbf9..6c55f4f228 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -20,7 +20,7 @@ use crate::{ function::{FuncMeta, Param, Parameters}, stmt::{HirAssignStatement, HirLValue, HirLetStatement, HirPattern, HirStatement}, }, - node_interner::{self, DefinitionKind, NodeInterner, StmtId}, + node_interner::{self, DefinitionKind, NodeInterner, StmtId, Mutability}, token::Attribute, CompTime, FunctionKind, TypeBinding, TypeBindings, }; @@ -204,7 +204,7 @@ impl<'interner> Monomorphizer<'interner> { /// Monomorphize each parameter, expanding tuple/struct patterns into multiple parameters /// and binding any generic types found. - fn parameters(&mut self, params: Parameters) -> Vec<(ast::LocalId, bool, String, ast::Type)> { + fn parameters(&mut self, params: Parameters) -> Vec<(ast::LocalId, Mutability, String, ast::Type)> { let mut new_params = Vec::with_capacity(params.len()); for parameter in params { self.parameter(parameter.0, ¶meter.1, &mut new_params); @@ -216,18 +216,19 @@ impl<'interner> Monomorphizer<'interner> { &mut self, param: HirPattern, typ: &HirType, - new_params: &mut Vec<(ast::LocalId, bool, String, ast::Type)>, + new_params: &mut Vec<(ast::LocalId, Mutability, String, ast::Type)>, ) { match param { HirPattern::Identifier(ident) => { - //let value = self.expand_parameter(typ, new_params); let new_id = self.next_local_id(); let definition = self.interner.definition(ident.id); let name = definition.name.clone(); - new_params.push((new_id, definition.mutable, name, Self::convert_type(typ))); + println!("Mutability of parameter is {}", definition.mutability); + new_params.push((new_id, definition.mutability, name, Self::convert_type(typ))); self.define_local(ident.id, new_id); } HirPattern::Mutable(pattern, _) => self.parameter(*pattern, typ, new_params), + HirPattern::MutableReference(pattern, _) => self.parameter(*pattern, typ, new_params), HirPattern::Tuple(fields, _) => { let tuple_field_types = unwrap_tuple_type(typ); @@ -382,7 +383,8 @@ impl<'interner> Monomorphizer<'interner> { | ast::Type::Integer(_, _) | ast::Type::Bool | ast::Type::Unit - | ast::Type::Function(_, _) => { + | ast::Type::Function(_, _) + | ast::Type::MutableReference(_) => { ast::Expression::Literal(ast::Literal::Array(ast::ArrayLiteral { contents: array_contents, element_type, @@ -428,7 +430,8 @@ impl<'interner> Monomorphizer<'interner> { | ast::Type::Integer(_, _) | ast::Type::Bool | ast::Type::Unit - | ast::Type::Function(_, _) => { + | ast::Type::Function(_, _) + | ast::Type::MutableReference(_) => { ast::Expression::Index(ast::Index { collection, index, element_type, location }) } @@ -495,7 +498,7 @@ impl<'interner> Monomorphizer<'interner> { new_exprs.push(ast::Expression::Let(ast::Let { id: new_id, - mutable: false, + mutability: Mutability::Immutable, name: field_name.0.contents, expression, })); @@ -510,8 +513,8 @@ impl<'interner> Monomorphizer<'interner> { }); let definition = Definition::Local(id); - let mutable = false; - ast::Expression::Ident(ast::Ident { definition, mutable, location: None, name, typ }) + let mutability = Mutability::Immutable; + ast::Expression::Ident(ast::Ident { definition, mutability, location: None, name, typ }) }); // Finally we can return the created Tuple from the new block @@ -537,12 +540,13 @@ impl<'interner> Monomorphizer<'interner> { ast::Expression::Let(ast::Let { id: new_id, - mutable: definition.mutable, + mutability: definition.mutability, name: definition.name.clone(), expression: Box::new(value), }) } HirPattern::Mutable(pattern, _) => self.unpack_pattern(*pattern, value, typ), + HirPattern::MutableReference(pattern, _) => self.unpack_pattern(*pattern, value, typ), HirPattern::Tuple(patterns, _) => { let fields = unwrap_tuple_type(typ); self.unpack_tuple_pattern(value, patterns.into_iter().zip(fields)) @@ -574,20 +578,20 @@ impl<'interner> Monomorphizer<'interner> { let mut definitions = vec![ast::Expression::Let(ast::Let { id: fresh_id, - mutable: false, + mutability: Mutability::Immutable, name: "_".into(), expression: Box::new(value), })]; for (i, (field_pattern, field_type)) in fields.into_iter().enumerate() { let location = None; - let mutable = false; + let mutability = Mutability::Immutable; let definition = Definition::Local(fresh_id); let name = i.to_string(); let typ = Self::convert_type(&field_type); let new_rhs = - ast::Expression::Ident(ast::Ident { location, mutable, definition, name, typ }); + ast::Expression::Ident(ast::Ident { location, mutability, definition, name, typ }); let new_rhs = ast::Expression::ExtractTupleField(Box::new(new_rhs), i); let new_expr = self.unpack_pattern(field_pattern, new_rhs, &field_type); @@ -601,26 +605,26 @@ impl<'interner> Monomorphizer<'interner> { fn local_ident(&mut self, ident: &HirIdent) -> Option { let definition = self.interner.definition(ident.id); let name = definition.name.clone(); - let mutable = definition.mutable; + let mutable = definition.mutability; let definition = self.lookup_local(ident.id)?; let typ = Self::convert_type(&self.interner.id_type(ident.id)); - Some(ast::Ident { location: Some(ident.location), mutable, definition, name, typ }) + Some(ast::Ident { location: Some(ident.location), mutability: mutable, definition, name, typ }) } fn ident(&mut self, ident: HirIdent, expr_id: node_interner::ExprId) -> ast::Expression { let definition = self.interner.definition(ident.id); match &definition.kind { DefinitionKind::Function(func_id) => { - let mutable = definition.mutable; + let mutability = definition.mutability; let location = Some(ident.location); let name = definition.name.clone(); let typ = self.interner.id_type(expr_id); let definition = self.lookup_function(*func_id, expr_id, &typ); let typ = Self::convert_type(&typ); - let ident = ast::Ident { location, mutable, definition, name, typ }; + let ident = ast::Ident { location, mutability, definition, name, typ }; ast::Expression::Ident(ident) } DefinitionKind::Global(expr_id) => self.expr(*expr_id), @@ -700,6 +704,11 @@ impl<'interner> Monomorphizer<'interner> { ast::Type::Vec(Box::new(element)) } + HirType::MutableReference(element) => { + let element = Self::convert_type(element); + ast::Type::MutableReference(Box::new(element)) + } + HirType::Forall(_, _) | HirType::Constant(_) | HirType::Error => { unreachable!("Unexpected type {} found", typ) } @@ -714,7 +723,8 @@ impl<'interner> Monomorphizer<'interner> { | ast::Type::Integer(_, _) | ast::Type::Bool | ast::Type::Unit - | ast::Type::Function(_, _) => ast::Type::Array(length, Box::new(element)), + | ast::Type::Function(_, _) + | ast::Type::MutableReference(_) => ast::Type::Array(length, Box::new(element)), ast::Type::Tuple(elements) => { ast::Type::Tuple(vecmap(elements, |typ| Self::aos_to_soa_type(length, typ))) @@ -943,7 +953,7 @@ impl<'interner> Monomorphizer<'interner> { let name = lambda_name.to_owned(); ast::Expression::Ident(ast::Ident { definition: Definition::Function(id), - mutable: false, + mutability: Mutability::Immutable, location: None, name, typ, @@ -978,6 +988,7 @@ impl<'interner> Monomorphizer<'interner> { ast::Type::Function(parameter_types, ret_type) => { self.create_zeroed_function(parameter_types, ret_type) } + ast::Type::MutableReference(element) => self.zeroed_value_of_type(element), ast::Type::Vec(_) => panic!("Cannot create a zeroed Vec value. This type is currently unimplemented and meant to be unusable outside of unconstrained functions"), } } @@ -996,7 +1007,7 @@ impl<'interner> Monomorphizer<'interner> { let lambda_name = "zeroed_lambda"; let parameters = vecmap(parameter_types, |parameter_type| { - (self.next_local_id(), false, "_".into(), parameter_type.clone()) + (self.next_local_id(), Mutability::Immutable, "_".into(), parameter_type.clone()) }); let body = self.zeroed_value_of_type(ret_type); @@ -1011,7 +1022,7 @@ impl<'interner> Monomorphizer<'interner> { ast::Expression::Ident(ast::Ident { definition: Definition::Function(id), - mutable: false, + mutability: Mutability::Immutable, location: None, name: lambda_name.to_owned(), typ: ast::Type::Function(parameter_types.to_owned(), Box::new(ret_type.clone())), diff --git a/crates/noirc_frontend/src/monomorphization/printer.rs b/crates/noirc_frontend/src/monomorphization/printer.rs index 39c6db8734..036f4c12a6 100644 --- a/crates/noirc_frontend/src/monomorphization/printer.rs +++ b/crates/noirc_frontend/src/monomorphization/printer.rs @@ -11,8 +11,8 @@ pub struct AstPrinter { impl AstPrinter { pub fn print_function(&mut self, function: &Function, f: &mut Formatter) -> std::fmt::Result { - let params = vecmap(&function.parameters, |(id, mutable, name, typ)| { - format!("{}{}$l{}: {}", if *mutable { "mut " } else { "" }, name, id.0, typ) + let params = vecmap(&function.parameters, |(id, mutability, name, typ)| { + format!("{}{}$l{}: {}", mutability, name, id.0, typ) }) .join(", "); diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index b61886a170..0ab27a823b 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -179,7 +179,7 @@ enum Node { #[derive(Debug, Clone)] pub struct DefinitionInfo { pub name: String, - pub mutable: bool, + pub mutability: Mutability, pub kind: DefinitionKind, } @@ -191,6 +191,23 @@ impl DefinitionInfo { } } +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum Mutability { + Immutable, + Mutable, + MutableReference, +} + +impl std::fmt::Display for Mutability { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Mutability::Immutable => Ok(()), + Mutability::Mutable => write!(f, "mut "), + Mutability::MutableReference => write!(f, "&mut "), + } + } +} + #[derive(Debug, Clone, Eq, PartialEq)] pub enum DefinitionKind { Function(FuncId), @@ -385,7 +402,7 @@ impl NodeInterner { pub fn push_definition( &mut self, name: String, - mutable: bool, + mutability: Mutability, definition: DefinitionKind, ) -> DefinitionId { let id = DefinitionId(self.definitions.len()); @@ -393,12 +410,12 @@ impl NodeInterner { self.function_definition_ids.insert(func_id, id); } - self.definitions.push(DefinitionInfo { name, mutable, kind: definition }); + self.definitions.push(DefinitionInfo { name, mutability, kind: definition }); id } pub fn push_function_definition(&mut self, name: String, func: FuncId) -> DefinitionId { - self.push_definition(name, false, DefinitionKind::Function(func)) + self.push_definition(name, Mutability::Immutable, DefinitionKind::Function(func)) } /// Returns the interned HIR function corresponding to `func_id` @@ -621,6 +638,7 @@ fn get_type_method_key(typ: &Type) -> Option { Type::Tuple(_) => Some(Tuple), Type::Function(_, _) => Some(Function), Type::Vec(_) => Some(Vec), + Type::MutableReference(element) => get_type_method_key(element), // We do not support adding methods to these types Type::TypeVariable(_) diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index deaa045ccf..c99c875f38 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -36,7 +36,7 @@ use crate::token::{Attribute, Keyword, Token, TokenKind}; use crate::{ BinaryOp, BinaryOpKind, BlockExpression, CompTime, ConstrainStatement, FunctionDefinition, Ident, IfExpression, InfixExpression, LValue, Lambda, NoirFunction, NoirImpl, NoirStruct, Path, - PathKind, Pattern, Recoverable, UnaryOp, UnresolvedTypeExpression, UseTree, UseTreeKind, + PathKind, Pattern, Recoverable, UnaryOp, UnresolvedTypeExpression, UseTree, UseTreeKind, ArgumentMode, }; use chumsky::prelude::*; @@ -270,7 +270,7 @@ fn lambda_parameters() -> impl NoirParser> { let typ = parse_type().recover_via(parameter_recovery()); let typ = just(Token::Colon).ignore_then(typ); - let parameter = pattern() + let parameter = pattern(true) .recover_via(parameter_name_recovery()) .then(typ.or_not().map(|typ| typ.unwrap_or(UnresolvedType::Unspecified))); @@ -285,7 +285,7 @@ fn function_parameters<'a>( ) -> impl NoirParser> + 'a { let typ = parse_type().recover_via(parameter_recovery()); - let full_parameter = pattern() + let full_parameter = pattern(true) .recover_via(parameter_name_recovery()) .then_ignore(just(Token::Colon)) .then(optional_visibility()) @@ -511,14 +511,14 @@ where P: ExprParser + 'a, { let p = - ignore_then_commit(keyword(Keyword::Let).labelled(ParsingRuleLabel::Statement), pattern()); + ignore_then_commit(keyword(Keyword::Let).labelled(ParsingRuleLabel::Statement), pattern(false)); let p = p.then(optional_type_annotation()); let p = then_commit_ignore(p, just(Token::Assign)); let p = then_commit(p, expr_parser); p.map(Statement::new_let) } -fn pattern() -> impl NoirParser { +fn pattern(allow_mutable_references: bool) -> impl NoirParser { recursive(|pattern| { let ident_pattern = ident().map(Pattern::Identifier); @@ -526,6 +526,14 @@ fn pattern() -> impl NoirParser { .ignore_then(pattern.clone()) .map_with_span(|inner, span| Pattern::Mutable(Box::new(inner), span)); + let mutref_pattern = if allow_mutable_references { + just(Token::Ampersand).ignore_then(keyword(Keyword::Mut)) + .ignore_then(pattern.clone()) + .map_with_span(|inner, span| Pattern::MutableReference(Box::new(inner), span)).boxed() + } else { + nothing().boxed() + }; + let short_field = ident().map(|name| (name.clone(), Pattern::Identifier(name))); let long_field = ident().then_ignore(just(Token::Colon)).then(pattern.clone()); @@ -543,7 +551,7 @@ fn pattern() -> impl NoirParser { .delimited_by(just(Token::LeftParen), just(Token::RightParen)) .map_with_span(Pattern::Tuple); - choice((mut_pattern, tuple_pattern, struct_pattern, ident_pattern)) + choice((mut_pattern, mutref_pattern, tuple_pattern, struct_pattern, ident_pattern)) }) .labelled(ParsingRuleLabel::Pattern) } @@ -845,14 +853,14 @@ where P: ExprParser + 'a, { enum UnaryRhs { - Call(Vec), + Call(Vec<(ArgumentMode, Expression)>), ArrayIndex(Expression), Cast(UnresolvedType), MemberAccess((Ident, Option>)), } // `(arg1, ..., argN)` in `my_func(arg1, ..., argN)` - let call_rhs = parenthesized(expression_list(expr_parser.clone())).map(UnaryRhs::Call); + let call_rhs = parenthesized(argument_list(expr_parser.clone())).map(UnaryRhs::Call); // `[expr]` in `arr[expr]` let array_rhs = expr_parser @@ -987,6 +995,18 @@ where expr_parser.separated_by(just(Token::Comma)).allow_trailing() } +fn argument_list

(expr_parser: P) -> impl NoirParser> +where + P: ExprParser, +{ + just(Token::Ampersand) + .ignore_then(keyword(Keyword::Mut)) + .or_not() + .map(|opt| opt.map_or(ArgumentMode::PassByValue, |_| ArgumentMode::PassByReference)) + .then(expr_parser) + .separated_by(just(Token::Comma)).allow_trailing() +} + fn not

(term_parser: P) -> impl NoirParser where P: ExprParser, From 2a264037fe624e9446553a4b52e81f85a5148c2a Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 30 Jun 2023 13:58:07 +0200 Subject: [PATCH 02/13] Cleanup --- crates/noirc_evaluator/src/ssa/context.rs | 6 +- crates/noirc_evaluator/src/ssa/function.rs | 9 ++- crates/noirc_evaluator/src/ssa/value.rs | 1 - .../src/ssa_refactor/ssa_gen/context.rs | 15 ++-- .../src/ssa_refactor/ssa_gen/mod.rs | 14 ++-- .../src/ssa_refactor/ssa_gen/value.rs | 21 ++--- crates/noirc_frontend/src/ast/expression.rs | 6 +- .../src/hir/resolution/resolver.rs | 81 +++++++++++-------- .../noirc_frontend/src/hir/type_check/stmt.rs | 2 +- crates/noirc_frontend/src/hir_def/types.rs | 14 ---- .../src/monomorphization/ast.rs | 6 +- .../src/monomorphization/mod.rs | 36 ++++----- .../src/monomorphization/printer.rs | 8 +- crates/noirc_frontend/src/node_interner.rs | 1 - crates/noirc_frontend/src/parser/parser.rs | 59 ++++++++++---- 15 files changed, 158 insertions(+), 121 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/context.rs b/crates/noirc_evaluator/src/ssa/context.rs index 64b44ebf02..639e2dd8d5 100644 --- a/crates/noirc_evaluator/src/ssa/context.rs +++ b/crates/noirc_evaluator/src/ssa/context.rs @@ -16,6 +16,7 @@ use acvm::FieldElement; use iter_extended::vecmap; use noirc_errors::Location; use noirc_frontend::monomorphization::ast::{Definition, Expression, FuncId, Literal, Type}; +use noirc_frontend::ArgumentMode; use num_bigint::BigUint; use std::collections::{HashMap, HashSet}; @@ -1155,7 +1156,7 @@ impl SsaContext { pub(crate) fn get_builtin_opcode( &self, node_id: NodeId, - arguments: &[Expression], + arguments: &[(ArgumentMode, Expression)], ) -> Option { match &self[node_id] { NodeObject::Function(FunctionKind::Builtin(opcode), ..) => match opcode { @@ -1166,7 +1167,7 @@ impl SsaContext { 1, "print statements currently only support one argument" ); - let is_string = match &arguments[0] { + let is_string = match &arguments[0].1 { Expression::Ident(ident) => match ident.typ { Type::String(_) => true, Type::Tuple(_) => { @@ -1206,7 +1207,6 @@ impl SsaContext { Signedness::Unsigned => ObjectType::unsigned_integer(*bit_size), } } - Type::MutableReference(element) => self.convert_type(element), Type::Array(..) => panic!("Cannot convert an array type {t} into an ObjectType since it is unknown which array it refers to"), Type::Unit => ObjectType::NotAnObject, Type::Function(..) => ObjectType::Function, diff --git a/crates/noirc_evaluator/src/ssa/function.rs b/crates/noirc_evaluator/src/ssa/function.rs index 59e6f7ce38..e6412b71d2 100644 --- a/crates/noirc_evaluator/src/ssa/function.rs +++ b/crates/noirc_evaluator/src/ssa/function.rs @@ -156,7 +156,8 @@ impl IrGenerator { //arguments: for (param_id, mutable, name, typ) in std::mem::take(&mut function.parameters) { let node_ids = self.create_function_parameter(param_id, &typ, &name); - func.arguments.extend(node_ids.into_iter().map(|id| (id, mutable == Mutability::Mutable))); + func.arguments + .extend(node_ids.into_iter().map(|id| (id, mutable == Mutability::Mutable))); } // ensure return types are defined in case of recursion call cycle @@ -229,7 +230,11 @@ impl IrGenerator { //generates an instruction for calling the function pub(super) fn call(&mut self, call: &Call) -> Result, RuntimeError> { let func = self.ssa_gen_expression(&call.func)?.unwrap_id(); - let arguments = self.ssa_gen_expression_list(&call.arguments); + let arguments = call + .arguments + .iter() + .flat_map(|(_, arg)| self.ssa_gen_expression(arg).unwrap().to_node_ids()) + .collect(); if let Some(opcode) = self.context.get_builtin_opcode(func, &call.arguments) { return self.call_low_level(opcode, arguments); diff --git a/crates/noirc_evaluator/src/ssa/value.rs b/crates/noirc_evaluator/src/ssa/value.rs index 83b73181b3..915effe480 100644 --- a/crates/noirc_evaluator/src/ssa/value.rs +++ b/crates/noirc_evaluator/src/ssa/value.rs @@ -93,7 +93,6 @@ impl Value { let values = vecmap(tup, |v| Self::reshape(v, iter)); Value::Tuple(values) } - Type::MutableReference(element) => Self::reshape(element, iter), Type::Unit | Type::Function(..) | Type::Array(..) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index 5237a071a9..3f5e9c5068 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -6,8 +6,8 @@ use acvm::FieldElement; use iter_extended::vecmap; use noirc_frontend::monomorphization::ast::{self, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; -use noirc_frontend::Signedness; use noirc_frontend::node_interner::Mutability; +use noirc_frontend::Signedness; use crate::ssa_refactor::ir::dfg::DataFlowGraph; use crate::ssa_refactor::ir::function::FunctionId as IrFunctionId; @@ -140,8 +140,8 @@ impl<'a> FunctionContext<'a> { match mutability { Mutability::Immutable => value.into(), - Mutability::Mutable => self.new_mutable_variable(value, false), - Mutability::MutableReference => Value::MutableReference(value, typ), + Mutability::Mutable => self.new_mutable_variable(value), + Mutability::MutableReference => Value::Mutable(value, typ), } }); @@ -150,15 +150,11 @@ impl<'a> FunctionContext<'a> { /// Allocate a single slot of memory and store into it the given initial value of the variable. /// Always returns a Value::Mutable wrapping the allocate instruction. - pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId, is_reference: bool) -> Value { + pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId) -> Value { let alloc = self.builder.insert_allocate(); self.builder.insert_store(alloc, value_to_store); let typ = self.builder.type_of_value(value_to_store); - if is_reference { - Value::MutableReference(alloc, typ) - } else { - Value::Mutable(alloc, typ) - } + Value::Mutable(alloc, typ) } /// Maps the given type to a Tree of the result type. @@ -207,7 +203,6 @@ impl<'a> FunctionContext<'a> { ast::Type::Unit => panic!("convert_non_tuple_type called on a unit type"), ast::Type::Tuple(_) => panic!("convert_non_tuple_type called on a tuple: {typ}"), ast::Type::Function(_, _) => Type::Function, - ast::Type::MutableReference(element) => Self::convert_non_tuple_type(element), // How should we represent Vecs? // Are they a struct of array + length + capacity? diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index c8b6cb17dc..a2bc4d4f2c 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -9,7 +9,10 @@ pub(crate) use program::Ssa; use context::SharedContext; use iter_extended::vecmap; use noirc_errors::Location; -use noirc_frontend::{monomorphization::ast::{self, Expression, Program}, node_interner::Mutability}; +use noirc_frontend::{ + monomorphization::ast::{self, Expression, Program}, + node_interner::Mutability, +}; use self::{ context::FunctionContext, @@ -95,7 +98,7 @@ impl<'a> FunctionContext<'a> { /// Codegen for identifiers fn codegen_ident(&mut self, ident: &ast::Ident) -> Values { match &ident.definition { - ast::Definition::Local(id) => self.lookup(*id).map(|value| value.eval(self).into()), + ast::Definition::Local(id) => self.lookup(*id), ast::Definition::Function(id) => self.get_or_queue_function(*id), ast::Definition::Oracle(name) => self.builder.import_foreign_function(name).into(), ast::Definition::Builtin(name) | ast::Definition::LowLevel(name) => { @@ -342,7 +345,9 @@ impl<'a> FunctionContext<'a> { let arguments = call .arguments .iter() - .flat_map(|argument| self.codegen_expression(argument).into_argument_list(self)) + .flat_map(|(mode, argument)| { + self.codegen_expression(argument).into_argument_list(self, *mode) + }) .collect(); let function = self.codegen_non_tuple_expression(&call.func); @@ -359,8 +364,7 @@ impl<'a> FunctionContext<'a> { if let_expr.mutability != Mutability::Immutable { values = values.map(|value| { let value = value.eval(self); - let is_mutable_reference = let_expr.mutability == Mutability::MutableReference; - Tree::Leaf(self.new_mutable_variable(value, is_mutable_reference)) + Tree::Leaf(self.new_mutable_variable(value)) }); } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs index 879e8b28f0..a3438fc9a2 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs @@ -1,4 +1,5 @@ use iter_extended::vecmap; +use noirc_frontend::ArgumentMode; use crate::ssa_refactor::ir::types::Type; use crate::ssa_refactor::ir::value::ValueId as IrValueId; @@ -32,8 +33,6 @@ pub(super) enum Value { /// A mutable variable that must be loaded as the given type before being used Mutable(IrValueId, Type), - - MutableReference(IrValueId, Type), } impl Value { @@ -44,7 +43,6 @@ impl Value { match self { Value::Normal(value) => value, Value::Mutable(address, typ) => ctx.builder.insert_load(address, typ), - Value::MutableReference(address, typ) => ctx.builder.insert_load(address, typ), } } @@ -54,15 +52,16 @@ impl Value { match self { Value::Normal(value) => value, Value::Mutable(address, _) => address, - Value::MutableReference(address, _) => address, } } - pub(super) fn eval_argument(self, ctx: &mut FunctionContext) -> IrValueId { + pub(super) fn eval_argument(self, ctx: &mut FunctionContext, mode: ArgumentMode) -> IrValueId { match self { Value::Normal(value) => value, - Value::Mutable(address, typ) => ctx.builder.insert_load(address, typ), - Value::MutableReference(address, _) => address, + Value::Mutable(address, typ) => match mode { + ArgumentMode::PassByValue => ctx.builder.insert_load(address, typ), + ArgumentMode::PassByReference => address, + }, } } } @@ -173,7 +172,11 @@ impl Tree { vecmap(self.flatten(), |value| value.eval(ctx)) } - pub(super) fn into_argument_list(self, ctx: &mut FunctionContext) -> Vec { - vecmap(self.flatten(), |value| value.eval_argument(ctx)) + pub(super) fn into_argument_list( + self, + ctx: &mut FunctionContext, + mode: ArgumentMode, + ) -> Vec { + vecmap(self.flatten(), |value| value.eval_argument(ctx, mode)) } } diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index f6496e94a0..23e223f90c 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -157,7 +157,11 @@ impl Expression { Expression::new(kind, span) } - pub fn call(lhs: Expression, arguments: Vec<(ArgumentMode, Expression)>, span: Span) -> Expression { + pub fn call( + lhs: Expression, + arguments: Vec<(ArgumentMode, Expression)>, + span: Span, + ) -> Expression { let func = Box::new(lhs); let kind = ExpressionKind::Call(Box::new(CallExpression { func, arguments })); Expression::new(kind, span) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 658eb5e9ca..d6a4ff6c77 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -25,7 +25,7 @@ use crate::graph::CrateId; use crate::hir::def_map::{ModuleDefId, TryFromModuleDefId, MAIN_FUNCTION}; use crate::hir_def::stmt::{HirAssignStatement, HirLValue, HirPattern}; use crate::node_interner::{ - DefinitionId, DefinitionKind, ExprId, FuncId, NodeInterner, StmtId, StructId, Mutability, + DefinitionId, DefinitionKind, ExprId, FuncId, Mutability, NodeInterner, StmtId, StructId, }; use crate::{ hir::{def_map::CrateDefMap, resolution::path_resolver::PathResolver}, @@ -252,7 +252,11 @@ impl<'a> Resolver<'a> { ident = hir_let_stmt.ident(); resolver_meta = ResolverMeta { num_times_used: 0, ident, warn_if_unused: true }; } else { - let id = self.interner.push_definition(name.0.contents.clone(), Mutability::Immutable, definition); + let id = self.interner.push_definition( + name.0.contents.clone(), + Mutability::Immutable, + definition, + ); let location = Location::new(name.span(), self.file); ident = HirIdent { location, id }; resolver_meta = ResolverMeta { num_times_used: 0, ident, warn_if_unused: true }; @@ -616,10 +620,8 @@ impl<'a> Resolver<'a> { self.push_err(ResolverError::UnnecessaryPub { ident: func.name_ident().clone() }); } - let typ = self.resolve_type_inner(typ, &mut generics); - let typ = self.add_mutable_reference_from_pattern_to_type(&pattern, typ); - let pattern = self.resolve_pattern(pattern, DefinitionKind::Local(None)); + let typ = self.resolve_type_inner(typ, &mut generics); parameters.push(Param(pattern, typ.clone(), visibility)); parameter_types.push(typ); @@ -724,7 +726,13 @@ impl<'a> Resolver<'a> { { let ident = Ident::new(name.to_string(), *span); let definition = DefinitionKind::GenericType(type_variable); - self.add_variable_decl_inner(ident, Mutability::Immutable, false, false, definition); + self.add_variable_decl_inner( + ident, + Mutability::Immutable, + false, + false, + definition, + ); } } } @@ -784,7 +792,6 @@ impl<'a> Resolver<'a> { } } Type::Vec(element) => Self::find_numeric_generics_in_type(element, found), - Type::MutableReference(element) => Self::find_numeric_generics_in_type(element, found), } } @@ -899,7 +906,8 @@ impl<'a> Resolver<'a> { ExpressionKind::Call(call_expr) => { // Get the span and name of path for error reporting let func = self.resolve_expression(*call_expr.func); - let arguments = vecmap(call_expr.arguments, |(_, arg)| self.resolve_expression(arg)); + let arguments = + vecmap(call_expr.arguments, |(_, arg)| self.resolve_expression(arg)); let location = Location::new(expr.span, self.file); HirExpression::Call(HirCallExpression { func, arguments, location }) } @@ -1040,7 +1048,12 @@ impl<'a> Resolver<'a> { self.push_err(ResolverError::UnnecessaryMut { first_mut, second_mut: span }); } - let pattern = self.resolve_pattern_mutable(*pattern, Mutability::Mutable, Some(span), definition); + let pattern = self.resolve_pattern_mutable( + *pattern, + Mutability::Mutable, + Some(span), + definition, + ); HirPattern::Mutable(Box::new(pattern), span) } Pattern::MutableReference(pattern, span) => { @@ -1048,12 +1061,22 @@ impl<'a> Resolver<'a> { self.push_err(ResolverError::UnnecessaryMut { first_mut, second_mut: span }); } - let pattern = self.resolve_pattern_mutable(*pattern, Mutability::MutableReference, Some(span), definition); + let pattern = self.resolve_pattern_mutable( + *pattern, + Mutability::MutableReference, + Some(span), + definition, + ); HirPattern::MutableReference(Box::new(pattern), span) } Pattern::Tuple(fields, span) => { let fields = vecmap(fields, |field| { - self.resolve_pattern_mutable(field, mutability, mutable_span, definition.clone()) + self.resolve_pattern_mutable( + field, + mutability, + mutable_span, + definition.clone(), + ) }); HirPattern::Tuple(fields, span) } @@ -1063,7 +1086,12 @@ impl<'a> Resolver<'a> { // shadowing here lets us avoid further errors if we define ERROR_IDENT // multiple times. let name = ERROR_IDENT.into(); - let identifier = this.add_variable_decl(name, Mutability::Immutable, true, definition.clone()); + let identifier = this.add_variable_decl( + name, + Mutability::Immutable, + true, + definition.clone(), + ); HirPattern::Identifier(identifier) }; @@ -1077,7 +1105,12 @@ impl<'a> Resolver<'a> { }; let resolve_field = |this: &mut Self, pattern| { - this.resolve_pattern_mutable(pattern, mutability, mutable_span, definition.clone()) + this.resolve_pattern_mutable( + pattern, + mutability, + mutable_span, + definition.clone(), + ) }; let typ = struct_type.clone(); @@ -1257,28 +1290,6 @@ impl<'a> Resolver<'a> { let module_id = self.path_resolver.module_id(); module_id.module(self.def_maps).is_contract } - - fn add_mutable_reference_from_pattern_to_type(&self, pattern: &Pattern, typ: Type, wrap_in_mutref: bool) -> Type { - match (pattern, typ) { - (Pattern::Identifier(ident), typ) => { - if wrap_in_mutref { - Type::MutableReference(Box::new(typ)) - } else { - typ - } - }, - (Pattern::Mutable(pattern, _), typ) => self.add_mutable_reference_from_pattern_to_type(pattern, typ, wrap_in_mutref), - (Pattern::MutableReference(pattern, _), typ) => self.add_mutable_reference_from_pattern_to_type(pattern, typ, true), - (Pattern::Tuple(patterns, _), Type::Tuple(fields)) => { - Type::Tuple(vecmap(patterns.iter().zip(fields), |(pattern, field_type)| { - self.add_mutable_reference_from_pattern_to_type(pattern, field_type, wrap_in_mutref) - })) - }, - (Pattern::Struct(_, patterns, _), Type::Struct(struct_type, generics)) => { - todo!() - }, - } - } } // XXX: These tests repeat a lot of code diff --git a/crates/noirc_frontend/src/hir/type_check/stmt.rs b/crates/noirc_frontend/src/hir/type_check/stmt.rs index 9c6908c564..0dc1902287 100644 --- a/crates/noirc_frontend/src/hir/type_check/stmt.rs +++ b/crates/noirc_frontend/src/hir/type_check/stmt.rs @@ -4,7 +4,7 @@ use crate::hir_def::stmt::{ HirAssignStatement, HirConstrainStatement, HirLValue, HirLetStatement, HirPattern, HirStatement, }; use crate::hir_def::types::Type; -use crate::node_interner::{DefinitionId, ExprId, StmtId, Mutability}; +use crate::node_interner::{DefinitionId, ExprId, Mutability, StmtId}; use crate::CompTime; use super::errors::TypeCheckError; diff --git a/crates/noirc_frontend/src/hir_def/types.rs b/crates/noirc_frontend/src/hir_def/types.rs index 59df276935..133d8a7905 100644 --- a/crates/noirc_frontend/src/hir_def/types.rs +++ b/crates/noirc_frontend/src/hir_def/types.rs @@ -44,9 +44,6 @@ pub enum Type { /// A tuple type with the given list of fields in the order they appear in source code. Tuple(Vec), - /// &mut T - MutableReference(Box), - /// TypeVariables are stand-in variables for some type which is not yet known. /// They are not to be confused with NamedGenerics. While the later mostly works /// as with normal types (ie. for two NamedGenerics T and U, T != U), TypeVariables @@ -581,7 +578,6 @@ impl Type { Type::Tuple(fields) => { fields.iter().any(|field| field.contains_numeric_typevar(target_id)) } - Type::MutableReference(typ) => typ.contains_numeric_typevar(target_id), Type::Function(parameters, return_type) => { parameters.iter().any(|parameter| parameter.contains_numeric_typevar(target_id)) || return_type.contains_numeric_typevar(target_id) @@ -633,7 +629,6 @@ impl std::fmt::Display for Type { let elements = vecmap(elements, ToString::to_string); write!(f, "({})", elements.join(", ")) } - Type::MutableReference(element) => write!(f, "&mut {element}"), Type::Bool(comp_time) => write!(f, "{comp_time}bool"), Type::String(len) => write!(f, "str<{len}>"), Type::Unit => write!(f, "()"), @@ -988,8 +983,6 @@ impl Type { (Vec(elem_a), Vec(elem_b)) => elem_a.try_unify(elem_b, span), - (MutableReference(elem_a), MutableReference(elem_b)) => elem_a.try_unify(elem_b, span), - (other_a, other_b) => { if other_a == other_b { Ok(()) @@ -1123,8 +1116,6 @@ impl Type { (Vec(elem_a), Vec(elem_b)) => elem_a.is_subtype_of(elem_b, span), - (MutableReference(elem_a), MutableReference(elem_b)) => elem_a.is_subtype_of(elem_b, span), - (other_a, other_b) => { if other_a == other_b { Ok(()) @@ -1195,7 +1186,6 @@ impl Type { Type::NamedGeneric(..) => unreachable!(), Type::Forall(..) => unreachable!(), Type::Function(_, _) => unreachable!(), - Type::MutableReference(_) => unreachable!("&mut cannot be used in the abi"), Type::Vec(_) => unreachable!("Vecs cannot be used in the abi"), } } @@ -1312,8 +1302,6 @@ impl Type { } Type::Vec(element) => Type::Vec(Box::new(element.substitute(type_bindings))), - Type::MutableReference(element) => Type::MutableReference(Box::new(element.substitute(type_bindings))), - Type::FieldElement(_) | Type::Integer(_, _, _) | Type::Bool(_) @@ -1343,7 +1331,6 @@ impl Type { args.iter().any(|arg| arg.occurs(target_id)) || ret.occurs(target_id) } Type::Vec(element) => element.occurs(target_id), - Type::MutableReference(element) => element.occurs(target_id), Type::FieldElement(_) | Type::Integer(_, _, _) @@ -1386,7 +1373,6 @@ impl Type { Function(args, ret) } Vec(element) => Vec(Box::new(element.follow_bindings())), - MutableReference(element) => MutableReference(Box::new(element.follow_bindings())), // Expect that this function should only be called on instantiated types Forall(..) => unreachable!(), diff --git a/crates/noirc_frontend/src/monomorphization/ast.rs b/crates/noirc_frontend/src/monomorphization/ast.rs index ef60f20aaa..915c18708e 100644 --- a/crates/noirc_frontend/src/monomorphization/ast.rs +++ b/crates/noirc_frontend/src/monomorphization/ast.rs @@ -3,7 +3,7 @@ use iter_extended::vecmap; use noirc_abi::FunctionSignature; use noirc_errors::Location; -use crate::{BinaryOpKind, Signedness, node_interner::Mutability}; +use crate::{node_interner::Mutability, ArgumentMode, BinaryOpKind, Signedness}; /// The monomorphized AST is expression-based, all statements are also /// folded into this expression enum. Compared to the HIR, the monomorphized @@ -124,7 +124,7 @@ pub struct ArrayLiteral { #[derive(Debug, Clone)] pub struct Call { pub func: Box, - pub arguments: Vec, + pub arguments: Vec<(ArgumentMode, Expression)>, pub return_type: Type, pub location: Location, } @@ -208,7 +208,6 @@ pub enum Type { Unit, Tuple(Vec), Vec(Box), - MutableReference(Box), Function(/*args:*/ Vec, /*ret:*/ Box), } @@ -322,7 +321,6 @@ impl std::fmt::Display for Type { write!(f, "fn({}) -> {}", args.join(", "), ret) } Type::Vec(element) => write!(f, "Vec<{element}>"), - Type::MutableReference(element) => write!(f, "&mut {element}"), } } } diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 6c55f4f228..4fae3c0603 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -20,9 +20,9 @@ use crate::{ function::{FuncMeta, Param, Parameters}, stmt::{HirAssignStatement, HirLValue, HirLetStatement, HirPattern, HirStatement}, }, - node_interner::{self, DefinitionKind, NodeInterner, StmtId, Mutability}, + node_interner::{self, DefinitionKind, Mutability, NodeInterner, StmtId}, token::Attribute, - CompTime, FunctionKind, TypeBinding, TypeBindings, + ArgumentMode, CompTime, FunctionKind, TypeBinding, TypeBindings, }; use self::ast::{Definition, FuncId, Function, LocalId, Program}; @@ -204,7 +204,10 @@ impl<'interner> Monomorphizer<'interner> { /// Monomorphize each parameter, expanding tuple/struct patterns into multiple parameters /// and binding any generic types found. - fn parameters(&mut self, params: Parameters) -> Vec<(ast::LocalId, Mutability, String, ast::Type)> { + fn parameters( + &mut self, + params: Parameters, + ) -> Vec<(ast::LocalId, Mutability, String, ast::Type)> { let mut new_params = Vec::with_capacity(params.len()); for parameter in params { self.parameter(parameter.0, ¶meter.1, &mut new_params); @@ -223,7 +226,6 @@ impl<'interner> Monomorphizer<'interner> { let new_id = self.next_local_id(); let definition = self.interner.definition(ident.id); let name = definition.name.clone(); - println!("Mutability of parameter is {}", definition.mutability); new_params.push((new_id, definition.mutability, name, Self::convert_type(typ))); self.define_local(ident.id, new_id); } @@ -383,8 +385,7 @@ impl<'interner> Monomorphizer<'interner> { | ast::Type::Integer(_, _) | ast::Type::Bool | ast::Type::Unit - | ast::Type::Function(_, _) - | ast::Type::MutableReference(_) => { + | ast::Type::Function(_, _) => { ast::Expression::Literal(ast::Literal::Array(ast::ArrayLiteral { contents: array_contents, element_type, @@ -430,8 +431,7 @@ impl<'interner> Monomorphizer<'interner> { | ast::Type::Integer(_, _) | ast::Type::Bool | ast::Type::Unit - | ast::Type::Function(_, _) - | ast::Type::MutableReference(_) => { + | ast::Type::Function(_, _) => { ast::Expression::Index(ast::Index { collection, index, element_type, location }) } @@ -610,7 +610,13 @@ impl<'interner> Monomorphizer<'interner> { let definition = self.lookup_local(ident.id)?; let typ = Self::convert_type(&self.interner.id_type(ident.id)); - Some(ast::Ident { location: Some(ident.location), mutability: mutable, definition, name, typ }) + Some(ast::Ident { + location: Some(ident.location), + mutability: mutable, + definition, + name, + typ, + }) } fn ident(&mut self, ident: HirIdent, expr_id: node_interner::ExprId) -> ast::Expression { @@ -704,11 +710,6 @@ impl<'interner> Monomorphizer<'interner> { ast::Type::Vec(Box::new(element)) } - HirType::MutableReference(element) => { - let element = Self::convert_type(element); - ast::Type::MutableReference(Box::new(element)) - } - HirType::Forall(_, _) | HirType::Constant(_) | HirType::Error => { unreachable!("Unexpected type {} found", typ) } @@ -723,8 +724,7 @@ impl<'interner> Monomorphizer<'interner> { | ast::Type::Integer(_, _) | ast::Type::Bool | ast::Type::Unit - | ast::Type::Function(_, _) - | ast::Type::MutableReference(_) => ast::Type::Array(length, Box::new(element)), + | ast::Type::Function(_, _) => ast::Type::Array(length, Box::new(element)), ast::Type::Tuple(elements) => { ast::Type::Tuple(vecmap(elements, |typ| Self::aos_to_soa_type(length, typ))) @@ -742,7 +742,8 @@ impl<'interner> Monomorphizer<'interner> { id: node_interner::ExprId, ) -> ast::Expression { let func = Box::new(self.expr(call.func)); - let arguments = vecmap(&call.arguments, |id| self.expr(*id)); + let arguments = + vecmap(&call.arguments, |id| (ArgumentMode::PassByReference, self.expr(*id))); let return_type = self.interner.id_type(id); let return_type = Self::convert_type(&return_type); let location = call.location; @@ -988,7 +989,6 @@ impl<'interner> Monomorphizer<'interner> { ast::Type::Function(parameter_types, ret_type) => { self.create_zeroed_function(parameter_types, ret_type) } - ast::Type::MutableReference(element) => self.zeroed_value_of_type(element), ast::Type::Vec(_) => panic!("Cannot create a zeroed Vec value. This type is currently unimplemented and meant to be unusable outside of unconstrained functions"), } } diff --git a/crates/noirc_frontend/src/monomorphization/printer.rs b/crates/noirc_frontend/src/monomorphization/printer.rs index 036f4c12a6..b7c997558d 100644 --- a/crates/noirc_frontend/src/monomorphization/printer.rs +++ b/crates/noirc_frontend/src/monomorphization/printer.rs @@ -245,7 +245,13 @@ impl AstPrinter { ) -> Result<(), std::fmt::Error> { self.print_expr(&call.func, f)?; write!(f, "(")?; - self.print_comma_separated(&call.arguments, f)?; + for (i, (mode, elem)) in call.arguments.iter().enumerate() { + write!(f, "{mode}")?; + self.print_expr(elem, f)?; + if i != call.arguments.len() - 1 { + write!(f, ", ")?; + } + } write!(f, ")") } diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index 0ab27a823b..0f797da484 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -638,7 +638,6 @@ fn get_type_method_key(typ: &Type) -> Option { Type::Tuple(_) => Some(Tuple), Type::Function(_, _) => Some(Function), Type::Vec(_) => Some(Vec), - Type::MutableReference(element) => get_type_method_key(element), // We do not support adding methods to these types Type::TypeVariable(_) diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index c99c875f38..f61dbf68c1 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -34,9 +34,10 @@ use crate::lexer::Lexer; use crate::parser::{force, ignore_then_commit, statement_recovery}; use crate::token::{Attribute, Keyword, Token, TokenKind}; use crate::{ - BinaryOp, BinaryOpKind, BlockExpression, CompTime, ConstrainStatement, FunctionDefinition, - Ident, IfExpression, InfixExpression, LValue, Lambda, NoirFunction, NoirImpl, NoirStruct, Path, - PathKind, Pattern, Recoverable, UnaryOp, UnresolvedTypeExpression, UseTree, UseTreeKind, ArgumentMode, + ArgumentMode, BinaryOp, BinaryOpKind, BlockExpression, CompTime, ConstrainStatement, + FunctionDefinition, Ident, IfExpression, InfixExpression, LValue, Lambda, NoirFunction, + NoirImpl, NoirStruct, Path, PathKind, Pattern, Recoverable, UnaryOp, UnresolvedTypeExpression, + UseTree, UseTreeKind, }; use chumsky::prelude::*; @@ -308,15 +309,36 @@ fn nothing() -> impl NoirParser { } fn self_parameter() -> impl NoirParser<(Pattern, UnresolvedType, AbiVisibility)> { - filter_map(move |span, found: Token| match found { - Token::Ident(ref word) if word == "self" => { - let ident = Ident::from_token(found, span); + let refmut_pattern = just(Token::Ampersand).then_ignore(keyword(Keyword::Mut)); + let mut_pattern = keyword(Keyword::Mut); + + refmut_pattern + .or(mut_pattern) + .map_with_span(|token, span| (token, span)) + .or_not() + .then(filter_map(move |span, found: Token| match found { + Token::Ident(ref word) if word == "self" => Ok(span), + _ => Err(ParserError::expected_label(ParsingRuleLabel::Parameter, found, span)), + })) + .map(|(pattern_keyword, span)| { + let ident = Ident::new("self".to_string(), span); let path = Path::from_single("Self".to_owned(), span); let self_type = UnresolvedType::Named(path, vec![]); - Ok((Pattern::Identifier(ident), self_type, AbiVisibility::Private)) - } - _ => Err(ParserError::expected_label(ParsingRuleLabel::Parameter, found, span)), - }) + let pattern = Pattern::Identifier(ident); + + let pattern = match pattern_keyword { + Some((Token::Ampersand, span)) => { + Pattern::MutableReference(Box::new(pattern), span) + } + Some((Token::Keyword(_), span)) => Pattern::Mutable(Box::new(pattern), span), + None => pattern, + Some(other) => { + unreachable!("Unexpected keyword {other:?} when parsing self parameter") + } + }; + + (pattern, self_type, AbiVisibility::Private) + }) } fn implementation() -> impl NoirParser { @@ -510,8 +532,10 @@ fn declaration<'a, P>(expr_parser: P) -> impl NoirParser + 'a where P: ExprParser + 'a, { - let p = - ignore_then_commit(keyword(Keyword::Let).labelled(ParsingRuleLabel::Statement), pattern(false)); + let p = ignore_then_commit( + keyword(Keyword::Let).labelled(ParsingRuleLabel::Statement), + pattern(false), + ); let p = p.then(optional_type_annotation()); let p = then_commit_ignore(p, just(Token::Assign)); let p = then_commit(p, expr_parser); @@ -527,9 +551,11 @@ fn pattern(allow_mutable_references: bool) -> impl NoirParser { .map_with_span(|inner, span| Pattern::Mutable(Box::new(inner), span)); let mutref_pattern = if allow_mutable_references { - just(Token::Ampersand).ignore_then(keyword(Keyword::Mut)) - .ignore_then(pattern.clone()) - .map_with_span(|inner, span| Pattern::MutableReference(Box::new(inner), span)).boxed() + just(Token::Ampersand) + .ignore_then(keyword(Keyword::Mut)) + .ignore_then(pattern.clone()) + .map_with_span(|inner, span| Pattern::MutableReference(Box::new(inner), span)) + .boxed() } else { nothing().boxed() }; @@ -1004,7 +1030,8 @@ where .or_not() .map(|opt| opt.map_or(ArgumentMode::PassByValue, |_| ArgumentMode::PassByReference)) .then(expr_parser) - .separated_by(just(Token::Comma)).allow_trailing() + .separated_by(just(Token::Comma)) + .allow_trailing() } fn not

(term_parser: P) -> impl NoirParser From 2692f87100f308681ebcfa909f5b8e56520dd2e2 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 3 Jul 2023 16:34:22 +0200 Subject: [PATCH 03/13] Implement first-class references --- .../references/Nargo.toml | 5 + .../references/Prover.toml | 0 .../references/src/main.nr | 21 +++ crates/noirc_evaluator/src/ssa/context.rs | 6 +- crates/noirc_evaluator/src/ssa/function.rs | 2 +- crates/noirc_evaluator/src/ssa/ssa_gen.rs | 10 ++ crates/noirc_evaluator/src/ssa/value.rs | 3 +- .../src/ssa_refactor/ir/function.rs | 1 + .../src/ssa_refactor/opt/inlining.rs | 1 + .../src/ssa_refactor/ssa_gen/context.rs | 36 ++++- .../src/ssa_refactor/ssa_gen/mod.rs | 35 ++++- .../src/ssa_refactor/ssa_gen/value.rs | 49 +++--- crates/noirc_frontend/src/ast/expression.rs | 29 +--- crates/noirc_frontend/src/ast/mod.rs | 4 + crates/noirc_frontend/src/ast/statement.rs | 10 +- .../src/hir/resolution/errors.rs | 10 ++ .../src/hir/resolution/resolver.rs | 58 ++++++-- .../noirc_frontend/src/hir/type_check/expr.rs | 140 ++++++++++++------ .../noirc_frontend/src/hir/type_check/stmt.rs | 54 ++++++- crates/noirc_frontend/src/hir_def/expr.rs | 42 ++++++ crates/noirc_frontend/src/hir_def/stmt.rs | 4 + crates/noirc_frontend/src/hir_def/types.rs | 36 +++++ .../src/monomorphization/ast.rs | 8 +- .../src/monomorphization/mod.rs | 33 ++++- .../src/monomorphization/printer.rs | 7 +- crates/noirc_frontend/src/node_interner.rs | 1 + crates/noirc_frontend/src/parser/parser.rs | 133 +++++++++-------- 27 files changed, 546 insertions(+), 192 deletions(-) create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/references/Nargo.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/references/Prover.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/references/Nargo.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/references/Nargo.toml new file mode 100644 index 0000000000..09a8dd8e8c --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/references/Nargo.toml @@ -0,0 +1,5 @@ +[package] +authors = [""] +compiler_version = "0.5.1" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/references/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/references/Prover.toml new file mode 100644 index 0000000000..e69de29bb2 diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr new file mode 100644 index 0000000000..d54147138b --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr @@ -0,0 +1,21 @@ +fn main() { + let mut x = 2; + add1(&mut x); + assert(x == 3); + + let mut s = S { y: x }; + s.add2(); + assert(s.y == 5); +} + +fn add1(x: &mut Field) { + *x += 1; +} + +struct S { y: Field } + +impl S { + fn add2(&mut self) { + self.y += 2; + } +} diff --git a/crates/noirc_evaluator/src/ssa/context.rs b/crates/noirc_evaluator/src/ssa/context.rs index 639e2dd8d5..ea9a062ec1 100644 --- a/crates/noirc_evaluator/src/ssa/context.rs +++ b/crates/noirc_evaluator/src/ssa/context.rs @@ -16,7 +16,6 @@ use acvm::FieldElement; use iter_extended::vecmap; use noirc_errors::Location; use noirc_frontend::monomorphization::ast::{Definition, Expression, FuncId, Literal, Type}; -use noirc_frontend::ArgumentMode; use num_bigint::BigUint; use std::collections::{HashMap, HashSet}; @@ -1156,7 +1155,7 @@ impl SsaContext { pub(crate) fn get_builtin_opcode( &self, node_id: NodeId, - arguments: &[(ArgumentMode, Expression)], + arguments: &[Expression], ) -> Option { match &self[node_id] { NodeObject::Function(FunctionKind::Builtin(opcode), ..) => match opcode { @@ -1167,7 +1166,7 @@ impl SsaContext { 1, "print statements currently only support one argument" ); - let is_string = match &arguments[0].1 { + let is_string = match &arguments[0] { Expression::Ident(ident) => match ident.typ { Type::String(_) => true, Type::Tuple(_) => { @@ -1208,6 +1207,7 @@ impl SsaContext { } } Type::Array(..) => panic!("Cannot convert an array type {t} into an ObjectType since it is unknown which array it refers to"), + Type::MutableReference(..) => panic!("Mutable reference types are unimplemented in the old ssa backend"), Type::Unit => ObjectType::NotAnObject, Type::Function(..) => ObjectType::Function, Type::Tuple(_) => todo!("Conversion to ObjectType is unimplemented for tuples"), diff --git a/crates/noirc_evaluator/src/ssa/function.rs b/crates/noirc_evaluator/src/ssa/function.rs index e6412b71d2..fdb5dbbd7e 100644 --- a/crates/noirc_evaluator/src/ssa/function.rs +++ b/crates/noirc_evaluator/src/ssa/function.rs @@ -233,7 +233,7 @@ impl IrGenerator { let arguments = call .arguments .iter() - .flat_map(|(_, arg)| self.ssa_gen_expression(arg).unwrap().to_node_ids()) + .flat_map(|arg| self.ssa_gen_expression(arg).unwrap().to_node_ids()) .collect(); if let Some(opcode) = self.context.get_builtin_opcode(func, &call.arguments) { diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen.rs b/crates/noirc_evaluator/src/ssa/ssa_gen.rs index 082758468a..11fc134215 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen.rs @@ -208,6 +208,9 @@ impl IrGenerator { self.context.new_instruction(op, rhs_type) } UnaryOp::Not => self.context.new_instruction(Operation::Not(rhs), rhs_type), + UnaryOp::MutableReference | UnaryOp::Dereference => { + unimplemented!("Mutable references are unimplemented in the old ssa backend") + } } } @@ -248,6 +251,9 @@ impl IrGenerator { let val = self.find_variable(ident_def).unwrap(); val.get_field_member(*field_index) } + LValue::Dereference { .. } => { + unreachable!("Mutable references are unsupported in the old ssa backend") + } } } @@ -256,6 +262,7 @@ impl IrGenerator { LValue::Ident(ident) => &ident.definition, LValue::Index { array, .. } => Self::lvalue_ident_def(array.as_ref()), LValue::MemberAccess { object, .. } => Self::lvalue_ident_def(object.as_ref()), + LValue::Dereference { reference, .. } => Self::lvalue_ident_def(reference.as_ref()), } } @@ -462,6 +469,9 @@ impl IrGenerator { let value = val.get_field_member(*field_index).clone(); self.assign_pattern(&value, rhs)?; } + LValue::Dereference { .. } => { + unreachable!("Mutable references are unsupported in the old ssa backend") + } } Ok(Value::dummy()) } diff --git a/crates/noirc_evaluator/src/ssa/value.rs b/crates/noirc_evaluator/src/ssa/value.rs index 915effe480..b640369cd5 100644 --- a/crates/noirc_evaluator/src/ssa/value.rs +++ b/crates/noirc_evaluator/src/ssa/value.rs @@ -100,7 +100,8 @@ impl Value { | Type::String(..) | Type::Integer(..) | Type::Bool - | Type::Field => Value::Node(*iter.next().unwrap()), + | Type::Field + | Type::MutableReference(_) => Value::Node(*iter.next().unwrap()), } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/function.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/function.rs index f5be19dd88..8fe2fe745f 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/function.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/function.rs @@ -13,6 +13,7 @@ pub(crate) enum RuntimeType { // Unconstrained function, to be compiled to brillig and executed by the Brillig VM Brillig, } + /// A function holds a list of instructions. /// These instructions are further grouped into Basic blocks /// diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs index a9077018df..9310279f4b 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs @@ -370,6 +370,7 @@ impl<'function> PerFunctionContext<'function> { ) { let old_results = self.source_function.dfg.instruction_results(call_id); let arguments = vecmap(arguments, |arg| self.translate_value(*arg)); + let new_results = self.context.inline_function(ssa, function, &arguments); let new_results = InsertInstructionResult::Results(&new_results); Self::insert_new_instruction_results(&mut self.values, old_results, new_results); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index 3f5e9c5068..50100d13de 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -167,12 +167,17 @@ impl<'a> FunctionContext<'a> { // This helper is needed because we need to take f by mutable reference, // otherwise we cannot move it multiple times each loop of vecmap. - fn map_type_helper(typ: &ast::Type, f: &mut impl FnMut(Type) -> T) -> Tree { + fn map_type_helper(typ: &ast::Type, f: &mut dyn FnMut(Type) -> T) -> Tree { match typ { ast::Type::Tuple(fields) => { Tree::Branch(vecmap(fields, |field| Self::map_type_helper(field, f))) } ast::Type::Unit => Tree::empty(), + // A mutable reference wraps each element into a reference. + // This can be multiple values if the element type is a tuple. + ast::Type::MutableReference(element) => { + Self::map_type_helper(element, &mut |_| f(Type::Reference)) + } other => Tree::Leaf(f(Self::convert_non_tuple_type(other))), } } @@ -203,6 +208,11 @@ impl<'a> FunctionContext<'a> { ast::Type::Unit => panic!("convert_non_tuple_type called on a unit type"), ast::Type::Tuple(_) => panic!("convert_non_tuple_type called on a tuple: {typ}"), ast::Type::Function(_, _) => Type::Function, + ast::Type::MutableReference(element) => { + // Recursive call to panic if element is a tuple + Self::convert_non_tuple_type(element); + Type::Reference + } // How should we represent Vecs? // Are they a struct of array + length + capacity? @@ -475,9 +485,22 @@ impl<'a> FunctionContext<'a> { let object_lvalue = Box::new(object_lvalue); LValue::MemberAccess { old_object, object_lvalue, index: *field_index } } + ast::LValue::Dereference { reference, .. } => { + let (reference, reference_lvalue) = self.extract_current_value_recursive(reference); + let reference_lvalue = Box::new(reference_lvalue); + LValue::Dereference { reference, reference_lvalue } + } } } + fn dereference(&mut self, values: &Values, element_type: &ast::Type) -> Values { + let element_types = Self::convert_type(element_type); + values.map_both(element_types, |value, element_type| { + let reference = value.eval_reference(); + self.builder.insert_load(reference, element_type).into() + }) + } + /// Compile the given identifier as a reference - ie. avoid calling .eval() fn ident_lvalue(&self, ident: &ast::Ident) -> Values { match &ident.definition { @@ -518,6 +541,12 @@ impl<'a> FunctionContext<'a> { let element = Self::get_field_ref(&old_object, *index).clone(); (element, LValue::MemberAccess { old_object, object_lvalue, index: *index }) } + ast::LValue::Dereference { reference, element_type } => { + let (reference, reference_lvalue) = self.extract_current_value_recursive(reference); + let reference_lvalue = Box::new(reference_lvalue); + let dereferenced = self.dereference(&reference, element_type); + (dereferenced, LValue::Dereference { reference, reference_lvalue }) + } } } @@ -540,6 +569,9 @@ impl<'a> FunctionContext<'a> { let new_object = Self::replace_field(old_object, index, new_value); self.assign_new_value(*object_lvalue, new_object); } + LValue::Dereference { reference, .. } => { + self.assign(reference, new_value); + } } } @@ -707,8 +739,10 @@ impl SharedContext { } /// Used to remember the results of each step of extracting a value from an ast::LValue +#[derive(Debug)] pub(super) enum LValue { Ident(Values), Index { old_array: ValueId, index: ValueId, array_lvalue: Box }, MemberAccess { old_object: Values, index: usize, object_lvalue: Box }, + Dereference { reference: Values, reference_lvalue: Box }, } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index a2bc4d4f2c..dce102cd4d 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -164,14 +164,39 @@ impl<'a> FunctionContext<'a> { } fn codegen_unary(&mut self, unary: &ast::Unary) -> Values { - let rhs = self.codegen_non_tuple_expression(&unary.rhs); + let rhs = self.codegen_expression(&unary.rhs); match unary.operator { - noirc_frontend::UnaryOp::Not => self.builder.insert_not(rhs).into(), + noirc_frontend::UnaryOp::Not => { + let rhs = rhs.into_leaf().eval(self); + self.builder.insert_not(rhs).into() + } noirc_frontend::UnaryOp::Minus => { + let rhs = rhs.into_leaf().eval(self); let typ = self.builder.type_of_value(rhs); let zero = self.builder.numeric_constant(0u128, typ); self.builder.insert_binary(zero, BinaryOp::Sub, rhs).into() } + noirc_frontend::UnaryOp::MutableReference => { + rhs.map(|rhs| { + match rhs { + value::Value::Normal(value) => { + let alloc = self.builder.insert_allocate(); + self.builder.insert_store(alloc, value); + Tree::Leaf(value::Value::Normal(alloc)) + } + // NOTE: The `.into()` here converts the Value::Mutable into + // a Value::Normal so it is no longer automatically dereferenced. + value::Value::Mutable(reference, _) => reference.into(), + } + }) + } + noirc_frontend::UnaryOp::Dereference => { + let typ = Self::convert_type(&unary.result_type); + rhs.map_both(typ, |rhs, element_type| { + let rhs = rhs.eval_reference(); + self.builder.insert_load(rhs, element_type).into() + }) + } } } @@ -342,15 +367,13 @@ impl<'a> FunctionContext<'a> { /// Generate SSA for a function call. Note that calls to built-in functions /// and intrinsics are also represented by the function call instruction. fn codegen_call(&mut self, call: &ast::Call) -> Values { + let function = self.codegen_non_tuple_expression(&call.func); let arguments = call .arguments .iter() - .flat_map(|(mode, argument)| { - self.codegen_expression(argument).into_argument_list(self, *mode) - }) + .flat_map(|argument| self.codegen_expression(argument).into_argument_list()) .collect(); - let function = self.codegen_non_tuple_expression(&call.func); self.insert_call(function, arguments, &call.return_type) } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs index a3438fc9a2..008567e944 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs @@ -1,5 +1,4 @@ use iter_extended::vecmap; -use noirc_frontend::ArgumentMode; use crate::ssa_refactor::ir::types::Type; use crate::ssa_refactor::ir::value::ValueId as IrValueId; @@ -54,16 +53,6 @@ impl Value { Value::Mutable(address, _) => address, } } - - pub(super) fn eval_argument(self, ctx: &mut FunctionContext, mode: ArgumentMode) -> IrValueId { - match self { - Value::Normal(value) => value, - Value::Mutable(address, typ) => match mode { - ArgumentMode::PassByValue => ctx.builder.insert_load(address, typ), - ArgumentMode::PassByReference => address, - }, - } - } } /// A tree of values. @@ -134,6 +123,36 @@ impl Tree { } } + /// Map two trees alongside each other. + /// This asserts each tree has the same internal structure. + pub(super) fn map_both( + &self, + other: Tree, + mut f: impl FnMut(T, U) -> Tree, + ) -> Tree + where + T: std::fmt::Debug + Clone, + U: std::fmt::Debug, + { + self.map_both_helper(other, &mut f) + } + + fn map_both_helper(&self, other: Tree, f: &mut impl FnMut(T, U) -> Tree) -> Tree + where + T: std::fmt::Debug + Clone, + U: std::fmt::Debug, + { + match (self, other) { + (Tree::Branch(self_trees), Tree::Branch(other_trees)) => { + assert_eq!(self_trees.len(), other_trees.len()); + let trees = self_trees.iter().zip(other_trees); + Tree::Branch(vecmap(trees, |(l, r)| l.map_both_helper(r, f))) + } + (Tree::Leaf(self_value), Tree::Leaf(other_value)) => f(self_value.clone(), other_value), + other => panic!("Found unexpected tree combination during SSA: {other:?}"), + } + } + /// Unwraps this Tree into the value of the leaf node. Panics if /// this Tree is a Branch pub(super) fn into_leaf(self) -> T { @@ -172,11 +191,7 @@ impl Tree { vecmap(self.flatten(), |value| value.eval(ctx)) } - pub(super) fn into_argument_list( - self, - ctx: &mut FunctionContext, - mode: ArgumentMode, - ) -> Vec { - vecmap(self.flatten(), |value| value.eval_argument(ctx, mode)) + pub(super) fn into_argument_list(self) -> Vec { + vecmap(self.flatten(), |value| value.eval_reference()) } } diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index 23e223f90c..670890fefd 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -157,23 +157,13 @@ impl Expression { Expression::new(kind, span) } - pub fn call( - lhs: Expression, - arguments: Vec<(ArgumentMode, Expression)>, - span: Span, - ) -> Expression { + pub fn call(lhs: Expression, arguments: Vec, span: Span) -> Expression { let func = Box::new(lhs); let kind = ExpressionKind::Call(Box::new(CallExpression { func, arguments })); Expression::new(kind, span) } } -#[derive(Debug, PartialEq, Eq, Copy, Clone)] -pub enum ArgumentMode { - PassByValue, - PassByReference, -} - #[derive(Debug, PartialEq, Eq, Clone)] pub struct ForExpression { pub identifier: Ident, @@ -267,6 +257,8 @@ impl BinaryOpKind { pub enum UnaryOp { Minus, Not, + MutableReference, + Dereference, } impl UnaryOp { @@ -366,7 +358,7 @@ pub enum ArrayLiteral { #[derive(Debug, PartialEq, Eq, Clone)] pub struct CallExpression { pub func: Box, - pub arguments: Vec<(ArgumentMode, Expression)>, + pub arguments: Vec, } #[derive(Debug, PartialEq, Eq, Clone)] @@ -485,6 +477,8 @@ impl Display for UnaryOp { match self { UnaryOp::Minus => write!(f, "-"), UnaryOp::Not => write!(f, "!"), + UnaryOp::MutableReference => write!(f, "&mut"), + UnaryOp::Dereference => write!(f, "*"), } } } @@ -497,20 +491,11 @@ impl Display for IndexExpression { impl Display for CallExpression { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let args = vecmap(&self.arguments, |(mode, arg)| format!("{}{}", mode, arg)); + let args = vecmap(&self.arguments, ToString::to_string); write!(f, "{}({})", self.func, args.join(", ")) } } -impl Display for ArgumentMode { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - ArgumentMode::PassByValue => Ok(()), - ArgumentMode::PassByReference => write!(f, "&mut "), - } - } -} - impl Display for MethodCallExpression { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let args = vecmap(&self.arguments, ToString::to_string); diff --git a/crates/noirc_frontend/src/ast/mod.rs b/crates/noirc_frontend/src/ast/mod.rs index 24004e34ff..2f11ecc156 100644 --- a/crates/noirc_frontend/src/ast/mod.rs +++ b/crates/noirc_frontend/src/ast/mod.rs @@ -47,6 +47,9 @@ pub enum UnresolvedType { /// generic argument is not given. Vec(Vec, Span), + /// &mut T + MutableReference(Box), + // Note: Tuples have no visibility, instead each of their elements may have one. Tuple(Vec), @@ -116,6 +119,7 @@ impl std::fmt::Display for UnresolvedType { let args = vecmap(args, ToString::to_string); write!(f, "Vec<{}>", args.join(", ")) } + MutableReference(element) => write!(f, "&mut {element}"), Unit => write!(f, "()"), Error => write!(f, "error"), Unspecified => write!(f, "unspecified"), diff --git a/crates/noirc_frontend/src/ast/statement.rs b/crates/noirc_frontend/src/ast/statement.rs index 2240b92806..f8093221cf 100644 --- a/crates/noirc_frontend/src/ast/statement.rs +++ b/crates/noirc_frontend/src/ast/statement.rs @@ -403,6 +403,7 @@ pub enum LValue { Ident(Ident), MemberAccess { object: Box, field_name: Ident }, Index { array: Box, index: Expression }, + Dereference(Box), } #[derive(Debug, PartialEq, Eq, Clone)] @@ -412,7 +413,6 @@ pub struct ConstrainStatement(pub Expression); pub enum Pattern { Identifier(Ident), Mutable(Box, Span), - MutableReference(Box, Span), Tuple(Vec, Span), Struct(Path, Vec<(Ident, Pattern)>, Span), } @@ -446,6 +446,12 @@ impl LValue { collection: array.as_expression(span), index: index.clone(), })), + LValue::Dereference(lvalue) => { + ExpressionKind::Prefix(Box::new(crate::PrefixExpression { + operator: crate::UnaryOp::Dereference, + rhs: lvalue.as_expression(span), + })) + } }; Expression::new(kind, span) } @@ -488,6 +494,7 @@ impl Display for LValue { LValue::Ident(ident) => ident.fmt(f), LValue::MemberAccess { object, field_name } => write!(f, "{object}.{field_name}"), LValue::Index { array, index } => write!(f, "{array}[{index}]"), + LValue::Dereference(lvalue) => write!(f, "*{lvalue}"), } } } @@ -524,7 +531,6 @@ impl Display for Pattern { match self { Pattern::Identifier(name) => name.fmt(f), Pattern::Mutable(name, _) => write!(f, "mut {name}"), - Pattern::MutableReference(name, _) => write!(f, "&mut {name}"), Pattern::Tuple(fields, _) => { let fields = vecmap(fields, ToString::to_string); write!(f, "({})", fields.join(", ")) diff --git a/crates/noirc_frontend/src/hir/resolution/errors.rs b/crates/noirc_frontend/src/hir/resolution/errors.rs index 87257cbb84..80638897a5 100644 --- a/crates/noirc_frontend/src/hir/resolution/errors.rs +++ b/crates/noirc_frontend/src/hir/resolution/errors.rs @@ -60,6 +60,10 @@ pub enum ResolverError { ParserError(Box), #[error("Function is not defined in a contract yet sets its contract visibility")] ContractFunctionTypeInNormalFunction { span: Span }, + #[error("Cannot create a mutable reference to {variable}, it was declared to be immutable")] + MutableReferenceToImmutableVariable { variable: String, span: Span }, + #[error("Mutable references to array indices are unsupported")] + MutableReferenceToArrayElement { span: Span }, } impl ResolverError { @@ -258,6 +262,12 @@ impl From for Diagnostic { "Non-contract functions cannot be 'open'".into(), span, ), + ResolverError::MutableReferenceToImmutableVariable { variable, span } => { + Diagnostic::simple_error(format!("Cannot mutably reference the immutable variable {variable}"), format!("{variable} is immutable"), span) + }, + 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) + }, } } } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index d6a4ff6c77..86b23d3e99 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -34,7 +34,7 @@ use crate::{ }; use crate::{ ArrayLiteral, ContractFunctionType, Generics, LValue, NoirStruct, Path, Pattern, Shared, - StructType, Type, TypeBinding, TypeVariable, UnresolvedGenerics, UnresolvedType, + StructType, Type, TypeBinding, TypeVariable, UnaryOp, UnresolvedGenerics, UnresolvedType, UnresolvedTypeExpression, ERROR_IDENT, }; use fm::FileId; @@ -366,6 +366,9 @@ impl<'a> Resolver<'a> { }; Type::Vec(Box::new(arg)) } + UnresolvedType::MutableReference(element) => { + Type::MutableReference(Box::new(self.resolve_type_inner(*element, new_variables))) + } } } @@ -792,6 +795,7 @@ impl<'a> Resolver<'a> { } } Type::Vec(element) => Self::find_numeric_generics_in_type(element, found), + Type::MutableReference(element) => Self::find_numeric_generics_in_type(element, found), } } @@ -852,6 +856,11 @@ impl<'a> Resolver<'a> { let index = self.resolve_expression(index); HirLValue::Index { array, index, typ: Type::Error } } + LValue::Dereference(lvalue) => { + let lvalue = Box::new(self.resolve_lvalue(*lvalue)); + let element_type = Type::Error; + HirLValue::Dereference { lvalue, element_type } + } } } @@ -891,6 +900,11 @@ impl<'a> Resolver<'a> { ExpressionKind::Prefix(prefix) => { let operator = prefix.operator; let rhs = self.resolve_expression(prefix.rhs); + + if operator == UnaryOp::MutableReference { + self.verify_mutable_reference(rhs); + } + HirExpression::Prefix(HirPrefixExpression { operator, rhs }) } ExpressionKind::Infix(infix) => { @@ -906,8 +920,7 @@ impl<'a> Resolver<'a> { ExpressionKind::Call(call_expr) => { // Get the span and name of path for error reporting let func = self.resolve_expression(*call_expr.func); - let arguments = - vecmap(call_expr.arguments, |(_, arg)| self.resolve_expression(arg)); + let arguments = vecmap(call_expr.arguments, |arg| self.resolve_expression(arg)); let location = Location::new(expr.span, self.file); HirExpression::Call(HirCallExpression { func, arguments, location }) } @@ -1056,19 +1069,6 @@ impl<'a> Resolver<'a> { ); HirPattern::Mutable(Box::new(pattern), span) } - Pattern::MutableReference(pattern, span) => { - if let Some(first_mut) = mutable_span { - self.push_err(ResolverError::UnnecessaryMut { first_mut, second_mut: span }); - } - - let pattern = self.resolve_pattern_mutable( - *pattern, - Mutability::MutableReference, - Some(span), - definition, - ); - HirPattern::MutableReference(Box::new(pattern), span) - } Pattern::Tuple(fields, span) => { let fields = vecmap(fields, |field| { self.resolve_pattern_mutable( @@ -1290,6 +1290,32 @@ impl<'a> Resolver<'a> { let module_id = self.path_resolver.module_id(); module_id.module(self.def_maps).is_contract } + + /// Gives an error if a user tries to create a mutable reference + /// to an immutable variable. + fn verify_mutable_reference(&mut self, rhs: ExprId) { + match self.interner.expression(&rhs) { + HirExpression::MemberAccess(member_access) => { + self.verify_mutable_reference(member_access.lhs); + } + HirExpression::Index(_) => { + let span = self.interner.expr_span(&rhs); + self.errors.push(ResolverError::MutableReferenceToArrayElement { span }); + } + HirExpression::Ident(ident) => { + let definition = self.interner.definition(ident.id); + if matches!(definition.mutability, Mutability::Immutable) { + let span = self.interner.expr_span(&rhs); + let variable = definition.name.clone(); + self.errors.push(ResolverError::MutableReferenceToImmutableVariable { + span, + variable, + }); + } + } + _ => (), + } + } } // XXX: These tests repeat a lot of code diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 7160d1f153..9fb1e09f66 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -3,7 +3,9 @@ use noirc_errors::Span; use crate::{ hir_def::{ - expr::{self, HirArrayLiteral, HirBinaryOp, HirExpression, HirLiteral}, + expr::{ + self, HirArrayLiteral, HirBinaryOp, HirExpression, HirLiteral, HirPrefixExpression, + }, types::Type, }, node_interner::{ExprId, FuncId}, @@ -74,13 +76,7 @@ impl<'interner> TypeChecker<'interner> { Type::Array(Box::new(length), Box::new(elem_type)) } HirLiteral::Bool(_) => Type::Bool(CompTime::new(self.interner)), - HirLiteral::Integer(_) => { - let id = self.interner.next_type_variable_id(); - Type::PolymorphicInteger( - CompTime::new(self.interner), - Shared::new(TypeBinding::Unbound(id)), - ) - } + HirLiteral::Integer(_) => Type::polymorphic_integer(self.interner), HirLiteral::Str(string) => { let len = Type::Constant(string.len() as u64); Type::String(Box::new(len)) @@ -119,6 +115,7 @@ impl<'interner> TypeChecker<'interner> { Some(method_id) => { let mut args = vec![(object_type, self.interner.expr_span(&method_call.object))]; + let mut arg_types = vecmap(&method_call.arguments, |arg| { let typ = self.check_expression(arg); (typ, self.interner.expr_span(arg)) @@ -128,8 +125,12 @@ impl<'interner> TypeChecker<'interner> { // Desugar the method call into a normal, resolved function call // so that the backend doesn't need to worry about methods let location = method_call.location; - let (function_id, function_call) = - method_call.into_function_call(method_id, location, self.interner); + let (function_id, function_call) = method_call.into_function_call( + method_id, + location, + &mut args, + self.interner, + ); let span = self.interner.expr_span(expr_id); let ret = self.check_method_call(&function_id, &method_id, args, span); @@ -216,14 +217,8 @@ impl<'interner> TypeChecker<'interner> { } HirExpression::Prefix(prefix_expr) => { let rhs_type = self.check_expression(&prefix_expr.rhs); - match prefix_operand_type_rules(&prefix_expr.operator, &rhs_type) { - Ok(typ) => typ, - Err(msg) => { - let rhs_span = self.interner.expr_span(&prefix_expr.rhs); - self.errors.push(TypeCheckError::Unstructured { msg, span: rhs_span }); - Type::Error - } - } + let span = self.interner.expr_span(&prefix_expr.rhs); + self.type_check_prefix_operand(&prefix_expr.operator, &rhs_type, span) } HirExpression::If(if_expr) => self.check_if_expr(&if_expr, expr_id), HirExpression::Constructor(constructor) => self.check_constructor(constructor, expr_id), @@ -462,13 +457,26 @@ impl<'interner> TypeChecker<'interner> { Type::Struct(typ, generics) } - fn check_member_access(&mut self, access: expr::HirMemberAccess, expr_id: ExprId) -> Type { + fn check_member_access(&mut self, mut access: expr::HirMemberAccess, expr_id: ExprId) -> Type { let lhs_type = self.check_expression(&access.lhs).follow_bindings(); let span = self.interner.expr_span(&expr_id); + let access_lhs = &mut access.lhs; + + let dereference_lhs = |this: &mut Self, lhs_type, element| { + let old_lhs = *access_lhs; + *access_lhs = this.interner.push_expr(HirExpression::Prefix(HirPrefixExpression { + operator: crate::UnaryOp::Dereference, + rhs: old_lhs, + })); + this.interner.push_expr_type(access_lhs, lhs_type); + this.interner.push_expr_type(&old_lhs, element); + }; - match self.check_field_access(&lhs_type, &access.rhs.0.contents, span) { + match self.check_field_access(&lhs_type, &access.rhs.0.contents, span, dereference_lhs) { Some((element_type, index)) => { self.interner.set_field_index(expr_id, index); + // We must update `access` in case we added any dereferences to it + self.interner.replace_expr(&expr_id, HirExpression::MemberAccess(access)); element_type } None => Type::Error, @@ -481,32 +489,50 @@ impl<'interner> TypeChecker<'interner> { /// /// This function is abstracted from check_member_access so that it can be shared between /// there and the HirLValue::MemberAccess case of check_lvalue. + /// + /// `dereference_lhs` is called when the lhs type is a Type::MutableReference that should be + /// automatically dereferenced so its field can be extracted. This function is expected to + /// perform any mutations necessary to wrap the lhs in a UnaryOp::Dereference prefix + /// expression. The second parameter of this function represents the lhs_type (which should + /// always be a Type::MutableReference if `dereference_lhs` is called) and the third + /// represents the element type. pub(super) fn check_field_access( &mut self, lhs_type: &Type, field_name: &str, span: Span, + mut dereference_lhs: impl FnMut(&mut Self, Type, Type), ) -> Option<(Type, usize)> { let lhs_type = lhs_type.follow_bindings(); - if let Type::Struct(s, args) = &lhs_type { - let s = s.borrow(); - if let Some((field, index)) = s.get_field(field_name, args) { - return Some((field, index)); + match &lhs_type { + Type::Struct(s, args) => { + let s = s.borrow(); + if let Some((field, index)) = s.get_field(field_name, args) { + return Some((field, index)); + } } - } else if let Type::Tuple(elements) = &lhs_type { - if let Ok(index) = field_name.parse::() { - let length = elements.len(); - if index < length { - return Some((elements[index].clone(), index)); - } else { - self.errors.push(TypeCheckError::Unstructured { - msg: format!("Index {index} is out of bounds for this tuple {lhs_type} of length {length}"), - span, - }); - return None; + Type::Tuple(elements) => { + if let Ok(index) = field_name.parse::() { + let length = elements.len(); + if index < length { + return Some((elements[index].clone(), index)); + } else { + self.errors.push(TypeCheckError::Unstructured { + msg: format!("Index {index} is out of bounds for this tuple {lhs_type} of length {length}"), + span, + }); + return None; + } } } + // If the lhs is a mutable reference we automatically transform + // lhs.field into (*lhs).field + Type::MutableReference(element) => { + dereference_lhs(self, lhs_type.clone(), element.as_ref().clone()); + return self.check_field_access(element, field_name, span, dereference_lhs); + } + _ => (), } // If we get here the type has no field named 'access.rhs'. @@ -830,22 +856,44 @@ impl<'interner> TypeChecker<'interner> { (lhs, rhs) => Err(make_error(format!("Unsupported types for binary operation: {lhs} and {rhs}"))), } } -} -fn prefix_operand_type_rules(op: &crate::UnaryOp, rhs_type: &Type) -> Result { - match op { - crate::UnaryOp::Minus => { - if !matches!(rhs_type, Type::Integer(..) | Type::Error) { - return Err("Only Integers can be used in a Minus expression".to_string()); + fn type_check_prefix_operand( + &mut self, + op: &crate::UnaryOp, + rhs_type: &Type, + span: Span, + ) -> Type { + let mut unify = |expected| { + rhs_type.unify(&expected, span, &mut self.errors, || TypeCheckError::TypeMismatch { + expr_typ: rhs_type.to_string(), + expected_typ: expected.to_string(), + expr_span: span, + }); + expected + }; + + match op { + crate::UnaryOp::Minus => unify(Type::polymorphic_integer(self.interner)), + crate::UnaryOp::Not => { + let rhs_type = rhs_type.follow_bindings(); + + // `!` can work on booleans or integers + if matches!(rhs_type, Type::Integer(..)) { + return rhs_type; + } + + unify(Type::Bool(CompTime::new(self.interner))) } - } - crate::UnaryOp::Not => { - if !matches!(rhs_type, Type::Integer(..) | Type::Bool(_) | Type::Error) { - return Err("Only Integers or Bool can be used in a Not expression".to_string()); + crate::UnaryOp::MutableReference => { + Type::MutableReference(Box::new(rhs_type.follow_bindings())) + } + crate::UnaryOp::Dereference => { + let element_type = Type::type_variable(self.interner.next_type_variable_id()); + unify(Type::MutableReference(Box::new(element_type.clone()))); + element_type } } } - Ok(rhs_type.clone()) } /// Taken from: https://stackoverflow.com/a/47127500 diff --git a/crates/noirc_frontend/src/hir/type_check/stmt.rs b/crates/noirc_frontend/src/hir/type_check/stmt.rs index 0dc1902287..680703759b 100644 --- a/crates/noirc_frontend/src/hir/type_check/stmt.rs +++ b/crates/noirc_frontend/src/hir/type_check/stmt.rs @@ -1,5 +1,6 @@ -use noirc_errors::Span; +use noirc_errors::{Location, Span}; +use crate::hir_def::expr::HirIdent; use crate::hir_def::stmt::{ HirAssignStatement, HirConstrainStatement, HirLValue, HirLetStatement, HirPattern, HirStatement, }; @@ -124,8 +125,14 @@ impl<'interner> TypeChecker<'interner> { let typ = if ident.id == DefinitionId::dummy_id() { Type::Error } else { + // Do we need to store TypeBindings here? + let typ = self.interner.id_type(ident.id).instantiate(self.interner).0; + let typ = typ.follow_bindings(); + let definition = self.interner.definition(ident.id); - if definition.mutability == Mutability::Immutable { + if definition.mutability == Mutability::Immutable + && !matches!(typ, Type::MutableReference(_)) + { self.errors.push(TypeCheckError::Unstructured { msg: format!( "Variable {} must be mutable to be assigned to", @@ -134,19 +141,36 @@ impl<'interner> TypeChecker<'interner> { span: ident.location.span, }); } - // Do we need to store TypeBindings here? - self.interner.id_type(ident.id).instantiate(self.interner).0 + + typ }; (typ.clone(), HirLValue::Ident(ident, typ)) } HirLValue::MemberAccess { object, field_name, .. } => { let (lhs_type, object) = self.check_lvalue(*object, assign_span); - let object = Box::new(object); - + let mut object = Box::new(object); let span = field_name.span(); + + let object_ref = &mut object; + let (typ, field_index) = self - .check_field_access(&lhs_type, &field_name.0.contents, span) + .check_field_access( + &lhs_type, + &field_name.0.contents, + span, + move |_, _, element_type| { + // We must create a temporary value first to move out of object_ref before + // we eventually reassign to it. + let id = DefinitionId::dummy_id(); + let location = Location::new(span, fm::FileId::dummy()); + let tmp_value = + HirLValue::Ident(HirIdent { location, id }, Type::Error); + + let lvalue = std::mem::replace(object_ref, Box::new(tmp_value)); + *object_ref = Box::new(HirLValue::Dereference { lvalue, element_type }); + }, + ) .unwrap_or((Type::Error, 0)); let field_index = Some(field_index); @@ -186,6 +210,22 @@ impl<'interner> TypeChecker<'interner> { (typ.clone(), HirLValue::Index { array, index, typ }) } + HirLValue::Dereference { lvalue, element_type: _ } => { + let (reference_type, lvalue) = self.check_lvalue(*lvalue, assign_span); + let lvalue = Box::new(lvalue); + + let element_type = Type::type_variable(self.interner.next_type_variable_id()); + let expected_type = Type::MutableReference(Box::new(element_type.clone())); + reference_type.unify(&expected_type, assign_span, &mut self.errors, || { + TypeCheckError::TypeMismatch { + expected_typ: expected_type.to_string(), + expr_typ: reference_type.to_string(), + expr_span: assign_span, + } + }); + + (element_type.clone(), HirLValue::Dereference { lvalue, element_type }) + } } } diff --git a/crates/noirc_frontend/src/hir_def/expr.rs b/crates/noirc_frontend/src/hir_def/expr.rs index b9ee6634cc..fa129f8e67 100644 --- a/crates/noirc_frontend/src/hir_def/expr.rs +++ b/crates/noirc_frontend/src/hir_def/expr.rs @@ -149,8 +149,16 @@ impl HirMethodCallExpression { mut self, func: FuncId, location: Location, + argument_types: &mut [(Type, noirc_errors::Span)], interner: &mut NodeInterner, ) -> (ExprId, HirExpression) { + // Automatically add `&mut` if the method expects a mutable reference and + // the object is not already one. + if func != FuncId::dummy_id() { + let func_meta = interner.function_meta(&func); + self.try_add_mutable_reference_to_object(&func_meta.typ, argument_types, interner); + } + let mut arguments = vec![self.object]; arguments.append(&mut self.arguments); @@ -160,6 +168,40 @@ impl HirMethodCallExpression { (func, HirExpression::Call(HirCallExpression { func, arguments, location })) } + + /// Check if the given function type requires a mutable reference to the object type, and check + /// if the given object type is already a mutable reference. If not, add one. + fn try_add_mutable_reference_to_object( + &mut self, + function_type: &Type, + argument_types: &mut [(Type, noirc_errors::Span)], + interner: &mut NodeInterner, + ) { + let expected_object_type = match function_type { + Type::Function(args, _) => args.get(0), + Type::Forall(_, typ) => match typ.as_ref() { + Type::Function(args, _) => args.get(0), + typ => unreachable!("Unexpected type for function: {typ}"), + }, + typ => unreachable!("Unexpected type for function: {typ}"), + }; + + if let Some(expected_object_type) = expected_object_type { + if matches!(expected_object_type.follow_bindings(), Type::MutableReference(_)) { + let actual_type = argument_types[0].0.follow_bindings(); + if !matches!(actual_type, Type::MutableReference(_)) { + let new_type = Type::MutableReference(Box::new(actual_type)); + + argument_types[0].0 = new_type.clone(); + self.object = interner.push_expr(HirExpression::Prefix(HirPrefixExpression { + operator: UnaryOp::MutableReference, + rhs: self.object, + })); + interner.push_expr_type(&self.object, new_type); + } + } + } + } } #[derive(Debug, Clone)] diff --git a/crates/noirc_frontend/src/hir_def/stmt.rs b/crates/noirc_frontend/src/hir_def/stmt.rs index c68f9588cc..b44d080062 100644 --- a/crates/noirc_frontend/src/hir_def/stmt.rs +++ b/crates/noirc_frontend/src/hir_def/stmt.rs @@ -99,4 +99,8 @@ pub enum HirLValue { index: ExprId, typ: Type, }, + Dereference { + lvalue: Box, + element_type: Type, + }, } diff --git a/crates/noirc_frontend/src/hir_def/types.rs b/crates/noirc_frontend/src/hir_def/types.rs index 133d8a7905..f4f9b27cf7 100644 --- a/crates/noirc_frontend/src/hir_def/types.rs +++ b/crates/noirc_frontend/src/hir_def/types.rs @@ -75,6 +75,9 @@ pub enum Type { /// .pop, and similar methods. Vec(Box), + /// &mut T + MutableReference(Box), + /// A type generic over the given type variables. /// Storing both the TypeVariableId and TypeVariable isn't necessary /// but it makes handling them both easier. The TypeVariableId should @@ -523,6 +526,11 @@ impl Type { Type::TypeVariable(Shared::new(TypeBinding::Unbound(id))) } + pub fn polymorphic_integer(interner: &mut NodeInterner) -> Type { + let id = interner.next_type_variable_id(); + Type::PolymorphicInteger(CompTime::new(interner), Shared::new(TypeBinding::Unbound(id))) + } + /// A bit of an awkward name for this function - this function returns /// true for type variables or polymorphic integers which are unbound. /// NamedGenerics will always be false as although they are bindable, @@ -592,6 +600,7 @@ impl Type { }) } Type::Vec(element) => element.contains_numeric_typevar(target_id), + Type::MutableReference(element) => element.contains_numeric_typevar(target_id), } } } @@ -651,6 +660,9 @@ impl std::fmt::Display for Type { Type::Vec(element) => { write!(f, "Vec<{element}>") } + Type::MutableReference(element) => { + write!(f, "&mut {element}") + } } } } @@ -983,6 +995,8 @@ impl Type { (Vec(elem_a), Vec(elem_b)) => elem_a.try_unify(elem_b, span), + (MutableReference(elem_a), MutableReference(elem_b)) => elem_a.try_unify(elem_b, span), + (other_a, other_b) => { if other_a == other_b { Ok(()) @@ -1116,6 +1130,22 @@ impl Type { (Vec(elem_a), Vec(elem_b)) => elem_a.is_subtype_of(elem_b, span), + // `T <: U => &mut T <: &mut U` would be unsound(*), so mutable + // references are never subtypes of each other. + // + // (*) Consider: + // ``` + // // Assume Dog <: Animal and Cat <: Animal + // let x: &mut Dog = ...; + // + // fn set_to_cat(y: &mut Animal) { + // *y = Cat; + // } + // + // set_to_cat(x); // uh-oh: x: Dog, yet it now holds a Cat + // ``` + (MutableReference(elem_a), MutableReference(elem_b)) => elem_a.try_unify(elem_b, span), + (other_a, other_b) => { if other_a == other_b { Ok(()) @@ -1186,6 +1216,7 @@ impl Type { Type::NamedGeneric(..) => unreachable!(), Type::Forall(..) => unreachable!(), Type::Function(_, _) => unreachable!(), + Type::MutableReference(_) => unreachable!("&mut cannot be used in the abi"), Type::Vec(_) => unreachable!("Vecs cannot be used in the abi"), } } @@ -1301,6 +1332,9 @@ impl Type { Type::Function(args, ret) } Type::Vec(element) => Type::Vec(Box::new(element.substitute(type_bindings))), + Type::MutableReference(element) => { + Type::MutableReference(Box::new(element.substitute(type_bindings))) + } Type::FieldElement(_) | Type::Integer(_, _, _) @@ -1331,6 +1365,7 @@ impl Type { args.iter().any(|arg| arg.occurs(target_id)) || ret.occurs(target_id) } Type::Vec(element) => element.occurs(target_id), + Type::MutableReference(element) => element.occurs(target_id), Type::FieldElement(_) | Type::Integer(_, _, _) @@ -1373,6 +1408,7 @@ impl Type { Function(args, ret) } Vec(element) => Vec(Box::new(element.follow_bindings())), + MutableReference(element) => MutableReference(Box::new(element.follow_bindings())), // Expect that this function should only be called on instantiated types Forall(..) => unreachable!(), diff --git a/crates/noirc_frontend/src/monomorphization/ast.rs b/crates/noirc_frontend/src/monomorphization/ast.rs index 915c18708e..5341e99540 100644 --- a/crates/noirc_frontend/src/monomorphization/ast.rs +++ b/crates/noirc_frontend/src/monomorphization/ast.rs @@ -3,7 +3,7 @@ use iter_extended::vecmap; use noirc_abi::FunctionSignature; use noirc_errors::Location; -use crate::{node_interner::Mutability, ArgumentMode, BinaryOpKind, Signedness}; +use crate::{node_interner::Mutability, BinaryOpKind, Signedness}; /// The monomorphized AST is expression-based, all statements are also /// folded into this expression enum. Compared to the HIR, the monomorphized @@ -89,6 +89,7 @@ pub enum Literal { pub struct Unary { pub operator: crate::UnaryOp, pub rhs: Box, + pub result_type: Type, } pub type BinaryOp = BinaryOpKind; @@ -124,7 +125,7 @@ pub struct ArrayLiteral { #[derive(Debug, Clone)] pub struct Call { pub func: Box, - pub arguments: Vec<(ArgumentMode, Expression)>, + pub arguments: Vec, pub return_type: Type, pub location: Location, } @@ -176,6 +177,7 @@ pub enum LValue { Ident(Ident), Index { array: Box, index: Box, element_type: Type, location: Location }, MemberAccess { object: Box, field_index: usize }, + Dereference { reference: Box, element_type: Type }, } pub type Parameters = Vec<(LocalId, Mutability, /*name:*/ String, Type)>; @@ -208,6 +210,7 @@ pub enum Type { Unit, Tuple(Vec), Vec(Box), + MutableReference(Box), Function(/*args:*/ Vec, /*ret:*/ Box), } @@ -321,6 +324,7 @@ impl std::fmt::Display for Type { write!(f, "fn({}) -> {}", args.join(", "), ret) } Type::Vec(element) => write!(f, "Vec<{element}>"), + Type::MutableReference(element) => write!(f, "&mut {element}"), } } } diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 4fae3c0603..84c09a3e33 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -22,7 +22,7 @@ use crate::{ }, node_interner::{self, DefinitionKind, Mutability, NodeInterner, StmtId}, token::Attribute, - ArgumentMode, CompTime, FunctionKind, TypeBinding, TypeBindings, + CompTime, FunctionKind, TypeBinding, TypeBindings, }; use self::ast::{Definition, FuncId, Function, LocalId, Program}; @@ -280,6 +280,7 @@ impl<'interner> Monomorphizer<'interner> { HirExpression::Prefix(prefix) => ast::Expression::Unary(ast::Unary { operator: prefix.operator, rhs: Box::new(self.expr(prefix.rhs)), + result_type: Self::convert_type(&self.interner.id_type(expr)), }), HirExpression::Infix(infix) => { @@ -385,7 +386,8 @@ impl<'interner> Monomorphizer<'interner> { | ast::Type::Integer(_, _) | ast::Type::Bool | ast::Type::Unit - | ast::Type::Function(_, _) => { + | ast::Type::Function(_, _) + | ast::Type::MutableReference(_) => { ast::Expression::Literal(ast::Literal::Array(ast::ArrayLiteral { contents: array_contents, element_type, @@ -431,7 +433,8 @@ impl<'interner> Monomorphizer<'interner> { | ast::Type::Integer(_, _) | ast::Type::Bool | ast::Type::Unit - | ast::Type::Function(_, _) => { + | ast::Type::Function(_, _) + | ast::Type::MutableReference(_) => { ast::Expression::Index(ast::Index { collection, index, element_type, location }) } @@ -710,6 +713,11 @@ impl<'interner> Monomorphizer<'interner> { ast::Type::Vec(Box::new(element)) } + HirType::MutableReference(element) => { + let element = Self::convert_type(element); + ast::Type::MutableReference(Box::new(element)) + } + HirType::Forall(_, _) | HirType::Constant(_) | HirType::Error => { unreachable!("Unexpected type {} found", typ) } @@ -724,7 +732,8 @@ impl<'interner> Monomorphizer<'interner> { | ast::Type::Integer(_, _) | ast::Type::Bool | ast::Type::Unit - | ast::Type::Function(_, _) => ast::Type::Array(length, Box::new(element)), + | ast::Type::Function(_, _) + | ast::Type::MutableReference(_) => ast::Type::Array(length, Box::new(element)), ast::Type::Tuple(elements) => { ast::Type::Tuple(vecmap(elements, |typ| Self::aos_to_soa_type(length, typ))) @@ -742,8 +751,7 @@ impl<'interner> Monomorphizer<'interner> { id: node_interner::ExprId, ) -> ast::Expression { let func = Box::new(self.expr(call.func)); - let arguments = - vecmap(&call.arguments, |id| (ArgumentMode::PassByReference, self.expr(*id))); + let arguments = vecmap(&call.arguments, |id| self.expr(*id)); let return_type = self.interner.id_type(id); let return_type = Self::convert_type(&return_type); let location = call.location; @@ -893,6 +901,13 @@ impl<'interner> Monomorphizer<'interner> { let element_type = Self::convert_type(&typ); (array, Some((index, element_type, location))) } + HirLValue::Dereference { lvalue, element_type } => { + let (reference, index) = self.lvalue(*lvalue); + let reference = Box::new(reference); + let element_type = Self::convert_type(&element_type); + let lvalue = ast::LValue::Dereference { reference, element_type }; + (lvalue, index) + } } } @@ -990,6 +1005,12 @@ impl<'interner> Monomorphizer<'interner> { self.create_zeroed_function(parameter_types, ret_type) } ast::Type::Vec(_) => panic!("Cannot create a zeroed Vec value. This type is currently unimplemented and meant to be unusable outside of unconstrained functions"), + ast::Type::MutableReference(element) => { + use crate::UnaryOp::MutableReference; + let rhs = Box::new(self.zeroed_value_of_type(element)); + let result_type = typ.clone(); + ast::Expression::Unary(ast::Unary { rhs, result_type, operator: MutableReference }) + }, } } diff --git a/crates/noirc_frontend/src/monomorphization/printer.rs b/crates/noirc_frontend/src/monomorphization/printer.rs index b7c997558d..b41b11a04f 100644 --- a/crates/noirc_frontend/src/monomorphization/printer.rs +++ b/crates/noirc_frontend/src/monomorphization/printer.rs @@ -245,8 +245,7 @@ impl AstPrinter { ) -> Result<(), std::fmt::Error> { self.print_expr(&call.func, f)?; write!(f, "(")?; - for (i, (mode, elem)) in call.arguments.iter().enumerate() { - write!(f, "{mode}")?; + for (i, elem) in call.arguments.iter().enumerate() { self.print_expr(elem, f)?; if i != call.arguments.len() - 1 { write!(f, ", ")?; @@ -268,6 +267,10 @@ impl AstPrinter { self.print_lvalue(object, f)?; write!(f, ".{field_index}") } + LValue::Dereference { reference, .. } => { + write!(f, "*")?; + self.print_lvalue(reference, f) + } } } } diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index 0f797da484..0ab27a823b 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -638,6 +638,7 @@ fn get_type_method_key(typ: &Type) -> Option { Type::Tuple(_) => Some(Tuple), Type::Function(_, _) => Some(Function), Type::Vec(_) => Some(Vec), + Type::MutableReference(element) => get_type_method_key(element), // We do not support adding methods to these types Type::TypeVariable(_) diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index f61dbf68c1..ec3357c56d 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -34,10 +34,9 @@ use crate::lexer::Lexer; use crate::parser::{force, ignore_then_commit, statement_recovery}; use crate::token::{Attribute, Keyword, Token, TokenKind}; use crate::{ - ArgumentMode, BinaryOp, BinaryOpKind, BlockExpression, CompTime, ConstrainStatement, - FunctionDefinition, Ident, IfExpression, InfixExpression, LValue, Lambda, NoirFunction, - NoirImpl, NoirStruct, Path, PathKind, Pattern, Recoverable, UnaryOp, UnresolvedTypeExpression, - UseTree, UseTreeKind, + BinaryOp, BinaryOpKind, BlockExpression, CompTime, ConstrainStatement, FunctionDefinition, + Ident, IfExpression, InfixExpression, LValue, Lambda, NoirFunction, NoirImpl, NoirStruct, Path, + PathKind, Pattern, Recoverable, UnaryOp, UnresolvedTypeExpression, UseTree, UseTreeKind, }; use chumsky::prelude::*; @@ -271,7 +270,7 @@ fn lambda_parameters() -> impl NoirParser> { let typ = parse_type().recover_via(parameter_recovery()); let typ = just(Token::Colon).ignore_then(typ); - let parameter = pattern(true) + let parameter = pattern() .recover_via(parameter_name_recovery()) .then(typ.or_not().map(|typ| typ.unwrap_or(UnresolvedType::Unspecified))); @@ -286,7 +285,7 @@ fn function_parameters<'a>( ) -> impl NoirParser> + 'a { let typ = parse_type().recover_via(parameter_recovery()); - let full_parameter = pattern(true) + let full_parameter = pattern() .recover_via(parameter_name_recovery()) .then_ignore(just(Token::Colon)) .then(optional_visibility()) @@ -323,19 +322,18 @@ fn self_parameter() -> impl NoirParser<(Pattern, UnresolvedType, AbiVisibility)> .map(|(pattern_keyword, span)| { let ident = Ident::new("self".to_string(), span); let path = Path::from_single("Self".to_owned(), span); - let self_type = UnresolvedType::Named(path, vec![]); - let pattern = Pattern::Identifier(ident); + let mut self_type = UnresolvedType::Named(path, vec![]); + let mut pattern = Pattern::Identifier(ident); - let pattern = match pattern_keyword { - Some((Token::Ampersand, span)) => { - Pattern::MutableReference(Box::new(pattern), span) + match pattern_keyword { + Some((Token::Ampersand, _)) => { + self_type = UnresolvedType::MutableReference(Box::new(self_type)); } - Some((Token::Keyword(_), span)) => Pattern::Mutable(Box::new(pattern), span), - None => pattern, - Some(other) => { - unreachable!("Unexpected keyword {other:?} when parsing self parameter") + Some((Token::Keyword(_), span)) => { + pattern = Pattern::Mutable(Box::new(pattern), span); } - }; + _ => (), + } (pattern, self_type, AbiVisibility::Private) }) @@ -532,17 +530,15 @@ fn declaration<'a, P>(expr_parser: P) -> impl NoirParser + 'a where P: ExprParser + 'a, { - let p = ignore_then_commit( - keyword(Keyword::Let).labelled(ParsingRuleLabel::Statement), - pattern(false), - ); + let p = + ignore_then_commit(keyword(Keyword::Let).labelled(ParsingRuleLabel::Statement), pattern()); let p = p.then(optional_type_annotation()); let p = then_commit_ignore(p, just(Token::Assign)); let p = then_commit(p, expr_parser); p.map(Statement::new_let) } -fn pattern(allow_mutable_references: bool) -> impl NoirParser { +fn pattern() -> impl NoirParser { recursive(|pattern| { let ident_pattern = ident().map(Pattern::Identifier); @@ -550,16 +546,6 @@ fn pattern(allow_mutable_references: bool) -> impl NoirParser { .ignore_then(pattern.clone()) .map_with_span(|inner, span| Pattern::Mutable(Box::new(inner), span)); - let mutref_pattern = if allow_mutable_references { - just(Token::Ampersand) - .ignore_then(keyword(Keyword::Mut)) - .ignore_then(pattern.clone()) - .map_with_span(|inner, span| Pattern::MutableReference(Box::new(inner), span)) - .boxed() - } else { - nothing().boxed() - }; - let short_field = ident().map(|name| (name.clone(), Pattern::Identifier(name))); let long_field = ident().then_ignore(just(Token::Colon)).then(pattern.clone()); @@ -577,7 +563,7 @@ fn pattern(allow_mutable_references: bool) -> impl NoirParser { .delimited_by(just(Token::LeftParen), just(Token::RightParen)) .map_with_span(Pattern::Tuple); - choice((mut_pattern, mutref_pattern, tuple_pattern, struct_pattern, ident_pattern)) + choice((mut_pattern, tuple_pattern, struct_pattern, ident_pattern)) }) .labelled(ParsingRuleLabel::Pattern) } @@ -632,12 +618,17 @@ where .delimited_by(just(Token::LeftBracket), just(Token::RightBracket)) .map(LValueRhs::Index); - l_ident.then(l_member_rhs.or(l_index).repeated()).foldl(|lvalue, rhs| match rhs { - LValueRhs::MemberAccess(field_name) => { - LValue::MemberAccess { object: Box::new(lvalue), field_name } - } - LValueRhs::Index(index) => LValue::Index { array: Box::new(lvalue), index }, - }) + let dereferences = just(Token::Star).repeated(); + + let lvalues = + l_ident.then(l_member_rhs.or(l_index).repeated()).foldl(|lvalue, rhs| match rhs { + LValueRhs::MemberAccess(field_name) => { + LValue::MemberAccess { object: Box::new(lvalue), field_name } + } + LValueRhs::Index(index) => LValue::Index { array: Box::new(lvalue), index }, + }); + + dereferences.then(lvalues).foldr(|_, lvalue| LValue::Dereference(Box::new(lvalue))) } fn parse_type<'a>() -> impl NoirParser + 'a { @@ -656,7 +647,8 @@ fn parse_type_inner( array_type(recursive_type_parser.clone()), tuple_type(recursive_type_parser.clone()), vec_type(recursive_type_parser.clone()), - function_type(recursive_type_parser), + function_type(recursive_type_parser.clone()), + mutable_reference_type(recursive_type_parser), )) } @@ -772,6 +764,16 @@ where .map(|(args, ret)| UnresolvedType::Function(args, Box::new(ret))) } +fn mutable_reference_type(type_parser: T) -> impl NoirParser +where + T: NoirParser, +{ + just(Token::Ampersand) + .ignore_then(keyword(Keyword::Mut)) + .ignore_then(type_parser) + .map(|element| UnresolvedType::MutableReference(Box::new(element))) +} + fn expression() -> impl ExprParser { recursive(|expr| expression_with_precedence(Precedence::Lowest, expr, false)) .labelled(ParsingRuleLabel::Expression) @@ -854,12 +856,17 @@ where P: ExprParser + 'a, { recursive(move |term_parser| { - choice((not(term_parser.clone()), negation(term_parser))) - .map_with_span(Expression::new) - // right-unary operators like a[0] or a.f bind more tightly than left-unary - // operators like - or !, so that !a[0] is parsed as !(a[0]). This is a bit - // awkward for casts so -a as i32 actually binds as -(a as i32). - .or(atom_or_right_unary(expr_parser)) + choice(( + not(term_parser.clone()), + negation(term_parser.clone()), + mutable_reference(term_parser.clone()), + dereference(term_parser), + )) + .map_with_span(Expression::new) + // right-unary operators like a[0] or a.f bind more tightly than left-unary + // operators like - or !, so that !a[0] is parsed as !(a[0]). This is a bit + // awkward for casts so -a as i32 actually binds as -(a as i32). + .or(atom_or_right_unary(expr_parser)) }) } @@ -879,14 +886,14 @@ where P: ExprParser + 'a, { enum UnaryRhs { - Call(Vec<(ArgumentMode, Expression)>), + Call(Vec), ArrayIndex(Expression), Cast(UnresolvedType), MemberAccess((Ident, Option>)), } // `(arg1, ..., argN)` in `my_func(arg1, ..., argN)` - let call_rhs = parenthesized(argument_list(expr_parser.clone())).map(UnaryRhs::Call); + let call_rhs = parenthesized(expression_list(expr_parser.clone())).map(UnaryRhs::Call); // `[expr]` in `arr[expr]` let array_rhs = expr_parser @@ -1021,19 +1028,6 @@ where expr_parser.separated_by(just(Token::Comma)).allow_trailing() } -fn argument_list

(expr_parser: P) -> impl NoirParser> -where - P: ExprParser, -{ - just(Token::Ampersand) - .ignore_then(keyword(Keyword::Mut)) - .or_not() - .map(|opt| opt.map_or(ArgumentMode::PassByValue, |_| ArgumentMode::PassByReference)) - .then(expr_parser) - .separated_by(just(Token::Comma)) - .allow_trailing() -} - fn not

(term_parser: P) -> impl NoirParser where P: ExprParser, @@ -1050,6 +1044,25 @@ where .map(|rhs| ExpressionKind::prefix(UnaryOp::Minus, rhs)) } +fn mutable_reference

(term_parser: P) -> impl NoirParser +where + P: ExprParser, +{ + just(Token::Ampersand) + .ignore_then(keyword(Keyword::Mut)) + .ignore_then(term_parser) + .map(|rhs| ExpressionKind::prefix(UnaryOp::MutableReference, rhs)) +} + +fn dereference

(term_parser: P) -> impl NoirParser +where + P: ExprParser, +{ + just(Token::Star) + .ignore_then(term_parser) + .map(|rhs| ExpressionKind::prefix(UnaryOp::Dereference, rhs)) +} + fn atom<'a, P>(expr_parser: P) -> impl NoirParser + 'a where P: ExprParser + 'a, From e213f792c703a9aa6fcad58bc15e56d33f5fdb74 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 3 Jul 2023 17:58:41 +0200 Subject: [PATCH 04/13] Fix frontend test --- .../noirc_frontend/src/hir/type_check/mod.rs | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index fa47477a94..23b902c5fa 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -159,7 +159,7 @@ mod test { function::{FuncMeta, HirFunction, Param}, stmt::HirStatement, }; - use crate::node_interner::{DefinitionKind, FuncId, NodeInterner}; + use crate::node_interner::{DefinitionKind, FuncId, Mutability, NodeInterner}; use crate::BinaryOpKind; use crate::{ hir::{ @@ -177,7 +177,11 @@ mod test { // let z = x + y; // // Push x variable - let x_id = interner.push_definition("x".into(), false, DefinitionKind::Local(None)); + let x_id = interner.push_definition( + "x".into(), + Mutability::Immutable, + DefinitionKind::Local(None), + ); // Safety: The FileId in a location isn't used for tests let file = FileId::default(); @@ -186,11 +190,19 @@ mod test { let x = HirIdent { id: x_id, location }; // Push y variable - let y_id = interner.push_definition("y".into(), false, DefinitionKind::Local(None)); + let y_id = interner.push_definition( + "y".into(), + Mutability::Immutable, + DefinitionKind::Local(None), + ); let y = HirIdent { id: y_id, location }; // Push z variable - let z_id = interner.push_definition("z".into(), false, DefinitionKind::Local(None)); + let z_id = interner.push_definition( + "z".into(), + Mutability::Immutable, + DefinitionKind::Local(None), + ); let z = HirIdent { id: z_id, location }; // Push x and y as expressions @@ -222,7 +234,11 @@ mod test { let name = HirIdent { location, - id: interner.push_definition("test_func".into(), false, DefinitionKind::Local(None)), + id: interner.push_definition( + "test_func".into(), + Mutability::Immutable, + DefinitionKind::Local(None), + ), }; // Add function meta From 25bde7b97700b6a82b02e5d9f1ee78290a763058 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 3 Jul 2023 18:07:12 +0200 Subject: [PATCH 05/13] Remove 'Mutability' struct, it is no longer needed --- crates/noirc_evaluator/src/ssa/function.rs | 4 +- .../src/ssa_refactor/ssa_gen/context.rs | 11 ++-- .../src/ssa_refactor/ssa_gen/mod.rs | 7 +-- .../src/hir/resolution/resolver.rs | 58 +++++-------------- .../noirc_frontend/src/hir/type_check/stmt.rs | 6 +- .../src/monomorphization/ast.rs | 8 +-- .../src/monomorphization/mod.rs | 45 ++++++-------- crates/noirc_frontend/src/node_interner.rs | 25 ++------ 8 files changed, 50 insertions(+), 114 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/function.rs b/crates/noirc_evaluator/src/ssa/function.rs index fdb5dbbd7e..44e9fa42d6 100644 --- a/crates/noirc_evaluator/src/ssa/function.rs +++ b/crates/noirc_evaluator/src/ssa/function.rs @@ -10,7 +10,6 @@ use crate::ssa::{ }; use iter_extended::try_vecmap; use noirc_frontend::monomorphization::ast::{Call, Definition, FuncId, LocalId, Type}; -use noirc_frontend::node_interner::Mutability; use std::collections::{HashMap, VecDeque}; #[derive(Clone, Debug, PartialEq, Eq, Copy)] @@ -156,8 +155,7 @@ impl IrGenerator { //arguments: for (param_id, mutable, name, typ) in std::mem::take(&mut function.parameters) { let node_ids = self.create_function_parameter(param_id, &typ, &name); - func.arguments - .extend(node_ids.into_iter().map(|id| (id, mutable == Mutability::Mutable))); + func.arguments.extend(node_ids.into_iter().map(|id| (id, mutable))); } // ensure return types are defined in case of recursion call cycle diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index 50100d13de..d4d3cb9d26 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -6,7 +6,6 @@ use acvm::FieldElement; use iter_extended::vecmap; use noirc_frontend::monomorphization::ast::{self, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; -use noirc_frontend::node_interner::Mutability; use noirc_frontend::Signedness; use crate::ssa_refactor::ir::dfg::DataFlowGraph; @@ -132,16 +131,16 @@ impl<'a> FunctionContext<'a> { &mut self, parameter_id: LocalId, parameter_type: &ast::Type, - mutability: Mutability, + mutable: bool, ) { // Add a separate parameter for each field type in 'parameter_type' let parameter_value = Self::map_type(parameter_type, |typ| { let value = self.builder.add_parameter(typ.clone()); - match mutability { - Mutability::Immutable => value.into(), - Mutability::Mutable => self.new_mutable_variable(value), - Mutability::MutableReference => Value::Mutable(value, typ), + if mutable { + self.new_mutable_variable(value) + } else { + value.into() } }); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index dce102cd4d..3f4fee9c48 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -9,10 +9,7 @@ pub(crate) use program::Ssa; use context::SharedContext; use iter_extended::vecmap; use noirc_errors::Location; -use noirc_frontend::{ - monomorphization::ast::{self, Expression, Program}, - node_interner::Mutability, -}; +use noirc_frontend::monomorphization::ast::{self, Expression, Program}; use self::{ context::FunctionContext, @@ -384,7 +381,7 @@ impl<'a> FunctionContext<'a> { fn codegen_let(&mut self, let_expr: &ast::Let) -> Values { let mut values = self.codegen_expression(&let_expr.expression); - if let_expr.mutability != Mutability::Immutable { + if let_expr.mutable { values = values.map(|value| { let value = value.eval(self); Tree::Leaf(self.new_mutable_variable(value)) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 86b23d3e99..341bb9f79f 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -25,7 +25,7 @@ use crate::graph::CrateId; use crate::hir::def_map::{ModuleDefId, TryFromModuleDefId, MAIN_FUNCTION}; use crate::hir_def::stmt::{HirAssignStatement, HirLValue, HirPattern}; use crate::node_interner::{ - DefinitionId, DefinitionKind, ExprId, FuncId, Mutability, NodeInterner, StmtId, StructId, + DefinitionId, DefinitionKind, ExprId, FuncId, NodeInterner, StmtId, StructId, }; use crate::{ hir::{def_map::CrateDefMap, resolution::path_resolver::PathResolver}, @@ -189,7 +189,7 @@ impl<'a> Resolver<'a> { fn add_variable_decl( &mut self, name: Ident, - mutable: Mutability, + mutable: bool, allow_shadowing: bool, definition: DefinitionKind, ) -> HirIdent { @@ -199,7 +199,7 @@ impl<'a> Resolver<'a> { fn add_variable_decl_inner( &mut self, name: Ident, - mutable: Mutability, + mutable: bool, allow_shadowing: bool, warn_if_unused: bool, definition: DefinitionKind, @@ -252,11 +252,7 @@ impl<'a> Resolver<'a> { ident = hir_let_stmt.ident(); resolver_meta = ResolverMeta { num_times_used: 0, ident, warn_if_unused: true }; } else { - let id = self.interner.push_definition( - name.0.contents.clone(), - Mutability::Immutable, - definition, - ); + let id = self.interner.push_definition(name.0.contents.clone(), false, definition); let location = Location::new(name.span(), self.file); ident = HirIdent { location, id }; resolver_meta = ResolverMeta { num_times_used: 0, ident, warn_if_unused: true }; @@ -729,13 +725,7 @@ impl<'a> Resolver<'a> { { let ident = Ident::new(name.to_string(), *span); let definition = DefinitionKind::GenericType(type_variable); - self.add_variable_decl_inner( - ident, - Mutability::Immutable, - false, - false, - definition, - ); + self.add_variable_decl_inner(ident, false, false, false, definition); } } } @@ -950,7 +940,7 @@ impl<'a> Resolver<'a> { let (identifier, block_id) = self.in_new_scope(|this| { let decl = this.add_variable_decl( identifier, - Mutability::Immutable, + false, false, DefinitionKind::Local(None), ); @@ -1035,13 +1025,13 @@ impl<'a> Resolver<'a> { } fn resolve_pattern(&mut self, pattern: Pattern, definition: DefinitionKind) -> HirPattern { - self.resolve_pattern_mutable(pattern, Mutability::Immutable, None, definition) + self.resolve_pattern_mutable(pattern, false, None, definition) } fn resolve_pattern_mutable( &mut self, pattern: Pattern, - mutability: Mutability, + mutable: bool, mutable_span: Option, definition: DefinitionKind, ) -> HirPattern { @@ -1053,7 +1043,7 @@ impl<'a> Resolver<'a> { (Some(_), DefinitionKind::Local(_)) => DefinitionKind::Local(None), (_, other) => other, }; - let id = self.add_variable_decl(name, mutability, false, definition); + let id = self.add_variable_decl(name, mutable, false, definition); HirPattern::Identifier(id) } Pattern::Mutable(pattern, span) => { @@ -1061,22 +1051,12 @@ impl<'a> Resolver<'a> { self.push_err(ResolverError::UnnecessaryMut { first_mut, second_mut: span }); } - let pattern = self.resolve_pattern_mutable( - *pattern, - Mutability::Mutable, - Some(span), - definition, - ); + let pattern = self.resolve_pattern_mutable(*pattern, true, Some(span), definition); HirPattern::Mutable(Box::new(pattern), span) } Pattern::Tuple(fields, span) => { let fields = vecmap(fields, |field| { - self.resolve_pattern_mutable( - field, - mutability, - mutable_span, - definition.clone(), - ) + self.resolve_pattern_mutable(field, mutable, mutable_span, definition.clone()) }); HirPattern::Tuple(fields, span) } @@ -1086,12 +1066,7 @@ impl<'a> Resolver<'a> { // shadowing here lets us avoid further errors if we define ERROR_IDENT // multiple times. let name = ERROR_IDENT.into(); - let identifier = this.add_variable_decl( - name, - Mutability::Immutable, - true, - definition.clone(), - ); + let identifier = this.add_variable_decl(name, false, true, definition.clone()); HirPattern::Identifier(identifier) }; @@ -1105,12 +1080,7 @@ impl<'a> Resolver<'a> { }; let resolve_field = |this: &mut Self, pattern| { - this.resolve_pattern_mutable( - pattern, - mutability, - mutable_span, - definition.clone(), - ) + this.resolve_pattern_mutable(pattern, mutable, mutable_span, definition.clone()) }; let typ = struct_type.clone(); @@ -1304,7 +1274,7 @@ impl<'a> Resolver<'a> { } HirExpression::Ident(ident) => { let definition = self.interner.definition(ident.id); - if matches!(definition.mutability, Mutability::Immutable) { + if !definition.mutable { let span = self.interner.expr_span(&rhs); let variable = definition.name.clone(); self.errors.push(ResolverError::MutableReferenceToImmutableVariable { diff --git a/crates/noirc_frontend/src/hir/type_check/stmt.rs b/crates/noirc_frontend/src/hir/type_check/stmt.rs index 680703759b..1be5d47a59 100644 --- a/crates/noirc_frontend/src/hir/type_check/stmt.rs +++ b/crates/noirc_frontend/src/hir/type_check/stmt.rs @@ -5,7 +5,7 @@ use crate::hir_def::stmt::{ HirAssignStatement, HirConstrainStatement, HirLValue, HirLetStatement, HirPattern, HirStatement, }; use crate::hir_def::types::Type; -use crate::node_interner::{DefinitionId, ExprId, Mutability, StmtId}; +use crate::node_interner::{DefinitionId, ExprId, StmtId}; use crate::CompTime; use super::errors::TypeCheckError; @@ -130,9 +130,7 @@ impl<'interner> TypeChecker<'interner> { let typ = typ.follow_bindings(); let definition = self.interner.definition(ident.id); - if definition.mutability == Mutability::Immutable - && !matches!(typ, Type::MutableReference(_)) - { + if !definition.mutable && !matches!(typ, Type::MutableReference(_)) { self.errors.push(TypeCheckError::Unstructured { msg: format!( "Variable {} must be mutable to be assigned to", diff --git a/crates/noirc_frontend/src/monomorphization/ast.rs b/crates/noirc_frontend/src/monomorphization/ast.rs index 5341e99540..305e6635df 100644 --- a/crates/noirc_frontend/src/monomorphization/ast.rs +++ b/crates/noirc_frontend/src/monomorphization/ast.rs @@ -3,7 +3,7 @@ use iter_extended::vecmap; use noirc_abi::FunctionSignature; use noirc_errors::Location; -use crate::{node_interner::Mutability, BinaryOpKind, Signedness}; +use crate::{BinaryOpKind, Signedness}; /// The monomorphized AST is expression-based, all statements are also /// folded into this expression enum. Compared to the HIR, the monomorphized @@ -61,7 +61,7 @@ pub struct FuncId(pub u32); pub struct Ident { pub location: Option, pub definition: Definition, - pub mutability: Mutability, + pub mutable: bool, pub name: String, pub typ: Type, } @@ -153,7 +153,7 @@ pub struct Index { #[derive(Debug, Clone)] pub struct Let { pub id: LocalId, - pub mutability: Mutability, + pub mutable: bool, pub name: String, pub expression: Box, } @@ -180,7 +180,7 @@ pub enum LValue { Dereference { reference: Box, element_type: Type }, } -pub type Parameters = Vec<(LocalId, Mutability, /*name:*/ String, Type)>; +pub type Parameters = Vec<(LocalId, /*mutable:*/ bool, /*name:*/ String, Type)>; #[derive(Debug, Clone)] pub struct Function { diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 84c09a3e33..a27a4d789f 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -20,7 +20,7 @@ use crate::{ function::{FuncMeta, Param, Parameters}, stmt::{HirAssignStatement, HirLValue, HirLetStatement, HirPattern, HirStatement}, }, - node_interner::{self, DefinitionKind, Mutability, NodeInterner, StmtId}, + node_interner::{self, DefinitionKind, NodeInterner, StmtId}, token::Attribute, CompTime, FunctionKind, TypeBinding, TypeBindings, }; @@ -204,10 +204,7 @@ impl<'interner> Monomorphizer<'interner> { /// Monomorphize each parameter, expanding tuple/struct patterns into multiple parameters /// and binding any generic types found. - fn parameters( - &mut self, - params: Parameters, - ) -> Vec<(ast::LocalId, Mutability, String, ast::Type)> { + fn parameters(&mut self, params: Parameters) -> Vec<(ast::LocalId, bool, String, ast::Type)> { let mut new_params = Vec::with_capacity(params.len()); for parameter in params { self.parameter(parameter.0, ¶meter.1, &mut new_params); @@ -219,14 +216,14 @@ impl<'interner> Monomorphizer<'interner> { &mut self, param: HirPattern, typ: &HirType, - new_params: &mut Vec<(ast::LocalId, Mutability, String, ast::Type)>, + new_params: &mut Vec<(ast::LocalId, bool, String, ast::Type)>, ) { match param { HirPattern::Identifier(ident) => { let new_id = self.next_local_id(); let definition = self.interner.definition(ident.id); let name = definition.name.clone(); - new_params.push((new_id, definition.mutability, name, Self::convert_type(typ))); + new_params.push((new_id, definition.mutable, name, Self::convert_type(typ))); self.define_local(ident.id, new_id); } HirPattern::Mutable(pattern, _) => self.parameter(*pattern, typ, new_params), @@ -501,7 +498,7 @@ impl<'interner> Monomorphizer<'interner> { new_exprs.push(ast::Expression::Let(ast::Let { id: new_id, - mutability: Mutability::Immutable, + mutable: false, name: field_name.0.contents, expression, })); @@ -516,8 +513,8 @@ impl<'interner> Monomorphizer<'interner> { }); let definition = Definition::Local(id); - let mutability = Mutability::Immutable; - ast::Expression::Ident(ast::Ident { definition, mutability, location: None, name, typ }) + let mutable = false; + ast::Expression::Ident(ast::Ident { definition, mutable, location: None, name, typ }) }); // Finally we can return the created Tuple from the new block @@ -543,7 +540,7 @@ impl<'interner> Monomorphizer<'interner> { ast::Expression::Let(ast::Let { id: new_id, - mutability: definition.mutability, + mutable: definition.mutable, name: definition.name.clone(), expression: Box::new(value), }) @@ -581,20 +578,20 @@ impl<'interner> Monomorphizer<'interner> { let mut definitions = vec![ast::Expression::Let(ast::Let { id: fresh_id, - mutability: Mutability::Immutable, + mutable: false, name: "_".into(), expression: Box::new(value), })]; for (i, (field_pattern, field_type)) in fields.into_iter().enumerate() { let location = None; - let mutability = Mutability::Immutable; + let mutable = false; let definition = Definition::Local(fresh_id); let name = i.to_string(); let typ = Self::convert_type(&field_type); let new_rhs = - ast::Expression::Ident(ast::Ident { location, mutability, definition, name, typ }); + ast::Expression::Ident(ast::Ident { location, mutable, definition, name, typ }); let new_rhs = ast::Expression::ExtractTupleField(Box::new(new_rhs), i); let new_expr = self.unpack_pattern(field_pattern, new_rhs, &field_type); @@ -608,32 +605,26 @@ impl<'interner> Monomorphizer<'interner> { fn local_ident(&mut self, ident: &HirIdent) -> Option { let definition = self.interner.definition(ident.id); let name = definition.name.clone(); - let mutable = definition.mutability; + let mutable = definition.mutable; let definition = self.lookup_local(ident.id)?; let typ = Self::convert_type(&self.interner.id_type(ident.id)); - Some(ast::Ident { - location: Some(ident.location), - mutability: mutable, - definition, - name, - typ, - }) + Some(ast::Ident { location: Some(ident.location), mutable, definition, name, typ }) } fn ident(&mut self, ident: HirIdent, expr_id: node_interner::ExprId) -> ast::Expression { let definition = self.interner.definition(ident.id); match &definition.kind { DefinitionKind::Function(func_id) => { - let mutability = definition.mutability; + let mutability = definition.mutable; let location = Some(ident.location); let name = definition.name.clone(); let typ = self.interner.id_type(expr_id); let definition = self.lookup_function(*func_id, expr_id, &typ); let typ = Self::convert_type(&typ); - let ident = ast::Ident { location, mutability, definition, name, typ }; + let ident = ast::Ident { location, mutable: mutability, definition, name, typ }; ast::Expression::Ident(ident) } DefinitionKind::Global(expr_id) => self.expr(*expr_id), @@ -969,7 +960,7 @@ impl<'interner> Monomorphizer<'interner> { let name = lambda_name.to_owned(); ast::Expression::Ident(ast::Ident { definition: Definition::Function(id), - mutability: Mutability::Immutable, + mutable: false, location: None, name, typ, @@ -1028,7 +1019,7 @@ impl<'interner> Monomorphizer<'interner> { let lambda_name = "zeroed_lambda"; let parameters = vecmap(parameter_types, |parameter_type| { - (self.next_local_id(), Mutability::Immutable, "_".into(), parameter_type.clone()) + (self.next_local_id(), false, "_".into(), parameter_type.clone()) }); let body = self.zeroed_value_of_type(ret_type); @@ -1043,7 +1034,7 @@ impl<'interner> Monomorphizer<'interner> { ast::Expression::Ident(ast::Ident { definition: Definition::Function(id), - mutability: Mutability::Immutable, + mutable: false, location: None, name: lambda_name.to_owned(), typ: ast::Type::Function(parameter_types.to_owned(), Box::new(ret_type.clone())), diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index 0ab27a823b..b2bbc0df0d 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -179,7 +179,7 @@ enum Node { #[derive(Debug, Clone)] pub struct DefinitionInfo { pub name: String, - pub mutability: Mutability, + pub mutable: bool, pub kind: DefinitionKind, } @@ -191,23 +191,6 @@ impl DefinitionInfo { } } -#[derive(Debug, Copy, Clone, PartialEq, Eq)] -pub enum Mutability { - Immutable, - Mutable, - MutableReference, -} - -impl std::fmt::Display for Mutability { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - match self { - Mutability::Immutable => Ok(()), - Mutability::Mutable => write!(f, "mut "), - Mutability::MutableReference => write!(f, "&mut "), - } - } -} - #[derive(Debug, Clone, Eq, PartialEq)] pub enum DefinitionKind { Function(FuncId), @@ -402,7 +385,7 @@ impl NodeInterner { pub fn push_definition( &mut self, name: String, - mutability: Mutability, + mutable: bool, definition: DefinitionKind, ) -> DefinitionId { let id = DefinitionId(self.definitions.len()); @@ -410,12 +393,12 @@ impl NodeInterner { self.function_definition_ids.insert(func_id, id); } - self.definitions.push(DefinitionInfo { name, mutability, kind: definition }); + self.definitions.push(DefinitionInfo { name, mutable, kind: definition }); id } pub fn push_function_definition(&mut self, name: String, func: FuncId) -> DefinitionId { - self.push_definition(name, Mutability::Immutable, DefinitionKind::Function(func)) + self.push_definition(name, false, DefinitionKind::Function(func)) } /// Returns the interned HIR function corresponding to `func_id` From f54b78cf3d82b9ebbefaff1acbfcb655db6a65a9 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 3 Jul 2023 18:12:42 +0200 Subject: [PATCH 06/13] Remove some extra lines --- crates/noirc_evaluator/src/ssa/function.rs | 6 +----- crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs | 1 - crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs | 4 +--- 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/function.rs b/crates/noirc_evaluator/src/ssa/function.rs index 44e9fa42d6..33957998e7 100644 --- a/crates/noirc_evaluator/src/ssa/function.rs +++ b/crates/noirc_evaluator/src/ssa/function.rs @@ -228,11 +228,7 @@ impl IrGenerator { //generates an instruction for calling the function pub(super) fn call(&mut self, call: &Call) -> Result, RuntimeError> { let func = self.ssa_gen_expression(&call.func)?.unwrap_id(); - let arguments = call - .arguments - .iter() - .flat_map(|arg| self.ssa_gen_expression(arg).unwrap().to_node_ids()) - .collect(); + let arguments = self.ssa_gen_expression_list(&call.arguments); if let Some(opcode) = self.context.get_builtin_opcode(func, &call.arguments) { return self.call_low_level(opcode, arguments); diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs index 9310279f4b..a9077018df 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs @@ -370,7 +370,6 @@ impl<'function> PerFunctionContext<'function> { ) { let old_results = self.source_function.dfg.instruction_results(call_id); let arguments = vecmap(arguments, |arg| self.translate_value(*arg)); - let new_results = self.context.inline_function(ssa, function, &arguments); let new_results = InsertInstructionResult::Results(&new_results); Self::insert_new_instruction_results(&mut self.values, old_results, new_results); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index d4d3cb9d26..8b21a2afa0 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -135,8 +135,7 @@ impl<'a> FunctionContext<'a> { ) { // Add a separate parameter for each field type in 'parameter_type' let parameter_value = Self::map_type(parameter_type, |typ| { - let value = self.builder.add_parameter(typ.clone()); - + let value = self.builder.add_parameter(typ); if mutable { self.new_mutable_variable(value) } else { @@ -738,7 +737,6 @@ impl SharedContext { } /// Used to remember the results of each step of extracting a value from an ast::LValue -#[derive(Debug)] pub(super) enum LValue { Ident(Values), Index { old_array: ValueId, index: ValueId, array_lvalue: Box }, From 95a1390a37fa3c88bbf934964110916308746591 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 3 Jul 2023 18:17:02 +0200 Subject: [PATCH 07/13] Remove another function --- crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs | 2 +- crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index 3f4fee9c48..690c27b550 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -368,7 +368,7 @@ impl<'a> FunctionContext<'a> { let arguments = call .arguments .iter() - .flat_map(|argument| self.codegen_expression(argument).into_argument_list()) + .flat_map(|argument| self.codegen_expression(argument).into_value_list(self)) .collect(); self.insert_call(function, arguments, &call.return_type) diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs index 008567e944..2d20963561 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs @@ -190,8 +190,4 @@ impl Tree { pub(super) fn into_value_list(self, ctx: &mut FunctionContext) -> Vec { vecmap(self.flatten(), |value| value.eval(ctx)) } - - pub(super) fn into_argument_list(self) -> Vec { - vecmap(self.flatten(), |value| value.eval_reference()) - } } From 9cc00ab11e61a71432a61e9e75d8e79df241363d Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 3 Jul 2023 18:37:35 +0200 Subject: [PATCH 08/13] Revert another line --- crates/noirc_frontend/src/monomorphization/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index a27a4d789f..de06c06390 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -617,14 +617,14 @@ impl<'interner> Monomorphizer<'interner> { let definition = self.interner.definition(ident.id); match &definition.kind { DefinitionKind::Function(func_id) => { - let mutability = definition.mutable; + let mutable = definition.mutable; let location = Some(ident.location); let name = definition.name.clone(); let typ = self.interner.id_type(expr_id); let definition = self.lookup_function(*func_id, expr_id, &typ); let typ = Self::convert_type(&typ); - let ident = ast::Ident { location, mutable: mutability, definition, name, typ }; + let ident = ast::Ident { location, mutable, definition, name, typ }; ast::Expression::Ident(ident) } DefinitionKind::Global(expr_id) => self.expr(*expr_id), From f2411e20af357e615698eb4b55f4e25080e7f3b4 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 4 Jul 2023 10:49:11 +0200 Subject: [PATCH 09/13] Fix test again --- .../noirc_frontend/src/hir/type_check/mod.rs | 26 ++++--------------- 1 file changed, 5 insertions(+), 21 deletions(-) diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index 23b902c5fa..fa47477a94 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -159,7 +159,7 @@ mod test { function::{FuncMeta, HirFunction, Param}, stmt::HirStatement, }; - use crate::node_interner::{DefinitionKind, FuncId, Mutability, NodeInterner}; + use crate::node_interner::{DefinitionKind, FuncId, NodeInterner}; use crate::BinaryOpKind; use crate::{ hir::{ @@ -177,11 +177,7 @@ mod test { // let z = x + y; // // Push x variable - let x_id = interner.push_definition( - "x".into(), - Mutability::Immutable, - DefinitionKind::Local(None), - ); + let x_id = interner.push_definition("x".into(), false, DefinitionKind::Local(None)); // Safety: The FileId in a location isn't used for tests let file = FileId::default(); @@ -190,19 +186,11 @@ mod test { let x = HirIdent { id: x_id, location }; // Push y variable - let y_id = interner.push_definition( - "y".into(), - Mutability::Immutable, - DefinitionKind::Local(None), - ); + let y_id = interner.push_definition("y".into(), false, DefinitionKind::Local(None)); let y = HirIdent { id: y_id, location }; // Push z variable - let z_id = interner.push_definition( - "z".into(), - Mutability::Immutable, - DefinitionKind::Local(None), - ); + let z_id = interner.push_definition("z".into(), false, DefinitionKind::Local(None)); let z = HirIdent { id: z_id, location }; // Push x and y as expressions @@ -234,11 +222,7 @@ mod test { let name = HirIdent { location, - id: interner.push_definition( - "test_func".into(), - Mutability::Immutable, - DefinitionKind::Local(None), - ), + id: interner.push_definition("test_func".into(), false, DefinitionKind::Local(None)), }; // Add function meta From 4f833377c81b9cf950be8dddfcb6e07d6e442634 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 4 Jul 2023 11:44:10 +0200 Subject: [PATCH 10/13] Fix a bug in mem2reg for nested references --- .../references/src/main.nr | 35 ++++++++++++++++ .../src/ssa_refactor/opt/mem2reg.rs | 40 +++++++++++++------ .../src/ssa_refactor/ssa_gen/context.rs | 22 +++++----- .../src/ssa_refactor/ssa_gen/mod.rs | 2 +- .../src/hir/resolution/resolver.rs | 3 +- 5 files changed, 74 insertions(+), 28 deletions(-) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr index d54147138b..6e5eddd805 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr @@ -6,6 +6,26 @@ fn main() { let mut s = S { y: x }; s.add2(); assert(s.y == 5); + + // Test that normal mutable variables are still copied + let mut a = 0; + mutate_copy(a); + assert(a == 0); + + // Test something 3 allocations deep + let mut nested_allocations = Nested { y: &mut &mut 0 }; + add1(*nested_allocations.y); + assert(**nested_allocations.y == 1); + + // Test nested struct allocations with a mutable reference to an array. + let mut c = C { + foo: 0, + bar: &mut C2 { + array: &mut [1, 2], + }, + }; + *c.bar.array = [3, 4]; + assert(*c.bar.array == [3, 4]); } fn add1(x: &mut Field) { @@ -14,8 +34,23 @@ fn add1(x: &mut Field) { struct S { y: Field } +struct Nested { y: &mut &mut Field } + +struct C { + foo: Field, + bar: &mut C2, +} + +struct C2 { + array: &mut [Field; 2] +} + impl S { fn add2(&mut self) { self.y += 2; } } + +fn mutate_copy(mut a: Field) { + a = 7; +} diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs index b65b42cb83..145ba25f5a 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/opt/mem2reg.rs @@ -64,20 +64,39 @@ impl PerBlockContext { dfg: &mut DataFlowGraph, ) -> HashSet { let mut protected_allocations = HashSet::new(); - let mut loads_to_substitute = HashMap::new(); let block = &dfg[self.block_id]; + // Maps Load instruction id -> value to replace the result of the load with + let mut loads_to_substitute = HashMap::new(); + + // Maps Load result id -> value to replace the result of the load with + let mut load_values_to_substitute = HashMap::new(); + for instruction_id in block.instructions() { match &dfg[*instruction_id] { - Instruction::Store { address, value } => { - self.last_stores.insert(*address, *value); + Instruction::Store { mut address, value } => { + if let Some(value) = load_values_to_substitute.get(&address) { + address = *value; + } + + self.last_stores.insert(address, *value); self.store_ids.push(*instruction_id); } - Instruction::Load { address } => { - if let Some(last_value) = self.last_stores.get(address) { + Instruction::Load { mut address } => { + if let Some(value) = load_values_to_substitute.get(&address) { + address = *value; + } + + if let Some(last_value) = self.last_stores.get(&address) { + let result_value = *dfg + .instruction_results(*instruction_id) + .first() + .expect("ICE: Load instructions should have single result"); + loads_to_substitute.insert(*instruction_id, *last_value); + load_values_to_substitute.insert(result_value, *last_value); } else { - protected_allocations.insert(*address); + protected_allocations.insert(address); } } Instruction::Call { arguments, .. } => { @@ -103,12 +122,9 @@ impl PerBlockContext { } // Substitute load result values - for (instruction_id, new_value) in &loads_to_substitute { - let result_value = *dfg - .instruction_results(*instruction_id) - .first() - .expect("ICE: Load instructions should have single result"); - dfg.set_value_from_id(result_value, *new_value); + for (result_value, new_value) in load_values_to_substitute { + let result_value = dfg.resolve(result_value); + dfg.set_value_from_id(result_value, new_value); } // Delete load instructions diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index 8b21a2afa0..8f22c3699d 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -484,9 +484,8 @@ impl<'a> FunctionContext<'a> { LValue::MemberAccess { old_object, object_lvalue, index: *field_index } } ast::LValue::Dereference { reference, .. } => { - let (reference, reference_lvalue) = self.extract_current_value_recursive(reference); - let reference_lvalue = Box::new(reference_lvalue); - LValue::Dereference { reference, reference_lvalue } + let (reference, _) = self.extract_current_value_recursive(reference); + LValue::Dereference { reference } } } } @@ -525,7 +524,7 @@ impl<'a> FunctionContext<'a> { fn extract_current_value_recursive(&mut self, lvalue: &ast::LValue) -> (Values, LValue) { match lvalue { ast::LValue::Ident(ident) => { - let variable = self.ident_lvalue(ident); + let variable = self.ident_lvalue(ident).map(|value| value.eval(self).into()); (variable.clone(), LValue::Ident(variable)) } ast::LValue::Index { array, index, element_type, location: _ } => { @@ -540,10 +539,9 @@ impl<'a> FunctionContext<'a> { (element, LValue::MemberAccess { old_object, object_lvalue, index: *index }) } ast::LValue::Dereference { reference, element_type } => { - let (reference, reference_lvalue) = self.extract_current_value_recursive(reference); - let reference_lvalue = Box::new(reference_lvalue); + let (reference, _) = self.extract_current_value_recursive(reference); let dereferenced = self.dereference(&reference, element_type); - (dereferenced, LValue::Dereference { reference, reference_lvalue }) + (dereferenced, LValue::Dereference { reference }) } } } @@ -551,10 +549,8 @@ impl<'a> FunctionContext<'a> { /// Assigns a new value to the given LValue. /// The LValue can be created via a previous call to extract_current_value. /// This method recurs on the given LValue to create a new value to assign an allocation - /// instruction within an LValue::Ident - see the comment on `extract_current_value` for more - /// details. - /// When first-class references are supported the nearest reference may be in any LValue - /// variant rather than just LValue::Ident. + /// instruction within an LValue::Ident or LValue::Dereference - see the comment on + /// `extract_current_value` for more details. pub(super) fn assign_new_value(&mut self, lvalue: LValue, new_value: Values) { match lvalue { LValue::Ident(references) => self.assign(references, new_value), @@ -567,7 +563,7 @@ impl<'a> FunctionContext<'a> { let new_object = Self::replace_field(old_object, index, new_value); self.assign_new_value(*object_lvalue, new_object); } - LValue::Dereference { reference, .. } => { + LValue::Dereference { reference } => { self.assign(reference, new_value); } } @@ -741,5 +737,5 @@ pub(super) enum LValue { Ident(Values), Index { old_array: ValueId, index: ValueId, array_lvalue: Box }, MemberAccess { old_object: Values, index: usize, object_lvalue: Box }, - Dereference { reference: Values, reference_lvalue: Box }, + Dereference { reference: Values }, } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index 690c27b550..1bfcaebdc3 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -190,7 +190,7 @@ impl<'a> FunctionContext<'a> { noirc_frontend::UnaryOp::Dereference => { let typ = Self::convert_type(&unary.result_type); rhs.map_both(typ, |rhs, element_type| { - let rhs = rhs.eval_reference(); + let rhs = rhs.eval(self); self.builder.insert_load(rhs, element_type).into() }) } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 341bb9f79f..92e54fb6d8 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -848,8 +848,7 @@ impl<'a> Resolver<'a> { } LValue::Dereference(lvalue) => { let lvalue = Box::new(self.resolve_lvalue(*lvalue)); - let element_type = Type::Error; - HirLValue::Dereference { lvalue, element_type } + HirLValue::Dereference { lvalue, element_type: Type::Error } } } } From 9a3bb941e0e9b99a2afdcf7ab3a955408fe084f6 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 5 Jul 2023 09:32:33 +0200 Subject: [PATCH 11/13] Fix inconsistent .eval during ssa-gen on assign statements --- .../test_data_ssa_refactor/tuples/src/main.nr | 28 +++++++++---------- .../src/ssa_refactor/ssa_gen/context.rs | 15 +++++++--- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/tuples/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/tuples/src/main.nr index b1d310b141..0051527e78 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/tuples/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/tuples/src/main.nr @@ -1,25 +1,25 @@ use dep::std; fn main(x: Field, y: Field) { - let pair = (x, y); - assert(pair.0 == 1); - assert(pair.1 == 0); + // let pair = (x, y); + // assert(pair.0 == 1); + // assert(pair.1 == 0); - let (a, b) = if true { (0, 1) } else { (2, 3) }; - assert(a == 0); - assert(b == 1); + // let (a, b) = if true { (0, 1) } else { (2, 3) }; + // assert(a == 0); + // assert(b == 1); - let (u,v) = if x as u32 < 1 { - (x, x + 1) - } else { - (x + 1, x) - }; - assert(u == x+1); - assert(v == x); + // let (u,v) = if x as u32 < 1 { + // (x, x + 1) + // } else { + // (x + 1, x) + // }; + // assert(u == x+1); + // assert(v == x); // Test mutating tuples let mut mutable = ((0, 0), 1, 2, 3); - mutable.0 = pair; + mutable.0 = (x, y); mutable.2 = 7; assert(mutable.0.0 == 1); assert(mutable.0.1 == 0); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index 8f22c3699d..065e4aff8e 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -493,13 +493,13 @@ impl<'a> FunctionContext<'a> { fn dereference(&mut self, values: &Values, element_type: &ast::Type) -> Values { let element_types = Self::convert_type(element_type); values.map_both(element_types, |value, element_type| { - let reference = value.eval_reference(); + let reference = value.eval(self); self.builder.insert_load(reference, element_type).into() }) } /// Compile the given identifier as a reference - ie. avoid calling .eval() - fn ident_lvalue(&self, ident: &ast::Ident) -> Values { + fn ident_lvalue(&mut self, ident: &ast::Ident) -> Values { match &ident.definition { ast::Definition::Local(id) => self.lookup(*id), other => panic!("Unexpected definition found for mutable value: {other}"), @@ -524,7 +524,7 @@ impl<'a> FunctionContext<'a> { fn extract_current_value_recursive(&mut self, lvalue: &ast::LValue) -> (Values, LValue) { match lvalue { ast::LValue::Ident(ident) => { - let variable = self.ident_lvalue(ident).map(|value| value.eval(self).into()); + let variable = self.ident_lvalue(ident); (variable.clone(), LValue::Ident(variable)) } ast::LValue::Index { array, index, element_type, location: _ } => { @@ -560,6 +560,7 @@ impl<'a> FunctionContext<'a> { self.assign_new_value(*array_lvalue, new_array.into()); } LValue::MemberAccess { old_object, index, object_lvalue } => { + let old_object = old_object.map(|value| value.eval(self).into()); let new_object = Self::replace_field(old_object, index, new_value); self.assign_new_value(*object_lvalue, new_object); } @@ -581,7 +582,12 @@ impl<'a> FunctionContext<'a> { } } (Tree::Leaf(lhs), Tree::Leaf(rhs)) => { - let (lhs, rhs) = (lhs.eval_reference(), rhs.eval(self)); + let (lhs2, rhs2) = (lhs.clone().eval_reference(), rhs.clone().eval(self)); + + println!("Created store {lhs2} <- {rhs2} from {lhs:?} <- {rhs:?}"); + + let lhs = lhs2; + let rhs = rhs2; self.builder.insert_store(lhs, rhs); } (lhs, rhs) => { @@ -733,6 +739,7 @@ impl SharedContext { } /// Used to remember the results of each step of extracting a value from an ast::LValue +#[derive(Debug)] pub(super) enum LValue { Ident(Values), Index { old_array: ValueId, index: ValueId, array_lvalue: Box }, From ff083b0b86149c895e0ccb5d3a7fc4daca054353 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 5 Jul 2023 12:41:17 +0200 Subject: [PATCH 12/13] Revert some code --- .../test_data_ssa_refactor/tuples/src/main.nr | 26 +++++++++---------- .../src/ssa_refactor/ssa_gen/context.rs | 12 +++------ .../src/ssa_refactor/ssa_gen/mod.rs | 8 +----- .../src/hir/resolution/resolver.rs | 17 ++++++------ .../noirc_frontend/src/hir/type_check/stmt.rs | 1 - crates/noirc_frontend/src/hir_def/function.rs | 2 -- crates/noirc_frontend/src/hir_def/stmt.rs | 2 -- .../src/monomorphization/mod.rs | 2 -- .../src/monomorphization/printer.rs | 11 +++----- 9 files changed, 28 insertions(+), 53 deletions(-) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/tuples/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/tuples/src/main.nr index 0051527e78..45d8380372 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/tuples/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/tuples/src/main.nr @@ -1,21 +1,21 @@ use dep::std; fn main(x: Field, y: Field) { - // let pair = (x, y); - // assert(pair.0 == 1); - // assert(pair.1 == 0); + let pair = (x, y); + assert(pair.0 == 1); + assert(pair.1 == 0); - // let (a, b) = if true { (0, 1) } else { (2, 3) }; - // assert(a == 0); - // assert(b == 1); + let (a, b) = if true { (0, 1) } else { (2, 3) }; + assert(a == 0); + assert(b == 1); - // let (u,v) = if x as u32 < 1 { - // (x, x + 1) - // } else { - // (x + 1, x) - // }; - // assert(u == x+1); - // assert(v == x); + let (u,v) = if x as u32 < 1 { + (x, x + 1) + } else { + (x + 1, x) + }; + assert(u == x+1); + assert(v == x); // Test mutating tuples let mut mutable = ((0, 0), 1, 2, 3); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index 065e4aff8e..2f13733a2d 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -490,7 +490,7 @@ impl<'a> FunctionContext<'a> { } } - fn dereference(&mut self, values: &Values, element_type: &ast::Type) -> Values { + pub(super) fn dereference(&mut self, values: &Values, element_type: &ast::Type) -> Values { let element_types = Self::convert_type(element_type); values.map_both(element_types, |value, element_type| { let reference = value.eval(self); @@ -499,7 +499,7 @@ impl<'a> FunctionContext<'a> { } /// Compile the given identifier as a reference - ie. avoid calling .eval() - fn ident_lvalue(&mut self, ident: &ast::Ident) -> Values { + fn ident_lvalue(&self, ident: &ast::Ident) -> Values { match &ident.definition { ast::Definition::Local(id) => self.lookup(*id), other => panic!("Unexpected definition found for mutable value: {other}"), @@ -560,7 +560,6 @@ impl<'a> FunctionContext<'a> { self.assign_new_value(*array_lvalue, new_array.into()); } LValue::MemberAccess { old_object, index, object_lvalue } => { - let old_object = old_object.map(|value| value.eval(self).into()); let new_object = Self::replace_field(old_object, index, new_value); self.assign_new_value(*object_lvalue, new_object); } @@ -582,12 +581,7 @@ impl<'a> FunctionContext<'a> { } } (Tree::Leaf(lhs), Tree::Leaf(rhs)) => { - let (lhs2, rhs2) = (lhs.clone().eval_reference(), rhs.clone().eval(self)); - - println!("Created store {lhs2} <- {rhs2} from {lhs:?} <- {rhs:?}"); - - let lhs = lhs2; - let rhs = rhs2; + let (lhs, rhs) = (lhs.eval_reference(), rhs.eval(self)); self.builder.insert_store(lhs, rhs); } (lhs, rhs) => { diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index 1bfcaebdc3..1baf8829a6 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -187,13 +187,7 @@ impl<'a> FunctionContext<'a> { } }) } - noirc_frontend::UnaryOp::Dereference => { - let typ = Self::convert_type(&unary.result_type); - rhs.map_both(typ, |rhs, element_type| { - let rhs = rhs.eval(self); - self.builder.insert_load(rhs, element_type).into() - }) - } + noirc_frontend::UnaryOp::Dereference => self.dereference(&rhs, &unary.result_type), } } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 92e54fb6d8..2d79b1aec0 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -1024,38 +1024,37 @@ impl<'a> Resolver<'a> { } fn resolve_pattern(&mut self, pattern: Pattern, definition: DefinitionKind) -> HirPattern { - self.resolve_pattern_mutable(pattern, false, None, definition) + self.resolve_pattern_mutable(pattern, None, definition) } fn resolve_pattern_mutable( &mut self, pattern: Pattern, - mutable: bool, - mutable_span: Option, + mutable: Option, definition: DefinitionKind, ) -> HirPattern { match pattern { Pattern::Identifier(name) => { // If this definition is mutable, do not store the rhs because it will // not always refer to the correct value of the variable - let definition = match (mutable_span, definition) { + let definition = match (mutable, definition) { (Some(_), DefinitionKind::Local(_)) => DefinitionKind::Local(None), (_, other) => other, }; - let id = self.add_variable_decl(name, mutable, false, definition); + let id = self.add_variable_decl(name, mutable.is_some(), false, definition); HirPattern::Identifier(id) } Pattern::Mutable(pattern, span) => { - if let Some(first_mut) = mutable_span { + if let Some(first_mut) = mutable { self.push_err(ResolverError::UnnecessaryMut { first_mut, second_mut: span }); } - let pattern = self.resolve_pattern_mutable(*pattern, true, Some(span), definition); + let pattern = self.resolve_pattern_mutable(*pattern, Some(span), definition); HirPattern::Mutable(Box::new(pattern), span) } Pattern::Tuple(fields, span) => { let fields = vecmap(fields, |field| { - self.resolve_pattern_mutable(field, mutable, mutable_span, definition.clone()) + self.resolve_pattern_mutable(field, mutable, definition.clone()) }); HirPattern::Tuple(fields, span) } @@ -1079,7 +1078,7 @@ impl<'a> Resolver<'a> { }; let resolve_field = |this: &mut Self, pattern| { - this.resolve_pattern_mutable(pattern, mutable, mutable_span, definition.clone()) + this.resolve_pattern_mutable(pattern, mutable, definition.clone()) }; let typ = struct_type.clone(); diff --git a/crates/noirc_frontend/src/hir/type_check/stmt.rs b/crates/noirc_frontend/src/hir/type_check/stmt.rs index 1be5d47a59..5606547a56 100644 --- a/crates/noirc_frontend/src/hir/type_check/stmt.rs +++ b/crates/noirc_frontend/src/hir/type_check/stmt.rs @@ -59,7 +59,6 @@ impl<'interner> TypeChecker<'interner> { match pattern { HirPattern::Identifier(ident) => self.interner.push_definition_type(ident.id, typ), HirPattern::Mutable(pattern, _) => self.bind_pattern(pattern, typ), - HirPattern::MutableReference(pattern, _) => self.bind_pattern(pattern, typ), HirPattern::Tuple(fields, span) => match typ { Type::Tuple(field_types) if field_types.len() == fields.len() => { for (field, field_type) in fields.iter().zip(field_types) { diff --git a/crates/noirc_frontend/src/hir_def/function.rs b/crates/noirc_frontend/src/hir_def/function.rs index 889e889605..60cb8ea4f8 100644 --- a/crates/noirc_frontend/src/hir_def/function.rs +++ b/crates/noirc_frontend/src/hir_def/function.rs @@ -44,7 +44,6 @@ fn get_param_name<'a>(pattern: &HirPattern, interner: &'a NodeInterner) -> Optio match pattern { HirPattern::Identifier(ident) => Some(interner.definition_name(ident.id)), HirPattern::Mutable(pattern, _) => get_param_name(pattern, interner), - HirPattern::MutableReference(pattern, _) => get_param_name(pattern, interner), HirPattern::Tuple(_, _) => None, HirPattern::Struct(_, _, _) => None, } @@ -69,7 +68,6 @@ impl Parameters { let mut spans = vecmap(&self.0, |param| match ¶m.0 { HirPattern::Identifier(ident) => ident.location.span, HirPattern::Mutable(_, span) => *span, - HirPattern::MutableReference(_, span) => *span, HirPattern::Tuple(_, span) => *span, HirPattern::Struct(_, _, span) => *span, }); diff --git a/crates/noirc_frontend/src/hir_def/stmt.rs b/crates/noirc_frontend/src/hir_def/stmt.rs index b44d080062..8cf9d82f58 100644 --- a/crates/noirc_frontend/src/hir_def/stmt.rs +++ b/crates/noirc_frontend/src/hir_def/stmt.rs @@ -52,7 +52,6 @@ pub struct HirConstrainStatement(pub ExprId, pub FileId); pub enum HirPattern { Identifier(HirIdent), Mutable(Box, Span), - MutableReference(Box, Span), Tuple(Vec, Span), Struct(Type, Vec<(Ident, HirPattern)>, Span), } @@ -62,7 +61,6 @@ impl HirPattern { match self { HirPattern::Identifier(_) => 0, HirPattern::Mutable(pattern, _) => pattern.field_count(), - HirPattern::MutableReference(pattern, _) => pattern.field_count(), HirPattern::Tuple(fields, _) => fields.len(), HirPattern::Struct(_, fields, _) => fields.len(), } diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index de06c06390..7412a4124d 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -227,7 +227,6 @@ impl<'interner> Monomorphizer<'interner> { self.define_local(ident.id, new_id); } HirPattern::Mutable(pattern, _) => self.parameter(*pattern, typ, new_params), - HirPattern::MutableReference(pattern, _) => self.parameter(*pattern, typ, new_params), HirPattern::Tuple(fields, _) => { let tuple_field_types = unwrap_tuple_type(typ); @@ -546,7 +545,6 @@ impl<'interner> Monomorphizer<'interner> { }) } HirPattern::Mutable(pattern, _) => self.unpack_pattern(*pattern, value, typ), - HirPattern::MutableReference(pattern, _) => self.unpack_pattern(*pattern, value, typ), HirPattern::Tuple(patterns, _) => { let fields = unwrap_tuple_type(typ); self.unpack_tuple_pattern(value, patterns.into_iter().zip(fields)) diff --git a/crates/noirc_frontend/src/monomorphization/printer.rs b/crates/noirc_frontend/src/monomorphization/printer.rs index b41b11a04f..929a14e07d 100644 --- a/crates/noirc_frontend/src/monomorphization/printer.rs +++ b/crates/noirc_frontend/src/monomorphization/printer.rs @@ -11,8 +11,8 @@ pub struct AstPrinter { impl AstPrinter { pub fn print_function(&mut self, function: &Function, f: &mut Formatter) -> std::fmt::Result { - let params = vecmap(&function.parameters, |(id, mutability, name, typ)| { - format!("{}{}$l{}: {}", mutability, name, id.0, typ) + let params = vecmap(&function.parameters, |(id, mutable, name, typ)| { + format!("{}{}$l{}: {}", if *mutable { "mut " } else { "" }, name, id.0, typ) }) .join(", "); @@ -245,12 +245,7 @@ impl AstPrinter { ) -> Result<(), std::fmt::Error> { self.print_expr(&call.func, f)?; write!(f, "(")?; - for (i, elem) in call.arguments.iter().enumerate() { - self.print_expr(elem, f)?; - if i != call.arguments.len() - 1 { - write!(f, ", ")?; - } - } + self.print_comma_separated(&call.arguments, f)?; write!(f, ")") } From a054a7b6af8be482a787aa074501f96a94181909 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 5 Jul 2023 13:10:31 +0200 Subject: [PATCH 13/13] Add check for mutating immutable self objects --- .../src/hir/resolution/resolver.rs | 47 +++++++------ .../src/hir/type_check/errors.rs | 4 ++ .../noirc_frontend/src/hir/type_check/expr.rs | 70 ++++++++++++++++--- crates/noirc_frontend/src/hir_def/expr.rs | 42 ----------- 4 files changed, 89 insertions(+), 74 deletions(-) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 2d79b1aec0..72b03689ec 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -891,7 +891,9 @@ impl<'a> Resolver<'a> { let rhs = self.resolve_expression(prefix.rhs); if operator == UnaryOp::MutableReference { - self.verify_mutable_reference(rhs); + if let Err(error) = verify_mutable_reference(self.interner, rhs) { + self.errors.push(error); + } } HirExpression::Prefix(HirPrefixExpression { operator, rhs }) @@ -1258,31 +1260,30 @@ impl<'a> Resolver<'a> { let module_id = self.path_resolver.module_id(); module_id.module(self.def_maps).is_contract } +} - /// Gives an error if a user tries to create a mutable reference - /// to an immutable variable. - fn verify_mutable_reference(&mut self, rhs: ExprId) { - match self.interner.expression(&rhs) { - HirExpression::MemberAccess(member_access) => { - self.verify_mutable_reference(member_access.lhs); - } - HirExpression::Index(_) => { - let span = self.interner.expr_span(&rhs); - self.errors.push(ResolverError::MutableReferenceToArrayElement { span }); - } - HirExpression::Ident(ident) => { - let definition = self.interner.definition(ident.id); - if !definition.mutable { - let span = self.interner.expr_span(&rhs); - let variable = definition.name.clone(); - self.errors.push(ResolverError::MutableReferenceToImmutableVariable { - span, - variable, - }); - } +/// Gives an error if a user tries to create a mutable reference +/// to an immutable variable. +pub fn verify_mutable_reference(interner: &NodeInterner, rhs: ExprId) -> Result<(), ResolverError> { + match interner.expression(&rhs) { + HirExpression::MemberAccess(member_access) => { + verify_mutable_reference(interner, member_access.lhs) + } + HirExpression::Index(_) => { + let span = interner.expr_span(&rhs); + Err(ResolverError::MutableReferenceToArrayElement { span }) + } + HirExpression::Ident(ident) => { + let definition = interner.definition(ident.id); + if !definition.mutable { + let span = interner.expr_span(&rhs); + let variable = definition.name.clone(); + Err(ResolverError::MutableReferenceToImmutableVariable { span, variable }) + } else { + Ok(()) } - _ => (), } + _ => Ok(()), } } diff --git a/crates/noirc_frontend/src/hir/type_check/errors.rs b/crates/noirc_frontend/src/hir/type_check/errors.rs index 83aac12847..b8d4cc66c1 100644 --- a/crates/noirc_frontend/src/hir/type_check/errors.rs +++ b/crates/noirc_frontend/src/hir/type_check/errors.rs @@ -2,6 +2,7 @@ use noirc_errors::CustomDiagnostic as Diagnostic; use noirc_errors::Span; use thiserror::Error; +use crate::hir::resolution::errors::ResolverError; use crate::hir_def::expr::HirBinaryOp; use crate::hir_def::types::Type; @@ -34,6 +35,8 @@ pub enum TypeCheckError { }, #[error("Cannot infer type of expression, type annotations needed before this point")] TypeAnnotationsNeeded { span: Span }, + #[error("{0}")] + ResolverError(ResolverError), } impl TypeCheckError { @@ -103,6 +106,7 @@ impl From for Diagnostic { "Type must be known at this point".to_string(), span, ), + TypeCheckError::ResolverError(error) => error.into(), } } } diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 9fb1e09f66..18f2bdc15e 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -2,14 +2,16 @@ use iter_extended::vecmap; use noirc_errors::Span; use crate::{ + hir::resolution::resolver::verify_mutable_reference, hir_def::{ expr::{ - self, HirArrayLiteral, HirBinaryOp, HirExpression, HirLiteral, HirPrefixExpression, + self, HirArrayLiteral, HirBinaryOp, HirExpression, HirLiteral, HirMethodCallExpression, + HirPrefixExpression, }, types::Type, }, node_interner::{ExprId, FuncId}, - CompTime, Shared, TypeBinding, + CompTime, Shared, TypeBinding, UnaryOp, }; use super::{errors::TypeCheckError, TypeChecker}; @@ -108,7 +110,7 @@ impl<'interner> TypeChecker<'interner> { let span = self.interner.expr_span(expr_id); self.bind_function_type(function, args, span) } - HirExpression::MethodCall(method_call) => { + HirExpression::MethodCall(mut method_call) => { let object_type = self.check_expression(&method_call.object).follow_bindings(); let method_name = method_call.method.0.contents.as_str(); match self.lookup_method(object_type.clone(), method_name, expr_id) { @@ -125,12 +127,20 @@ impl<'interner> TypeChecker<'interner> { // Desugar the method call into a normal, resolved function call // so that the backend doesn't need to worry about methods let location = method_call.location; - let (function_id, function_call) = method_call.into_function_call( - method_id, - location, - &mut args, - self.interner, - ); + + // Automatically add `&mut` if the method expects a mutable reference and + // the object is not already one. + if method_id != FuncId::dummy_id() { + let func_meta = self.interner.function_meta(&method_id); + self.try_add_mutable_reference_to_object( + &mut method_call, + &func_meta.typ, + &mut args, + ); + } + + let (function_id, function_call) = + method_call.into_function_call(method_id, location, self.interner); let span = self.interner.expr_span(expr_id); let ret = self.check_method_call(&function_id, &method_id, args, span); @@ -251,6 +261,48 @@ impl<'interner> TypeChecker<'interner> { typ } + /// Check if the given method type requires a mutable reference to the object type, and check + /// if the given object type is already a mutable reference. If not, add one. + /// This is used to automatically transform a method call: `foo.bar()` into a function + /// call: `bar(&mut foo)`. + fn try_add_mutable_reference_to_object( + &mut self, + method_call: &mut HirMethodCallExpression, + function_type: &Type, + argument_types: &mut [(Type, noirc_errors::Span)], + ) { + let expected_object_type = match function_type { + Type::Function(args, _) => args.get(0), + Type::Forall(_, typ) => match typ.as_ref() { + Type::Function(args, _) => args.get(0), + typ => unreachable!("Unexpected type for function: {typ}"), + }, + typ => unreachable!("Unexpected type for function: {typ}"), + }; + + if let Some(expected_object_type) = expected_object_type { + if matches!(expected_object_type.follow_bindings(), Type::MutableReference(_)) { + let actual_type = argument_types[0].0.follow_bindings(); + + if let Err(error) = verify_mutable_reference(self.interner, method_call.object) { + self.errors.push(TypeCheckError::ResolverError(error)); + } + + if !matches!(actual_type, Type::MutableReference(_)) { + let new_type = Type::MutableReference(Box::new(actual_type)); + + argument_types[0].0 = new_type.clone(); + method_call.object = + self.interner.push_expr(HirExpression::Prefix(HirPrefixExpression { + operator: UnaryOp::MutableReference, + rhs: method_call.object, + })); + self.interner.push_expr_type(&method_call.object, new_type); + } + } + } + } + fn check_index_expression(&mut self, index_expr: expr::HirIndexExpression) -> Type { let index_type = self.check_expression(&index_expr.index); let span = self.interner.expr_span(&index_expr.index); diff --git a/crates/noirc_frontend/src/hir_def/expr.rs b/crates/noirc_frontend/src/hir_def/expr.rs index fa129f8e67..b9ee6634cc 100644 --- a/crates/noirc_frontend/src/hir_def/expr.rs +++ b/crates/noirc_frontend/src/hir_def/expr.rs @@ -149,16 +149,8 @@ impl HirMethodCallExpression { mut self, func: FuncId, location: Location, - argument_types: &mut [(Type, noirc_errors::Span)], interner: &mut NodeInterner, ) -> (ExprId, HirExpression) { - // Automatically add `&mut` if the method expects a mutable reference and - // the object is not already one. - if func != FuncId::dummy_id() { - let func_meta = interner.function_meta(&func); - self.try_add_mutable_reference_to_object(&func_meta.typ, argument_types, interner); - } - let mut arguments = vec![self.object]; arguments.append(&mut self.arguments); @@ -168,40 +160,6 @@ impl HirMethodCallExpression { (func, HirExpression::Call(HirCallExpression { func, arguments, location })) } - - /// Check if the given function type requires a mutable reference to the object type, and check - /// if the given object type is already a mutable reference. If not, add one. - fn try_add_mutable_reference_to_object( - &mut self, - function_type: &Type, - argument_types: &mut [(Type, noirc_errors::Span)], - interner: &mut NodeInterner, - ) { - let expected_object_type = match function_type { - Type::Function(args, _) => args.get(0), - Type::Forall(_, typ) => match typ.as_ref() { - Type::Function(args, _) => args.get(0), - typ => unreachable!("Unexpected type for function: {typ}"), - }, - typ => unreachable!("Unexpected type for function: {typ}"), - }; - - if let Some(expected_object_type) = expected_object_type { - if matches!(expected_object_type.follow_bindings(), Type::MutableReference(_)) { - let actual_type = argument_types[0].0.follow_bindings(); - if !matches!(actual_type, Type::MutableReference(_)) { - let new_type = Type::MutableReference(Box::new(actual_type)); - - argument_types[0].0 = new_type.clone(); - self.object = interner.push_expr(HirExpression::Prefix(HirPrefixExpression { - operator: UnaryOp::MutableReference, - rhs: self.object, - })); - interner.push_expr_type(&self.object, new_type); - } - } - } - } } #[derive(Debug, Clone)]