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

Issue:
[Bug][move-compiler-v2] ExpRewriterFunctions handling of Match isn't quite right
# 🐛 Bug

The `ExpRewriterFunctions::rewrite_exp_descent` handling of `Match` needs to call `rewrite_exit_scope` after calling `internal_rewrite_exp`.  Compare the code for `Block` with that for `Match`:
```
            Block(id, pat, binding, body) => {
                ...
                let (body_changed, new_body) = self.internal_rewrite_exp(body);
                self.rewrite_exit_scope(new_id);
                let newer_pat = if let Some(rewritten_pat) = optional_pat {
                ...
```
with
```
            Match(id, disc, arms) => {
                ...
                    let (body_changed, new_body) = self.internal_rewrite_exp(&arm.body);
                    new_arms.push(MatchArm {
```

The `Match` handler also ignores the result value when it calls `self.rewrite_enter_block_scope()`.  (Compare with `optional_pat = ...` in the `Body` clause).  Apparently that functionality was never use, as I see no current implementation of `ExpRewriterFunctions::rewrite_enter_block_scope()` returning anything other than `None`.  So we should simplify `rewrite_enter_block_scope` to not return a value and also simplify the `Block` clause.

The following example fails to generate the correct code due to the bug:
```
    /// Check for scoping issues
    public fun check_scoping(self: &Inner): u64 {
        let x = 3;
        {
            let x = 4;
            {
                match (self) {
                    Inner1{x: _} => true,
                    _ => false
                };
            };
        };
        x
    }
```

With the current code, the Simplifier causes this to yield code returning 4 instead of 3:
```
[variant baseline]
public fun m::check_scoping($t0: &m::Inner): u64 {
     var $t1: u64
     var $t2: bool
     var $t3: bool
     var $t4: &m::Inner
     var $t5: u64
  0: $t3 := test_variant m::Inner::Inner1($t0)
  1: if ($t3) goto 2 else goto 5
  2: label L2
  3: $t2 := true
  4: goto 12
  5: label L1
  6: $t4 := infer($t0)
  7: $t2 := false
  8: goto 12
  9: label L3
 10: $t5 := 15621340914461310977
 11: abort($t5)
 12: label L0
 13: $t1 := 4
 14: return $t1
}
```




Discussion / hints:
@wrwg may want to fix this.


Fix patch:
diff --git a/third_party/move/move-model/src/exp_rewriter.rs b/third_party/move/move-model/src/exp_rewriter.rs
index 2221274b43b5d..21b33132f475f 100644
--- a/third_party/move/move-model/src/exp_rewriter.rs
+++ b/third_party/move/move-model/src/exp_rewriter.rs
@@ -7,13 +7,12 @@ use crate::{
         Condition, Exp, ExpData, MatchArm, MemoryLabel, Operation, Pattern, Spec, SpecBlockTarget,
         TempIndex, Value,
     },
-    model::{GlobalEnv, ModuleId, NodeId, SpecVarId},
+    model::{GlobalEnv, Loc, ModuleId, NodeId, SpecVarId},
     symbol::Symbol,
     ty::Type,
 };
 use codespan_reporting::diagnostic::Severity;
 use itertools::Itertools;
-use log::trace;
 use std::collections::{BTreeMap, BTreeSet};
 
 /// Rewriter for expressions, allowing to substitute locals by expressions as well as instantiate
@@ -224,6 +223,18 @@ pub trait ExpRewriterFunctions {
     ) -> Option<Exp> {
         None
     }
+    // Note that `rewrite_match_arm` is called *after* `rewrite_exit_scope`.
+    // (So all parameters here have already been processed.)
+    fn rewrite_match_arm(
+        &mut self,
+        id: NodeId, // id of the parent match expression
+        loc: &Loc,
+        pat: &Pattern,
+        condition: &Option<Exp>,
+        body: &Exp,
+    ) -> Option<MatchArm> {
+        None
+    }
     // Optionally rewrite a pattern, which may be in `Let`, `Lambda`, or `Assign` expression.
     //
     // Parameter`creating_scope` is `true` for `Let` and `Lambda` operations, which create a new
@@ -363,26 +374,14 @@ pub trait ExpRewriterFunctions {
                 } else {
                     (false, None)
                 };
-                let (mut pat_changed, new_pat) = self.internal_rewrite_pattern(pat, true);
+                let (pat_changed, new_pat) = self.internal_rewrite_pattern(pat, true);
                 let optional_pat = self.rewrite_enter_block_scope(new_id, &new_pat, &new_binding);
                 let (body_changed, new_body) = self.internal_rewrite_exp(body);
                 self.rewrite_exit_scope(new_id);
-                let newer_pat = if let Some(rewritten_pat) = optional_pat {
-                    pat_changed = true;
-                    trace!(
-                        "Node {} Pat changed from {:#?} to  {:#?}",
-                        id.as_usize(),
-                        &new_pat,
-                        &rewritten_pat,
-                    );
-                    rewritten_pat
+                let (pat_changed, newer_pat) = if let Some(rewritten_pat) = optional_pat {
+                    (true, rewritten_pat)
                 } else {
-                    trace!(
-                        "Node {} Pat unchanged {:#?} unchanged",
-                        id.as_usize(),
-                        &new_pat,
-                    );
-                    new_pat
+                    (pat_changed, new_pat)
                 };
                 if let Some(new_exp) =
                     self.rewrite_block(new_id, &newer_pat, &new_binding, &new_body)
@@ -457,24 +456,39 @@ pub trait ExpRewriterFunctions {
             Match(id, disc, arms) => {
                 let (id_changed, new_id) = self.internal_rewrite_id(*id);
                 let (disc_changed, new_disc) = self.internal_rewrite_exp(disc);
+
                 let (mut arms_changed, mut new_arms) = (false, vec![]);
                 for arm in arms {
                     let (pat_changed, new_pat) = self.internal_rewrite_pattern(&arm.pattern, true);
-                    self.rewrite_enter_block_scope(new_id, &new_pat, &None);
+                    let optional_pat = self.rewrite_enter_block_scope(new_id, &new_pat, &None);
                     let (cond_changed, new_cond) = if let Some(c) = &arm.condition {
                         let (c, e) = self.internal_rewrite_exp(c);
                         (c, Some(e))
                     } else {
-                        (false, arm.condition.clone())
+                        (false, None)
                     };
                     let (body_changed, new_body) = self.internal_rewrite_exp(&arm.body);
-                    new_arms.push(MatchArm {
-                        loc: arm.loc.clone(),
-                        pattern: new_pat,
-                        condition: new_cond,
-                        body: new_body,
-                    });
-                    arms_changed = arms_changed || pat_changed || cond_changed || body_changed;
+                    self.rewrite_exit_scope(new_id);
+                    let (pat_changed, newer_pat) = if let Some(rewritten_pat) = optional_pat {
+                        (true, rewritten_pat)
+                    } else {
+                        (pat_changed, new_pat)
+                    };
+                    let (arm_changed, new_arm) = if let Some(new_exp) =
+                        self.rewrite_match_arm(new_id, &arm.loc, &newer_pat, &new_cond, &new_body)
+                    {
+                        (true, new_exp)
+                    } else {
+                        (false, MatchArm {
+                            loc: arm.loc.clone(),
+                            pattern: newer_pat,
+                            condition: new_cond,
+                            body: new_body,
+                        })
+                    };
+                    new_arms.push(new_arm);
+                    arms_changed =
+                        arms_changed || arm_changed || pat_changed || cond_changed || body_changed;
                 }
                 if id_changed || disc_changed || arms_changed {
                     Match(*id, new_disc, new_arms).into_exp()


Test patch:
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.move b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.move
new file mode 100644
index 0000000000000..8f557ba91dbec
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.move
@@ -0,0 +1,45 @@
+//# publish
+module 0x42::m {
+
+    enum Inner has drop {
+        Inner1{ x: u64 }
+        Inner2{ x: u64, y: u64 }
+    }
+
+    struct Box has drop {
+        x: u64
+    }
+
+    enum Outer has drop {
+        None,
+        One{i: Inner},
+        Two{i: Inner, b: Box},
+    }
+
+    /// Check for enum scoping bug;
+    /// result should be 3, not 4.
+    public fun check_scoping(self: &Inner): u64 {
+        let x = 3;
+        {
+            let x = 4;
+            {
+                match (self) {
+                    Inner1{x: _} => true,
+                    _ => false
+                };
+            };
+        };
+        x
+    }
+
+    fun t1_check_scoping(): u64 {
+        let o = Outer::One{i: Inner::Inner1{x: 43}};
+        match (o) {
+            None => 0,
+            One{i} => i.check_scoping(),
+            Two{i, b} => 3,
+        }
+    }
+}
+
+//# run 0x42::m::t1_check_scoping
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.no-optimize.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.no-optimize.exp
new file mode 100644
index 0000000000000..a152b902ea216
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.no-optimize.exp
@@ -0,0 +1,43 @@
+processed 2 tasks
+
+task 0 'publish'. lines 1-43:
+warning: Unused local variable `x`. Consider removing or prefixing with an underscore: `_x`
+   ┌─ TEMPFILE:20:17
+   │
+20 │             let x = 4;
+   │                 ^
+
+warning: Unused local variable `i`. Consider removing or prefixing with an underscore: `_i`
+   ┌─ TEMPFILE:35:17
+   │
+35 │             Two{i, b} => 3,
+   │                 ^
+
+warning: Unused local variable `b`. Consider removing or prefixing with an underscore: `_b`
+   ┌─ TEMPFILE:35:20
+   │
+35 │             Two{i, b} => 3,
+   │                    ^
+
+warning: Unused assignment to `x`. Consider removing or prefixing with an underscore: `_x`
+   ┌─ TEMPFILE:20:21
+   │
+20 │             let x = 4;
+   │                     ^
+
+warning: Unused assignment to `b`. Consider removing or prefixing with an underscore: `_b`
+   ┌─ TEMPFILE:35:13
+   │
+35 │             Two{i, b} => 3,
+   │             ^^^^^^^^^
+
+warning: Unused assignment to `i`. Consider removing or prefixing with an underscore: `_i`
+   ┌─ TEMPFILE:35:13
+   │
+35 │             Two{i, b} => 3,
+   │             ^^^^^^^^^
+
+
+
+task 1 'run'. lines 45-45:
+return values: 3
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.optimize-no-simplify.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.optimize-no-simplify.exp
new file mode 100644
index 0000000000000..a152b902ea216
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.optimize-no-simplify.exp
@@ -0,0 +1,43 @@
+processed 2 tasks
+
+task 0 'publish'. lines 1-43:
+warning: Unused local variable `x`. Consider removing or prefixing with an underscore: `_x`
+   ┌─ TEMPFILE:20:17
+   │
+20 │             let x = 4;
+   │                 ^
+
+warning: Unused local variable `i`. Consider removing or prefixing with an underscore: `_i`
+   ┌─ TEMPFILE:35:17
+   │
+35 │             Two{i, b} => 3,
+   │                 ^
+
+warning: Unused local variable `b`. Consider removing or prefixing with an underscore: `_b`
+   ┌─ TEMPFILE:35:20
+   │
+35 │             Two{i, b} => 3,
+   │                    ^
+
+warning: Unused assignment to `x`. Consider removing or prefixing with an underscore: `_x`
+   ┌─ TEMPFILE:20:21
+   │
+20 │             let x = 4;
+   │                     ^
+
+warning: Unused assignment to `b`. Consider removing or prefixing with an underscore: `_b`
+   ┌─ TEMPFILE:35:13
+   │
+35 │             Two{i, b} => 3,
+   │             ^^^^^^^^^
+
+warning: Unused assignment to `i`. Consider removing or prefixing with an underscore: `_i`
+   ┌─ TEMPFILE:35:13
+   │
+35 │             Two{i, b} => 3,
+   │             ^^^^^^^^^
+
+
+
+task 1 'run'. lines 45-45:
+return values: 3
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.optimize.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.optimize.exp
new file mode 100644
index 0000000000000..4cef654b4343a
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/enum/enum_scoping.optimize.exp
@@ -0,0 +1,37 @@
+processed 2 tasks
+
+task 0 'publish'. lines 1-43:
+warning: Unused local variable `x`. Consider removing or prefixing with an underscore: `_x`
+   ┌─ TEMPFILE:20:17
+   │
+20 │             let x = 4;
+   │                 ^
+
+warning: Unused local variable `i`. Consider removing or prefixing with an underscore: `_i`
+   ┌─ TEMPFILE:35:17
+   │
+35 │             Two{i, b} => 3,
+   │                 ^
+
+warning: Unused local variable `b`. Consider removing or prefixing with an underscore: `_b`
+   ┌─ TEMPFILE:35:20
+   │
+35 │             Two{i, b} => 3,
+   │                    ^
+
+warning: Unused assignment to `b`. Consider removing or prefixing with an underscore: `_b`
+   ┌─ TEMPFILE:35:13
+   │
+35 │             Two{i, b} => 3,
+   │             ^^^^^^^^^
+
+warning: Unused assignment to `i`. Consider removing or prefixing with an underscore: `_i`
+   ┌─ TEMPFILE:35:13
+   │
+35 │             Two{i, b} => 3,
+   │             ^^^^^^^^^
+
+
+
+task 1 'run'. lines 45-45:
+return values: 3
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/tests.rs b/third_party/move/move-compiler-v2/transactional-tests/tests/tests.rs
index e62c4d203d8a4..02844cc5e1a87 100644
--- a/third_party/move/move-compiler-v2/transactional-tests/tests/tests.rs
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/tests.rs
@@ -102,6 +102,8 @@ const SEPARATE_BASELINE: &[&str] = &[
     // Creates different code if optimized or not
     "no-v1-comparison/enum/enum_field_select.move",
     "no-v1-comparison/enum/enum_field_select_different_offsets.move",
+    // Flaky redundant unused assignment error
+    "no-v1-comparison/enum/enum_scoping.move",
 ];
 
 fn get_config_by_name(name: &str) -> TestConfig {