[Feature] [Compiler-V2] += and the like #14583
Conversation
fEst1ck
force-pushed
the
plus-equal
branch
3 times, most recently
from
c68bb50 to
68d8d04
Compare
fEst1ck
requested review from
brmataptos,
wrwg and
rahxephon89
and removed request for
brmataptos
There was a problem hiding this comment.
We may want another variant of LValue for vector indexing.
|
Your top-level description is currently inconsistent. Can you please fix it? |
third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/op_equal/valid.exp
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,166 @@ | |||
| //# publish | |||
| module 0x42::test { | |||
There was a problem hiding this comment.
- The following tests differ in their semantics from (corresponding) Rust. This needs to be investigated and discussed. We need to have a good reason to differ from the Rust semantics in these cases.
// 1
module 0xc0ffee::m {
public fun test(): u64 {
let v = 1;
v += {v += {v += 2; v}; v};
v
}
}
// 2
module 0xc0ffee::m {
public fun test(): u64 {
let v = 1;
v += {v += 2; v};
v
}
}
// 3
module 0xc0ffee::m {
fun mod(r: &mut u64) {
*r += 2;
}
public fun test(): u64 {
let v = 1;
v += {mod(&mut v); v};
v
}
}
// 4
module 0xc0ffee::m {
fun mod(r: &mut u64): u64 {
*r += 2;
*r
}
public fun test(): u64 {
let v = 1;
v += mod(&mut v);
v
}
}
- The following test correctly fails to compile, we just need to include it somewhere as a test.
module 0xc0ffee::m {
struct S has drop {
x: u64,
}
fun foo(self: &mut S): u64 {
self.x += 1;
1
}
public fun test(): u64 {
let s = S { x: 0 };
s.x += s.foo();
s.x
}
}
- The following test cases have the correct behavior, they just need to be included to showcase no double evaluation of the lhs.
// 1
module 0xc0ffee::m {
fun foo(r: &mut u64): &mut u64 {
*r += 1;
r
}
public fun test(): u64 {
let x = 1;
*{foo(&mut x)} += 1;
x
}
}
// 2
module 0xc0ffee::m {
fun foo(r: &mut u64) {
*r += 2;
}
public fun test(): u64 {
let x = 1;
*{foo(&mut x); foo(&mut x); &mut x} += 1;
x
}
}
There was a problem hiding this comment.
It turns out that in Rust, the RHS of the += operator is evaluated first.
If both types are primitives, then the modifying operand will be evaluated first followed by the assigned operand. It will then set the value of the assigned operand’s place to the value of performing the operation of the operator with the values of the assigned operand and modifying operand.
https://doc.rust-lang.org/reference/expressions/operator-expr.html#compound-assignment-expressions
We are aligned with Rust now. The Rust version of the test is here for convenience
fn test0() -> u64 {
let mut v = 1;
v += {
v += {
v += 2;
v
};
v
};
v
}
fn test1() -> u64 {
let mut v = 1;
v += {
v += 2;
v
};
v
}
fn mod1(r: &mut u64) {
*r += 2;
}
fn test2() -> u64 {
let mut v = 1;
v += {
mod1(&mut v);
v
};
v
}
fn mod2(r: &mut u64) -> u64 {
*r += 2;
*r
}
fn test3() -> u64 {
let mut v = 1;
v += mod2(&mut v);
v
}
fn main() {
println!("{:?}", test0());
println!("{:?}", test1());
println!("{:?}", test2());
println!("{:?}", test3());
}
There was a problem hiding this comment.
Nice.
I saw that you added the tests I posted under bullet point 1, but don't see the tests I suggested in bullet points 2 and 3. If you have already added them, can you point them to me? If not, could you please add them (and point me to them)?
There was a problem hiding this comment.
Bullet point2: third_party/move/move-compiler-v2/tests/op-equal/valid2.exp
It however compiles in the latest commit, because of the change in semantics. The corresponding Rust program also compiles
struct S {
x: u64,
}
impl S {
fn foo(self: &mut S) -> u64 {
self.x += 1;
1
}
}
fn test() -> u64 {
let mut s = S { x: 0 };
let p = &mut s.x;
*p = *p +
s.x += s.foo();
s.x
}Bullet point3: third_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/op_equal/no_double_eval.move
| @@ -96,6 +96,29 @@ fn require_move_2(context: &mut Context, loc: Loc, description: &str) -> bool { | |||
| } | |||
| } | |||
|
|
|||
| fn require_language_version( | |||
There was a problem hiding this comment.
Can we use this (more general function) instead of the require_move_2, so that we reduce code duplication?
There was a problem hiding this comment.
Done. This gives different error messages though.
There was a problem hiding this comment.
I think we can change all the calls of require_move_2, which is just a specialization of require_language_version, AFAICT (barring the slight difference in error messages). But we can do it in a later PR, no need to do it here.
|
Still looks ok, though Vineeth's right that some things could be simplified with a bit more abstraction. |
| @@ -96,6 +96,29 @@ fn require_move_2(context: &mut Context, loc: Loc, description: &str) -> bool { | |||
| } | |||
| } | |||
|
|
|||
| fn require_language_version( | |||
There was a problem hiding this comment.
I think we can change all the calls of require_move_2, which is just a specialization of require_language_version, AFAICT (barring the slight difference in error messages). But we can do it in a later PR, no need to do it here.
| @@ -0,0 +1,166 @@ | |||
| //# publish | |||
| module 0x42::test { | |||
There was a problem hiding this comment.
Nice.
I saw that you added the tests I posted under bullet point 1, but don't see the tests I suggested in bullet points 2 and 3. If you have already added them, can you point them to me? If not, could you please add them (and point me to them)?
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.
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.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
### Description Add documentation for the op equals aptos-labs/aptos-core#14583. ### Checklist - Do all Lints pass? - [x] Have you ran `pnpm spellcheck`? - [x] Have you ran `pnpm fmt`? - [x] Have you ran `pnpm lint`?
Description
Introduces operators
+=,-=,*=,%=,/=,|=,&=,^=,<<=,>>=, to Move 2.1.Implementation
We expand
e1 += e2toFor the special case
*e1 += e2, it's expanded toand for
var += eThe expansion is done in the expansion phase.
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Tests under
third_party/move/move-compiler-v2/tests/plus-equalandthird_party/move/move-compiler-v2/transactional-tests/tests/no-v1-comparison/plus-equal.