From c3fbf42b8e4de826b5950f80c7e7700cf6e4146d Mon Sep 17 00:00:00 2001 From: "Brian R. Murphy" <132495859+brmataptos@users.noreply.github.com> Date: Tue, 12 Mar 2024 09:36:59 -0700 Subject: [PATCH] Don't eliminate variables due to issue #12475 --- .../move-compiler-v2/src/ast_simplifier.rs | 56 ++++++------ .../src/bytecode_generator.rs | 1 + .../tests/checking/inlining/bug_11223.exp | 9 +- .../tests/checking/inlining/bug_9717.exp | 28 ++++-- .../checking/inlining/bug_9717_looponly.exp | 9 +- .../assign_unpack_references.exp | 86 +++---------------- .../bind_with_type_annot.exp | 2 +- .../else_assigns_if_doesnt.exp | 6 -- .../if_assigns_else_doesnt.exp | 6 -- .../moved_var_not_simplified2.exp | 15 +++- .../use_before_assign_loop.exp | 27 ++---- .../use_before_assign_while.exp | 20 +---- .../simplifier/moved_var_not_simplified2.exp | 12 ++- .../tests/simplifier/random.exp | 43 +++++----- 14 files changed, 134 insertions(+), 186 deletions(-) diff --git a/third_party/move/move-compiler-v2/src/ast_simplifier.rs b/third_party/move/move-compiler-v2/src/ast_simplifier.rs index b1d9c1c33ab4b..6732d4190678a 100644 --- a/third_party/move/move-compiler-v2/src/ast_simplifier.rs +++ b/third_party/move/move-compiler-v2/src/ast_simplifier.rs @@ -987,33 +987,39 @@ impl<'env> ExpRewriterFunctions for SimplifierRewriter<'env> { return Some(body.clone()); } - // (3) If some pattern vars are unused in the body, turn them into wildcards. - let new_pat = if !unused_bound_vars.is_empty() { - Some(pat.clone().remove_vars(&unused_bound_vars)) - } else { - None - }; + // The following is disabled for now until we figure out whether there is a fix + // for #12475. If that is fixed, then we can safely rewrite unused variable + // definitions to wildcards. + // + // // (3) If some pattern vars are unused in the body, turn them into wildcards. + // let new_pat = if !unused_bound_vars.is_empty() { + // Some(pat.clone().remove_vars(&unused_bound_vars)) + // } else { + // None + // }; - // Ideas not yet implemented: - // (4) simplify the pattern: if subpat is wildcard and subexpr is side-effect-free, - // can remove it and corresponding subexpr. - // (5) simplify the pattern: if subpat is wildcard, corresponding subexpr can be - // simplified to not produce a value - // (6) if body is also a block and its binding has no references to our bound vars, - // then merge patterns and blocks - // (7) if pattern is a singleton `Tuple` and binding is a `Tuple`, turn it into let x = val. + // // Ideas not yet implemented: + // // (4) simplify the pattern: if subpat is wildcard and subexpr is side-effect-free, + // // can remove it and corresponding subexpr. + // // (5) simplify the pattern: if subpat is wildcard, corresponding subexpr can be + // // simplified to not produce a value + // // (6) if body is also a block and its binding has no references to our bound vars, + // // then merge patterns and blocks + // // (7) if pattern is a singleton `Tuple` and binding is a `Tuple`, turn it into let x = val. - if let Some(pat) = new_pat { - let exp = ExpData::Block(id, pat, opt_binding.clone(), body.clone()).into_exp(); - trace!( - "Dropping some vars for rewrite_block(id={}), result = {}", - id.as_usize(), - exp.display_verbose(self.env()), - ); - Some(exp) - } else { - None - } + // if let Some(pat) = new_pat { + // let exp = ExpData::Block(id, pat, opt_binding.clone(), body.clone()).into_exp(); + // trace!( + // "Dropping some vars for rewrite_block(id={}), result = {}", + // id.as_usize(), + // exp.display_verbose(self.env()), + // ); + // Some(exp) + // } else { + // None + // } + + None } fn rewrite_if_else(&mut self, _id: NodeId, cond: &Exp, then: &Exp, else_: &Exp) -> Option { 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 76801f6fd818b..21b357a4f7a96 100644 --- a/third_party/move/move-compiler-v2/src/bytecode_generator.rs +++ b/third_party/move/move-compiler-v2/src/bytecode_generator.rs @@ -1247,6 +1247,7 @@ impl<'env> Generator<'env> { match pat { Pattern::Wildcard(_) => { // Nothing to do + // TODO(#12475) Should we copy to a temp here? }, Pattern::Var(var_id, sym) => { let local = self.find_local_for_pattern(*var_id, *sym, next_scope); diff --git a/third_party/move/move-compiler-v2/tests/checking/inlining/bug_11223.exp b/third_party/move/move-compiler-v2/tests/checking/inlining/bug_11223.exp index cadbe4a4dd12e..d3ba0962744a2 100644 --- a/third_party/move/move-compiler-v2/tests/checking/inlining/bug_11223.exp +++ b/third_party/move/move-compiler-v2/tests/checking/inlining/bug_11223.exp @@ -5,10 +5,13 @@ module 0xcafe::vectors { { let flipsref5: &vector = Borrow(Immutable)(flips); { - let _: vector = Copy(flips); + let _v: vector = Copy(flips); { - let x: &vector = flipsref5; - vector::length(x) + let _v2: vector = flips; + { + let x: &vector = flipsref5; + vector::length(x) + } } } } diff --git a/third_party/move/move-compiler-v2/tests/checking/inlining/bug_9717.exp b/third_party/move/move-compiler-v2/tests/checking/inlining/bug_9717.exp index fd83b118735c5..c3c1d0fe1e7c8 100644 --- a/third_party/move/move-compiler-v2/tests/checking/inlining/bug_9717.exp +++ b/third_party/move/move-compiler-v2/tests/checking/inlining/bug_9717.exp @@ -23,8 +23,11 @@ module 0xcafe::vectors { } }; { - let _: vector = Copy(flips); - Tuple() + let _v: vector = Copy(flips); + { + let _v2: vector = flips; + Tuple() + } } } public entry fun guess_flips_directly(flips: vector) { @@ -44,16 +47,22 @@ module 0xcafe::vectors { } }; { - let _: vector = Copy(flips); - Tuple() + let _v: vector = Copy(flips); + { + let _v2: vector = flips; + Tuple() + } } } } public entry fun guess_with_break_without_inline(flips: vector) { vectors::loops_with_break_no_inline(Borrow(Immutable)(flips)); { - let _: vector = Copy(flips); - Tuple() + let _v: vector = Copy(flips); + { + let _v2: vector = flips; + Tuple() + } } } public entry fun guess_without_break_with_inline(flips: vector) { @@ -78,8 +87,11 @@ module 0xcafe::vectors { } }; { - let _: vector = Copy(flips); - Tuple() + let _v: vector = flips; + { + let _v2: vector = Copy(flips); + Tuple() + } } } private fun loops_with_break_no_inline(flips: &vector) { diff --git a/third_party/move/move-compiler-v2/tests/checking/inlining/bug_9717_looponly.exp b/third_party/move/move-compiler-v2/tests/checking/inlining/bug_9717_looponly.exp index ab23ee707ec25..0c51b05556d8c 100644 --- a/third_party/move/move-compiler-v2/tests/checking/inlining/bug_9717_looponly.exp +++ b/third_party/move/move-compiler-v2/tests/checking/inlining/bug_9717_looponly.exp @@ -25,10 +25,13 @@ module 0xcafe::vectors { } }; { - let _: vector = Copy(flips); + let _v: vector = Copy(flips); { - let x: &vector = flipsref5; - vector::length(x) + let _v2: vector = flips; + { + let x: &vector = flipsref5; + vector::length(x) + } } } } diff --git a/third_party/move/move-compiler-v2/tests/simplifier-elimination/assign_unpack_references.exp b/third_party/move/move-compiler-v2/tests/simplifier-elimination/assign_unpack_references.exp index 8277517e8b12d..bf076b20eb029 100644 --- a/third_party/move/move-compiler-v2/tests/simplifier-elimination/assign_unpack_references.exp +++ b/third_party/move/move-compiler-v2/tests/simplifier-elimination/assign_unpack_references.exp @@ -1,77 +1,3 @@ - -Diagnostics: -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:8:65 - │ -8 │ R { s1: S { f }, s2 } = R { s1: S{f: 0}, s2: S{f: 1} }; f; s2; - │ ^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:8:68 - │ -8 │ R { s1: S { f }, s2 } = R { s1: S{f: 0}, s2: S{f: 1} }; f; s2; - │ ^^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:11:9 - │ -11 │ f; s2; - │ ^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:11:12 - │ -11 │ f; s2; - │ ^^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:17:66 - │ -17 │ R { s1: S { f }, s2 } = &R { s1: S{f: 0}, s2: S{f: 1} }; f; s2; - │ ^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:17:69 - │ -17 │ R { s1: S { f }, s2 } = &R { s1: S{f: 0}, s2: S{f: 1} }; f; s2; - │ ^^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:20:9 - │ -20 │ f; s2; - │ ^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:20:12 - │ -20 │ f; s2; - │ ^^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:27:70 - │ -27 │ R { s1: S { f }, s2 } = &mut R { s1: S{f: 0}, s2: S{f: 1} }; f; s2; - │ ^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:27:73 - │ -27 │ R { s1: S { f }, s2 } = &mut R { s1: S{f: 0}, s2: S{f: 1} }; f; s2; - │ ^^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:30:9 - │ -30 │ f; s2; - │ ^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/assign_unpack_references.move:30:12 - │ -30 │ f; s2; - │ ^^ - // -- Model dump before bytecode pipeline module 0x8675309::M { struct R { @@ -87,8 +13,12 @@ module 0x8675309::M { { let s2: M::S; M::R{ s1: M::S{ f: f: u64 }, s2: s2: M::S }: M::R = pack M::R(pack M::S(0), pack M::S(1)); + f; + s2; f: u64 = 0; s2: M::S = pack M::S(0); + f; + s2; Tuple() } } @@ -99,8 +29,12 @@ module 0x8675309::M { { let s2: &M::S; M::R{ s1: M::S{ f: f: &u64 }, s2: s2: &M::S }: M::R = Borrow(Immutable)(pack M::R(pack M::S(0), pack M::S(1))); + f; + s2; f: &u64 = Borrow(Immutable)(0); s2: &M::S = Borrow(Immutable)(pack M::S(0)); + f; + s2; Tuple() } } @@ -111,8 +45,12 @@ module 0x8675309::M { { let s2: &mut M::S; M::R{ s1: M::S{ f: f: &mut u64 }, s2: s2: &mut M::S }: M::R = Borrow(Mutable)(pack M::R(pack M::S(0), pack M::S(1))); + f; + s2; f: &mut u64 = Borrow(Mutable)(0); s2: &mut M::S = Borrow(Mutable)(pack M::S(0)); + f; + s2; Tuple() } } diff --git a/third_party/move/move-compiler-v2/tests/simplifier-elimination/bind_with_type_annot.exp b/third_party/move/move-compiler-v2/tests/simplifier-elimination/bind_with_type_annot.exp index fd368dd8b1cc5..38b60b351a17b 100644 --- a/third_party/move/move-compiler-v2/tests/simplifier-elimination/bind_with_type_annot.exp +++ b/third_party/move/move-compiler-v2/tests/simplifier-elimination/bind_with_type_annot.exp @@ -31,7 +31,7 @@ module 0x8675309::M { } private fun t0() { { - let (_, _, M::R{ f: _ }): (u64, bool, M::R) = Tuple(0, false, pack M::R(0)); + let (x: u64, b: bool, M::R{ f: f: u64 }): (u64, bool, M::R) = Tuple(0, false, pack M::R(0)); Tuple() } } diff --git a/third_party/move/move-compiler-v2/tests/simplifier-elimination/else_assigns_if_doesnt.exp b/third_party/move/move-compiler-v2/tests/simplifier-elimination/else_assigns_if_doesnt.exp index 69bc77e962c9d..e2963cfa3d5e0 100644 --- a/third_party/move/move-compiler-v2/tests/simplifier-elimination/else_assigns_if_doesnt.exp +++ b/third_party/move/move-compiler-v2/tests/simplifier-elimination/else_assigns_if_doesnt.exp @@ -15,12 +15,6 @@ warning: If condition is always true, so else branch code eliminated as dead cod 10 │ │ }; │ ╰─────' else branch eliminated -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/else_assigns_if_doesnt.move:9:9 - │ -9 │ x; - │ ^ - // -- Model dump before bytecode pipeline module _0 { private fun main() { diff --git a/third_party/move/move-compiler-v2/tests/simplifier-elimination/if_assigns_else_doesnt.exp b/third_party/move/move-compiler-v2/tests/simplifier-elimination/if_assigns_else_doesnt.exp index 4479cebfb5867..eb6bc40ad0c4b 100644 --- a/third_party/move/move-compiler-v2/tests/simplifier-elimination/if_assigns_else_doesnt.exp +++ b/third_party/move/move-compiler-v2/tests/simplifier-elimination/if_assigns_else_doesnt.exp @@ -15,12 +15,6 @@ warning: If condition is always true, so else branch code eliminated as dead cod 10 │ │ }; │ ╰─────' else branch eliminated -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/if_assigns_else_doesnt.move:9:9 - │ -9 │ y; - │ ^ - // -- Model dump before bytecode pipeline module _0 { private fun main() { diff --git a/third_party/move/move-compiler-v2/tests/simplifier-elimination/moved_var_not_simplified2.exp b/third_party/move/move-compiler-v2/tests/simplifier-elimination/moved_var_not_simplified2.exp index c0a66c1e7ba9a..a0bf1c1bd0a8c 100644 --- a/third_party/move/move-compiler-v2/tests/simplifier-elimination/moved_var_not_simplified2.exp +++ b/third_party/move/move-compiler-v2/tests/simplifier-elimination/moved_var_not_simplified2.exp @@ -5,7 +5,10 @@ module 0xc0ffee::m { let x: u8 = 40; { let y: u8 = Move(x); - y + { + let _z: u8 = x; + y + } } } } @@ -22,3 +25,13 @@ module 0xc0ffee::m { } } } // end 0xc0ffee::m + + +Diagnostics: +error: cannot move local `x` since it is still in use + ┌─ tests/simplifier-elimination/moved_var_not_simplified2.move:4:17 + │ +4 │ let y = move x; + │ ^^^^^^ attempted to move here +5 │ let _z = x; + │ -- used here diff --git a/third_party/move/move-compiler-v2/tests/simplifier-elimination/use_before_assign_loop.exp b/third_party/move/move-compiler-v2/tests/simplifier-elimination/use_before_assign_loop.exp index ec6dd4ab3f2dd..fcc23bdce4045 100644 --- a/third_party/move/move-compiler-v2/tests/simplifier-elimination/use_before_assign_loop.exp +++ b/third_party/move/move-compiler-v2/tests/simplifier-elimination/use_before_assign_loop.exp @@ -1,23 +1,3 @@ - -Diagnostics: -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/use_before_assign_loop.move:4:43 - │ -4 │ loop { let y = move x + 1; x = 0; y; } - │ ^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/use_before_assign_loop.move:9:62 - │ -9 │ loop { let y = x + 1; if (cond) { continue }; x = 0; y; } - │ ^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/use_before_assign_loop.move:20:9 - │ -20 │ x; - │ ^ - // -- Model dump before bytecode pipeline module 0x8675309::M { private fun tborrow1() { @@ -47,6 +27,7 @@ module 0x8675309::M { break } }; + x; Tuple() } } @@ -55,13 +36,14 @@ module 0x8675309::M { let x: u64; loop { { - let _: u64 = Add(x, 1); + let y: u64 = Add(x, 1); if cond { continue } else { Tuple() }; x: u64 = 0; + y; Tuple() } } @@ -72,8 +54,9 @@ module 0x8675309::M { let x: u64; loop { { - let _: u64 = Add(Move(x), 1); + let y: u64 = Add(Move(x), 1); x: u64 = 0; + y; Tuple() } } diff --git a/third_party/move/move-compiler-v2/tests/simplifier-elimination/use_before_assign_while.exp b/third_party/move/move-compiler-v2/tests/simplifier-elimination/use_before_assign_while.exp index 6ea5ea5d815f6..0e3895e383a11 100644 --- a/third_party/move/move-compiler-v2/tests/simplifier-elimination/use_before_assign_while.exp +++ b/third_party/move/move-compiler-v2/tests/simplifier-elimination/use_before_assign_while.exp @@ -1,17 +1,3 @@ - -Diagnostics: -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/use_before_assign_while.move:4:51 - │ -4 │ while (cond) { let y = move x + 1; x = 0; y; } - │ ^ - -warning: Expression value unused and side-effect free, so eliminated as dead code - ┌─ tests/simplifier-elimination/use_before_assign_while.move:9:75 - │ -9 │ while (cond) { let y = move x + 1; if (cond) { continue }; x = 0; y; } - │ ^ - // -- Model dump before bytecode pipeline module 0x8675309::M { private fun tborrow1(cond: bool) { @@ -57,13 +43,14 @@ module 0x8675309::M { loop { if cond { { - let _: u64 = Add(Move(x), 1); + let y: u64 = Add(Move(x), 1); if cond { continue } else { Tuple() }; x: u64 = 0; + y; Tuple() } } else { @@ -78,8 +65,9 @@ module 0x8675309::M { loop { if cond { { - let _: u64 = Add(Move(x), 1); + let y: u64 = Add(Move(x), 1); x: u64 = 0; + y; Tuple() } } else { diff --git a/third_party/move/move-compiler-v2/tests/simplifier/moved_var_not_simplified2.exp b/third_party/move/move-compiler-v2/tests/simplifier/moved_var_not_simplified2.exp index 31dd1c31bb9ad..d7e54813d8776 100644 --- a/third_party/move/move-compiler-v2/tests/simplifier/moved_var_not_simplified2.exp +++ b/third_party/move/move-compiler-v2/tests/simplifier/moved_var_not_simplified2.exp @@ -6,7 +6,7 @@ module 0xc0ffee::m { { let y: u8 = Move(x); { - let _: u8 = x; + let _z: u8 = x; y } } @@ -25,3 +25,13 @@ module 0xc0ffee::m { } } } // end 0xc0ffee::m + + +Diagnostics: +error: cannot move local `x` since it is still in use + ┌─ tests/simplifier/moved_var_not_simplified2.move:4:17 + │ +4 │ let y = move x; + │ ^^^^^^ attempted to move here +5 │ let _z = x; + │ -- used here diff --git a/third_party/move/move-compiler-v2/tests/simplifier/random.exp b/third_party/move/move-compiler-v2/tests/simplifier/random.exp index 176610aa67ade..d8e0fcb185fb1 100644 --- a/third_party/move/move-compiler-v2/tests/simplifier/random.exp +++ b/third_party/move/move-compiler-v2/tests/simplifier/random.exp @@ -35,33 +35,36 @@ module 0x8675::M { Tuple() }; { - let x: &mut u64 = M::id_mut(Borrow(Mutable)(v)); + let q: u64 = v; { - let y: &mut u64 = Borrow(Mutable)(v); - Deref(x); - Deref(y); + let x: &mut u64 = M::id_mut(Borrow(Mutable)(v)); { - let x: &u64 = Borrow(Immutable)(v); + let y: &mut u64 = Borrow(Mutable)(v); + Deref(x); + Deref(y); { - let y: &mut u64 = Borrow(Mutable)(v); - Deref(y); - Deref(x); - Deref(y); + let x: &u64 = Borrow(Immutable)(v); { - let x: &u64 = Borrow(Immutable)(v); + let y: &mut u64 = Borrow(Mutable)(v); + Deref(y); + Deref(x); + Deref(y); { - let y: &u64 = Borrow(Immutable)(v); - Deref(x); - Deref(y); - Deref(x); + let x: &u64 = Borrow(Immutable)(v); { - let x: &u64 = M::id(Borrow(Immutable)(v)); + let y: &u64 = Borrow(Immutable)(v); + Deref(x); + Deref(y); + Deref(x); { - let y: &u64 = Borrow(Immutable)(v); - Deref(x); - Deref(y); - Deref(x); - Tuple() + let x: &u64 = M::id(Borrow(Immutable)(v)); + { + let y: &u64 = Borrow(Immutable)(v); + Deref(x); + Deref(y); + Deref(x); + Tuple() + } } } }