[move-compiler-v2] Fix #14329: Make code optional in `assert!(cond, code)` #14334

brmataptos · 2024-08-19T21:15:56Z

Description

Fix #14329 by making code optional in assert!(cond, code).

We use the code well_known::UNSPECIFIED_ABORT_CODE value by default,
which is set to WELL_KNOWN_ABORT_CODE_BASE = 0xD8CA26CBD9BE << 16,
which is 15621340914461310976 in decimal.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Existing and new tests.

Key Areas to Review

Should we get this on Language v2? I don't think it's necessary, but...

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-08-19T21:15:59Z

⏱️ 6h 29m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	1h 4m	🟥 🟩 🟩
forge-framework-upgrade-test / forge	30m	🟩 🟩
forge-e2e-test / forge	27m	🟩 🟩
forge-compat-test / forge	27m	🟩 🟩
general-lints	16m	🟩 ⬜ 🟩 🟩 🟩 (+4 more)
rust-cargo-deny	15m	🟩 ⬜ 🟩 🟩 🟩 (+4 more)
test-target-determinator	13m	🟩 🟩 🟩
execution-performance / test-target-determinator	13m	🟩 🟩 🟩
rust-move-unit-coverage	11m	🟩
check	11m	🟩 🟩 🟩
check-dynamic-deps	10m	🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-move-unit-coverage	10m	🟩
rust-move-unit-coverage	10m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
rust-move-tests	8m	🟩
rust-move-tests	8m	🟩
rust-move-tests	8m	🟩
rust-move-tests	8m	⬜
rust-move-unit-coverage	8m	🟩
rust-move-unit-coverage	8m	⬜
rust-move-unit-coverage	8m	🟩
rust-move-unit-coverage	8m	🟩
rust-move-unit-coverage	7m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
semgrep/ci	4m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+4 more)
rust-move-tests	1m	⬜
permission-check	40s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	37s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	36s	🟩 🟩 🟩 🟩 🟩 (+4 more)
file_change_determinator	35s	🟩 🟩 🟩
permission-check	31s	🟩 🟩 🟩 🟩 🟩 (+4 more)
permission-check	6s	🟩 🟩 🟩
Backport PR	6s	🟥
determine-docker-build-metadata	6s	🟩 🟩 🟩
permission-check	3s	🟩

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

codecov · 2024-08-19T21:24:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.5%. Comparing base (996d51d) to head (b18245e).
Report is 3 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (996d51d) and HEAD (b18245e). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (996d51d) HEAD (b18245e)

2 1

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #14334       +/-   ##
===========================================
- Coverage    70.7%    59.5%    -11.2%     
===========================================
  Files        2370      838     -1532     
  Lines      475443   205043   -270400     
===========================================
- Hits       336456   122163   -214293     
+ Misses     138987    82880    -56107

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

vineethk

Looks good so far - minor comments.

I think we should make this a Move 2 language behavior (and produce errors for Move 1, like compiler v1).

vineethk · 2024-08-20T01:14:01Z

third_party/move/move-model/src/well_known.rs

@@ -51,4 +51,5 @@ pub const RECEIVER_PARAM_NAME: &str = "self";
 /// TODO: add a check at runtime that user is not clashing with reserved
 /// codes?
 pub const WELL_KNOWN_ABORT_CODE_BASE: u64 = 0xD8CA26CBD9BE << 16;
+pub const UNSPECIFIED_ABORT_CODE: u64 = WELL_KNOWN_ABORT_CODE_BASE;


Maybe add a brief note on what this unspecified abort code is? (i.e., the abort code used when user does not explicitly specify one in an assert!).

vineethk · 2024-08-20T01:14:39Z

third_party/move/move-model/src/builder/macros.rs

@@ -5,7 +5,7 @@
 //! Module for expanding macros, as `assert!(cond, code)`. This are expanded to


Maybe add a note here that assert!(cond) is also allowed?

vineethk · 2024-08-20T01:20:00Z

third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.move

+
+    let i = 0;
+    while (i < Test::len(&x)) {
+        assert!(Test::modify(&mut x) == i + 1);  // this will fail


So currently, this behavior (of allowing 1 arg to assert!) is compiler v2 only? In that case, I think we should make this a Move 2 language change. If not, depending on the compiler used, we can have differing behavior for Move 1, without any warning or indication.

It is a Move 2 feature. Lets add a test to checking-lang-v1 directory to verify we get the right error message for v1

vineethk · 2024-08-20T01:21:24Z

third_party/move/move-compiler-v2/tests/checking/naming/assert_one_arg.exp

-error: assert macro must have two arguments
-  ┌─ tests/checking/naming/assert_one_arg.move:5:16
+error: undeclared `y`
+  ┌─ tests/checking/naming/assert_one_arg.move:5:22
  │
 5 │         assert!(x != y);


We should also document this change in the Move book, given that it is fairly commonly used everywhere.

Filed #14343 to track.

I'll take care of this in next PR.

wrwg

Nice!

wrwg · 2024-08-20T14:30:51Z

third_party/move/move-compiler-v2/tests/checking/naming/assert_one_arg.exp

-error: assert macro must have two arguments
-  ┌─ tests/checking/naming/assert_one_arg.move:5:16
+error: undeclared `y`
+  ┌─ tests/checking/naming/assert_one_arg.move:5:22
  │
 5 │         assert!(x != y);


I'll take care of this in next PR.

wrwg · 2024-08-20T14:33:15Z

third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.move

+
+    let i = 0;
+    while (i < Test::len(&x)) {
+        assert!(Test::modify(&mut x) == i + 1);  // this will fail


It is a Move 2 feature. Lets add a test to checking-lang-v1 directory to verify we get the right error message for v1

fEst1ck · 2024-08-20T17:09:23Z

third_party/move/move-model/src/builder/macros.rs

-        let abort_code = args.value[1].clone();
+        let (cond, abort_code) = match args.value.len() {
+            1 => {
+                self.check_language_version(


Can we add a test in third_party/move/move-compiler/tests/move_check/parser to see this is guarded for V1? Does the V1 compiler use the move model for compilation?

We already have a test in third_party/move/move-compiler/tests/move_check/naming/assert_one_arg.move. I've added a separate warning in the V1 code to clarify that this is not supported in that compiler.

brmataptos

Addressed all comments.

brmataptos · 2024-08-20T01:42:18Z

third_party/move/move-model/src/well_known.rs

@@ -51,4 +51,5 @@ pub const RECEIVER_PARAM_NAME: &str = "self";
 /// TODO: add a check at runtime that user is not clashing with reserved
 /// codes?
 pub const WELL_KNOWN_ABORT_CODE_BASE: u64 = 0xD8CA26CBD9BE << 16;
+pub const UNSPECIFIED_ABORT_CODE: u64 = WELL_KNOWN_ABORT_CODE_BASE;


brmataptos · 2024-08-20T01:42:28Z

third_party/move/move-model/src/builder/macros.rs

@@ -5,7 +5,7 @@
 //! Module for expanding macros, as `assert!(cond, code)`. This are expanded to


brmataptos · 2024-08-20T01:42:38Z

third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.move

+
+    let i = 0;
+    while (i < Test::len(&x)) {
+        assert!(Test::modify(&mut x) == i + 1);  // this will fail


brmataptos · 2024-08-20T01:46:54Z

third_party/move/move-compiler-v2/tests/checking/naming/assert_one_arg.exp

-error: assert macro must have two arguments
-  ┌─ tests/checking/naming/assert_one_arg.move:5:16
+error: undeclared `y`
+  ┌─ tests/checking/naming/assert_one_arg.move:5:22
  │
 5 │         assert!(x != y);


Filed #14343 to track.

brmataptos · 2024-08-20T17:19:14Z

third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.move

+
+    let i = 0;
+    while (i < Test::len(&x)) {
+        assert!(Test::modify(&mut x) == i + 1);  // this will fail


brmataptos · 2024-08-20T17:29:05Z

third_party/move/move-model/src/builder/macros.rs

-        let abort_code = args.value[1].clone();
+        let (cond, abort_code) = match args.value.len() {
+            1 => {
+                self.check_language_version(


We already have a test in third_party/move/move-compiler/tests/move_check/naming/assert_one_arg.move. I've added a separate warning in the V1 code to clarify that this is not supported in that compiler.

… abort

github-actions · 2024-08-22T06:33:56Z

✅ Forge suite `realistic_env_max_load` success on `b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac`

two traffics test: inner traffic : committed: 12641.58 txn/s, latency: 3148.58 ms, (p50: 3000 ms, p90: 3600 ms, p99: 4200 ms), latency samples: 4806620
two traffics test : committed: 99.98 txn/s, latency: 2594.05 ms, (p50: 2400 ms, p90: 2800 ms, p99: 6500 ms), latency samples: 1720
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.251, avg: 0.224", "QsPosToProposal: max: 0.407, avg: 0.358", "ConsensusProposalToOrdered: max: 0.334, avg: 0.317", "ConsensusOrderedToCommit: max: 0.615, avg: 0.584", "ConsensusProposalToCommit: max: 0.939, avg: 0.901"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.94s no progress at version 2609435 (avg 0.22s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 7.80s no progress at version 2609433 (avg 7.80s) [limit 15].
Test Ok

github-actions · 2024-08-22T06:34:54Z

✅ Forge suite `compat` success on `d1bf834728a0cf166d993f4728dfca54f3086fb0` ==> `b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac`

Compatibility test results for d1bf834728a0cf166d993f4728dfca54f3086fb0 ==> b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac (PR)
1. Check liveness of validators at old version: d1bf834728a0cf166d993f4728dfca54f3086fb0
compatibility::simple-validator-upgrade::liveness-check : committed: 13548.16 txn/s, latency: 2497.08 ms, (p50: 2400 ms, p90: 3300 ms, p99: 4900 ms), latency samples: 442540
2. Upgrading first Validator to new version: b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7611.66 txn/s, latency: 3732.35 ms, (p50: 4200 ms, p90: 4600 ms, p99: 4800 ms), latency samples: 138080
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6788.49 txn/s, latency: 4479.93 ms, (p50: 4600 ms, p90: 4900 ms, p99: 6200 ms), latency samples: 253720
3. Upgrading rest of first batch to new version: b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6184.06 txn/s, latency: 4267.95 ms, (p50: 4800 ms, p90: 5200 ms, p99: 5300 ms), latency samples: 127100
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6709.70 txn/s, latency: 4763.59 ms, (p50: 4700 ms, p90: 7100 ms, p99: 7500 ms), latency samples: 230720
4. upgrading second batch to new version: b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 10992.71 txn/s, latency: 2452.35 ms, (p50: 2700 ms, p90: 2900 ms, p99: 3000 ms), latency samples: 192900
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11593.70 txn/s, latency: 2715.72 ms, (p50: 2600 ms, p90: 3200 ms, p99: 4200 ms), latency samples: 381100
5. check swarm health
Compatibility test for d1bf834728a0cf166d993f4728dfca54f3086fb0 ==> b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac passed
Test Ok

github-actions · 2024-08-22T06:35:38Z

✅ Forge suite `framework_upgrade` success on `d1bf834728a0cf166d993f4728dfca54f3086fb0` ==> `b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac`

Compatibility test results for d1bf834728a0cf166d993f4728dfca54f3086fb0 ==> b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac (PR)
Upgrade the nodes to version: b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1271.49 txn/s, submitted: 1272.65 txn/s, failed submission: 1.16 txn/s, expired: 1.16 txn/s, latency: 2608.00 ms, (p50: 2400 ms, p90: 4200 ms, p99: 6400 ms), latency samples: 109820
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1009.12 txn/s, submitted: 1011.56 txn/s, failed submission: 2.45 txn/s, expired: 2.45 txn/s, latency: 2921.22 ms, (p50: 2700 ms, p90: 4300 ms, p99: 6500 ms), latency samples: 90740
5. check swarm health
Compatibility test for d1bf834728a0cf166d993f4728dfca54f3086fb0 ==> b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac passed
Upgrade the remaining nodes to version: b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1141.54 txn/s, submitted: 1143.96 txn/s, failed submission: 2.42 txn/s, expired: 2.42 txn/s, latency: 2769.11 ms, (p50: 2400 ms, p90: 4800 ms, p99: 6100 ms), latency samples: 103640
Test Ok

brmataptos requested review from wrwg, vineethk, fEst1ck and rahxephon89 August 19, 2024 21:16

brmataptos changed the title ~~Fix #14329: Make code optional in assert!(cond, code)~~ [move-compiler-v2] Fix #14329: Make code optional in assert!(cond, code) Aug 19, 2024

vineethk reviewed Aug 20, 2024

View reviewed changes

brmataptos mentioned this pull request Aug 20, 2024

[move-compiler-v2] Update Move Book with Move 2 features #14343

Closed

brmataptos force-pushed the brm-issue-14329 branch from 20a0b3b to d3ea6cf Compare August 20, 2024 01:48

wrwg reviewed Aug 20, 2024

View reviewed changes

wrwg approved these changes Aug 20, 2024

View reviewed changes

fEst1ck reviewed Aug 20, 2024

View reviewed changes

brmataptos commented Aug 20, 2024

View reviewed changes

brmataptos force-pushed the brm-issue-14329 branch from 49bc685 to 213f7c0 Compare August 20, 2024 19:22

fEst1ck approved these changes Aug 20, 2024

View reviewed changes

brmataptos added 4 commits August 21, 2024 17:58

Fix #14329: Make code optional in assert!(cond, code)

1788549

update in response to Vineeth's review

97e42a7

add a test to show we have right error message for v1 lang

b2d25f3

add a language version warning to v1

d2afa8f

brmataptos force-pushed the brm-issue-14329 branch from 213f7c0 to d2afa8f Compare August 22, 2024 01:03

brmataptos enabled auto-merge (squash) August 22, 2024 01:05

This comment has been minimized.

Sign in to view

brmataptos added 2 commits August 21, 2024 22:56

add a separate baseline for assert_one, which changes location of the…

97b19af

… abort

Merge branch 'main' into brm-issue-14329

b18245e

This comment has been minimized.

Sign in to view

brmataptos merged commit 1a5d58a into main Aug 22, 2024
51 checks passed

brmataptos deleted the brm-issue-14329 branch August 22, 2024 06:35

zzjas mentioned this pull request Aug 22, 2024

[Bug][move-compiler-v2] Differing behavior w.r.t ability checks in the presence of unreachable code #14223

Closed

		@@ -5,7 +5,7 @@
		//! Module for expanding macros, as `assert!(cond, code)`. This are expanded to

[move-compiler-v2] Fix #14329: Make code optional in assert!(cond, code) #14334

[move-compiler-v2] Fix #14329: Make code optional in assert!(cond, code) #14334

Conversation

brmataptos commented Aug 19, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Aug 19, 2024 • edited Loading

codecov bot commented Aug 19, 2024 • edited Loading

Codecov Report

vineethk 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

wrwg 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

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

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Aug 22, 2024

✅ Forge suite realistic_env_max_load success on b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac

github-actions bot commented Aug 22, 2024

✅ Forge suite compat success on d1bf834728a0cf166d993f4728dfca54f3086fb0 ==> b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac

github-actions bot commented Aug 22, 2024

✅ Forge suite framework_upgrade success on d1bf834728a0cf166d993f4728dfca54f3086fb0 ==> b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac

[move-compiler-v2] Fix #14329: Make code optional in `assert!(cond, code)` #14334

[move-compiler-v2] Fix #14329: Make code optional in `assert!(cond, code)` #14334

brmataptos commented Aug 19, 2024 •

edited

Loading

trunk-io bot commented Aug 19, 2024 •

edited

Loading

codecov bot commented Aug 19, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac`

✅ Forge suite `compat` success on `d1bf834728a0cf166d993f4728dfca54f3086fb0` ==> `b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac`

✅ Forge suite `framework_upgrade` success on `d1bf834728a0cf166d993f4728dfca54f3086fb0` ==> `b18245e9acec2e1a5dc78759b2bcd1b88bcfdbac`