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

vineethk · 2024-04-06T01:07:11Z

Description

During stackless bytecode generation, if we have the code let _ = rexp, then we would skip this assignment (after evaluating any side-effects in rexp). This would prevent any stackless-bytecode based error checkers from seeing this assignment, and potentially miss reporting an error based on it.

We now preserve this assignment, because of which we can report some previously missing errors (or provide better error locations).

Additionally, we run the full compiler pipeline for bytecode-generator test folder (so that we can see all errors reported, rather than stop right after bytecode generation). The old comment claimed that we ran the bytecode pipeline, but we did not (which is fixed in this PR).

The first commit showcases the issue, the second commit showcases the fix, and the final commit shows the test baseline changes.

Going back to comments made in #12475:

due to this fix, we provide better locations for illegal drop error messages
we don't need to worry about _ matching a tuple, as it is caught by the type checker.

Tests have been added for both these cases.

Type of Change

Bug fix

Which Components or Systems Does This Change Impact?

Move/Aptos Virtual Machine

How Has This Been Tested?

Other than all the existing tests, I have added additional tests wildcard*.move, and moved moved_var_not_simplified3.move (the OG test showing the issue which this PR fixes) from no-simplifier to bytecode-generator (which is where I think is a better home).
Verified all baseline updates are acceptable.

Key Areas to Review

The first two commits showcasing the issue and the fix.

trunk-io · 2024-04-06T01:07:15Z

⏱️ 11h 49m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-unit-coverage	1h 43m	🟥 🟥 ⬜ 🟥 🟥 (+1 more)
rust-unit-tests	1h 36m	🟥 🟥 🟩
rust-targeted-unit-tests	1h 22m	🟥 🟥 ⬜ 🟥 🟩
rust-smoke-tests	1h 4m	🟩 🟩
rust-move-tests	50m	🟩 🟩 ⬜ 🟩 🟩 (+1 more)
windows-build	48m	🟩 🟩
execution-performance / single-node-performance	48m	🟩 🟩
forge-e2e-test / forge	33m	🟩 🟩
run-tests-main-branch	31m	🟩 🟩 🟩 🟥 🟩 (+2 more)
rust-images / rust-all	29m	🟩 🟩
rust-lints	27m	🟩 🟩 ⬜ 🟩 🟩 (+1 more)
forge-compat-test / forge	25m	🟩 🟩
cli-e2e-tests / run-cli-tests	21m	🟩 🟥
check-dynamic-deps	15m	🟩 🟩 🟩 🟩 🟩 (+2 more)
general-lints	12m	🟩 🟩 ⬜ 🟩 🟩 (+1 more)
check	8m	🟩 🟩
rust-build-cached-packages	7m	🟩 🟩
semgrep/ci	3m	🟩 🟩 🟩 🟩 🟩 (+2 more)
node-api-compatibility-tests / node-api-compatibility-tests	2m	🟩 🟩
file_change_determinator	2m	🟩 🟩 ⬜ 🟩 🟩 (+3 more)
file_change_determinator	1m	🟩 🟩 ⬜ 🟩 🟩 (+2 more)
file_change_determinator	1m	🟩 🟩 ⬜ 🟩 🟩 (+2 more)
execution-performance / file_change_determinator	30s	🟩 🟩
file_change_determinator	25s	🟩 🟩
permission-check	24s	🟩 🟩 🟩 🟩 🟩 (+3 more)
permission-check	21s	🟩 🟩 🟩 🟩 🟩 (+3 more)
permission-check	21s	🟩 🟩 🟩 🟩 🟩 (+3 more)
permission-check	21s	🟩 🟩 🟩 🟩 🟩 (+3 more)
determine-docker-build-metadata	7s	🟩 🟩
permission-check	5s	🟩 🟩

🚨 2 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
rust-unit-tests	46m	28m
windows-build	15m	20m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

vineethk · 2024-04-06T01:20:45Z

third_party/move/move-compiler-v2/tests/bytecode-generator/assign.move

@@ -1,11 +1,11 @@
 module 0x42::assign {

-    struct S {
+    struct S has drop {


Made this fix because running checkers reported errors later.

vineethk · 2024-04-06T01:20:59Z

third_party/move/move-compiler-v2/tests/bytecode-generator/fields.move

@@ -1,15 +1,15 @@
 module 0x42::fields {

-    struct S {
+    struct S has drop {


Made this fix because running checkers reported errors later.

vineethk · 2024-04-06T01:21:54Z

third_party/move/move-compiler-v2/tests/bytecode-generator/freeze_mut_ref.exp

@@ -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:


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.

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.

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

vineethk · 2024-04-06T01:22:22Z

third_party/move/move-compiler-v2/tests/bytecode-generator/moved_var_not_simplified3.exp

+
+
+Diagnostics:
+error: cannot move local `x` since it is still in use


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

vineethk · 2024-04-06T01:22:34Z

third_party/move/move-compiler-v2/tests/bytecode-generator/operators.move

@@ -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 {


Made this fix because running checkers reported errors later.

vineethk · 2024-04-06T01:23:11Z

third_party/move/move-compiler-v2/tests/bytecode-generator/wildcard2.exp

+
+
+Diagnostics:
+error: use of unassigned local `x`


We were missing this error before.

vineethk · 2024-04-06T01:24:58Z

third_party/move/move-compiler-v2/tests/file-format-generator/vector.exp

@@ -80,6 +80,8 @@ L1:	loc1: u64
 L2:	loc2: u64
 L3:	loc3: u64
 L4:	loc4: u64
+L5:	loc5: u64


Note that this is the unoptimized output (and therefore, it is worse due the change in this PR).

The optimized output is better than this, and does not change due to the fix.

vineethk · 2024-04-06T01:25:49Z

third_party/move/move-compiler-v2/tests/testsuite.rs

@@ -222,11 +222,11 @@ const TEST_CONFIGS: Lazy<BTreeMap<&str, TestConfig>> = Lazy::new(|| {
            exclude: vec![],
            exp_suffix: None,
            options: opts.clone(),
-            // Run the bytecode pipeline as well for double-checking the result


We were not running the bytecode pipeline, now we do.

codecov · 2024-04-06T01:27:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.6%. Comparing base (7647a94) to head (870f631).

❗ Current head 870f631 differs from pull request most recent head 4fa81cc. Consider uploading reports for the commit 4fa81cc to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #12818     +/-   ##
=========================================
- Coverage    62.6%    62.6%   -0.1%     
=========================================
  Files         823      821      -2     
  Lines      184343   184181    -162     
=========================================
- Hits       115499   115393    -106     
+ Misses      68844    68788     -56

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

brmataptos · 2024-04-06T02:36:32Z

Is it possible to have _ in a struct in an assignment? Perhaps something like

struct S { x: u64, y: u64};
let s = S { x: 3, y: 4 };
let S { x: a, y: _ } = s;

and can we have a tuple with wildcards in an assignment as an example? It shouldn't be different, but who knows.j

module 0xc0ffee::m {
    public fun test(): u8 {
        let x = 40;
        let (y, _) = (move x, x);
        y
    }
}

and some variations:

module 0xc0ffee::m {
    public fun test(): u8 {
        let x = 40;
        let z = 30;
        let y = move x;
        let (_, q) = (x, z);
        y
    }
}

etc.

brmataptos

Some questions about outputs. Also, need a few new test cases and to add the test cases to v1 for comparison.

brmataptos · 2024-04-06T01:54:25Z

third_party/move/move-compiler-v2/tests/bytecode-generator/wildcard1.move

+    }
+
+    public fun bar() {
+        let _ = tup();


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

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.

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.

brmataptos · 2024-04-06T02:00:27Z

third_party/move/move-compiler-v2/tests/bytecode-generator/freeze_mut_ref.exp

@@ -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:


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.

brmataptos · 2024-04-06T02:17:06Z

third_party/move/move-compiler-v2/tests/bytecode-generator/wildcard1.exp

@@ -0,0 +1,7 @@
+
+Diagnostics:
+error: the left-hand side has 2 items but the right-hand side provided 1


Other occurrences of this error message have the correct direction. Why does this one seem backwards? Let's fix it here.

Also, V1 shows the type of tup() here, which is useful:

error[E04005]: expected a single type ┌─ tests/move_check/v2/wildcard1.move:7:13 │ 2 │ fun tup(): (u64, u64) { │ ---------- Expected a single type, but found expression list type: '(u64, u64)' · 7 │ let _ = tup(); │ ^ Invalid type for local

Please make sure we have a bug filed to improve these sorts of error messages.

Why does this one seem backwards? Let's fix it here.

It was a pre-existing bug in the exp builder, fixed it.

Nice catch.

Please make sure we have a bug filed to improve these sorts of error messages.

There was another issue you filed that was close enough to this one, I added this test case to that issue (#12669).

brmataptos · 2024-04-06T02:19:26Z

third_party/move/move-compiler-v2/tests/bytecode-generator/wildcard2.exp

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


Why does the error point to _ instead of x?

Filed an issue for this: #12843

brmataptos · 2024-04-06T02:21:38Z

third_party/move/move-compiler-v2/tests/bytecode-generator/wildcard3.exp

+
+
+Diagnostics:
+error: value of type `m::S` does not have the `drop` ability


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.

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.

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

vineethk · 2024-04-10T16:33:57Z

Is it possible to have _ in a struct in an assignment? Perhaps something like
struct S { x: u64, y: u64};
let s = S { x: 3, y: 4 };
let S { x: a, y: _ } = s;
and can we have a tuple with wildcards in an assignment as an example? It shouldn't be different, but who knows.j
module 0xc0ffee::m {
    public fun test(): u8 {
        let x = 40;
        let (y, _) = (move x, x);
        y
    }
}
and some variations:
module 0xc0ffee::m {
    public fun test(): u8 {
        let x = 40;
        let z = 30;
        let y = move x;
        let (_, q) = (x, z);
        y
    }
}
etc.

@brmataptos Thank you for the test cases, I have added all these. Note that some of the error outputs have the same issue filed #12843, which I reference in that issue.

vineethk · 2024-04-10T16:40:51Z

@brmataptos PTAL.

github-actions · 2024-04-12T20:37:35Z

✅ Forge suite `compat` success on `aptos-node-v1.10.1` ==> `4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5`

Compatibility test results for aptos-node-v1.10.1 ==> 4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5 (PR)
1. Check liveness of validators at old version: aptos-node-v1.10.1
compatibility::simple-validator-upgrade::liveness-check : committed: 6571 txn/s, latency: 5095 ms, (p50: 4800 ms, p90: 9400 ms, p99: 10200 ms), latency samples: 230000
2. Upgrading first Validator to new version: 4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1746 txn/s, latency: 16327 ms, (p50: 19300 ms, p90: 22500 ms, p99: 22900 ms), latency samples: 90800
3. Upgrading rest of first batch to new version: 4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1553 txn/s, latency: 18791 ms, (p50: 23800 ms, p90: 30200 ms, p99: 30500 ms), latency samples: 79220
4. upgrading second batch to new version: 4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3460 txn/s, latency: 8949 ms, (p50: 9800 ms, p90: 12300 ms, p99: 12900 ms), latency samples: 145320
5. check swarm health
Compatibility test for aptos-node-v1.10.1 ==> 4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5 passed
Test Ok

github-actions · 2024-04-12T20:42:40Z

✅ Forge suite `realistic_env_max_load` success on `4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5`

two traffics test: inner traffic : committed: 7897 txn/s, latency: 4967 ms, (p50: 4800 ms, p90: 5700 ms, p99: 11100 ms), latency samples: 3403820
two traffics test : committed: 100 txn/s, latency: 1887 ms, (p50: 1900 ms, p90: 2100 ms, p99: 3300 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.209, avg: 0.203", "QsPosToProposal: max: 0.296, avg: 0.250", "ConsensusProposalToOrdered: max: 0.462, avg: 0.432", "ConsensusOrderedToCommit: max: 0.333, avg: 0.322", "ConsensusProposalToCommit: max: 0.766, avg: 0.754"]
Max round gap was 1 [limit 4] at version 821917. Max no progress secs was 4.854678 [limit 15] at version 3319481.
Test Ok

vineethk commented Apr 6, 2024

View reviewed changes

vineethk requested review from wrwg, brmataptos, rahxephon89 and fEst1ck April 6, 2024 01:26

brmataptos reviewed Apr 6, 2024

View reviewed changes

vineethk force-pushed the vk/wildcard-preserve branch from 870f631 to fdc19d3 Compare April 10, 2024 16:32

vineethk force-pushed the vk/wildcard-preserve branch from fdc19d3 to 2207559 Compare April 10, 2024 16:40

vineethk requested a review from brmataptos April 10, 2024 16:40

rahxephon89 approved these changes Apr 10, 2024

View reviewed changes

brmataptos approved these changes Apr 11, 2024

View reviewed changes

vineethk added 4 commits April 12, 2024 13:08

Showcase the problem

f0eef27

Showcase fix

50f18b5

Update baselines and some tests

90a4f07

Addressing code review comments.

1999914

vineethk added 3 commits April 12, 2024 13:08

rebase update

6be0e77

Another test

596d247

Adding new wildcard tests back to v1

a414bf4

vineethk force-pushed the vk/wildcard-preserve branch from 8bcec3d to a414bf4 Compare April 12, 2024 18:42

Merge branch 'main' into vk/wildcard-preserve

7ec123f

vineethk enabled auto-merge (squash) April 12, 2024 18:44

This comment has been minimized.

Sign in to view

Update prover baseline

4fa81cc

This comment has been minimized.

Sign in to view

vineethk merged commit 851f9ee into main Apr 12, 2024
44 of 46 checks passed

vineethk deleted the vk/wildcard-preserve branch April 12, 2024 21:00



		Diagnostics:
		error: cannot move local `x` since it is still in use

		@@ -0,0 +1,7 @@

		Diagnostics:
		error: the left-hand side has 2 items but the right-hand side provided 1



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

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

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

Conversation

vineethk commented Apr 6, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

trunk-io bot commented Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 6, 2024 • edited Loading

Codecov Report

brmataptos commented Apr 6, 2024

brmataptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vineethk commented Apr 10, 2024 • edited Loading

vineethk commented Apr 10, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Apr 12, 2024

✅ Forge suite compat success on aptos-node-v1.10.1 ==> 4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5

github-actions bot commented Apr 12, 2024

✅ Forge suite realistic_env_max_load success on 4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5

vineethk commented Apr 6, 2024 •

edited

Loading

trunk-io bot commented Apr 6, 2024 •

edited

Loading

codecov bot commented Apr 6, 2024 •

edited

Loading

vineethk commented Apr 10, 2024 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.10.1` ==> `4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5`

✅ Forge suite `realistic_env_max_load` success on `4fa81cccb12a15b1bf9d2479ff767a6a1deb7ec5`