Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compiler-v2] Preserve wildcard assignments during stackless bytecode generation #12818

Merged
merged 9 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions third_party/move/move-compiler-v2/src/bytecode_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1271,8 +1271,13 @@ impl<'env> Generator<'env> {
) {
match pat {
Pattern::Wildcard(_) => {
// Nothing to do
// TODO(#12475) Should we copy to a temp here?
let ty = self.temp_type(arg).to_owned();
let temp = self.new_temp(ty);
// Assign to a temporary to allow stackless bytecode checkers to report any errors
// due to the assignment.
self.emit_with(id, |attr| {
Bytecode::Assign(attr, temp, arg, AssignKind::Inferred)
})
},
Pattern::Var(var_id, sym) => {
let local = self.find_local_for_pattern(*var_id, *sym, next_scope);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,6 @@ fun assign::assign_struct($t0: &mut assign::S) {
4: write_ref($t0, $t1)
5: return ()
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
module 0x42::assign {

struct S {
struct S has drop {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this fix because running checkers reported errors later.

f: u64,
g: T
}

struct T {
struct T has drop {
h: u64
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,6 @@ public fun assign::main(): (u64, u64) {
1: $t1 := 3
2: return ($t0, $t1)
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,6 @@ fun borrow::mut_param($t0: u64): u64 {
3: $t1 := read_ref($t2)
4: return $t1
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -350,3 +350,6 @@ public fun M::testb(): u64 {
6: $t0 := +($t1, $t4)
7: return $t0
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,6 @@ fun m::will_autoref(): address {
3: $t0 := read_ref($t3)
4: return $t0
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -211,3 +211,6 @@ fun fields::write_val($t0: fields::S): fields::S {
5: $t1 := infer($t0)
6: return $t1
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
module 0x42::fields {

struct S {
struct S has drop {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this fix because running checkers reported errors later.

f: u64,
g: T
}

struct T {
struct T has drop {
h: u64
}

struct G<X> {
struct G<X> has drop {
f: X
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,3 +292,16 @@ fun freeze_mut_ref::t8($t0: bool, $t1: &mut freeze_mut_ref::S, $t2: &freeze_mut_
6: label L2
7: return ()
}


Diagnostics:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about fixing the error in the test file, but it changes the test a bit too much for my liking, so left it in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note that v1 gives a bunch of warnings about "unused assignment or binding" for this file (58x2, 66, 69, 74x2). Are we suppressing such errors on this file or are we just not hitting it yet due to phase ordering?

Btw, V1 doesn't have the borrow issue below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused assignment warning is not yet implemented in v2 (tracked here: #11710)

error: mutable reference in local `s` requires exclusive access but is borrowed
┌─ tests/bytecode-generator/freeze_mut_ref.move:66:35
65 │ let f = &mut ({x = x + 1; s}).f;
│ ----------------------- previous mutable field borrow
66 │ let g = &mut ({x = x + 1; s}).f;
│ ^ requirement enforced here
·
69 │ *({*f = 0; z = y; g}) = 2;
│ ------ conflicting reference `f` used here
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,6 @@ fun globals::write($t0: address, $t1: u64): u64 {
4: $t2 := 9
5: return $t2
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,6 @@ fun if_else::if_else_nested($t0: bool, $t1: u64): u64 {
19: label L5
20: return $t2
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,6 @@ fun inline_specs::succ($t0: u64): u64 {
1: $t1 := +($t0, $t2)
2: return $t1
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,6 @@ fun loops::while_loop_with_break_and_continue($t0: u64): u64 {
30: $t1 := infer($t0)
31: return $t1
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// -- Model dump before bytecode pipeline
module 0xc0ffee::m {
public fun test(): u8 {
{
let x: u8 = 40;
{
let y: u8 = Move(x);
{
let _: u8 = x;
y
}
}
}
}
} // end 0xc0ffee::m

============ initial bytecode ================

[variant baseline]
public fun m::test(): u8 {
var $t0: u8
var $t1: u8
var $t2: u8
var $t3: u8
0: $t1 := 40
1: $t2 := move($t1)
2: $t3 := infer($t1)
3: $t0 := infer($t2)
4: return $t0
}


Diagnostics:
error: cannot move local `x` since it is still in use
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new error we get, the main subject of the fix.

┌─ tests/bytecode-generator/moved_var_not_simplified3.move:4:17
4 │ let y = move x;
│ ^^^^^^ attempted to move here
5 │ let _ = x;
│ - used here
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,6 @@ fun operators::order($t0: u64, $t1: u64): bool {
23: label L8
24: return $t2
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@ module 0x42::operators {
x && y || x && !y || !x && y || !x && !y
}

fun equality<T>(x: T, y: T): bool {
fun equality<T: drop>(x: T, y: T): bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this fix because running checkers reported errors later.

x == y
}

fun inequality<T>(x: T, y: T): bool {
fun inequality<T: drop>(x: T, y: T): bool {
x != y
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,6 @@ fun pack_unpack::pack6($t0: u8, $t1: u8, $t2: u8): pack_unpack::S {
2: $t3 := pack pack_unpack::S($t2, $t5, $t4)
3: return $t3
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,6 @@ fun pack_unpack::unpack($t0: pack_unpack::S): (u64, u64) {
3: $t2 := infer($t4)
4: return ($t1, $t2)
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,6 @@ fun reference_conversion::use_it(): u64 {
5: $t0 := reference_conversion::deref($t4)
6: return $t0
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,6 @@ public fun m::foo($t0: &m::S): u8 {
4: $t1 := read_ref($t5)
5: return $t1
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,6 @@ fun vector::create(): vector<u64> {
0: $t0 := ["1", "2", "3"]
1: return $t0
}


============ bytecode verification succeeded ========
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@

Diagnostics:
error: the left-hand side has 1 item but the right-hand side provided 2
┌─ tests/bytecode-generator/wildcard1.move:7:13
7 │ let _ = tup();
│ ^
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module 0xc0ffee::m {
fun tup(): (u64, u64) {
(0, 0)
}

public fun bar() {
let _ = tup();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to add these new tests to v1 so we can compare the outputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am bit confused as to where to add these tests back in v1 (unlike, say inlining, it is not obvious which pass/folder these wildcard tests belong to in v1). Suggestions would be appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be ok with a v2/ directory. :-) We can always change it at some later point. As a compiler user, I don't care what pass finds the errors, just that they are found and make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// -- Model dump before bytecode pipeline
module 0xc0ffee::m {
public fun baz() {
{
let x: u64;
{
let _: u64 = x;
Tuple()
}
}
}
} // end 0xc0ffee::m

============ initial bytecode ================

[variant baseline]
public fun m::baz() {
var $t0: u64
var $t1: u64
0: $t1 := infer($t0)
1: return ()
}


Diagnostics:
error: use of unassigned local `x`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were missing this error before.

┌─ tests/bytecode-generator/wildcard2.move:4:13
4 │ let _ = x;
│ ^
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the error point to _ instead of x?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed an issue for this: #12843

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module 0xc0ffee::m {
public fun baz() {
let x: u64;
let _ = x;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// -- Model dump before bytecode pipeline
module 0xc0ffee::m {
struct S {
dummy_field: bool,
}
public fun bar() {
{
let s: m::S = pack m::S(false);
{
let _: m::S = s;
Tuple()
}
}
}
public fun foo(s: m::S) {
{
let _: m::S = s;
Tuple()
}
}
} // end 0xc0ffee::m

============ initial bytecode ================

[variant baseline]
public fun m::bar() {
var $t0: m::S
var $t1: bool
var $t2: m::S
0: $t1 := false
1: $t0 := pack m::S($t1)
2: $t2 := infer($t0)
3: return ()
}


[variant baseline]
public fun m::foo($t0: m::S) {
var $t1: m::S
0: $t1 := infer($t0)
1: return ()
}


Diagnostics:
error: value of type `m::S` does not have the `drop` ability
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that V1 shows the reason for the error type again:

error[E05001]: ability constraint not satisfied
  ┌─ tests/move_check/v2/wildcard3.move:5:13
  │
2 │     struct S {}
  │            - To satisfy the constraint, the 'drop' ability would need to be added here
3 │ 
4 │     public fun foo(s: S) {
  │                       - The type '0xC0FFEE::m::S' does not have the ability 'drop'
5 │         let _ = s;
  │             ^ Cannot ignore values without the 'drop' ability. The value must be used

Please add this test case to existing bug (or file one) to improve the error message for such cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find what we are suggesting here pretty reasonable. We also show the reason (implicitly dropped here ...).

This also has nothing to do with this PR (exact same error is reported with non-wildcard variable).

Due to the above reasons, I am not filing a bug. But if you feel strongly about some aspect of this reporting, do feel free to file one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in this case there is a declared type. It's the inferred types that can be nonobvious.

┌─ tests/bytecode-generator/wildcard3.move:5:13
5 │ let _ = s;
│ ^ implicitly dropped here since it is no longer used

error: value of type `m::S` does not have the `drop` ability
┌─ tests/bytecode-generator/wildcard3.move:10:13
10 │ let _ = s;
│ ^ implicitly dropped here since it is no longer used
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
module 0xc0ffee::m {
struct S {}

public fun foo(s: S) {
let _ = s;
}

public fun bar() {
let s = S{};
let _ = s;
}
}
Loading
Loading