Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove duplicate terminus checking code #3541

Merged
merged 13 commits into from
Jan 14, 2024

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Jul 27, 2023

What I did

Remove check_single_exit and is_return_from_function which duplicates is_terminus_node/check_for_terminus.

also fixes #3265

How I did it

Delete

How to verify it

Tests still pass

Commit message

refactor: remove duplicate terminus checking code

remove `check_single_exit` and `is_return_from_function` which duplicate
functionality in `is_terminus_node`/`check_for_terminus`.

additionally rewrite termination checking routine to be simpler, and
also fix an outstanding analysis bug where the following program would
not be rejected:

```vyper
@external
def foo(a: bool) -> uint256:
    if a:
        return 1
    else:
        return 2
    pass  # unreachable
```

Description for the changelog

Remove duplicate check for terminus node.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

vyper/codegen/stmt.py Fixed Show fixed Hide fixed
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (9cf66c9) 84.18% compared to head (d5c94a9) 84.20%.

Files Patch % Lines
vyper/ast/nodes.py 90.90% 1 Missing and 1 partial ⚠️
vyper/semantics/analysis/local.py 94.44% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3541      +/-   ##
==========================================
+ Coverage   84.18%   84.20%   +0.01%     
==========================================
  Files          92       92              
  Lines       13137    13133       -4     
  Branches     2928     2926       -2     
==========================================
- Hits        11059    11058       -1     
+ Misses       1659     1655       -4     
- Partials      419      420       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tserg tserg marked this pull request as ready for review July 27, 2023 10:37
vyper/codegen/stmt.py Fixed Show fixed Hide fixed
vyper/ast/nodes.py Fixed Show fixed Hide fixed
vyper/ast/nodes.py Fixed Show fixed Hide fixed
vyper/ast/nodes.py Fixed Show fixed Hide fixed
@charles-cooper
Copy link
Member

@tserg could you take a look at the merge conflict?

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the following program is (correctly) rejected on master but allowed on this branch:

@external
def foo():
    return
    return

fix dead code detection
fix detection of returns after valid branching returns
add tests
vyper/ast/nodes.py Dismissed Show dismissed Hide dismissed
@charles-cooper
Copy link
Member

looks like the following program is (correctly) rejected on master but allowed on this branch:

@external
def foo():
    return
    return

i fixed this and also rewrote the check_terminator routine to be much simpler. i also fixed #3265 at the same time (see: 7b2c74b#diff-26eeb855b192798bd168b623451c4787dcee61bb51b19999cfc6a97881d6feb5R90-R91)

@charles-cooper charles-cooper changed the title chore: remove duplicate check for terminus node refactor: remove duplicate terminus checking code Jan 14, 2024
@charles-cooper charles-cooper enabled auto-merge (squash) January 14, 2024 18:02
@charles-cooper charles-cooper enabled auto-merge (squash) January 14, 2024 18:03
@charles-cooper charles-cooper merged commit af5c49f into vyperlang:master Jan 14, 2024
84 checks passed
@tserg tserg deleted the refactor/single_exit branch January 15, 2024 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

if else with exit statements in both branch with succeeding code not prevented
3 participants