test: indexed (compound) assignment evaluation order and single-evaluation#16756
test: indexed (compound) assignment evaluation order and single-evaluation#16756msooseth wants to merge 4 commits into
Conversation
35aab63 to
e11b7a4
Compare
cameel
left a comment
There was a problem hiding this comment.
The test looks useful, we should definitely have something that checks this evaluation order.
Should be renamed/moved though. Also some minor elements in it are more convoluted than they need to be. We could fix those before merging.
There was a problem hiding this comment.
Naming: ir_lvalue_compound_assignment.sol -> compound_assignment_evaluation_order_and_single_eval.sol. It does n
Let's also put it in operators/. Or create a separate dir for evaluation order. This viaYul/ dir is a legacy thing and we should get rid of it, moving tests to more relevant locations, not add more. I'm not even sure why they were singled out in the first place (most of them don't seem particularly IR-specific). Now it just makes it harder to check if we have a test for something already.
There was a problem hiding this comment.
I didn't know. I have moved it now :)
| // - In `a[f()] += v`, the LHS index `f()` is evaluated exactly once; | ||
| // the slot computation is not re-evaluated for the write side. | ||
| contract C { | ||
| uint[4] public a; |
There was a problem hiding this comment.
Why even have 4 items if you only ever check what's in the the first two of them? If the other two get written to due to a bug, you won't notice. Better to reduce it to 2 and get a revert in such a case.
Making them public is also pointless if you never access them from expectations.
| uint[4] public a; | |
| uint[2] a; |
There was a problem hiding this comment.
Wait, you are actually using 3 of them. Why not return them all then?
There was a problem hiding this comment.
Good catch. I am now only using 2, and returning 2!
| return counter - 1; | ||
| } | ||
|
|
||
| function bumpVal() internal returns (uint) { |
There was a problem hiding this comment.
bump sounds as if you were writing the result somewhere (bumping some state).
| function bumpVal() internal returns (uint) { | |
| function calculateVal() internal view returns (uint) { |
There was a problem hiding this comment.
Thanks, I applied your fix manually (file was moved, etc). Let me know what you think!
Pin down evaluation order and single-evaluation in indexed
(compound) assignment:
- In `a[f()] = g()`, RHS `g()` is evaluated before the LHS index
`f()`, so RHS side effects are observable when the index is
computed.
- In `a[f()] += v`, the LHS index `f()` is evaluated exactly once;
the slot computation is not re-evaluated for the write side.
Complements viaYul/tuple_evaluation_order.sol.
e11b7a4 to
dc1396b
Compare
Adds a semantic test pinning down two invariants for indexed assignment:
a[f()] = g(), the RHSg()is evaluated before the LHS indexf(), so RHS side effects are observable when the index is computed.a[f()] += v, the LHS indexf()is evaluated exactly once; the slot computation is not re-evaluated for the write side.Coverage gap
No existing semantic test exercises a side-effecting function call as the LHS index of a compound assignment. Closest neighbors and why they don't cover it:
viaYul/tuple_evaluation_order.sol— tuple-LHS order; not indexed, not compound.operators/compound_assign{,_transient_storage}.sol— compound assign on plain state vars; no side-effecting LHS.modifiers/evaluation_order.sol,operators/userDefined/operator_evaluation_order.sol,errors/require_error_evaluation_order_*.sol— evaluation order in modifiers / user-defined operators /requireargs; different mechanism.cleanup/cleanup_in_compound_assign.sol— truncation cleanup, not order or single-eval.variables/mapping_local_compound_assignment.sol— LHS-with-side-effect, but plain=.array/indexAccess/bytes_index_access.sol,libraries/internal_call_attached_with_parentheses.sol— indexed compound assign, but constant index and no side effects.Complements
viaYul/tuple_evaluation_order.sol.