Skip to content

Reject certain code that should return but doesn't#502

Merged
cburgdorf merged 1 commit intoargotorg:masterfrom
cburgdorf:christoph/fix/497
Aug 7, 2021
Merged

Reject certain code that should return but doesn't#502
cburgdorf merged 1 commit intoargotorg:masterfrom
cburgdorf:christoph/fix/497

Conversation

@cburgdorf
Copy link
Copy Markdown
Collaborator

What was wrong?

As described in #497 the following code does compile in master but should generate a user error instead. It needs to error because the code is missing a return statement since it is not guaranteed that the execution follows the arm of the if statement.

contract Foo:
    pub def bar(val: u256) -> u256:
        if val > 1:
            return 5

How was it fixed?

The check in all_paths_return_or_revert is wrong because it counts a missing else branch as if the code returns when clearly a missing else branch shouldn't indicate such thing.

@cburgdorf cburgdorf requested a review from sbillig August 6, 2021 12:04
if val > 1:
return 5
else:
x = 1
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was an extra error in the test file that I think we didn't intent to test for here

@cburgdorf cburgdorf force-pushed the christoph/fix/497 branch 2 times, most recently from 2f5cff9 to 17e434f Compare August 6, 2021 12:21
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 6, 2021

Codecov Report

Merging #502 (f4ddda2) into master (71a2430) will decrease coverage by 0.02%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
- Coverage   87.72%   87.69%   -0.03%     
==========================================
  Files          80       80              
  Lines        5272     5276       +4     
==========================================
+ Hits         4625     4627       +2     
- Misses        647      649       +2     
Impacted Files Coverage Δ
crates/test-files/src/lib.rs 50.00% <50.00%> (ø)
crates/analyzer/src/traversal/functions.rs 91.17% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71a2430...f4ddda2. Read the comment docs.

} => {
let body_returns = all_paths_return_or_revert(body);
let or_else_returns = or_else.is_empty() || all_paths_return_or_revert(or_else);
let or_else_returns = !or_else.is_empty() && all_paths_return_or_revert(or_else);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we could just get rid of the is_empty check, as all_paths_return_or_revert(or_else) will return false for an empty vec

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ah sure, I'll get that updated 👍

@g-r-a-n-t g-r-a-n-t self-requested a review August 6, 2021 16:24
@cburgdorf cburgdorf merged commit e362fe1 into argotorg:master Aug 7, 2021
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.

3 participants