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

fix: complex arguments to builtin functions #3167

Merged
merged 25 commits into from
May 18, 2023

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Dec 4, 2022

What I did

Fix #3164, and also ecadd and ecmul.

How I did it

Cache the argument.

How to verify it

See tests.

Commit message

prior to this commit, some builtin functions including ceil, would panic
if their arguments were function calls (or otherwise determined to be
complex expressions by `is_complex_ir`). this commit fixes the relevant
builtin functions by using `cache_when_complex` where appropriate.

Description for the changelog

Fix codegen for function calls as argument in builtin functions

Cute Animal Picture

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

@tserg tserg changed the title fix: codegen for function calls as argument in ceil fix: codegen for function calls as argument in builtin functions Dec 4, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2022

Codecov Report

Merging #3167 (5323481) into master (f450cb1) will increase coverage by 0.27%.
The diff coverage is 92.59%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #3167      +/-   ##
==========================================
+ Coverage   89.05%   89.32%   +0.27%     
==========================================
  Files          86       84       -2     
  Lines       10792    10777      -15     
  Branches     2449     2459      +10     
==========================================
+ Hits         9611     9627      +16     
+ Misses        781      754      -27     
+ Partials      400      396       -4     
Impacted Files Coverage Δ
vyper/builtins/functions.py 90.51% <92.59%> (+1.29%) ⬆️

... and 25 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tserg tserg marked this pull request as ready for review December 4, 2022 14:29
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.

codegen looks good, but i don't think the test cases are testing the case properly. i think a better test would have (non-idempotent) side effects in the called contract so you can check if the side effects are getting invoked once or multiple times.

@charles-cooper
Copy link
Member

also, since the test functionality is repeated (here, and also in other places but we don't need to worry about the other places just now), it could be good to have some scaffolding / utilities which make this easier. i'm thinking like, a common contract which is intended to be called for its side effects, and a utility function which checks that the side effect was invoked exactly one time.

@tserg
Copy link
Collaborator Author

tserg commented Dec 5, 2022

I have added two fixtures side_effects_contract and assert_side_effect_invoked_once to standardise the test for side effects. What do you think?

@charles-cooper
Copy link
Member

I have added two fixtures side_effects_contract and assert_side_effect_invoked_once to standardise the test for side effects. What do you think?

the fixtures look a bit weird to me, but i'm not sure there is a much cleaner way to do it

@tserg
Copy link
Collaborator Author

tserg commented Dec 8, 2022

I am not sure why this call to foo() reverts if this contract was in the test suite. As long as there is more than one external call in uint256_addmod regardless of the argument position, the transaction reverts even if the gas is set to a high value. If tested in boa, foo() works fine.

@external
def foo(addr: address) -> uint256:
    f: Foo = Foo(addr)
    return uint256_addmod(f.a(), f.b(), f.c())

interface Foo:
    def a() -> uint256: payable
    def b() -> uint256: payable
    def c() -> uint256: payable

I will set the argument for only c to the external call (e.g. uint256_addmod(32, 2, f.c())) for the purpose of this PR.

tests/conftest.py Outdated Show resolved Hide resolved
@charles-cooper charles-cooper added this to the v0.3.8 milestone May 14, 2023
@charles-cooper charles-cooper changed the title fix: codegen for function calls as argument in builtin functions fix: codegen panic for complex arguments to builtin functions May 18, 2023
@charles-cooper charles-cooper changed the title fix: codegen panic for complex arguments to builtin functions fix: complex arguments to builtin functions May 18, 2023
@charles-cooper charles-cooper enabled auto-merge (squash) May 18, 2023 17:05
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.

cleaned up the side effects checker API a little in 5323481

@charles-cooper charles-cooper merged commit 6ee74f5 into vyperlang:master May 18, 2023
@tserg tserg deleted the fix/ceil branch May 19, 2023 02:11
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.

Multiple evaluation of argument leads to non-unique symbol error in some built-in fuctions
4 participants