fix[codegen]: fix double eval of start in range expr

cyberthirst commented 4 months ago

What I did

fix of: https://github.com/vyperlang/vyper/issues/3964

How I did it

cache the start argument to the stack

How to verify it

added a double eval test

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

![Put a link to a cute animal picture inside the parenthesis-->]()

charles-cooper commented 4 months ago

we should have tests for both 1 and 2 argument cases

charles-cooper commented 4 months ago

also this seems fine, but didn't we just add a fence for pop()? this has a performance penalty as well.

cyberthirst commented 4 months ago

we should have tests for both 1 and 2 argument cases

addressed in: https://github.com/vyperlang/vyper/pull/4033/commits/8b5b364d99448de5df69f01edb1b12f934dc17e8

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 52.70%. Comparing base (f0afaf0) to head (8b5b364).

:exclamation: Current head 8b5b364 differs from pull request most recent head d329685

Please upload reports for the commit d329685 to get more accurate results.

Files	Patch %	Lines
vyper/codegen/stmt.py	61.53%	4 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #4033 +/- ## =========================================== - Coverage 91.12% 52.70% -38.42% =========================================== Files 106 106 Lines 15308 15309 +1 Branches 3365 3366 +1 =========================================== - Hits 13949 8069 -5880 - Misses 925 6603 +5678 - Partials 434 637 +203 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cyberthirst commented 4 months ago

also this seems fine, but didn't we just add a fence for pop()? this has a performance penalty as well.

as discussed, it's a question of panicking for certain classes of contracts vs having some perf penalty

eg contracts like this wouldn't compile without the cache


@internal
@view
def bar() -> uint256:
    return 0

@external
def foo():
    for i:uint256 in range(self.bar(), 5, bound = 3):
        pass

vyperlang / vyper