fix: Fix sequencing of side-effects in lvalue

compiler/noirc_frontend/src/elaborator/statements.rs

@@ -14,10 +14,13 @@ use crate::{
         type_check::{Source, TypeCheckError},
     },
     hir_def::{
-        expr::HirIdent,
-        stmt::{HirAssignStatement, HirForStatement, HirLValue, HirLetStatement, HirStatement},
+        expr::{HirBlockExpression, HirExpression, HirIdent},
+        stmt::{
+            HirAssignStatement, HirForStatement, HirLValue, HirLetStatement, HirPattern,
+            HirStatement,
+        },
     },
-    node_interner::{DefinitionId, DefinitionKind, GlobalId, StmtId},
+    node_interner::{DefinitionId, DefinitionKind, ExprId, GlobalId, StmtId},
 };
 
 use super::{Elaborator, Loop, lints};
@@ -152,7 +155,9 @@ impl Elaborator<'_> {
     pub(super) fn elaborate_assign(&mut self, assign: AssignStatement) -> (HirStatement, Type) {
         let expr_location = assign.expression.location;
         let (expression, expr_type) = self.elaborate_expression(assign.expression);
-        let (lvalue, lvalue_type, mutable) = self.elaborate_lvalue(assign.lvalue);
+
+        let (lvalue, lvalue_type, mutable, mut new_statements) =
+            self.elaborate_lvalue(assign.lvalue);
 
         if !mutable {
             let (_, name, location) = self.get_lvalue_error_info(&lvalue);
@@ -171,8 +176,19 @@ impl Elaborator<'_> {
             }
         });
 
-        let stmt = HirAssignStatement { lvalue, expression };
-        (HirStatement::Assign(stmt), Type::Unit)
+        let assign = HirAssignStatement { lvalue, expression };
+        let assign = HirStatement::Assign(assign);
+
+        if new_statements.is_empty() {
+            (assign, Type::Unit)
+        } else {
+            let assign = self.interner.push_stmt(assign);
+            self.interner.push_stmt_location(assign, expr_location);
+            new_statements.push(assign);
+            let block = HirExpression::Block(HirBlockExpression { statements: new_statements });
+            let block = self.interner.push_expr_full(block, expr_location, Type::Unit);
+            (HirStatement::Expression(block), Type::Unit)
+        }
     }
 
     pub(super) fn elaborate_for(&mut self, for_loop: ForLoopStatement) -> (HirStatement, Type) {
@@ -354,7 +370,13 @@ impl Elaborator<'_> {
         }
     }
 
-    fn elaborate_lvalue(&mut self, lvalue: LValue) -> (HirLValue, Type, bool) {
+    /// Elaborates an lvalue returning:
+    /// - The HirLValue equivalent of the given `lvalue`
+    /// - The type being assigned to
+    /// - Whether the underlying variable is mutable
+    /// - A vector of new statements which need to prefix the resulting assign statement.
+    ///   This hoists out any sub-expressions to simplify sequencing of side-effects.
+    fn elaborate_lvalue(&mut self, lvalue: LValue) -> (HirLValue, Type, bool, Vec<StmtId>) {
         match lvalue {
             LValue::Ident(ident) => {
                 let mut mutable = true;
@@ -384,10 +406,10 @@ impl Elaborator<'_> {
 
                 self.interner.add_local_reference(ident.id, location);
 
-                (HirLValue::Ident(ident.clone(), typ.clone()), typ, mutable)
+                (HirLValue::Ident(ident.clone(), typ.clone()), typ, mutable, Vec::new())
             }
             LValue::MemberAccess { object, field_name, location } => {
-                let (object, lhs_type, mut mutable) = self.elaborate_lvalue(*object);
+                let (object, lhs_type, mut mutable, statements) = self.elaborate_lvalue(*object);
                 let mut object = Box::new(object);
                 let field_name = field_name.clone();
 
@@ -425,11 +447,11 @@ impl Elaborator<'_> {
                 let typ = object_type.clone();
                 let lvalue =
                     HirLValue::MemberAccess { object, field_name, field_index, typ, location };
-                (lvalue, object_type, mutable)
+                (lvalue, object_type, mutable, statements)
             }
             LValue::Index { array, index, location } => {
                 let expr_location = index.location;
-                let (index, index_type) = self.elaborate_expression(index);
+                let (mut index, index_type) = self.elaborate_expression(index);
 
                 self.push_index_to_check(index);
 
@@ -440,7 +462,19 @@ impl Elaborator<'_> {
                     expr_location,
                 });
 
-                let (mut lvalue, mut lvalue_type, mut mutable) = self.elaborate_lvalue(*array);
+                let (mut lvalue, mut lvalue_type, mut mutable, mut statements) =
+                    self.elaborate_lvalue(*array);
+
+                // Push the index expression to the end of the new statements list, referring to it
+                // afterward with a let binding. Note that since we recur first then push to the
+                // end of the list we're evaluating side-effects such that in `a[i][j]`, `i` will
+                // be evaluated first, followed by `j`.
+                if let Some((index_definition, new_index)) =
+                    self.fresh_definition_for_lvalue_index(index, index_type, expr_location)
+                {
+                    index = new_index;
+                    statements.push(index_definition);
+                }
 
                 // Before we check that the lvalue is an array, try to dereference it as many times
                 // as needed to unwrap any `&` or `&mut` wrappers.
@@ -485,10 +519,10 @@ impl Elaborator<'_> {
 
                 let array = Box::new(lvalue);
                 let array_type = typ.clone();
-                (HirLValue::Index { array, index, typ, location }, array_type, mutable)
+                (HirLValue::Index { array, index, typ, location }, array_type, mutable, statements)
             }
             LValue::Dereference(lvalue, location) => {
-                let (lvalue, reference_type, _) = self.elaborate_lvalue(*lvalue);
+                let (lvalue, reference_type, _, statements) = self.elaborate_lvalue(*lvalue);
                 let lvalue = Box::new(lvalue);
 
                 let element_type = Type::type_variable(self.interner.next_type_variable_id());
@@ -510,7 +544,7 @@ impl Elaborator<'_> {
                     location,
                     implicitly_added: false,
                 };
-                (lvalue, typ, true)
+                (lvalue, typ, true, statements)
             }
             LValue::Interned(id, location) => {
                 let lvalue = self.interner.get_lvalue(id, location).clone();
@@ -519,6 +553,41 @@ impl Elaborator<'_> {
         }
     }
 
+    fn fresh_definition_for_lvalue_index(
+        &mut self,
+        expr: ExprId,
+        typ: Type,
+        location: Location,
+    ) -> Option<(StmtId, ExprId)> {
+        // If the original expression trivially cannot have side-effects, don't bother cluttering
+        // the output with a let binding. Note that array literals can have side-effects but these
+        // would produce type errors anyway.
+        if matches!(
+            self.interner.expression(&expr),
+            HirExpression::Ident(..) | HirExpression::Literal(..)
+        ) {
+            return None;
+        }
+
+        let id = self.interner.push_definition(
+            format!("i_{}", self.interner.definition_count()),
+            false,
+            false,
+            DefinitionKind::Local(None),
+            location,
+        );
+        let ident = HirIdent::non_trait_method(id, location);
+        let ident_expr = HirExpression::Ident(ident.clone(), None);
+
+        let ident_id = self.interner.push_expr_full(ident_expr, location, typ.clone());
+
+        let pattern = HirPattern::Identifier(ident);
+        let let_ = HirStatement::Let(HirLetStatement::basic(pattern, typ, expr));
+        let let_ = self.interner.push_stmt(let_);
+        self.interner.push_stmt_location(let_, location);
+        Some((let_, ident_id))
+    }
+
     /// Type checks a field access, adding dereference operators as necessary
     pub(super) fn check_field_access(
         &mut self,

compiler/noirc_frontend/src/hir_def/stmt.rs

@@ -143,6 +143,9 @@ pub enum HirLValue {
     },
     Index {
         array: Box<HirLValue>,
+        /// `index` is required to be an identifier to simplify sequencing of side-effects.
+        /// However we also store types and locations on ExprIds which makes these necessary
+        /// for evaluating/compiling HirIdents so we don't directly require a HirIdent type here.
         index: ExprId,
         typ: Type,
         location: Location,

compiler/noirc_frontend/src/node_interner.rs

@@ -218,12 +218,12 @@
     interned_statement_kinds: noirc_arena::Arena<StatementKind>,
 
     // Interned `UnresolvedTypeData`s during comptime code.
     interned_unresolved_type_datas: noirc_arena::Arena<UnresolvedTypeData>,
 
     // Interned `Pattern`s during comptime code.
     interned_patterns: noirc_arena::Arena<Pattern>,
 
     /// Determins whether to run in LSP mode. In LSP mode references are tracked.
     pub(crate) lsp_mode: bool,
 
     /// Whether to avoid comptime println from producing output
@@ -706,7 +706,7 @@
             quoted_types: Default::default(),
             interned_expression_kinds: Default::default(),
             interned_statement_kinds: Default::default(),
             interned_unresolved_type_datas: Default::default(),
             interned_patterns: Default::default(),
             lsp_mode: false,
             disable_comptime_printing: false,
@@ -980,6 +980,10 @@
         self.func_meta.insert(func_id, func_data);
     }
 
+    pub fn definition_count(&self) -> usize {
+        self.definitions.len()
+    }
+
     pub fn push_definition(
         &mut self,
         name: String,
@@ -2212,11 +2216,11 @@
         &mut self,
         typ: UnresolvedTypeData,
     ) -> InternedUnresolvedTypeData {
         InternedUnresolvedTypeData(self.interned_unresolved_type_datas.insert(typ))
     }
 
     pub fn get_unresolved_type_data(&self, id: InternedUnresolvedTypeData) -> &UnresolvedTypeData {
         &self.interned_unresolved_type_datas[id.0]
     }
 
     /// Returns the type of an operator (which is always a function), along with its return type.

test_programs/compile_failure/assign_mutation_in_lvalue/src/main.nr → test_programs/execution_success/assign_mutation_in_lvalue/src/main.nr

@@ -10,8 +10,5 @@ fn bug() {
     1
     }] = 7;
 
-    // acir+brillig yield ([1, 7], 3)
-    // comptime yields ([1, 7], 6)
-    // both should yield:
     assert_eq(a, ([4, 7], 6));
 }