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

Issue:
[Bug] [Compiler-V2] Vector indexing bug
# 🐛 Bug

Vector indexing followed by a field selection works unexpected
```move
struct Coin(u64);

fun foo(x: vector<Coin>, index: u64) {
  let p = &mut x[index].0;
  ...
}
```
gives warning
```
Diagnostics:
error: cannot mutably borrow from an immutable ref
   ┌─ ...
   │
42 │         let p = &mut x[index].0;
   │                 ^^^^^^^^^^^^^^^
```


Discussion / hints:

Fix patch:
diff --git a/third_party/move/move-model/src/builder/exp_builder.rs b/third_party/move/move-model/src/builder/exp_builder.rs
index b931eb77adf01..adf8c6c828fbd 100644
--- a/third_party/move/move-model/src/builder/exp_builder.rs
+++ b/third_party/move/move-model/src/builder/exp_builder.rs
@@ -1799,7 +1799,11 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo
                         return call;
                     }
                 }
-                let target_exp = self.translate_exp(exp, &target_ty);
+                let target_exp = if let EA::Exp_::ExpDotted(exp) = &exp.value {
+                    self.translate_dotted(exp, &target_ty, *mutable, context)
+                } else {
+                    self.translate_exp(exp, &target_ty)
+                };
                 if self.subs.specialize(&target_ty).is_reference() {
                     self.error(&loc, "cannot borrow from a reference")
                 }
@@ -3557,12 +3561,17 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo
         } else {
             expected_type.clone()
         };
-        let ref_vec_exp_e = &sp(
-            vec_exp.loc,
-            EA::Exp_::Borrow(mutable, Box::new(vec_exp.clone())),
-        );
+        let (ty, _) = self.translate_exp_free(vec_exp);
+        let ref_vec_exp_e = if ty.is_reference() {
+            vec_exp.clone()
+        } else {
+            sp(
+                vec_exp.loc,
+                EA::Exp_::Borrow(mutable, Box::new(vec_exp.clone())),
+            )
+        };
         let vec_exp_e = self.translate_exp_in_context(
-            ref_vec_exp_e,
+            &ref_vec_exp_e,
             &Type::Reference(
                 ReferenceKind::from_is_mut(mutable),
                 Box::new(Type::Vector(Box::new(inner_ty.clone()))),
@@ -3718,7 +3727,7 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo
         &mut self,
         dotted: &EA::ExpDotted,
         expected_type: &Type,
-        index_mutate: bool,
+        index_mutate: bool, // type of reference for type checking of index expression
         context: &ErrorMessageContext,
     ) -> ExpData {
         match &dotted.value {


Test patch:
diff --git a/aptos-move/framework/aptos-framework/tests/compiler-v2-doc/fungible_asset.md b/aptos-move/framework/aptos-framework/tests/compiler-v2-doc/fungible_asset.md
index 0cac9507ce215..eb352c025f1ce 100644
--- a/aptos-move/framework/aptos-framework/tests/compiler-v2-doc/fungible_asset.md
+++ b/aptos-move/framework/aptos-framework/tests/compiler-v2-doc/fungible_asset.md
@@ -3170,19 +3170,29 @@ Mutate specified fields of the fungible asset's <code><a href="fungible_asset.md
     <b>let</b> mutable_metadata = <b>borrow_global_mut</b>&lt;<a href="fungible_asset.md#0x1_fungible_asset_Metadata">Metadata</a>&gt;(metadata_address);
 
     <b>if</b> (<a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_is_some">option::is_some</a>(&name)){
-        mutable_metadata.name = <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> name);
+        <b>let</b> name = <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> name);
+        <b>assert</b>!(<a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_length">string::length</a>(&name) &lt;= <a href="fungible_asset.md#0x1_fungible_asset_MAX_NAME_LENGTH">MAX_NAME_LENGTH</a>, <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/error.md#0x1_error_out_of_range">error::out_of_range</a>(<a href="fungible_asset.md#0x1_fungible_asset_ENAME_TOO_LONG">ENAME_TOO_LONG</a>));
+        mutable_metadata.name = name;
     };
     <b>if</b> (<a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_is_some">option::is_some</a>(&symbol)){
-        mutable_metadata.symbol = <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> symbol);
+        <b>let</b> symbol = <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> symbol);
+        <b>assert</b>!(<a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_length">string::length</a>(&symbol) &lt;= <a href="fungible_asset.md#0x1_fungible_asset_MAX_SYMBOL_LENGTH">MAX_SYMBOL_LENGTH</a>, <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/error.md#0x1_error_out_of_range">error::out_of_range</a>(<a href="fungible_asset.md#0x1_fungible_asset_ESYMBOL_TOO_LONG">ESYMBOL_TOO_LONG</a>));
+        mutable_metadata.symbol = symbol;
     };
     <b>if</b> (<a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_is_some">option::is_some</a>(&decimals)){
-        mutable_metadata.decimals = <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> decimals);
+        <b>let</b> decimals = <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> decimals);
+        <b>assert</b>!(<a href="fungible_asset.md#0x1_fungible_asset_decimals">decimals</a> &lt;= <a href="fungible_asset.md#0x1_fungible_asset_MAX_DECIMALS">MAX_DECIMALS</a>, <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/error.md#0x1_error_out_of_range">error::out_of_range</a>(<a href="fungible_asset.md#0x1_fungible_asset_EDECIMALS_TOO_LARGE">EDECIMALS_TOO_LARGE</a>));
+        mutable_metadata.decimals = decimals;
     };
     <b>if</b> (<a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_is_some">option::is_some</a>(&icon_uri)){
-        mutable_metadata.icon_uri = <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> icon_uri);
+        <b>let</b> icon_uri = <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> icon_uri);
+        <b>assert</b>!(<a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_length">string::length</a>(&icon_uri) &lt;= <a href="fungible_asset.md#0x1_fungible_asset_MAX_URI_LENGTH">MAX_URI_LENGTH</a>, <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/error.md#0x1_error_out_of_range">error::out_of_range</a>(<a href="fungible_asset.md#0x1_fungible_asset_EURI_TOO_LONG">EURI_TOO_LONG</a>));
+        mutable_metadata.icon_uri = icon_uri;
     };
     <b>if</b> (<a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_is_some">option::is_some</a>(&project_uri)){
-        mutable_metadata.project_uri = <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> project_uri);
+        <b>let</b> project_uri = <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> project_uri);
+        <b>assert</b>!(<a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_length">string::length</a>(&project_uri) &lt;= <a href="fungible_asset.md#0x1_fungible_asset_MAX_URI_LENGTH">MAX_URI_LENGTH</a>, <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/error.md#0x1_error_out_of_range">error::out_of_range</a>(<a href="fungible_asset.md#0x1_fungible_asset_EURI_TOO_LONG">EURI_TOO_LONG</a>));
+        mutable_metadata.project_uri = project_uri;
     };
 }
 </code></pre>
diff --git a/third_party/move/move-compiler-v2/tests/file-format-generator/index_then_field_select.exp b/third_party/move/move-compiler-v2/tests/file-format-generator/index_then_field_select.exp
new file mode 100644
index 0000000000000..44a3e29f4bebb
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/file-format-generator/index_then_field_select.exp
@@ -0,0 +1,58 @@
+
+============ disassembled file-format ==================
+// Move bytecode v7
+module 42.test {
+struct Coin has copy, drop {
+	_0: u256
+}
+
+inc_vec_new(Arg0: &mut vector<u256>, Arg1: u64) /* def_idx: 0 */ {
+B0:
+	0: CopyLoc[0](Arg0: &mut vector<u256>)
+	1: FreezeRef
+	2: CopyLoc[1](Arg1: u64)
+	3: VecImmBorrow(2)
+	4: ReadRef
+	5: LdU256(1)
+	6: Add
+	7: MoveLoc[0](Arg0: &mut vector<u256>)
+	8: MoveLoc[1](Arg1: u64)
+	9: VecMutBorrow(2)
+	10: WriteRef
+	11: Ret
+}
+test0(Arg0: &mut vector<Coin>, Arg1: u64) /* def_idx: 1 */ {
+L2:	loc0: &mut u256
+B0:
+	0: MoveLoc[0](Arg0: &mut vector<Coin>)
+	1: MoveLoc[1](Arg1: u64)
+	2: VecMutBorrow(4)
+	3: MutBorrowField[0](Coin._0: u256)
+	4: StLoc[2](loc0: &mut u256)
+	5: MoveLoc[2](loc0: &mut u256)
+	6: Pop
+	7: Ret
+}
+test1(Arg0: vector<Coin>, Arg1: u64) /* def_idx: 2 */ {
+L2:	loc0: &mut u256
+B0:
+	0: MutBorrowLoc[0](Arg0: vector<Coin>)
+	1: MoveLoc[1](Arg1: u64)
+	2: VecMutBorrow(4)
+	3: MutBorrowField[0](Coin._0: u256)
+	4: StLoc[2](loc0: &mut u256)
+	5: MoveLoc[2](loc0: &mut u256)
+	6: Pop
+	7: Ret
+}
+test3(Arg0: &vector<Coin>, Arg1: u64) /* def_idx: 3 */ {
+B0:
+	0: MoveLoc[0](Arg0: &vector<Coin>)
+	1: MoveLoc[1](Arg1: u64)
+	2: VecImmBorrow(4)
+	3: ReadRef
+	4: Pop
+	5: Ret
+}
+}
+============ bytecode verification succeeded ========
diff --git a/third_party/move/move-compiler-v2/tests/file-format-generator/index_then_field_select.move b/third_party/move/move-compiler-v2/tests/file-format-generator/index_then_field_select.move
new file mode 100644
index 0000000000000..b337b051d183e
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/file-format-generator/index_then_field_select.move
@@ -0,0 +1,19 @@
+module 0x42::test {
+    struct Coin(u256) has drop, copy;
+
+    fun test0(x: &mut vector<Coin>, index: u64) {
+        let _p = &mut x[index].0;
+    }
+
+    fun test1(x: vector<Coin>, index: u64) {
+        let _p = &mut x[index].0;
+    }
+
+    fun test3(x: &vector<Coin>, index: u64) {
+        let _p = x[index];
+    }
+
+    fun inc_vec_new(x: &mut vector<u256>, index: u64) {
+        x[index] = x[index] + 1;
+    }
+}
diff --git a/third_party/move/move-compiler-v2/tests/file-format-generator/index_then_field_select.opt.exp b/third_party/move/move-compiler-v2/tests/file-format-generator/index_then_field_select.opt.exp
new file mode 100644
index 0000000000000..90386c16ab2f7
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/file-format-generator/index_then_field_select.opt.exp
@@ -0,0 +1,54 @@
+
+============ disassembled file-format ==================
+// Move bytecode v7
+module 42.test {
+struct Coin has copy, drop {
+	_0: u256
+}
+
+inc_vec_new(Arg0: &mut vector<u256>, Arg1: u64) /* def_idx: 0 */ {
+B0:
+	0: CopyLoc[0](Arg0: &mut vector<u256>)
+	1: FreezeRef
+	2: CopyLoc[1](Arg1: u64)
+	3: VecImmBorrow(2)
+	4: ReadRef
+	5: LdU256(1)
+	6: Add
+	7: MoveLoc[0](Arg0: &mut vector<u256>)
+	8: MoveLoc[1](Arg1: u64)
+	9: VecMutBorrow(2)
+	10: WriteRef
+	11: Ret
+}
+test0(Arg0: &mut vector<Coin>, Arg1: u64) /* def_idx: 1 */ {
+L2:	loc0: &mut u256
+B0:
+	0: MoveLoc[0](Arg0: &mut vector<Coin>)
+	1: MoveLoc[1](Arg1: u64)
+	2: VecMutBorrow(4)
+	3: MutBorrowField[0](Coin._0: u256)
+	4: Pop
+	5: Ret
+}
+test1(Arg0: vector<Coin>, Arg1: u64) /* def_idx: 2 */ {
+L2:	loc0: &mut u256
+B0:
+	0: MutBorrowLoc[0](Arg0: vector<Coin>)
+	1: MoveLoc[1](Arg1: u64)
+	2: VecMutBorrow(4)
+	3: MutBorrowField[0](Coin._0: u256)
+	4: Pop
+	5: Ret
+}
+test3(Arg0: &vector<Coin>, Arg1: u64) /* def_idx: 3 */ {
+B0:
+	0: MoveLoc[0](Arg0: &vector<Coin>)
+	1: MoveLoc[1](Arg1: u64)
+	2: VecImmBorrow(4)
+	3: ReadRef
+	4: Pop
+	5: Ret
+}
+}
+============ bytecode verification succeeded ========
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/index.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/index.exp
index c40f8232f0285..996ef686af3f8 100644
--- a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/index.exp
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/index.exp
@@ -1,1 +1,1 @@
-processed 21 tasks
+processed 25 tasks
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/index.move b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/index.move
index 1129ee1d16605..c365bd45a1e3a 100644
--- a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/index.move
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/index.move
@@ -227,6 +227,47 @@ module 0x42::test {
         bubble_sort(v);
     }
 
+    fun test_index_then_field_select_1() {
+        let x1 = X {
+            value: true
+        };
+        let v = vector[x1];
+        let p = &mut v[0].value;
+        *p = false;
+        assert!(v[0].value == false, 0);
+    }
+
+    fun test_index_then_field_select_2() {
+        let x1 = X {
+            value: true
+        };
+        let v = &mut vector[x1];
+        let p = &mut v[0].value;
+        *p = false;
+        assert!(v[0].value == false, 0);
+    }
+
+    fun test_index_then_field_select_3() {
+        let x1 = X {
+            value: true
+        };
+        let v = &vector[x1];
+        assert!(v[0].value == true, 0);
+    }
+
+    fun inc_vec_new(x: &mut vector<u256>, index: u64) {
+        x[index] = x[index] + 1;
+    }
+
+    fun inc_vec_new_test() {
+        let x = vector[0];
+        x[0] = x[0] + 1;
+        assert!(x[0] == 1, 0);
+        let y = &mut x;
+        inc_vec_new(y, 0);
+        assert!(y[0] == 2, 0);
+    }
+
 }
 
 //# run --verbose --signers 0x1 -- 0x42::test::init
@@ -268,3 +309,11 @@ module 0x42::test {
 //# run --verbose -- 0x42::test::test_resource_with_vector
 
 //# run --verbose -- 0x42::test::call_sort
+
+//# run --verbose -- 0x42::test::test_index_then_field_select_1
+
+//# run --verbose -- 0x42::test::test_index_then_field_select_2
+
+//# run --verbose -- 0x42::test::test_index_then_field_select_3
+
+//# run --verbose -- 0x42::test::inc_vec_new_test