Skip to content

Commit

Permalink
[move][move-2024] Add match fix for typing around literal switches, p…
Browse files Browse the repository at this point in the history
…lus tests (MystenLabs#19133)

## Description 

This addresses a bug where `abort` was causing mistyped literal arm
binders in match compilation. It also addresses some false-positive dead
code complaints that I discovered while fixing the bug up.

Longer-term, it would be nice to eliminate temp binders of the form
`#%1: _|_ = unreachable` from HLIR generation so that CFGIR can
invariantly ensure none exist, catching these sorts of issues, but due
to multiple-binding forms `(x, y, z) = (abort 0, abort 1, abort 2)` and
the current structure of the pass, that is work left for another day.

## Test plan 

Several more tests to cover these cases, though still never enough.

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
  • Loading branch information
cgswords authored Aug 30, 2024
1 parent 070a2c3 commit 1310047
Show file tree
Hide file tree
Showing 22 changed files with 231 additions and 20 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
processed 3 tasks
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//# init --edition 2024.beta

//# publish
module 0x42::m {
public fun from_index(index: u64): u64 {
match (index) {
0 => 1,
1 => 2,
2 => 3,
_ => abort 0,
}
}

public fun run() {
assert!(from_index(2) == 3)
}
}

//# run 0x42::m::run
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
processed 3 tasks
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//# init --edition 2024.beta

//# publish
module 0x42::m {
public struct S has drop { x: u64 }

public fun from_index(s: S): u64 {
match (s) {
S { x: 0} => 1,
_ => abort 0,
}
}

public fun run() {
assert!(from_index(S { x: 0 }) == 1)
}
}

//# run 0x42::m::run
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
processed 3 tasks
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//# init --edition 2024.beta

//# publish
module 0x42::m {
public fun test(value: bool): u64 {
match (value) {
true => abort 0,
false => 0,
}
}

public fun run() {
assert!(test(false) == 0)
}
}

//# run 0x42::m::run
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
processed 3 tasks
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//# init --edition 2024.beta

//# publish
module 0x42::m {
public fun test(value: bool): u64 {
match (value) {
true => match (value) { true => abort 0, false => abort 0 },
false => match (value) { true => abort 0, false => 1 },
}
}

public fun run() {
assert!(test(false) == 1)
}
}

//# run 0x42::m::run
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,10 @@ fn tail(context: &mut Context, e: &T::Exp) -> Option<ControlFlow> {
};
let conseq_flow = tail(context, conseq);
let alt_flow = tail(context, alt);
if matches!(ty, sp!(_, N::Type_::Unit | N::Type_::Anything)) {
return None;
};
match (conseq_flow, alt_flow) {
_ if matches!(ty, sp!(_, N::Type_::Unit)) => None,
(Some(cflow), Some(aflow)) => {
if cflow.value == aflow.value {
context.report_tail_error(sp(*eloc, cflow.value));
Expand Down Expand Up @@ -340,6 +342,13 @@ fn tail(context: &mut Context, e: &T::Exp) -> Option<ControlFlow> {
tail(context, &arm.rhs)
})
.collect::<Vec<_>>();
if matches!(ty, sp!(_, N::Type_::Unit | N::Type_::Anything))
|| arms.value.iter().all(|sp!(_, arm)| {
matches!(arm.rhs.ty, sp!(_, N::Type_::Unit | N::Type_::Anything))
})
{
return None;
};
if arm_somes.iter().all(|arm_opt| arm_opt.is_some()) {
for arm_opt in arm_somes {
let sp!(aloc, arm_error) = arm_opt.unwrap();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -592,11 +592,11 @@ fn resolve_result(

let true_arm = resolve_result(context, init_subject, true_arm_result);
let false_arm = resolve_result(context, init_subject, false_arm_result);
let result_type = true_arm.ty.clone();
let result_ty = context.output_type().clone();

make_copy_bindings(
bindings,
make_if_else(lit_subject, true_arm, false_arm, result_type),
make_if_else(lit_subject, true_arm, false_arm, result_ty),
)
}
WorkResult::LiteralSwitch {
Expand All @@ -622,7 +622,7 @@ fn resolve_result(
let work_result = context.work_result(result_ndx);
let match_arm = resolve_result(context, init_subject, work_result);
let test_exp = make_lit_test(lit_subject.clone(), key);
let result_ty = out_exp.ty.clone();
let result_ty = context.output_type().clone();
out_exp = make_if_else(test_exp, match_arm, out_exp, result_ty);
}
make_copy_bindings(bindings, out_exp)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,6 @@ warning[Lint W04004]: unneeded return
= This warning can be suppressed with '#[allow(lint(unneeded_return))]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[W09005]: dead or unreachable code
┌─ tests/linter/move_2024/unneeded_return_branch.move:5:35
5 │ if (cond) { return 5 } else { abort ZERO }
│ ^^^^^^^^^^ Unreachable code. This statement (and any following statements) will not be executed.
= This warning can be suppressed with '#[allow(dead_code)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[Lint W04004]: unneeded return
┌─ tests/linter/move_2024/unneeded_return_branch.move:9:17
Expand Down Expand Up @@ -97,14 +89,6 @@ warning[Lint W04004]: unneeded return
= This warning can be suppressed with '#[allow(lint(unneeded_return))]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[W09005]: dead or unreachable code
┌─ tests/linter/move_2024/unneeded_return_branch.move:53:18
53 │ E::V1 => abort ZERO,
│ ^^^^^^^^^^ Unreachable code. This statement (and any following statements) will not be executed.
= This warning can be suppressed with '#[allow(dead_code)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[Lint W04004]: unneeded return
┌─ tests/linter/move_2024/unneeded_return_branch.move:58:5
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
warning[W09005]: dead or unreachable code
┌─ tests/move_2024/hlir/abort_pair.move:4:6
4 │ (abort 0, abort 0)
│ ^^^^^^^ Expected a value. Any code surrounding or after this expression will not be reached
= This warning can be suppressed with '#[allow(dead_code)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module 0x42::m;

public fun test(): (u64, u64) {
(abort 0, abort 0)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module 0x42::m;

public fun report_from_value(code: u64) {
if (code < 10) abort 0 else abort 1
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module 0x42::m;

fun test() {
if (true) {
if (true) abort 0 else abort 0
} else {
if (true) abort 0 else abort 0
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
warning[W09005]: dead or unreachable code
┌─ tests/move_2024/hlir/nested_if_abort_statement.move:5:9
5 │ if (true) abort 0 else abort 0
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Expected a value. Any code surrounding or after this expression will not be reached
= This warning can be suppressed with '#[allow(dead_code)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

warning[W09005]: dead or unreachable code
┌─ tests/move_2024/hlir/nested_if_abort_statement.move:7:9
7 │ if (true) abort 0 else abort 0
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Expected a value. Any code surrounding or after this expression will not be reached
= This warning can be suppressed with '#[allow(dead_code)]' applied to the 'module' or module member ('const', 'fun', or 'struct')

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module 0x42::m;

fun test(): u64 {
let x: u64 = if (true) {
if (true) abort 0 else abort 0
} else {
if (true) abort 0 else abort 0
};
x
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//# init --edition 2024.beta

//# publish
module 0x42::m {
public fun test(value: bool): u64 {
match (value) {
true => match (value) { true => abort 0, false => abort 0 },
false => match (value) { true => abort 0, false => 1 },
}
}

public fun run() {
assert!(test(false) == 1)
}
}

//# run 0x42::m::run
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//# init --edition 2024.beta

//# publish
module 0x42::m {
public fun from_index(index: u64): u64 {
match (index) {
0 => 1,
1 => 2,
2 => 3,
_ => abort 0,
}
}

public fun run() {
assert!(from_index(2) == 3)
}
}

//# run 0x42::m::run
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//# init --edition 2024.beta

//# publish
module 0x42::m {
public struct S has drop { x: u64 }

public fun from_index(s: S): u64 {
match (s) {
S { x: 0} => 1,
_ => abort 0,
}
}

public fun run() {
assert!(from_index(S { x: 0 }) == 1)
}
}

//# run 0x42::m::run
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//# init --edition 2024.beta

//# publish
module 0x42::m {
public fun test(value: bool): u64 {
match (value) {
true => abort 0,
false => 0,
}
}

public fun run() {
assert!(test(false) == 0)
}
}

//# run 0x42::m::run
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//# init --edition 2024.beta

//# publish
module 0x42::m {
public fun test(value: bool): u64 {
match (value) {
true => match (value) { true => abort 0, false => abort 0 },
false => match (value) { true => abort 0, false => 1 },
}
}

public fun run() {
assert!(test(false) == 1)
}
}

//# run 0x42::m::run

0 comments on commit 1310047

Please sign in to comment.