Repository: aptos-labs/aptos-core (language: python)

Issue:
[compiler-v2][release] Make `code` optional in `assert!(cond, code)`
The macro is expanded manually somewhere between expansion and exp_builder. We should allow it to have only one parameter and abort with a standard code. For compiler generated codes, see `well_known.rs` in move model.


Discussion / hints:

Fix patch:
diff --git a/third_party/move/move-compiler/src/naming/translate.rs b/third_party/move/move-compiler/src/naming/translate.rs
index 50d32992a31fb..9080f025707f5 100644
--- a/third_party/move/move-compiler/src/naming/translate.rs
+++ b/third_party/move/move-compiler/src/naming/translate.rs
@@ -1043,6 +1043,15 @@ fn exp_(context: &mut Context, e: E::Exp) -> N::Exp {
                 ));
             }
             let nes = call_args(context, rhs);
+            if nes.value.len() == 1 {
+                context.env.add_diag(diag!(
+                    Syntax::UnsupportedLanguageItem,
+                    (
+                        mloc,
+                        "single-parameter assert! macro not supported by this compiler"
+                    )
+                ));
+            }
             NE::Builtin(sp(mloc, BF::Assert(true)), nes)
         },
         EE::Call(sp!(mloc, _), CallKind::Receiver, ..) => {
diff --git a/third_party/move/move-model/src/builder/exp_builder.rs b/third_party/move/move-model/src/builder/exp_builder.rs
index 52ea2c06c2aac..8a7ac190db9ff 100644
--- a/third_party/move/move-model/src/builder/exp_builder.rs
+++ b/third_party/move/move-model/src/builder/exp_builder.rs
@@ -5161,10 +5161,7 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo
             self.error(loc, "macro invocation cannot have type arguments");
             self.new_error_exp()
         } else if let EA::ModuleAccess_::Name(name) = &maccess.value {
-            let expansion = self
-                .parent
-                .parent
-                .expand_macro(maccess.loc, name.value.as_str(), args);
+            let expansion = self.expand_macro(maccess.loc, name.value.as_str(), args);
             self.translate_exp(&expansion, expected_type)
         } else {
             let qsym = self.parent.module_access_to_qualified(maccess);
diff --git a/third_party/move/move-model/src/builder/macros.rs b/third_party/move/move-model/src/builder/macros.rs
index 5f54ae60aef8f..a8837e49f6b35 100644
--- a/third_party/move/move-model/src/builder/macros.rs
+++ b/third_party/move/move-model/src/builder/macros.rs
@@ -3,13 +3,17 @@
 // SPDX-License-Identifier: Apache-2.0
 
 //! Module for expanding macros, as `assert!(cond, code)`. This are expanded to
-//! the input AST before type checking.
+//! the input AST before type checking.  We also allow `assert!(cond)`, for Move 2,
+//! which generates the "well-known" abort code `UNSPECIFIED_ABORT_CODDE`,
+//! which is `(0xD8CA26CBD9BE << 16)`.
 
-use crate::builder::model_builder::ModelBuilder;
+use crate::{
+    builder::exp_builder::ExpTranslator, well_known::UNSPECIFIED_ABORT_CODE, LanguageVersion,
+};
 use move_compiler::expansion::ast as EA;
 use move_ir_types::location::{sp, Loc, Spanned};
 
-impl<'env> ModelBuilder<'env> {
+impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'module_translator> {
     pub fn expand_macro(&self, loc: Loc, name: &str, args: &Spanned<Vec<EA::Exp>>) -> EA::Exp {
         // Currently, there is only the assert! macro, and no user definable ones.
         let expansion_ = match name {
@@ -23,15 +27,30 @@ impl<'env> ModelBuilder<'env> {
     }
 
     fn expand_assert(&self, loc: Loc, args: &Spanned<Vec<EA::Exp>>) -> EA::Exp_ {
-        if args.value.len() != 2 {
-            self.error(
-                &self.to_loc(&args.loc),
-                "assert macro must have two arguments",
-            );
-            return EA::Exp_::UnresolvedError;
-        }
-        let cond = args.value[0].clone();
-        let abort_code = args.value[1].clone();
+        let (cond, abort_code) = match args.value.len() {
+            1 => {
+                self.check_language_version(
+                    &self.to_loc(&loc),
+                    "single-argument `assert!` macro",
+                    LanguageVersion::V2_0,
+                );
+                (
+                    args.value[0].clone(),
+                    sp(
+                        loc,
+                        EA::Exp_::Value(sp(loc, EA::Value_::U64(UNSPECIFIED_ABORT_CODE))),
+                    ),
+                )
+            },
+            2 => (args.value[0].clone(), args.value[1].clone()),
+            _ => {
+                self.error(
+                    &self.to_loc(&args.loc),
+                    "assert macro must have one or two arguments",
+                );
+                return EA::Exp_::UnresolvedError;
+            },
+        };
         EA::Exp_::IfElse(
             Box::new(cond),
             Box::new(sp(loc, EA::Exp_::Unit { trailing: false })),
diff --git a/third_party/move/move-model/src/well_known.rs b/third_party/move/move-model/src/well_known.rs
index 39702c9231a62..40cdb510a6d44 100644
--- a/third_party/move/move-model/src/well_known.rs
+++ b/third_party/move/move-model/src/well_known.rs
@@ -51,4 +51,9 @@ 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;
+
+// Used when user omits an abort code.
+pub const UNSPECIFIED_ABORT_CODE: u64 = WELL_KNOWN_ABORT_CODE_BASE;
+
+// Used when a runtime value falls through a match.
 pub const INCOMPLETE_MATCH_ABORT_CODE: u64 = WELL_KNOWN_ABORT_CODE_BASE | 0x1;


Test patch:
diff --git a/third_party/move/move-compiler-v2/tests/checking-lang-v1/assert_one_arg.exp b/third_party/move/move-compiler-v2/tests/checking-lang-v1/assert_one_arg.exp
new file mode 100644
index 0000000000000..1440755948acd
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/checking-lang-v1/assert_one_arg.exp
@@ -0,0 +1,25 @@
+
+Diagnostics:
+error: not supported before language version `2.0-unstable`: single-argument `assert!` macro
+  ┌─ tests/checking-lang-v1/assert_one_arg.move:3:9
+  │
+3 │         assert!(false);
+  │         ^^^^^^
+
+error: not supported before language version `2.0-unstable`: single-argument `assert!` macro
+  ┌─ tests/checking-lang-v1/assert_one_arg.move:4:9
+  │
+4 │         assert!(0 != 1);
+  │         ^^^^^^
+
+error: not supported before language version `2.0-unstable`: single-argument `assert!` macro
+  ┌─ tests/checking-lang-v1/assert_one_arg.move:5:9
+  │
+5 │         assert!(x != y);
+  │         ^^^^^^
+
+error: undeclared `y`
+  ┌─ tests/checking-lang-v1/assert_one_arg.move:5:22
+  │
+5 │         assert!(x != y);
+  │                      ^
diff --git a/third_party/move/move-compiler-v2/tests/checking-lang-v1/assert_one_arg.move b/third_party/move/move-compiler-v2/tests/checking-lang-v1/assert_one_arg.move
new file mode 100644
index 0000000000000..ede2797e1bdc7
--- /dev/null
+++ b/third_party/move/move-compiler-v2/tests/checking-lang-v1/assert_one_arg.move
@@ -0,0 +1,7 @@
+module 0x42::M {
+    fun foo(x: &mut u64) {
+        assert!(false);
+        assert!(0 != 1);
+        assert!(x != y);
+    }
+}
diff --git a/third_party/move/move-compiler-v2/tests/checking/naming/assert_one_arg.exp b/third_party/move/move-compiler-v2/tests/checking/naming/assert_one_arg.exp
index 28a59b9f11ec5..9f9ca7320722e 100644
--- a/third_party/move/move-compiler-v2/tests/checking/naming/assert_one_arg.exp
+++ b/third_party/move/move-compiler-v2/tests/checking/naming/assert_one_arg.exp
@@ -1,19 +1,7 @@
 
 Diagnostics:
-error: assert macro must have two arguments
-  ┌─ tests/checking/naming/assert_one_arg.move:3:16
-  │
-3 │         assert!(false);
-  │                ^^^^^^^
-
-error: assert macro must have two arguments
-  ┌─ tests/checking/naming/assert_one_arg.move:4:16
-  │
-4 │         assert!(0 != 1);
-  │                ^^^^^^^^
-
-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);
-  │                ^^^^^^^^
+  │                      ^
diff --git a/third_party/move/move-compiler-v2/tests/checking/naming/other_builtins_invalid.exp b/third_party/move/move-compiler-v2/tests/checking/naming/other_builtins_invalid.exp
index 1293752325bc6..98129c95320b4 100644
--- a/third_party/move/move-compiler-v2/tests/checking/naming/other_builtins_invalid.exp
+++ b/third_party/move/move-compiler-v2/tests/checking/naming/other_builtins_invalid.exp
@@ -12,20 +12,8 @@ error: expected 1 type argument but 0 were provided
 4 │         freeze<>(x);
   │         ^^^^^^^^^^^
 
-error: assert macro must have two arguments
+error: assert macro must have one or two arguments
   ┌─ tests/checking/naming/other_builtins_invalid.move:5:16
   │
 5 │         assert!();
   │                ^^
-
-error: assert macro must have two arguments
-  ┌─ tests/checking/naming/other_builtins_invalid.move:6:16
-  │
-6 │         assert!(false);
-  │                ^^^^^^^
-
-error: assert macro must have two arguments
-  ┌─ tests/checking/naming/other_builtins_invalid.move:7:16
-  │
-7 │         assert!(0 != 1);
-  │                ^^^^^^^^
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.move b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.move
new file mode 100644
index 0000000000000..f999efdb95f45
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.move
@@ -0,0 +1,34 @@
+//# publish
+module 0x42::Test {
+    struct Tester has drop {
+        f: u64
+    }
+
+    public fun new(): Tester {
+        Tester { f: 10 }
+    }
+
+    public fun len(t: &Tester): u64 {
+        t.f
+    }
+
+    public fun modify(t: &mut Tester): u64 {
+        t.f = t.f - 1;
+        9 - t.f
+    }
+}
+
+//# run
+script {
+use 0x42::Test;
+fun main() {
+    let x = Test::new();
+    assert!(Test::len(&x) == 10, 70002);
+
+    let i = 0;
+    while (i < Test::len(&x)) {
+        assert!(Test::modify(&mut x) == i + 1);  // this will fail
+        i = i + 1
+    }
+}
+}
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.no-optimize.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.no-optimize.exp
new file mode 100644
index 0000000000000..7caa3f7a2e94d
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.no-optimize.exp
@@ -0,0 +1,10 @@
+processed 2 tasks
+
+task 1 'run'. lines 21-34:
+Error: Script execution failed with VMError: {
+    major_status: ABORTED,
+    sub_status: Some(15621340914461310976),
+    location: script,
+    indices: [],
+    offsets: [(FunctionDefinitionIndex(0), 32)],
+}
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.optimize-no-simplify.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.optimize-no-simplify.exp
new file mode 100644
index 0000000000000..38c9b4a11c3ad
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.optimize-no-simplify.exp
@@ -0,0 +1,10 @@
+processed 2 tasks
+
+task 1 'run'. lines 21-34:
+Error: Script execution failed with VMError: {
+    major_status: ABORTED,
+    sub_status: Some(15621340914461310976),
+    location: script,
+    indices: [],
+    offsets: [(FunctionDefinitionIndex(0), 30)],
+}
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.optimize.exp b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.optimize.exp
new file mode 100644
index 0000000000000..38c9b4a11c3ad
--- /dev/null
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/assert_one.optimize.exp
@@ -0,0 +1,10 @@
+processed 2 tasks
+
+task 1 'run'. lines 21-34:
+Error: Script execution failed with VMError: {
+    major_status: ABORTED,
+    sub_status: Some(15621340914461310976),
+    location: script,
+    indices: [],
+    offsets: [(FunctionDefinitionIndex(0), 30)],
+}
diff --git a/third_party/move/move-compiler-v2/transactional-tests/tests/tests.rs b/third_party/move/move-compiler-v2/transactional-tests/tests/tests.rs
index 02844cc5e1a87..1189f6bd01a1b 100644
--- a/third_party/move/move-compiler-v2/transactional-tests/tests/tests.rs
+++ b/third_party/move/move-compiler-v2/transactional-tests/tests/tests.rs
@@ -102,6 +102,7 @@ const SEPARATE_BASELINE: &[&str] = &[
     // Creates different code if optimized or not
     "no-v1-comparison/enum/enum_field_select.move",
     "no-v1-comparison/enum/enum_field_select_different_offsets.move",
+    "no-v1-comparison/assert_one.move",
     // Flaky redundant unused assignment error
     "no-v1-comparison/enum/enum_scoping.move",
 ];
diff --git a/third_party/move/move-compiler/tests/move_check/naming/assert_one_arg.exp b/third_party/move/move-compiler/tests/move_check/naming/assert_one_arg.exp
index a4a7db80e0454..119fe395be684 100644
--- a/third_party/move/move-compiler/tests/move_check/naming/assert_one_arg.exp
+++ b/third_party/move/move-compiler/tests/move_check/naming/assert_one_arg.exp
@@ -1,3 +1,9 @@
+error[E01013]: unsupported language construct
+  ┌─ tests/move_check/naming/assert_one_arg.move:3:9
+  │
+3 │         assert!(false);
+  │         ^^^^^^ single-parameter assert! macro not supported by this compiler
+
 error[E04016]: too few arguments
   ┌─ tests/move_check/naming/assert_one_arg.move:3:9
   │
@@ -7,6 +13,12 @@ error[E04016]: too few arguments
   │         │      Found 1 argument(s) here
   │         Invalid call of 'assert'. The call expected 2 argument(s) but got 1
 
+error[E01013]: unsupported language construct
+  ┌─ tests/move_check/naming/assert_one_arg.move:4:9
+  │
+4 │         assert!(0 != 1);
+  │         ^^^^^^ single-parameter assert! macro not supported by this compiler
+
 error[E04016]: too few arguments
   ┌─ tests/move_check/naming/assert_one_arg.move:4:9
   │
@@ -16,6 +28,12 @@ error[E04016]: too few arguments
   │         │      Found 1 argument(s) here
   │         Invalid call of 'assert'. The call expected 2 argument(s) but got 1
 
+error[E01013]: unsupported language construct
+  ┌─ tests/move_check/naming/assert_one_arg.move:5:9
+  │
+5 │         assert!(x != y);
+  │         ^^^^^^ single-parameter assert! macro not supported by this compiler
+
 error[E04016]: too few arguments
   ┌─ tests/move_check/naming/assert_one_arg.move:5:9
   │
diff --git a/third_party/move/move-compiler/tests/move_check/naming/other_builtins_invalid.exp b/third_party/move/move-compiler/tests/move_check/naming/other_builtins_invalid.exp
index abd87e6316041..13198dcace4cc 100644
--- a/third_party/move/move-compiler/tests/move_check/naming/other_builtins_invalid.exp
+++ b/third_party/move/move-compiler/tests/move_check/naming/other_builtins_invalid.exp
@@ -25,6 +25,12 @@ error[E04016]: too few arguments
   │         │      Found 0 argument(s) here
   │         Invalid call of 'assert'. The call expected 2 argument(s) but got 0
 
+error[E01013]: unsupported language construct
+  ┌─ tests/move_check/naming/other_builtins_invalid.move:6:9
+  │
+6 │         assert!(false);
+  │         ^^^^^^ single-parameter assert! macro not supported by this compiler
+
 error[E04016]: too few arguments
   ┌─ tests/move_check/naming/other_builtins_invalid.move:6:9
   │
@@ -34,6 +40,12 @@ error[E04016]: too few arguments
   │         │      Found 1 argument(s) here
   │         Invalid call of 'assert'. The call expected 2 argument(s) but got 1
 
+error[E01013]: unsupported language construct
+  ┌─ tests/move_check/naming/other_builtins_invalid.move:7:9
+  │
+7 │         assert!(0 != 1);
+  │         ^^^^^^ single-parameter assert! macro not supported by this compiler
+
 error[E04016]: too few arguments
   ┌─ tests/move_check/naming/other_builtins_invalid.move:7:9
   │