aptos-labs / aptos-core

Aptos is a layer 1 blockchain built to support the widespread use of blockchain through better technology and user experience.
https://aptosfoundation.org
Other
6.18k stars 3.65k forks source link

[Bug][move-compiler-v2] move-model AST represents short-cut `||` and `&&` as simple Calls, check for potential errors #13685

Open brmataptos opened 4 months ago

brmataptos commented 4 months ago

🐛 Bug

I (Brian) implemented various optimizations on the movel-model AST without realizing that short-cutting || and && haven't been lowered to more explicit control flow, but remain as Call expressions. This means that the potential control flow is more complex and assumptions about execution order may have been broken.

We need to re-examine all AST passes to make sure there are no erronsous implications from this.

In many cases, this won't matter, as analyses are flow-insensitive. And in some cases this may be good. For example, false && expr can be constant-folded to eliminate expr, even if expr has side-effects or does other interesting things.

brmataptos commented 4 months ago

Note that these operations are lowered in gen_logical_shortcut() in third_party/move/move-compiler-v2/src/bytecode_generator.rs.

A thorough manual check suggests that there are no problems with the current implementation, except that constant folding is overly conservative, only folding And and Or expressions whose operands are all constant, although in some cases the expression could be simplified to remove a later operand which has side-effects. This would be an example of an "aggressive" simplification which might cause problems for errors/warnings, so we can probably leave it out.

I note that Operation::And and Operation::Or may be re-introduced after lowering in Spec code, but in specs there should be no difference between short-cutting and non-short-cutting boolean operations.