Repository: vyperlang/vyper

Issue:
right-to-left evaluation in certain cases
### Version Information

* vyper Version (output of `vyper --version` OR linkable commit hash vyperlang/vyper@<commithash>): vyperlang/vyper@b43fface148b45cbd2c90b5d24f77398a63745b5
### Issue Description

The order of evaluation in Vyper is specified as left-to-right; in the
same order as the expressions appear in the the source code.

  - Due to the way IR nodes that match directly EVM opcodes are
    converted to assembly, for some built-ins and operators, it is known
    that it might not be the case as described in
    [GHSA-g2xh-c426-v8mf](https://github.com/vyperlang/vyper/security/advisories/GHSA-g2xh-c426-v8mf).

  - The compiler uses a pattern `cache_when_complex` to cache the result
    of a complex expression and avoid double evaluation. However, this
    pattern appears to be problematic regarding the order of evaluation.
    
    >   - Given an IR node that has multiple children that should be
    >     evaluated in a specific order, it might be that not all
    >     children are cached. A cached complex child would hence be
    >     evaluated before any non-cached children independently of
    >     their order in the source code. This behavior is notably seen
    >     in assignments given that `make_setter()` caches `src` but not
    >     `dst`.
    >   - A more subtle issue is that the compiler might cache all
    >     children of an IR node, however, one or multiple nodes are not
    >     considered as complex and their evaluation is inlined.
    >     although in that case, a non-complex node cannot have side
    >     effects, its evaluation can still read (e.g a `SLOAD`) side
    >     effects from a complex node that was cached and evaluated
    >     first. This case appears to be problematic for the `slice()`
    >     and `extract32()` built-ins.

  - Another issue regarding the order of evaluation is that the order of
    evaluation of the kwargs passed to built-ins and call expressions do
    not follow the order of the source code.

#### POC

The following examples show multiple cases where the order of evaluation
is right-to-left.

``` python
i:uint256

@internal
def change_i() -> uint256:
    self.i += 1
    return 12

@external
def foo() -> DynArray[uint256,2]:
    x:DynArray[uint256,2] = [1,2]
    x[self.i] += self.change_i()
    return x # returns [13,2]
```

``` python
boo:Bytes[32]

@internal
def bar() -> uint256:
    self.boo = b'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
    return 0

@internal
def baz() -> Bytes[32]:
    return self.boo

@external
def slice() -> (Bytes[32], Bytes[32]):
    self.boo = b'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
    s: Bytes[1] = slice(self.boo, self.bar(), 1)

    self.boo = b'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
    t: Bytes[1] = slice(self.baz(), self.bar(), 1)

    return s,t # (b'a', b'b')

@external
def extract32() -> (bytes32, bytes32):
    self.boo = b'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
    s: bytes32 = extract32(self.boo, self.bar())

    self.boo = b'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
    t: bytes32 = extract32(self.baz(), self.bar())

    return s,t #(b'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', b'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb')
```

``` python
@external
@payable
def bar(): return

interface Bar:
    def bar(): payable

x: uint256

@internal
def gas() -> uint256:
    self.x = 2
    return 100000

@internal
def value() -> uint256:
    self.x = 1
    return 0

@external
@payable
def foo():
    extcall Bar(self).bar(gas=self.gas(), value=self.value())    
    temp: uint256 = self.x
    extcall Bar(self).bar(value=self.value(), gas=self.gas())    
    assert self.x == temp # passes
```


Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/vyperlang/vyper/pull/4156?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vyperlang) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 92.09%. Comparing base [(`4d693b7`)](https://app.codecov.io/gh/vyperlang/vyper/commit/4d693b7212be28171a2a75a452d44659c0d80444?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vyperlang) to head [(`05ff408`)](https://app.codecov.io/gh/vyperlang/vyper/commit/05ff4080134682d07cdfbd6c33ae95eb4650c18b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vyperlang).

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##           master    #4156   +/-   ##
=======================================
  Coverage   92.09%   92.09%           
=======================================
  Files         119      119           
  Lines       16931    16936    +5     
  Branches     2865     2865           
=======================================
+ Hits        15593    15598    +5     
  Misses        919      919           
  Partials      419      419           
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/vyperlang/vyper/pull/4156?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vyperlang).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vyperlang).


Patch:
diff --git a/vyper/builtins/functions.py b/vyper/builtins/functions.py
index af5045d2..d9f8f48e 100644
--- a/vyper/builtins/functions.py
+++ b/vyper/builtins/functions.py
@@ -21,6 +21,7 @@ from vyper.codegen.core import (
     clamp_basetype,
     clamp_nonzero,
     copy_bytes,
+    create_memory_copy,
     dummy_node_for_type,
     ensure_eval_once,
     ensure_in_memory,
@@ -357,9 +358,8 @@ class Slice(BuiltinFunctionT):
             # copy_bytes works on pointers.
             assert is_bytes32, src
             src = ensure_in_memory(src, context)
-
-        if potential_overlap(src, start) or potential_overlap(src, length):
-            raise CompilerPanic("risky overlap")
+        elif potential_overlap(src, start) or potential_overlap(src, length):
+            src = create_memory_copy(src, context)
 
         with src.cache_when_complex("src") as (b1, src), start.cache_when_complex("start") as (
             b2,
@@ -867,7 +867,7 @@ class Extract32(BuiltinFunctionT):
         ret_type = kwargs["output_type"]
 
         if potential_overlap(bytez, index):
-            raise CompilerPanic("risky overlap")
+            bytez = create_memory_copy(bytez, context)
 
         def finalize(ret):
             annotation = "extract32"
diff --git a/vyper/codegen/core.py b/vyper/codegen/core.py
index 789cc775..490cfd53 100644
--- a/vyper/codegen/core.py
+++ b/vyper/codegen/core.py
@@ -1120,10 +1120,13 @@ def ensure_in_memory(ir_var, context):
     if ir_var.location == MEMORY:
         return ir_var
 
+    return create_memory_copy(ir_var, context)
+
+
+def create_memory_copy(ir_var, context):
     typ = ir_var.typ
     buf = context.new_internal_variable(typ)
     do_copy = make_setter(buf, ir_var)
-
     return IRnode.from_list(["seq", do_copy, buf], typ=typ, location=MEMORY)
 
 


Test patch:
diff --git a/tests/functional/builtins/codegen/test_extract32.py b/tests/functional/builtins/codegen/test_extract32.py
index f8db51ee..36c1ea5c 100644
--- a/tests/functional/builtins/codegen/test_extract32.py
+++ b/tests/functional/builtins/codegen/test_extract32.py
@@ -1,7 +1,6 @@
 import pytest
 
 from vyper.evm.opcodes import version_check
-from vyper.exceptions import CompilerPanic
 
 
 @pytest.mark.parametrize("location", ["storage", "transient"])
@@ -101,8 +100,6 @@ def foq(inp: Bytes[32]) -> address:
         c.foq(b"crow" * 8)
 
 
-# to fix in future release
-@pytest.mark.xfail(raises=CompilerPanic, reason="risky overlap")
 def test_extract32_order_of_eval(get_contract):
     extract32_code = """
 var:DynArray[Bytes[96], 1]
@@ -123,8 +120,6 @@ def foo() -> bytes32:
     assert c.foo() == b"defghijklmnopqrstuvwxyz123456789"
 
 
-# to fix in future release
-@pytest.mark.xfail(raises=CompilerPanic, reason="risky overlap")
 def test_extract32_order_of_eval_extcall(get_contract):
     slice_code = """
 var:DynArray[Bytes[96], 1]
diff --git a/tests/functional/builtins/codegen/test_slice.py b/tests/functional/builtins/codegen/test_slice.py
index 3f2ce44e..0777b2f0 100644
--- a/tests/functional/builtins/codegen/test_slice.py
+++ b/tests/functional/builtins/codegen/test_slice.py
@@ -5,12 +5,7 @@ from hypothesis import given, settings
 from vyper.compiler import compile_code
 from vyper.compiler.settings import OptimizationLevel, Settings
 from vyper.evm.opcodes import version_check
-from vyper.exceptions import (
-    ArgumentException,
-    CompilerPanic,
-    StaticAssertionException,
-    TypeMismatch,
-)
+from vyper.exceptions import ArgumentException, StaticAssertionException, TypeMismatch
 
 _fun_bytes32_bounds = [(0, 32), (3, 29), (27, 5), (0, 5), (5, 3), (30, 2)]
 
@@ -575,8 +570,6 @@ def foo(cs: String[64]) -> uint256:
     assert c.foo(arg) == 1
 
 
-# to fix in future release
-@pytest.mark.xfail(raises=CompilerPanic, reason="risky overlap")
 def test_slice_order_of_eval(get_contract):
     slice_code = """
 var:DynArray[Bytes[96], 1]
@@ -600,8 +593,6 @@ def foo() -> Bytes[96]:
     assert c.foo() == b"defghijklmnopqrstuvwxyz123456789"
 
 
-# to fix in future release
-@pytest.mark.xfail(raises=CompilerPanic, reason="risky overlap")
 def test_slice_order_of_eval2(get_contract):
     slice_code = """
 var:DynArray[Bytes[96], 1]