Adapting freeze to new analysis, fixing a bug in reference analysis

third_party/move/move-compiler-v2/src/pipeline/reference_safety_processor.rs

@@ -216,6 +216,8 @@ enum BorrowEdgeKind {
     /// call outcome can be different, they are distinguished by code offset -- two call edges are never the
     /// same.
     Call(bool, Operation, CodeOffset),
+    /// Freezes a mutable reference.
+    Freeze,
 }
 
 impl BorrowEdgeKind {
@@ -226,16 +228,7 @@ impl BorrowEdgeKind {
             | BorrowGlobal(is_mut)
             | BorrowField(is_mut, _)
             | Call(is_mut, _, _) => *is_mut,
-        }
-    }
-
-    fn set_mut(&mut self, value: bool) {
-        use BorrowEdgeKind::*;
-        match self {
-            BorrowLocal(is_mut)
-            | BorrowGlobal(is_mut)
-            | BorrowField(is_mut, _)
-            | Call(is_mut, _, _) => *is_mut = value,
+            Freeze => false,
         }
     }
 }
@@ -1020,7 +1013,15 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
     ///
     /// If we walk this graph now from the root to the leaves, we can determine safety by directly comparing
     /// hyper edge siblings.
-    fn check_borrow_safety(&mut self, temps: &BTreeSet<TempIndex>) {
+    fn check_borrow_safety(&mut self, temps_vec: &[TempIndex]) {
+        // First check direct duplicates
+        for (i, temp) in temps_vec.iter().enumerate() {
+            if temps_vec[i + 1..].contains(temp) {
+                self.exclusive_access_direct_dup_error(*temp)
+            }
+        }
+        // Now build and analyze the hyper graph
+        let temps = &temps_vec.iter().cloned().collect::<BTreeSet<_>>();
         let filtered_leaves = self
             .state
             .leaves()
@@ -1083,9 +1084,9 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
                     let target = edge.target;
                     targets.insert(target);
                     if edge.kind.is_mut() {
-                        if let Some(temps) = filtered_leaves.get(&target) {
+                        if let Some(ts) = filtered_leaves.get(&target) {
                             let mut inter =
-                                temps.intersection(temps).cloned().collect::<BTreeSet<_>>();
+                                ts.intersection(temps).cloned().collect::<BTreeSet<_>>();
                             if !inter.is_empty() {
                                 if !self.state.is_leaf(&target) {
                                     // A mut leaf node must have exclusive access
@@ -1098,7 +1099,7 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
                 }
                 if mapped_temps.len() > 1 {
                     // We cannot associate the same mut node with more than one local
-                    self.exclusive_access_dup_error(&hyper, &mapped_temps)
+                    self.exclusive_access_indirect_dup_error(&hyper, &mapped_temps)
                 }
                 hyper_nodes.insert(targets);
             }
@@ -1171,8 +1172,9 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
     }
 
     /// Reports an error about exclusive access requirement for duplicate usage. See safety
-    /// condition (d) in the file header documentation.
-    fn exclusive_access_dup_error(
+    /// condition (d) in the file header documentation. This handles the case were the
+    /// same node is used by multiple temps
+    fn exclusive_access_indirect_dup_error(
         &self,
         labels: &BTreeSet<LifetimeLabel>,
         temps: &BTreeSet<TempIndex>,
@@ -1182,7 +1184,7 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
         self.error_with_hints(
             self.cur_loc(),
             format!(
-                "same mutable reference in {} is also used in other {} in same argument list",
+                "same mutable reference in {} is also used in other {} in argument list",
                 self.display(*ts[0]),
                 self.display(*ts[1])
             ),
@@ -1191,6 +1193,21 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
         )
     }
 
+    /// Reports an error about exclusive access requirement for duplicate usage. See safety
+    /// condition (d) in the file header documentation. This handles the case were the
+    /// same local is used multiple times.
+    fn exclusive_access_direct_dup_error(&self, temp: TempIndex) {
+        self.error_with_hints(
+            self.cur_loc(),
+            format!(
+                "same mutable reference in {} is used again in argument list",
+                self.display(temp),
+            ),
+            "requirement enforced here",
+            iter::empty(),
+        )
+    }
+
     /// Reports an error together with hints
     fn error_with_hints(
         &self,
@@ -1262,6 +1279,7 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
                 BorrowGlobal(_) => "global borrow",
                 BorrowField(..) => "field borrow",
                 Call(..) => "call result",
+                Freeze => "freeze",
             },),
         )
     }
@@ -1474,18 +1492,12 @@ impl<'env, 'state> LifetimeAnalysisStep<'env, 'state> {
             .label_for_temp(src)
             .expect("label for reference")
             .clone();
-        let redirected = self.state.make_temp(dest, code_offset, 0, false);
-        for (parent, mut edge) in self
-            .state
-            .parent_edges(&label)
-            .map(|(p, e)| (p, e.clone()))
-            .collect::<Vec<_>>()
-        // need to mutate state
-        {
-            edge.target = redirected.clone();
-            edge.kind.set_mut(false);
-            self.state.add_edge(parent, edge)
-        }
+        let target = self.state.make_temp(dest, code_offset, 0, false);
+        self.state.add_edge(label, BorrowEdge {
+            kind: BorrowEdgeKind::Freeze,
+            loc: self.cur_loc(),
+            target,
+        })
     }
 
     /// Process a MoveFrom instruction.
@@ -1614,14 +1626,21 @@ impl<'env> TransferFunctions for LifeTimeAnalysis<'env> {
                 | Operation::Function(..)
                 | Operation::Eq
                 | Operation::Neq => {
-                    let exclusive_refs =
-                        srcs.iter().filter(|t| step.is_ref(**t)).cloned().collect();
+                    let exclusive_refs = srcs
+                        .iter()
+                        .filter(|t| step.is_ref(**t))
+                        .cloned()
+                        .collect_vec();
                     step.check_borrow_safety(&exclusive_refs)
                 },
                 _ => {},
             },
             Ret(_, srcs) => {
-                let exclusive_refs = srcs.iter().filter(|t| step.is_ref(**t)).cloned().collect();
+                let exclusive_refs = srcs
+                    .iter()
+                    .filter(|t| step.is_ref(**t))
+                    .cloned()
+                    .collect_vec();
                 step.check_borrow_safety(&exclusive_refs)
             },
             _ => {},
@@ -1816,6 +1835,7 @@ impl<'a> Display for BorrowEdgeDisplay<'a> {
             BorrowGlobal(is_mut) => write!(f, "borrow_global({})", is_mut),
             BorrowField(is_mut, _) => write!(f, "borrow_field({})", is_mut),
             Call(is_mut, _, _) => write!(f, "call({})", is_mut),
+            Freeze => write!(f, "freeze"),
         })?;
         if display_child {
             write!(f, " -> {}", edge.target)

third_party/move/move-compiler-v2/tests/checking/inlining/bug_11112.exp

@@ -235,23 +235,6 @@ fun vectors::test_for_each_mut() {
  44: return ()
 }
 
-
-Diagnostics:
-error: cannot mutably borrow since immutable references exist
-    ┌─ tests/checking/inlining/bug_11112.move:8:23
-    │
-  8 │     vector::for_each_mut(&mut v, |e| { *e = s; s = s + 1 });
-    │     ---------------------^^^^^^----------------------------
-    │     │                    │
-    │     │                    mutable borrow attempted here
-    │     │                    previous local borrow
-    │     from a call inlined at this callsite
-    │
-    ┌─ /Users/wrwg/aptos-core/third_party/move/move-compiler-v2/../move-stdlib/sources/vector.move:181:20
-    │
-181 │         while (i < length(v)) {
-    │                    --------- requirement enforced here
-
 ============ after ReferenceSafetyProcessor: ================
 
 [variant baseline]
@@ -374,7 +357,7 @@ fun vectors::test_for_each_mut() {
      #
  12: $t13 := freeze_ref($t7)
      # live vars: $t0, $t5, $t7, $t9, $t13
-     # graph: {@700=local($t0)[borrow(false) -> @C00,borrow(true) -> @701],@701=derived[],@C00=derived[]}
+     # graph: {@700=local($t0)[borrow(true) -> @701],@701=derived[freeze -> @C00],@C00=derived[]}
      # locals: {$t0=@700,$t7=@701,$t13=@C00}
      # globals: {}
      # moved: {$t2,$t3,$t4,$t7,$t12,$t13,$t17,$t19}

third_party/move/move-compiler-v2/tests/checking/inlining/lambda_cast.exp

@@ -72,16 +72,3 @@ module 0x12391283::M {
     spec fun $test_1(): u64;
     spec fun $test_2(): u64;
 } // end 0x12391283::M
-
-
-Diagnostics:
-error: cannot mutably borrow since immutable references exist
-   ┌─ /Users/wrwg/aptos-core/third_party/move/move-compiler-v2/../move-stdlib/sources/vector.move:65:33
-   │
-65 │     public fun reverse<Element>(v: &mut vector<Element>) {
-   │                                 ^
-   │                                 │
-   │                                 mutable borrow attempted here
-   │                                 previous local borrow
-66 │         let len = length(v);
-   │                   --------- requirement enforced here

third_party/move/move-compiler-v2/tests/file-format-generator/vector.exp

@@ -1,29 +1,4 @@
 
-Diagnostics:
-error: cannot mutably borrow since immutable references exist
-   ┌─ tests/file-format-generator/vector.move:13:32
-   │
-13 │     public fun remove<Element>(v: &mut vector<Element>, i: u64): Element {
-   │                                ^
-   │                                │
-   │                                mutable borrow attempted here
-   │                                previous local borrow
-14 │         use std::vector;
-15 │         let len = vector::length(v);
-   │                   ----------------- requirement enforced here
-
-error: cannot mutably borrow since immutable references exist
-   ┌─ /Users/wrwg/aptos-core/third_party/move/move-compiler-v2/../move-stdlib/sources/vector.move:65:33
-   │
-65 │     public fun reverse<Element>(v: &mut vector<Element>) {
-   │                                 ^
-   │                                 │
-   │                                 mutable borrow attempted here
-   │                                 previous local borrow
-66 │         let len = length(v);
-   │                   --------- requirement enforced here
-
-
 ============ disassembled file-format ==================
 // Move bytecode v7
 module 42.vector {

third_party/move/move-compiler-v2/tests/reference-safety/eq_ref.move

@@ -13,6 +13,6 @@ module 0x42::m {
     }
 
     fun mut_ref_id(x: u64) {
-        &mut x == &mut x; // error expected, will be fixed with new reference safety
+        &mut x == &mut x;
     }
 }

third_party/move/move-compiler-v2/tests/reference-safety/freeze.move

@@ -0,0 +1,34 @@
+module 0x42::m {
+
+    fun ref_mut_mut(x: &mut u64, y: &mut u64) {
+        x == y;
+    }
+
+    fun ref_imm_mut(x: &u64, y: &mut u64) {
+        x == y;
+    }
+
+    fun ref_imm_imm(x: &u64, y: &u64) {
+        x == y;
+    }
+
+    fun f1() {
+        let x = 1;
+        let r = &mut x;
+        ref_mut_mut(r, r); // error
+    }
+
+    fun f2() {
+        let x = 1;
+        let r = &mut x;
+        ref_imm_mut(r, r); // error
+    }
+
+    fun f3() {
+        let x = 1;
+        let r = &mut x;
+        ref_imm_imm(r, r); // ok
+    }
+
+
+}

third_party/move/move-compiler-v2/tests/reference-safety/multiple_use_invalid.exp

@@ -1,6 +1,6 @@
 
 Diagnostics:
-error: same mutable reference in value is also used in other value in same argument list
+error: same mutable reference in value is also used in other value in argument list
   ┌─ tests/reference-safety/multiple_use_invalid.move:5:9
   │
 5 │         s(&mut s.x, &mut s.x)
@@ -10,7 +10,7 @@ error: same mutable reference in value is also used in other value in same argum
   │         │ previous mutable field borrow
   │         requirement enforced here
 
-error: same mutable reference in local `r` is also used in other local `x` in same argument list
+error: same mutable reference in local `r` is also used in other local `x` in argument list
    ┌─ tests/reference-safety/multiple_use_invalid.move:11:9
    │
  9 │         let r = &mut s.x;
@@ -19,7 +19,7 @@ error: same mutable reference in local `r` is also used in other local `x` in sa
 11 │         s(r, x)
    │         ^^^^^^^ requirement enforced here
 
-error: same mutable reference in local `r1` is also used in other local `r2` in same argument list
+error: same mutable reference in local `r1` is also used in other local `r2` in argument list
    ┌─ tests/reference-safety/multiple_use_invalid.move:20:9
    │
 18 │         let r1 = &mut x;