Repository: aptos-labs/aptos-core (language: python)

Issue:
[Bug][Compiler-v2] STLOC_TYPE_MISMATCH_ERROR when the return type is a tuple containing mutable refs while the target type is immutable ref
# 🐛 Bug

When compiling the following code:

```
fun t2(u1: &mut u64, u2: &mut u64): (&mut u64, &mut u64) {
        (u1, u2)
    }
    fun test_4() {
         let x: &u64;
         let y: &u64;
         (x, y) = t2(&mut 3, &mut 4);
   }
```

The bytecode verifier will generate an error with the status code `STLOC_TYPE_MISMATCH_ERROR`. 




Discussion / hints:
When compiling the following code:

```
    public fun borrow_mut<Element>(
        v: &mut Element,
    ): &Element {
        v
    }
```

The following bytecode verification error will be generated:

```
   ┌─ tests/file-format-generator/simple_set.move:1:1
   │
 1 │ ╭ module 0x42::simple_set {
 2 │ │     // use std::vector;
 3 │ │
 4 │ │     // public fun borrow_mut<Element>(
   · │
14 │ │
15 │ │ }
   │ ╰─^ ICE failed bytecode verifier: VMError {
    major_status: STLOC_TYPE_MISMATCH_ERROR,
    sub_status: None,
    message: None,
    exec_state: None,
    location: Module(
        ModuleId {
            address: 0000000000000000000000000000000000000000000000000000000000000042,
            name: Identifier(
                "simple_set",
            ),
        },
    ),
    indices: [
        (
            FunctionDefinition,
            0,
        ),
    ],
    offsets: [
        (
            FunctionDefinitionIndex(0),
            1,
        ),
    ],
}
```





Here is another case that the PR #12525 does not handle:

```
    fun t2(u1: &mut u64, u2: &mut u64): (&mut u64, &mut u64) {
        (u1, u2)
    }

    fun test_4() {
         let x: &u64;
         let y: &u64;
         (x, y) = t2(&mut 3, &mut 4);
   }
```


Fix patch:
diff --git a/third_party/move/move-compiler-v2/src/bytecode_generator.rs b/third_party/move/move-compiler-v2/src/bytecode_generator.rs
index dc810d8eb6f17..785fa528b3396 100644
--- a/third_party/move/move-compiler-v2/src/bytecode_generator.rs
+++ b/third_party/move/move-compiler-v2/src/bytecode_generator.rs
@@ -564,16 +564,42 @@ impl<'env> Generator<'env> {
 impl<'env> Generator<'env> {
     fn gen_local(&mut self, targets: Vec<TempIndex>, id: NodeId, name: Symbol) {
         let target = self.require_unary_target(id, targets);
-        let attr = self.new_loc_attr(id);
         let temp = self.find_local(id, name);
-        self.emit(Bytecode::Assign(attr, target, temp, AssignKind::Inferred));
+        self.emit_assign_with_convert(id, target, temp, AssignKind::Inferred)
     }
 
     fn gen_temporary(&mut self, targets: Vec<TempIndex>, id: NodeId, temp: TempIndex) {
         let target = self.require_unary_target(id, targets);
-        self.emit_with(id, |attr| {
-            Bytecode::Assign(attr, target, temp, AssignKind::Inferred)
-        })
+        self.emit_assign_with_convert(id, target, temp, AssignKind::Inferred)
+    }
+
+    fn emit_assign_with_convert(
+        &mut self,
+        id: NodeId,
+        target: TempIndex,
+        mut temp: TempIndex,
+        kind: AssignKind,
+    ) {
+        let temp_ty = self.temp_type(temp).clone();
+        let target_ty = self.temp_type(target).clone();
+        if let Some((new_temp, oper)) = self.get_conversion(&target_ty, &temp_ty) {
+            self.emit_call(id, vec![new_temp], oper, vec![temp]);
+            temp = new_temp
+        }
+        self.emit_with(id, |attr| Bytecode::Assign(attr, target, temp, kind))
+    }
+
+    fn get_conversion(
+        &mut self,
+        expected_type: &Type,
+        actual_type: &Type,
+    ) -> Option<(TempIndex, BytecodeOperation)> {
+        if actual_type.is_mutable_reference() && expected_type.is_immutable_reference() {
+            let new_temp = self.new_temp(actual_type.clone());
+            Some((new_temp, BytecodeOperation::FreezeRef(false)))
+        } else {
+            None
+        }
     }
 }
 
@@ -725,7 +751,7 @@ impl<'env> Generator<'env> {
                 } else {
                     AssignKind::Move
                 };
-                self.emit_with(id, |attr| Bytecode::Assign(attr, target, arg, assign_kind))
+                self.emit_assign_with_convert(id, target, arg, assign_kind)
             },
             Operation::Borrow(kind) => {
                 let target = self.require_unary_target(id, targets);
@@ -943,9 +969,8 @@ impl<'env> Generator<'env> {
             .unwrap_or_default();
         // Function calls can have implicit conversion of &mut to &, need to compute implicit
         // conversions.
-        let param_types: Vec<Type> = self
-            .env()
-            .get_function(fun)
+        let fun_env = self.env().get_function(fun);
+        let param_types: Vec<Type> = fun_env
             .get_parameters()
             .into_iter()
             .map(|Parameter(_, ty, _)| ty.instantiate(&type_args))
@@ -960,19 +985,36 @@ impl<'env> Generator<'env> {
             .map(|(e, t)| self.maybe_convert(e, &t))
             .collect::<Vec<_>>();
         let args = self.gen_arg_list(&args);
+        // Targets may need conversion as well.
+        let fun_env = self.env().get_function(fun);
+        let mut actual_targets = vec![];
+        let mut conversion_ops = vec![];
+        for (target, actual_type) in targets.iter().zip(fun_env.get_result_type().flatten()) {
+            let target_ty = self.temp_type(*target).clone();
+            if let Some((new_target, oper)) = self.get_conversion(&target_ty, &actual_type) {
+                // Conversion required
+                actual_targets.push(new_target);
+                conversion_ops.push((*target, oper, new_target))
+            } else {
+                actual_targets.push(*target)
+            }
+        }
         self.emit_with(id, |attr| {
             Bytecode::Call(
                 attr,
-                targets,
+                actual_targets,
                 BytecodeOperation::Function(fun.module_id, fun.id, type_args),
                 args,
                 None,
             )
-        })
+        });
+        for (target, oper, new_target) in conversion_ops {
+            self.emit_call(id, vec![target], oper, vec![new_target])
+        }
     }
 
     /// Convert the expression so it matches the expected type. This is currently only needed
-    /// for `&mut` to `&` conversion, in which case we need to to introduce a Freeze operation.
+    /// for `&mut` to `&` conversion, in which case we need to introduce a Freeze operation.
     fn maybe_convert(&self, exp: &Exp, expected_ty: &Type) -> Exp {
         let id = exp.node_id();
         let exp_ty = self.get_node_type(id);
@@ -1730,17 +1772,13 @@ impl<'env> Generator<'env> {
                 let pat_ty = self.env().get_node_type(*id);
                 if !match_mode.is_probing() {
                     let temp = self.new_temp(pat_ty);
-                    self.emit_with(*id, |attr| {
-                        Bytecode::Assign(attr, temp, values[0], AssignKind::Inferred)
-                    })
+                    self.emit_assign_with_convert(*id, temp, values[0], AssignKind::Inferred)
                 }
             },
             Pattern::Var(var_id, sym) => {
                 if match_mode.shall_bind_var(*sym) {
                     let local = self.find_local_for_pattern(*var_id, *sym, next_scope);
-                    self.emit_with(id, |attr| {
-                        Bytecode::Assign(attr, local, values[0], AssignKind::Inferred)
-                    })
+                    self.emit_assign_with_convert(id, local, values[0], AssignKind::Inferred)
                 }
             },
             Pattern::Struct(id, str, variant, args) => {


Test patch:
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.exp b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.exp
deleted file mode 100644
index 209cbc1c02212..0000000000000
--- a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.exp
+++ /dev/null
@@ -1,23 +0,0 @@
-
-Diagnostics:
-warning: Unused assignment to `x`. Consider removing or prefixing with an underscore: `_x`
-  ┌─ tests/reference-safety/bug_12394.move:9:18
-  │
-9 │         (x, y) = t2(&mut 3, &mut 4);
-  │                  ^^^^^^^^^^^^^^^^^^
-
-warning: Unused assignment to `y`. Consider removing or prefixing with an underscore: `_y`
-  ┌─ tests/reference-safety/bug_12394.move:9:18
-  │
-9 │         (x, y) = t2(&mut 3, &mut 4);
-  │                  ^^^^^^^^^^^^^^^^^^
-
-
-============ bytecode verification failed ========
-
-Diagnostics:
-bug: bytecode verification failed with unexpected status code `STLOC_TYPE_MISMATCH_ERROR`. This is a compiler bug, consider reporting it.
-  ┌─ tests/reference-safety/bug_12394.move:9:18
-  │
-9 │         (x, y) = t2(&mut 3, &mut 4);
-  │                  ^^^^^^^^^^^^^^^^^^
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.move b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.move
deleted file mode 100644
index 371ce0b5d3e63..0000000000000
--- a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.move
+++ /dev/null
@@ -1,11 +0,0 @@
-// TODO(#12394): after fix, rename file to reflect issue (`bug_nnn.move` to `bug_nnn_<issue>.move`)
-module 0x815::m {
-    fun t2(u1: &mut u64, u2: &mut u64): (&mut u64, &mut u64) {
-        (u1, u2)
-    }
-    fun test_4() {
-        let x: &u64;
-        let y: &u64;
-        (x, y) = t2(&mut 3, &mut 4);
-    }
-}
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.no-opt.exp b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.no-opt.exp
deleted file mode 100644
index 209cbc1c02212..0000000000000
--- a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.no-opt.exp
+++ /dev/null
@@ -1,23 +0,0 @@
-
-Diagnostics:
-warning: Unused assignment to `x`. Consider removing or prefixing with an underscore: `_x`
-  ┌─ tests/reference-safety/bug_12394.move:9:18
-  │
-9 │         (x, y) = t2(&mut 3, &mut 4);
-  │                  ^^^^^^^^^^^^^^^^^^
-
-warning: Unused assignment to `y`. Consider removing or prefixing with an underscore: `_y`
-  ┌─ tests/reference-safety/bug_12394.move:9:18
-  │
-9 │         (x, y) = t2(&mut 3, &mut 4);
-  │                  ^^^^^^^^^^^^^^^^^^
-
-
-============ bytecode verification failed ========
-
-Diagnostics:
-bug: bytecode verification failed with unexpected status code `STLOC_TYPE_MISMATCH_ERROR`. This is a compiler bug, consider reporting it.
-  ┌─ tests/reference-safety/bug_12394.move:9:18
-  │
-9 │         (x, y) = t2(&mut 3, &mut 4);
-  │                  ^^^^^^^^^^^^^^^^^^
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.old.exp b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.old.exp
deleted file mode 100644
index 209cbc1c02212..0000000000000
--- a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394.old.exp
+++ /dev/null
@@ -1,23 +0,0 @@
-
-Diagnostics:
-warning: Unused assignment to `x`. Consider removing or prefixing with an underscore: `_x`
-  ┌─ tests/reference-safety/bug_12394.move:9:18
-  │
-9 │         (x, y) = t2(&mut 3, &mut 4);
-  │                  ^^^^^^^^^^^^^^^^^^
-
-warning: Unused assignment to `y`. Consider removing or prefixing with an underscore: `_y`
-  ┌─ tests/reference-safety/bug_12394.move:9:18
-  │
-9 │         (x, y) = t2(&mut 3, &mut 4);
-  │                  ^^^^^^^^^^^^^^^^^^
-
-
-============ bytecode verification failed ========
-
-Diagnostics:
-bug: bytecode verification failed with unexpected status code `STLOC_TYPE_MISMATCH_ERROR`. This is a compiler bug, consider reporting it.
-  ┌─ tests/reference-safety/bug_12394.move:9:18
-  │
-9 │         (x, y) = t2(&mut 3, &mut 4);
-  │                  ^^^^^^^^^^^^^^^^^^
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.exp b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.exp
new file mode 100644
index 0000000000000..90b32906711cc
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.exp
@@ -0,0 +1,2 @@
+
+============ bytecode verification succeeded ========
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.move b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.move
new file mode 100644
index 0000000000000..5c58c89b52d6b
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.move
@@ -0,0 +1,27 @@
+module 0x815::m {
+    fun t2(u1: &mut u64, u2: &mut u64): (&mut u64, &mut u64) {
+        (u1, u2)
+    }
+    fun test_4() {
+        let _x: &u64;
+        let _y: &u64;
+        (_x, _y) = t2(&mut 3, &mut 4);
+    }
+
+    fun test_5() {
+        let (_x, _y): (&u64, &u64) = t2(&mut 3, &mut 4);
+    }
+
+    fun foo(x: &mut u64): &u64 {
+        *x = 2;
+        x
+    }
+
+    fun bar(x: &u64): &u64 {
+        x
+    }
+
+    public fun foobar(x: u64): u64 {
+        *bar(foo(&mut x))
+    }
+}
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.no-opt.exp b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.no-opt.exp
new file mode 100644
index 0000000000000..90b32906711cc
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.no-opt.exp
@@ -0,0 +1,2 @@
+
+============ bytecode verification succeeded ========
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.old.exp b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.old.exp
new file mode 100644
index 0000000000000..90b32906711cc
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/bug_12394_missing_freeze.old.exp
@@ -0,0 +1,2 @@
+
+============ bytecode verification succeeded ========
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.exp b/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.exp
index cbe6b5a9470a2..acfff9c2a486e 100644
--- a/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.exp
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.exp
@@ -1,15 +1,9 @@
 
+============ bytecode verification failed ========
+
 Diagnostics:
-error: cannot immutably borrow since mutable references exist
-   ┌─ tests/reference-safety/immu_borrow_bug.move:15:9
+bug: bytecode verification failed with unexpected status code `STLOC_TYPE_MISMATCH_ERROR`. This is a compiler bug, consider reporting it.
+   ┌─ tests/reference-safety/immu_borrow_bug.move:14:9
    │
 14 │         common.x = 15;
-   │         --------
-   │         │
-   │         used by field borrow
-   │         previous mutable local borrow
-15 │         common.x
-   │         ^^^^^^--
-   │         │
-   │         requirement enforced here
-   │         immutable borrow attempted here
+   │         ^^^^^^^^
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.no-opt.exp b/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.no-opt.exp
index cbe6b5a9470a2..acfff9c2a486e 100644
--- a/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.no-opt.exp
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.no-opt.exp
@@ -1,15 +1,9 @@
 
+============ bytecode verification failed ========
+
 Diagnostics:
-error: cannot immutably borrow since mutable references exist
-   ┌─ tests/reference-safety/immu_borrow_bug.move:15:9
+bug: bytecode verification failed with unexpected status code `STLOC_TYPE_MISMATCH_ERROR`. This is a compiler bug, consider reporting it.
+   ┌─ tests/reference-safety/immu_borrow_bug.move:14:9
    │
 14 │         common.x = 15;
-   │         --------
-   │         │
-   │         used by field borrow
-   │         previous mutable local borrow
-15 │         common.x
-   │         ^^^^^^--
-   │         │
-   │         requirement enforced here
-   │         immutable borrow attempted here
+   │         ^^^^^^^^
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.old.exp b/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.old.exp
new file mode 100644
index 0000000000000..cbe6b5a9470a2
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/immu_borrow_bug.old.exp
@@ -0,0 +1,15 @@
+
+Diagnostics:
+error: cannot immutably borrow since mutable references exist
+   ┌─ tests/reference-safety/immu_borrow_bug.move:15:9
+   │
+14 │         common.x = 15;
+   │         --------
+   │         │
+   │         used by field borrow
+   │         previous mutable local borrow
+15 │         common.x
+   │         ^^^^^^--
+   │         │
+   │         requirement enforced here
+   │         immutable borrow attempted here
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.exp b/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.exp
index 1067c6c40aa2a..90b32906711cc 100644
--- a/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.exp
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.exp
@@ -1,9 +1,2 @@
 
-============ bytecode verification failed ========
-
-Diagnostics:
-bug: bytecode verification failed with unexpected status code `STLOC_TYPE_MISMATCH_ERROR`. This is a compiler bug, consider reporting it.
-   ┌─ tests/reference-safety/v1-tests/borrow_global.move:29:27
-   │
-29 │         let r1; if (cond) r1 = borrow_global<R>(addr) else r1 = &mut r; // See #14274
-   │                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+============ bytecode verification succeeded ========
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.no-opt.exp b/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.no-opt.exp
index e7259b3356e93..90b32906711cc 100644
--- a/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.no-opt.exp
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.no-opt.exp
@@ -1,9 +1,2 @@
 
-============ bytecode verification failed ========
-
-Diagnostics:
-bug: bytecode verification failed with unexpected status code `STLOC_TYPE_MISMATCH_ERROR`. This is a compiler bug, consider reporting it.
-   ┌─ tests/reference-safety/v1-tests/borrow_global.move:29:60
-   │
-29 │         let r1; if (cond) r1 = borrow_global<R>(addr) else r1 = &mut r; // See #14274
-   │                                                            ^^^^^^^^^^^
+============ bytecode verification succeeded ========
diff --git a/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.old.exp b/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.old.exp
index 4c1ae98145b4e..c50e3db5b12dc 100644
--- a/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.old.exp
+++ b/third_party/move/move-compiler-v2/tests/reference-safety/v1-tests/borrow_global.old.exp
@@ -12,3 +12,12 @@ error: cannot immutably borrow since mutable references exist
   │                  ^^^^^^^^^^^^^^^^^^^^^^ immutable borrow attempted here
 7 │         f == &r1.f
   │         ---------- requirement enforced here
+
+error: cannot implicitly freeze value since other mutable usages for this reference exist
+   ┌─ tests/reference-safety/v1-tests/borrow_global.move:29:60
+   │
+29 │         let r1; if (cond) r1 = borrow_global<R>(addr) else r1 = &mut r; // See #14274
+   │                                                            ^^^^^^^^^^^
+   │                                                            │
+   │                                                            implicitly frozen here
+   │                                                            used here
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_12394_missing_freeze.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_12394_missing_freeze.exp
new file mode 100644
index 0000000000000..3925e304f708b
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_12394_missing_freeze.exp
@@ -0,0 +1,25 @@
+comparison between v1 and v2 failed:
+- processed 2 tasks
++ processed 2 tasks
+= 
+- task 0 'publish'. lines 1-8:
+- Error: error[E07002]: mutable ownership violated
+-   ┌─ TEMPFILE:5:45
+-   │
+- 5 │         let (x, y): (&u64, &u64) = (&mut a, freeze(&mut a)); // bug in v1, works in v2
+-   │                                     ------  ^^^^^^^^^^^^^^ Invalid freeze.
+-   │                                     │        
+-   │                                     It is still being mutably borrowed by this reference
+- 
+- 
+- 
+= task 1 'run'. lines 10-10:
+- Error: Function execution failed with VMError: {
+-     major_status: LINKER_ERROR,
+-     sub_status: None,
+-     location: undefined,
+-     indices: redacted,
+-     offsets: redacted,
+- }
++ return values: 2
+= 
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_12394_missing_freeze.move b/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_12394_missing_freeze.move
new file mode 100644
index 0000000000000..39c821a87acbf
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/misc/bug_12394_missing_freeze.move
@@ -0,0 +1,10 @@
+//# publish
+module 0xc0ffee::m {
+    public fun test(): u64 {
+        let a = 1;
+        let (x, y): (&u64, &u64) = (&mut a, freeze(&mut a)); // bug in v1, works in v2
+        *x + *y
+    }
+}
+
+//# run 0xc0ffee::m::test