-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[compiler] New macros using abort with message #18412
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
base: main
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
6e4ed8d to
a43331c
Compare
7c56a57 to
bfd3c95
Compare
cee2eb9 to
3eae191
Compare
830781c to
3a838c7
Compare
|
Re-drafting this following discussion in standup. Need to enforce the message is a constant. |
ef8dfc6 to
01c7eef
Compare
01c7eef to
fbedd91
Compare
| } | ||
|
|
||
| /// Calls a standard library function `std::module_name::function_name(args)`. | ||
| fn call_std_function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice to reuse std functions. I am just a bit worried about dependencies. If the users do not explicitly import string_utils/string, the dependency between the current module and string_utils/string will not be built. In result, the users may see an error message saying that string_utils/string is not bound, creating some confusion.
Maybe you can do some tests using the CLI to compile an exmple with assert_eq! but does not include any dependency in its Move.toml, and see what happens.
We used to face similar problems when dealing with std::vector and std::cmp. Here is a trick we did to avoid such problems:
https://github.com/aptos-labs/aptos-core/pull/16812/files#diff-9b8fbd37b0c05da93fc009674561ea37f0fdc242b021e9ce5300358e717368f2. Meanwhile, we were better positioned to check if the std::cmp module is present as we already have the move model: https://github.com/aptos-labs/aptos-core/pull/16812/files#diff-17c49bd2efe73148cec80384375c0a69d31778daafd97196485318719fdb7a74R184
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can do some tests using the CLI to compile an exmple with
assert_eq!but does not include any dependency in itsMove.toml, and see what happens.
It will give an error message that the function names are not bound, e.g.
error: no function named `string::into_bytes` found
┌─ ./sources/test.move:4:9
│
4 │ assert_eq!(1, 2);
│ ^^^^^^^^^
error: no function named `string_utils::format2` found
┌─ ./sources/test.move:4:9
│
4 │ assert_eq!(1, 2);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I did a similar trick to what we do for cmp and vector. Now, if you try to use assert_eq! without having either move-stdlib or aptos-stdlib in your dependencies, you’ll get a more helpful error:
error: cannot find `string_utils` module
┌─ ./sources/test.move:4:9
│
9 │ assert_eq!(1, 2);
│ ^^^^^^^^^
error: cannot find `string` module
┌─ ./sources/test.move:4:9
│
9 │ assert_eq!(1, 2);
│ ^^^^^^^^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks better.
It feels even nicer if we could let users know that they need to incldue move-stdlib as a dependency when using assert with strings, which is, however, not doable here as dependencies may not have been resolved. Maybe we should do an AST rewrite after the movel model is built, but that would be a different implementation.
Anyways, time to move forward.
fbedd91 to
5611a6e
Compare
5611a6e to
4f98ddd
Compare
b9863b4 to
36fa32c
Compare
36fa32c to
965eee0
Compare
wrwg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!

Description
This PR introduces new assertion macros built on top of the overloaded abort added in an earlier PR.
New
assert!formThis now supports formatted messages with arguments:
Example:
which aborts with message:
New
assert_eq!andassert_ne!macrosThese macros mirror the behaviour and ergonomics of Rust’s
assert_eq!andassert_ne!, including structured evaluation and rich error messages. There are three forms depending on the number of arguments.Two arguments
Three arguments (custom message)
More than three arguments (formatted message)
Assertion failed message format
The assertion failure message format is inspired by Rust and shows both evaluated values:
It can also include a custom message:
How Has This Been Tested?
Added unit tests for all possible forms of
assert!,assert_eq!, andassert_ne!.Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist
Note
Introduces assertion macros with rich messages and compiler support.
assert!,assert_eq!, andassert_ne!inmove-model/src/builder/macros.rs, generating conditionals/matches and formatted messages viastd::string/std::string_utilsvector,cmp,string, andstring_utilsduring dependency closure; adds helpers for UTF8/bytes conversionstring::into_bytesto stdlibstring.movemacros.rs, Move package) and numerous compiler/bytecode/typing/naming/macros test expectations; adds amacrostest configvector.move, docgen link adjustmentsWritten by Cursor Bugbot for commit 965eee0. This will update automatically on new commits. Configure here.