fix: ensure that purity analysis pass explores all functions

compiler/noirc_evaluator/src/ssa/opt/pure.rs

@@ -49,10 +49,28 @@ impl Ssa {
             function.dfg.set_function_purities(purities.clone());
         }
 
+        #[cfg(debug_assertions)]
+        purity_analysis_post_check(&self);
+
         self
     }
 }
 
+/// Post-check condition for [Ssa::purity_analysis].
+///
+/// Succeeds if:
+///   - all functions have a purity status attached to it.
+///
+/// Otherwise panics.
+#[cfg(debug_assertions)]
+fn purity_analysis_post_check(ssa: &Ssa) {
+    if let Some((id, _)) =
+        ssa.functions.iter().find(|(id, function)| function.dfg.purity_of(**id).is_none())
+    {
+        panic!("Function {id} does not have a purity status")
+    }
+}
+
 pub(crate) type FunctionPurities = HashMap<FunctionId, Purity>;
 
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
@@ -100,14 +118,24 @@ impl std::fmt::Display for Purity {
 
 impl Function {
     fn is_pure(&self) -> (Purity, BTreeSet<FunctionId>) {
+        // Note, this function must be allowed to complete despite the fact that once the function is marked as impure
+        // then its final purity is known. This is because we need to collect all of the dependencies of the function
+        // to ensure that they are processed.
+        //
+        // This can be relaxed if we calculate the callgraph separately.
+
         let contains_reference = |value_id: &ValueId| {
             let typ = self.dfg.type_of_value(*value_id);
             typ.contains_reference()
         };
 
-        if self.parameters().iter().any(&contains_reference) {
-            return (Purity::Impure, BTreeSet::new());
-        }
+        let mut result = if self.runtime().is_acir() {
+            Purity::Pure
+        } else {
+            // Because we return bogus values when a brillig function is called from acir
+            // in a disabled predicate, brillig functions can never be truly pure unfortunately.
+            Purity::PureWithPredicate
+        };
 
         // Set of functions we call which the purity result depends on.
         // `is_pure` is intended to be called on each function, building
@@ -116,13 +144,9 @@ impl Function {
         // result here could be overridden by one of these dependencies being impure.
         let mut dependencies = BTreeSet::new();
 
-        let mut result = if self.runtime().is_acir() {
-            Purity::Pure
-        } else {
-            // Because we return bogus values when a brillig function is called from acir
-            // in a disabled predicate, brillig functions can never be truly pure unfortunately.
-            Purity::PureWithPredicate
-        };
+        if self.parameters().iter().any(&contains_reference) {
+            result = Purity::Impure;
+        }
 
         for block in self.reachable_blocks() {
             for instruction in self.dfg[block].instructions() {
@@ -132,12 +156,10 @@ impl Function {
                 // parameters or returned, we can ignore them.
                 // We even ignore Constrain instructions. As long as the external parameters are
                 // identical, we should be constraining the same values anyway.
-                match &self.dfg[*instruction] {
+                let instruction_purity = match &self.dfg[*instruction] {
                     Instruction::Constrain(..)
                     | Instruction::ConstrainNotEqual(..)
-                    | Instruction::RangeCheck { .. } => {
-                        result = Purity::PureWithPredicate;
-                    }
+                    | Instruction::RangeCheck { .. } => Purity::PureWithPredicate,
 
                     // These instructions may be pure unless:
                     // - We may divide by zero
@@ -148,7 +170,9 @@ impl Function {
                     | Instruction::ArrayGet { .. }
                     | Instruction::ArraySet { .. }) => {
                         if ins.requires_acir_gen_predicate(&self.dfg) {
-                            result = Purity::PureWithPredicate;
+                            Purity::PureWithPredicate
+                        } else {
+                            result
                         }
                     }
                     Instruction::Call { func, .. } => {
@@ -157,25 +181,24 @@ impl Function {
                                 // We don't know if this function is pure or not yet,
                                 // so track it as a dependency for now.
                                 dependencies.insert(*function_id);
+                                result
                             }
                             Value::Intrinsic(intrinsic) => match intrinsic.purity() {
-                                Purity::Pure => (),
-                                Purity::PureWithPredicate => result = Purity::PureWithPredicate,
-                                Purity::Impure => return (Purity::Impure, BTreeSet::new()),
+                                Purity::Pure => result,
+                                Purity::PureWithPredicate => Purity::PureWithPredicate,
+                                Purity::Impure => Purity::Impure,
                             },
-                            Value::ForeignFunction(_) => return (Purity::Impure, BTreeSet::new()),
+                            Value::ForeignFunction(_) => Purity::Impure,
                             // The function we're calling is unknown in the remaining cases,
                             // so just assume the worst.
                             Value::Global(_)
                             | Value::Instruction { .. }
                             | Value::Param { .. }
-                            | Value::NumericConstant { .. } => {
-                                return (Purity::Impure, BTreeSet::new());
-                            }
+                            | Value::NumericConstant { .. } => Purity::Impure,
                         }
                     }
 
-                    // The rest are always pure (including allocate, load, & store)
+                    // The rest are always pure (including allocate, load, & store) and so don't affect purity
                     Instruction::Cast(_, _)
                     | Instruction::Not(_)
                     | Instruction::Truncate { .. }
@@ -187,15 +210,17 @@ impl Function {
                     | Instruction::DecrementRc { .. }
                     | Instruction::IfElse { .. }
                     | Instruction::MakeArray { .. }
-                    | Instruction::Noop => (),
-                }
+                    | Instruction::Noop => result,
+                };
+
+                result = result.unify(instruction_purity);
             }
 
             // If the function returns a reference it is impure
             let terminator = self.dfg[block].terminator();
             if let Some(TerminatorInstruction::Return { return_values, .. }) = terminator {
                 if return_values.iter().any(&contains_reference) {
-                    return (Purity::Impure, BTreeSet::new());
+                    result = Purity::Impure;
                 }
             }
         }
@@ -401,4 +426,44 @@ mod test {
         }
         ");
     }
+
+    #[test]
+    fn regression_8625() {
+        // This test checks for a case which would result in some functions not having a purity status applied.
+        // See https://github.com/noir-lang/noir/issues/8625
+        let src = r#"
+        brillig(inline) fn main f0 {
+          b0(v0: [u8; 3]):
+            inc_rc v0
+            v1 = allocate -> &mut [u8; 3]
+            store v0 at v1
+            inc_rc v0
+            inc_rc v0
+            call f1(v1, u32 0, u32 2, Field 3)
+            return
+        }
+        brillig(inline) fn impure_because_reference_arg f1 {
+          b0(v0: &mut [u8; 3], v1: u32, v2: u32, v3: Field):
+            call f2(v0, v1, v2, v3)
+            return
+        }
+        brillig(inline) fn also_impure_because_reference_arg f2 {
+          b0(v0: &mut [u8; 3], v1: u32, v2: u32, v3: Field):
+            call f3()
+            return
+        }
+        brillig(inline) fn pure_with_predicate_func f3 {
+          b0():
+            return
+        }"#;
+
+        let ssa = Ssa::from_str(src).unwrap();
+        let ssa = ssa.purity_analysis();
+
+        let purities = &ssa.main().dfg.function_purities;
+        assert_eq!(purities[&FunctionId::test_new(0)], Purity::Impure);
+        assert_eq!(purities[&FunctionId::test_new(1)], Purity::Impure);
+        assert_eq!(purities[&FunctionId::test_new(2)], Purity::Impure);
+        assert_eq!(purities[&FunctionId::test_new(3)], Purity::PureWithPredicate);
+    }
 }

compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs

@@ -79,6 +79,9 @@ impl Function {
 
             context.replace_value(old_result, new_result);
         });
+
+        #[cfg(debug_assertions)]
+        remove_bit_shifts_post_check(self);
     }
 }
 
@@ -349,6 +352,34 @@ impl Context<'_, '_, '_> {
     }
 }
 
+/// Post-check condition for [Function::remove_bit_shifts].
+///
+/// Succeeds if:
+///   - `func` is not an ACIR function, OR
+///   - `func` does not contain any bitshift instructions.
+///
+/// Otherwise panics.
+#[cfg(debug_assertions)]
+fn remove_bit_shifts_post_check(func: &Function) {
+    // Non-ACIR functions should be unaffected.
+    if !func.runtime().is_acir() {
+        return;
+    }
+
+    // Otherwise there should be no shift-left or shift-right instructions in any reachable block.
+    for block_id in func.reachable_blocks() {
+        let instruction_ids = func.dfg[block_id].instructions();
+        for instruction_id in instruction_ids {
+            if matches!(
+                func.dfg[*instruction_id],
+                Instruction::Binary(Binary { operator: BinaryOp::Shl | BinaryOp::Shr, .. })
+            ) {
+                panic!("Bitshift instruction still remains in ACIR function");
+            }
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use crate::{assert_ssa_snapshot, ssa::ssa_gen::Ssa};

compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs

@@ -43,6 +43,9 @@ impl Function {
         } else {
             Context::default().remove_if_else(self);
         }
+
+        #[cfg(debug_assertions)]
+        remove_if_else_post_check(self);
     }
 }
 
@@ -224,6 +227,31 @@ fn slice_capacity_change(
     }
 }
 
+/// Post-check condition for [Function::remove_if_else].
+///
+/// Succeeds if:
+///   - `func` is a Brillig function, OR
+///   - `func` does not contain any if-else instructions.
+///
+/// Otherwise panics.
+#[cfg(debug_assertions)]
+fn remove_if_else_post_check(func: &Function) {
+    // Brillig functions should be unaffected.
+    if func.runtime().is_brillig() {
+        return;
+    }
+
+    // Otherwise there should be no if-else instructions in any reachable block.
+    for block_id in func.reachable_blocks() {
+        let instruction_ids = func.dfg[block_id].instructions();
+        for instruction_id in instruction_ids {
+            if matches!(func.dfg[*instruction_id], Instruction::IfElse { .. }) {
+                panic!("IfElse instruction still remains in ACIR function");
+            }
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use crate::{assert_ssa_snapshot, ssa::ssa_gen::Ssa};

cspell.json

@@ -40,6 +40,7 @@
         "bytecount",
         "cachix",
         "callees",
+        "callgraph",
         "callsite",
         "callsites",
         "callstack",