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: call internal functions from constructor #2496

Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Oct 20, 2021

What I did

allow calling internal functions from constructor. fixes #1631 and #2318

How I did it

Commit message

this commit allows the user to call internal functions from the     
`__init__` function. it does this by generating a call graph during the
annotation phase and then generating code for the functions called from
the init function for during deploy code generation                 
          
this also has a performance benefit (compiler time) because we can get
rid of the two-pass method for tracing frame size.                  
          
now that we have a call graph, this commit also introduces a topsort of
functions based on the call dependency tree. this ensures we can compile
functions that call functions that occur after them in the source code.
          
lastly, this commit also refactors vyper/codegen/module.py so that the
payable logic is cleaner, it uses properties instead of calculations
more, and cleans up properties on IRnode, FunctionSignature and Context.

How to verify it

Description for the changelog

Cute Animal Picture

image

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #2496 (38ddc0f) into master (46ec802) will decrease coverage by 16.37%.
The diff coverage is 94.08%.

@@             Coverage Diff             @@
##           master    #2496       +/-   ##
===========================================
- Coverage   87.37%   71.00%   -16.38%     
===========================================
  Files          94       94               
  Lines       10054    10115       +61     
  Branches     2490     2443       -47     
===========================================
- Hits         8785     7182     -1603     
- Misses        802     2260     +1458     
- Partials      467      673      +206     
Impacted Files Coverage Δ
vyper/ir/optimizer.py 73.93% <ø> (-6.42%) ⬇️
vyper/compiler/output.py 29.37% <50.00%> (-64.41%) ⬇️
vyper/semantics/types/function.py 66.28% <87.50%> (-20.83%) ⬇️
vyper/codegen/module.py 89.71% <91.25%> (-0.18%) ⬇️
vyper/ast/signatures/function_signature.py 86.56% <93.33%> (-1.04%) ⬇️
vyper/codegen/ir_node.py 90.56% <96.29%> (-2.27%) ⬇️
vyper/ast/signatures/__init__.py 100.00% <100.00%> (ø)
vyper/ast/signatures/interface.py 40.74% <100.00%> (-40.75%) ⬇️
vyper/codegen/context.py 87.71% <100.00%> (-0.32%) ⬇️
vyper/codegen/function_definitions/__init__.py 100.00% <100.00%> (ø)
... and 65 more

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 46ec802...38ddc0f. Read the comment docs.

this commit allows the user to call internal functions from the
`__init__` function. it does this by tracing all the internal functions
called from every function's execution (during the annotation phase),
and then generating code for the functions called from the init function
for during deploy code generation

this also has a performance benefit (compiler time) because we can get
rid of the two-pass method for tracing frame size.

also, this opens the door to topsort of internal functions, if desired
(so that the user does not have to order them manually).
@charles-cooper charles-cooper changed the title wip: call internal functions from constructor feat: call internal functions from constructor May 5, 2022
@charles-cooper charles-cooper marked this pull request as ready for review May 5, 2022 19:26
fubuloubu
fubuloubu previously approved these changes May 5, 2022
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Needs tests and documentation internally talking about the flow that is taken

@charles-cooper
Copy link
Member Author

ah i ended up doing some refactoring and forgot to turn this back into a draft

@charles-cooper charles-cooper marked this pull request as draft May 6, 2022 17:28
@charles-cooper
Copy link
Member Author

btw it now depends on a change to build_gas_estimates introduced in #2647

the old way depended on traversing the IR and looking for nodes with
specific fields. this uses the function signatures produced during
codegen which are more well structured.
@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 6, 2022

This pull request introduces 3 alerts when merging a22be84 into 31e2b4e - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Wrong name for an argument in a call

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 7, 2022

This pull request introduces 1 alert when merging f75404f into 31e2b4e - view on LGTM.com

new alerts:

  • 1 for Unused import

@charles-cooper charles-cooper marked this pull request as ready for review May 8, 2022 09:00
Comment on lines +148 to +156
@internal
def _stockAvailable() -> uint256:
return self.holdings[self.company]

# Find out how much stock any address (that's owned by someone) has.
@view
@internal
def _getHolding(_stockholder: address) -> uint256:
return self.holdings[_stockholder]
Copy link
Member

Choose a reason for hiding this comment

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

These two methods aren't really that useful

Copy link
Member Author

Choose a reason for hiding this comment

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

true, i think they are good there just to demonstrate that internal functions can be defined after they are called

Comment on lines 74 to 77
else:
# frame size for internal function does not need to be adjusted
# since it is already accounted for by the caller
o.total_gas = o.gas
# note: internal functions do not need to adjust gas estimate since
# it is already accounted for by the caller.
pass
Copy link
Member

Choose a reason for hiding this comment

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

remove else case

@charles-cooper charles-cooper merged commit 4b44ee7 into vyperlang:master May 10, 2022
@charles-cooper charles-cooper deleted the feat/ctor_internal_funcs branch May 10, 2022 06:56
@charles-cooper charles-cooper changed the title feat: call internal functions from constructor fix: call internal functions from constructor May 10, 2022
tserg added a commit to tserg/vyper that referenced this pull request May 13, 2022
this commit allows the user to call internal functions from the
`__init__` function. it does this by generating a call graph during the
annotation phase and then generating code for the functions called from
the init function for during deploy code generation

this also has a performance benefit (compiler time) because we can get
rid of the two-pass method for tracing frame size.

now that we have a call graph, this commit also introduces a topsort of
functions based on the call dependency tree. this ensures we can compile
functions that call functions that occur after them in the source code.

lastly, this commit also refactors vyper/codegen/module.py so that the
payable logic is cleaner, it uses properties instead of calculations
more, and cleans up properties on IRnode, FunctionSignature and Context.
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.

vyper init function doesn't recognize private function
3 participants