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

Force return statement check #590

Closed
jacqueswww opened this issue Dec 15, 2017 · 22 comments · Fixed by #1384
Closed

Force return statement check #590

jacqueswww opened this issue Dec 15, 2017 · 22 comments · Fixed by #1384

Comments

@jacqueswww
Copy link
Collaborator

@jacqueswww jacqueswww commented Dec 15, 2017

What's your issue about?

Currently you can have a return type set on a function, yet omit the return statement. Add a exception when this happens forcing the programming to add a return, if specified.

e.g.

@public
def foo() -> num:
        pass

How can it be fixed?

Off the top of my head, check parse_func.

Cute Animal Picture

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 15, 2017

As I think @yzhang90 has noted, this is actually subtletly very hard (but I'd argue still necessary) because of blocks, for example the following would pass your check but still fail to return anything:

def Foo(a: bool) -> num:
    if a:
         return 1
@DavidKnott

This comment has been minimized.

Copy link
Contributor

@DavidKnott DavidKnott commented Dec 15, 2017

I think we could track each path and make sure each one has a return. That being said, a good first step would be to check for the existence of any return.

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 15, 2017

Agreed. And I think currently viper programs will probably be simple enough that most if branches in methods would be nested only once or twice (e.g. if: if: ... else: ... else: if: ... else: ...), so the number of total branches to keep track of should be fairly managable. I think the right upper bound is 2^n branches where n is the amount of nesting. The gas limit should impose an economic upper bound on this I would think.

@jacqueswww

This comment has been minimized.

Copy link
Collaborator Author

@jacqueswww jacqueswww commented Dec 15, 2017

Actually it's probably much simpler than that, we have a checker that check correct return statements by type. And we just then need to make sure each function has at minimum one return. All those in the ifs will be handled by the return type checker, ie. Not None. ;)

@jacqueswww

This comment has been minimized.

Copy link
Collaborator Author

@jacqueswww jacqueswww commented Dec 15, 2017

I will tackle this one next, it's just building a map of functions that have return and erroring if any are missing (my gut feel says just populating a map in parse_return) , will show you :)

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 15, 2017

@jacqueswww How does your proposal deal with this:

def foo(a: bool) -> num:
    if a:
        return 1
@jacqueswww

This comment has been minimized.

Copy link
Collaborator Author

@jacqueswww jacqueswww commented Dec 15, 2017

Ah I see, now. I have a few ideas. Looking forward to challenge ;)

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 15, 2017

or this (this should pass):

def foo(a: bool) -> num:
    b: num = 0
    if a:
        b = 1
    else:
        return 0
    return b
@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 15, 2017

It shouldn't be too hard, my point is it is a harder algorithm because you have to explore 2**n branches

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 15, 2017

And also the result if no return is taken in some subset of branches (and deferred to the end)

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 15, 2017

P.S. I think this is actually a formal verification problem lol

@jacqueswww

This comment has been minimized.

Copy link
Collaborator Author

@jacqueswww jacqueswww commented Dec 15, 2017

Yes you are right, one will have to figure out a way for the branches to take the default else into account, hmmm let me research it some more. 😂

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 15, 2017

Yeah, I think there are standard FV algorithms for doing this, I think it usually involves holding an additional bookkeping variables while you're parsing a given function, then testing those instead.

@jacqueswww

This comment has been minimized.

Copy link
Collaborator Author

@jacqueswww jacqueswww commented Dec 18, 2017

@fubuloubu @DavidKnott consider this rule set:

1.) There must be at least one return statement in every function.
2.) If there is a nested return statement a default return must be set OR trace all other nested returns and ensure they exist.

The problem with a nested return comes in that all paths have to be considered as you mentioned, however if we force the rule of a default return statement. Are we perhaps gaining clarity in the program ? And thereby simplifying this check. I personally always read default returns better than nested ones:

def a() -> num:
    if ...:
        return 1
    else:
        return 2

Is more difficult for me to read than:

def a() -> num:
    if ...:
        return 1
    return 2

Maybe I am the exception ;) But the one thing we can agree on to implement so long, is the check for rule 1. I will proceed with the partial PR so long that does that check.

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 18, 2017

1 is a good start and get's us halfway there, I think that is okay to add.

2 requires that much more advanced check, and since checking for a default return is a special case of the general problem of branched returns, I don't think we want to arbitrarily disallow a certain type of style as a first step, so I think we need to leave 2 to when both parts of the check can be performed. Which means really just the general case check of branched returns, since default return is simply a special case.

2 will override 1 when it is complete.

jacqueswww added a commit to jacqueswww/vyper that referenced this issue Dec 19, 2017
… statement.
@yzhang90

This comment has been minimized.

Copy link

@yzhang90 yzhang90 commented Dec 20, 2017

@fubuloubu @jacqueswww
After @daejunpark and I think about this issue twice, I think a sound and complete analysis (both in general and specific to this) is not decidable. So, you need to be conservative, and there is a very simple but reasonable conservative algorithm for this, similar to that of Java ( the complexity is linear to the number of statement in a program, not 2**n).

Let me first start with an example(just some pseudo code):

def foo(n:num)->num:
for i in range(10):
    if i == f(n):
        return 0

This is valid in case f(n) < 10 for any n, but it may be involving, if not undecidable, to fully automatically analyze this. Also, in many cases, this style of coding is not best practice. So, it is reasonable to force users to conservatively add return statements even in places that are not reachable (e.g., outside of the loop in the example), which allows us to have a very simple and efficient analysis as outlined below. Note that Java compiler does the same thing.

With this in mind, let me describe the algorithm:

  1. You can think of if ... as if ... else pass, so you only need to consider about if...else... case.
  2. Just a sketch of the code:
// return true means the function pass the return check.
def check_return(statements){ 
    for st in statements{
        if(st is return){
            return true
         }
        if(statement is if...else...) {
            then_br_return = check_return(then_stmts)
            else_br_return = check_return(else_stmts)
            if(then_br_return && else_br_return) {
                return true
            }
       }
    }
    return false
}

You just need to call check_return on the the function body.
P.S., this function can also be slightly modified to check dead code as well.

@DavidKnott

This comment has been minimized.

Copy link
Contributor

@DavidKnott DavidKnott commented Dec 21, 2017

@yzhang90 The idea would be adding a return at the end even without for loops correct?
The trigger being lone if or if..elif statements because a return from them cannot easily (or not at all if based on input) be guaranteed.

@yzhang90

This comment has been minimized.

Copy link

@yzhang90 yzhang90 commented Dec 21, 2017

@DavidKnott I'm not sure I fully got your questions. What we propose here is an simple algorithm to conservatively check whether a function has return statement on all possible path. If the check_return function returns false, the compiler should simply reject the program (even though the program is actually correct/valid, e.g., you change f(n) to 8 in my previous example). I'm NOT proposing adding return at the end (or do any transformation of the program). I think it is better to let programmers know that they're missing some cases instead of return some default values for them.

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 21, 2017

@DavidKnott Kind of. You can say that an end return statement is a special case of this algorithm, and given it's existence you can just skip the check because the answer is trivial.

The if in for loop is the real problem, you cannot gain insight into whether a return is possible without simulating all possibilities of the code, which is infeasible. I think a good shortcut for that case is to basically skip checking inside a for loop block.

@yzhang90

This comment has been minimized.

Copy link

@yzhang90 yzhang90 commented Dec 21, 2017

The following program should be rejected:

def foo()->num:
    for i in range(10):
        return 0

People should instead write:

def foo()->num:
    for i in range(10):
        return 0
    return 0

Though the second return is never reachable.

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 21, 2017

Summarize:

REJECT

for i in list:
    if i == n:
        return i

ACCEPT

for i in list:
    if i == n:
        return i
return 0 # Accept because "End return statement" exists

REJECT

for i in list:
    if i == n:
        return i
if n > len(list):
    return 0
@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Dec 21, 2017

So slight modification to their algorithm:

def check_return(statements):
    if statements[-1] is 'return' and statements[-1] is at base indentation:
        return True # Trivial case, probably 60% of all cases
    for st in statements:
        if(st is 'return'):
            return True
        if(statement is 'if then_stmts else else_stmts'):
            then_br_return = check_return(then_stmts)
            else_br_return = check_return(else_stmts)
            if(then_br_return and else_br_return):
                return True
    return False
DavidKnott added a commit to DavidKnott/viper that referenced this issue Dec 22, 2017
Dexaran added a commit to Dexaran/viper that referenced this issue Dec 25, 2017
* Added get_logs method to return a list of logs with optional filtering; changed get_log fixture to use last entry in get_logs list for specific log name

* WIP overhaul of types

* Draft of viper-by-example.

* fixes voting with delegation example

* use falsey-ness of 0x0000000000000000000000000000000000000000

* use falsey-ness of 0x0000000000000000000000000000000000000000 (reverted from commit 21cc53a)

* use falsey-ness of 0x0000000000000000000000000000000000000000

* iff --> if and only if

* Adds basic for loop for in memory list.

* Adds test for literal list.

* Adds for i in list support from storage and list literals.

* Adds support for i in list of other BaseTypes.

* Add StructureException testcase for iterating over a nested list.

* Allow skip of the event name

* Change function to func

* Changed get_log() to get_last_log(), removed optional log name from usages

* Make corrections according to suggestions.

* Minor typo.

* Adds basic bytes as map keys support.

* Adds support for mapping literal bytes.

* fix the problems when install viper in Ubuntu16.04

* Adds test for long than 32 bytes keys.

* Adds test for mismatched bytes size map assign.

* Adds variable name to i_pos, so it doesn't conflict.

* Adds exceptions for altering lists whilst iterating them.

* Fixe for altering list in iteration exception check.

* Adds tests for assert x in list

* Added reference types.

* Add the packaging metadata to build the viper snap

* Improves ownership test - to be more realistic.

* Modified assert_tx_failed() to remove reference to tester as tester reference inside setup_transaction_tests.py was referencing the same module

* Add 65 and 25 gas to CLAMP and ASSERT pseudo opcodes, based on test_decimals.py

* Various changes to gas estimation.

- Fixes with statement estimation to total all underlying args.
- Adds add_gas parameter to LLLNode to allow more advanced gas calculations to
be assigned to an LLLNode, see make_byte_array_copier as an example.

* Replace get_contract with get_contract_with_gas_estimation.

* Adds test for keys with a very long byte length.

* Removed compound calling, small touchups

* Minor spelling fix

* Remove empty test

* Refactor code parsing

* Fix typo in folder name

* Make pseudo opcode gas estimate a single value.

* Add test for colored gas estimate output when using -f ir.

* Remove tester from assert_tx_failed.

* Move test_company file into proper folder

* Fix constant clamp checks / change constant clamp checks to compile time

* Add fixture to directly test LLL

* Add clamp tests at the LLL level

* Break out `test_parser.py` into test specific files

* Change to assert_tx_failed

* Add staticcall opcode for constant function calls

* Test constant external function calls

* Remove repetitive assert_tx_failed

* Create Viper VIP template

* Add link to VIP template to the issue template

* Improve rlp decoding gas estimation

* Add VIP link to contributing docs

* fix issue vyperlang#453

* use clamp constants for literals as well

* fix num_min_literal parsing failure

* fix test coverage

* fix issue vyperlang#448: compile_lll.py: buggy translation of LLL operator `sha3_32`

* add sha3_32 compilation test

* Add checks preventing assigning values on defining types.

* Fix gas estimation error

* Fix repeat LLL with <= 0 iteration

* Add test for repat lll

* Add depth check to with "set" statement

* Require function visibility declaration

* Add tests for public decorator

* Add public decorator to tests (vyperlang#474)

* Document function visibility requirements

* Clarifies expectation in error message

This would have helped me when I saw this error learning Viper for the first time.

* Note internal or public decorator requirement

* Fixes test with for function visibility.

* Add the install instructions for the Viper snap

This requries vyperlang#445, and enabling the continuous delivery on https://build.snapcraft.io

* Allow for gas estimation tests for constants

* Add gas estimation for create_with_code_of

* ad logs to company.v.py with tests

* document event in structure of a contract

* move events before global declarations

* remove unused import

* types.py: Remove duplicated method

Fix vyperlang#488

* Adds support -f bytecode_runtime.

* cleans up test_logs

* Use revert opcode for assert statements

* Show lineno of base_type_conversion error

Fix vyperlang#498

* Show lineno of undeclared function visibility

Fix vyperlang#489

* Adds basic test for bytecode_runtime generation.

* add clamp_nonzero to modulo arithemetic

* Add modulo tests and refactor simple auction example

* Use `with` to decrease repetition

* Change vipercoin from returning false to throwing

* Add check to as_num256 to make sure number is greater than 0

* Improve contributing .rst

- Fixes a link that wasn't displaying correctly.
- Makes some instructions more concise.
- Changes old `:ref:building-from-source`, to link to installation instructions.

* Added testing and deployment doc

* Fix augassignment checks

* Test for in list invalids

* Remove repetitive error and fix typo

* Begin adding built in erc20 external call functionality

* Add implied erc20 token abi fucntionality

* Add on chain market maker example

* Test implied erc20 abi functionality

* Update on chain market maker and erc20 abi

* Change @Internal decorator to @Private decorator.

* Add support for logging a variable of list type.

* Adds tests for logging of list types.

* Fix for log packing on list variable.

* Add error if log has wrong number of arguments

* Add log test and improve bytes as input

* Improve variable naming / fix bytes as input

* Clean up tests

* Adds Fix vyperlang#531. Check return type of lists match.

* Adds more tests for logging of lists.

* Adds Fix vyperlang#504 for clamping decimal literals correctly.

* Use revert for clamps

* Fix exponents with units

* Create a contract data type

* Remove .python-version, update .gitignore

* Improve vipercoin example: link out to resources describing attack vectors

* Test the contract data type

* Update version of ethereum

* Improves gas estimate on internal call.

* Removed suicide keyword

* Carry over add_gas_estimate in optimizer.

* Adds gas estimation for internal calls.

* Added test for invalid keyword

* not official a language > officially

* Changing notes to be indented blocks starting on the line after .. note:: 

https://sublime-and-sphinx-guide.readthedocs.io/en/latest/notes_warnings.html

* Adds storage list logging support.

* Adds test to check list logging is of correct (sub)type.

* Splitting comments over two lines where necessary to fit on the page on readthedocs

* Update to use conftest; fix some incorrect tests

* Update Safe Remote Purchases: line numbers, minor edits

* Updates from code review

* Add modulo math checks

* Adds support for logging bytes from storage.

* Adds exception to limit to 32 bytes of bytes type logging (restriction for the time being).

* virtualenv -p /usr/local/lib/python3.6/bin/python3 > virtualenv -p python3.6

Related: vyperlang#558

* Add a note on how to fix `fatal error: openssl/aes.h: No such file or directory` in the `make` output

* removed extraneous backtick `

* Minor edits

with ``num`` type > with the ``num`` type
crowdfunding period is over - as determined> crowdfunding period is over—as determined

* Minor edits voting

add a comma > to vote, and each
add a comma > to vote, or

* Improve for loop error

* Adds test for logging a decimal list.

* :ref:`function-calls` > :ref:`Function-calls`

* Minor edits types.rst

* Add length check to event topics

* Fix and test logging with units

* Improve type parsing error message

* Add valid call keyword list to utils

* Add length check to event topics

* Partial fix for vyperlang#590. Adds check for a minimum of one return statement.

* Fixes test that were missing return statements.

* Adds example of return checker behaving incorrectly.

* Update dockerfile to use slim image base

* Remove commented out directives

* Reduce num256_add gas cost

* Tag built image as viper

* Add built-in function docs

* Fix on chain market maker

* Add comments to on chain market maker

* Change badges from viper to vyper
anistark added a commit to anistark/viper that referenced this issue Jan 12, 2018
* Change to assert_tx_failed

* Add staticcall opcode for constant function calls

* Test constant external function calls

* Remove repetitive assert_tx_failed

* Create Viper VIP template

* Add link to VIP template to the issue template

* Improve rlp decoding gas estimation

* Add VIP link to contributing docs

* fix issue vyperlang#453

* use clamp constants for literals as well

* fix num_min_literal parsing failure

* fix test coverage

* fix issue vyperlang#448: compile_lll.py: buggy translation of LLL operator `sha3_32`

* add sha3_32 compilation test

* Add checks preventing assigning values on defining types.

* Fix gas estimation error

* Fix repeat LLL with <= 0 iteration

* Add test for repat lll

* Add depth check to with "set" statement

* Require function visibility declaration

* Add tests for public decorator

* Add public decorator to tests (vyperlang#474)

* Document function visibility requirements

* Clarifies expectation in error message

This would have helped me when I saw this error learning Viper for the first time.

* Note internal or public decorator requirement

* Fixes test with for function visibility.

* Add the install instructions for the Viper snap

This requries vyperlang#445, and enabling the continuous delivery on https://build.snapcraft.io

* Allow for gas estimation tests for constants

* Add gas estimation for create_with_code_of

* ad logs to company.v.py with tests

* document event in structure of a contract

* move events before global declarations

* remove unused import

* types.py: Remove duplicated method

Fix vyperlang#488

* Adds support -f bytecode_runtime.

* cleans up test_logs

* Use revert opcode for assert statements

* Show lineno of base_type_conversion error

Fix vyperlang#498

* Show lineno of undeclared function visibility

Fix vyperlang#489

* Adds basic test for bytecode_runtime generation.

* add clamp_nonzero to modulo arithemetic

* Add modulo tests and refactor simple auction example

* Use `with` to decrease repetition

* Change vipercoin from returning false to throwing

* Add check to as_num256 to make sure number is greater than 0

* Improve contributing .rst

- Fixes a link that wasn't displaying correctly.
- Makes some instructions more concise.
- Changes old `:ref:building-from-source`, to link to installation instructions.

* Added testing and deployment doc

* Fix augassignment checks

* Test for in list invalids

* Remove repetitive error and fix typo

* Begin adding built in erc20 external call functionality

* Add implied erc20 token abi fucntionality

* Add on chain market maker example

* Test implied erc20 abi functionality

* Update on chain market maker and erc20 abi

* Change @Internal decorator to @Private decorator.

* Add support for logging a variable of list type.

* Adds tests for logging of list types.

* Fix for log packing on list variable.

* Add error if log has wrong number of arguments

* Add log test and improve bytes as input

* Improve variable naming / fix bytes as input

* Clean up tests

* Adds Fix vyperlang#531. Check return type of lists match.

* Adds more tests for logging of lists.

* Adds Fix vyperlang#504 for clamping decimal literals correctly.

* Use revert for clamps

* Fix exponents with units

* Create a contract data type

* Remove .python-version, update .gitignore

* Improve vipercoin example: link out to resources describing attack vectors

* Test the contract data type

* Update version of ethereum

* Improves gas estimate on internal call.

* Removed suicide keyword

* Carry over add_gas_estimate in optimizer.

* Adds gas estimation for internal calls.

* Added test for invalid keyword

* not official a language > officially

* Changing notes to be indented blocks starting on the line after .. note:: 

https://sublime-and-sphinx-guide.readthedocs.io/en/latest/notes_warnings.html

* Adds storage list logging support.

* Adds test to check list logging is of correct (sub)type.

* Splitting comments over two lines where necessary to fit on the page on readthedocs

* Update to use conftest; fix some incorrect tests

* Update Safe Remote Purchases: line numbers, minor edits

* Updates from code review

* Add modulo math checks

* Adds support for logging bytes from storage.

* Adds exception to limit to 32 bytes of bytes type logging (restriction for the time being).

* virtualenv -p /usr/local/lib/python3.6/bin/python3 > virtualenv -p python3.6

Related: vyperlang#558

* Add a note on how to fix `fatal error: openssl/aes.h: No such file or directory` in the `make` output

* removed extraneous backtick `

* Minor edits

with ``num`` type > with the ``num`` type
crowdfunding period is over - as determined> crowdfunding period is over—as determined

* Minor edits voting

add a comma > to vote, and each
add a comma > to vote, or

* Adds basic single list argument call functionality.

* Improve for loop error

* Adds test for logging a decimal list.

* :ref:`function-calls` > :ref:`Function-calls`

* Minor edits types.rst

* Fixes handling of static array in call.

- Also adds tests for many mixed cases of calling with a list parameter.

* Adds test for mixed bytes - list calling arguments.

* Add length check to event topics

* Fix and test logging with units

* Improve type parsing error message

* Add valid call keyword list to utils

* Add length check to event topics

* Partial fix for vyperlang#590. Adds check for a minimum of one return statement.

* Fixes test that were missing return statements.

* Adds example of return checker behaving incorrectly.

* Adds if statement block scoping.

* Add blockscoping to for loop.

* Add blockscope tests.

* Update dockerfile to use slim image base

* Remove commented out directives

* Reduce num256_add gas cost

* Tag built image as viper

* Add built-in function docs

* Fix on chain market maker

* Add comments to on chain market maker

* Change badges from viper to vyper

* Fix rlp decoding loop memory

* Fix call data copy gas estimation

* Implement continue

Add `continue` to pseudo_opcodes, LLL, and viper.

* Fix example ERC20 token

Discovered that it doesn't compile by trying to compile all of the
example contracts in `examples/`. This fixes ERC20.v.py by resolving
compiler complaints.

* Add internal call arg count check

* cleaned up linter errors under tests/compiler

* cleaned up imports and replaced boolean assertion under test_simple_auction

* fixed linter errors in tests/examples/company

* fixed linter errors on tests/examples/market_maker

* fixed linter errors under tests/exmaples/safe_remote_purchase

* cleaned up linter errors tests/examples/tokens

* fixed linter errors on tests/examples/voting

* fixed linter errors on tests/examples/wallets

* fixed linter errors in tests/parsers/features

* test_extract32.py: renamed first function to test_extract32_extraction

* fixed linter errors

* cleaned up linter errors tests/parser

* Adds test for passing list from storage.

* Make blockscopes use set() instead of list().

* specified flake8 script on travis to include tests/

* cleaned up exception block for test_test_bytes

* cleaned up bare except under test_arbitration_code

* cleaned up bare imports to handle ValueOutOfBounds exception in tests/parser/features/test_constructor.py

* cleaned up bare imports test_raw_call

* test_test_slice4: cleaned up test assertions for raised exceptions

* cleaned up bare excepts in test_extract32

* fixed linter error tests/parser/features/test_internal_call.py

* setup.cfg: set flake8 exclusion only for docs

* Fixes vyperlang#547, correct handling of units for exponent notation.

* Adds tests for unit exponents.

* edited travis script for flake8

* Fix err msg for functions without enough arguments

* Add an error message for calls to undefined top-level functions

Previously, the compiler would error ungracefully if an undefined
top-level function was called, due to an unhandled case in `Expr.call`.

The compiler now also makes suggestions when a non-existent top-level function
is mistakenly called instead of a contract method.

* Add `.gitattributes` syntax highlighting trick

* Add .gitattributes file

* Prevent mixed list type from occuring.

* Fixes test that contain mixed list types.

* pep8 fixes

* Remove num256 greater than 0 check for bytes32

* Bump version: 0.0.2 → 0.0.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.