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

feat[lang]: singleton modules with ownership hierarchy #3729

Merged
merged 164 commits into from
Feb 10, 2024

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Jan 13, 2024

What I did

implement #3722 (comment)

also partially implements #3740

incidentally fixes #3161, #3554
fixes #3766

this PR seems to add 5% to the compile time for some large-ish contracts. with #3761 applied (cherry-picked), the compile time overhead (that is, comparing this branch directly to #3761) comes down to ~1.3%.

How I did it

How to verify it

Commit message

this commit implements "singleton modules with ownership hierarchy" as
described in https://github.com/vyperlang/vyper/issues/3722.

to accomplish this, two new language constructs are added: `UsesDecl`
and `InitializesDecl`. these are exposed to the user as `uses:` and
`initializes:`. they are also accompanied by new `AnalysisResult` data
structures: `UsesInfo` and `InitializesInfo`.

`uses` and `initializes` can be thought of as a constraint system on the
module system. a `uses: my-module` annotation is required if
`my_module`'s state is accessed (read or written), and
`initializes: my_module` is required to call `my_module.__init__()`. a
module can be `use`d any number of times; it can only be `initialize`d
once. a module which has been used (directly, or transitively) by the
compilation target (main entry point module), must be `initialize`d
exactly once. `initializes:` is also required to declare which modules
it has been `initialize`d with. for example, if `mod1` declares it
`uses: mod2`, then any `initializes: mod1` statement must declare
*which* instance of `mod2` it has been initialized with. although there
is only ever a single instance of `mod2`, this user-facing requirement
improves readability by forcing the user to be aware of what the state
access dependencies are for a given, `initialize`d module.

the `NamedExpr` node ("walrus operator") has been added to the AST to
support the initializer syntax. (note: the walrus operator is used,
because the originally proposed syntax, `mod1[mod2 = mod2]` is rejected
by the python parser).

a new compiler pass, `vyper/semantics/analysis/global.py` has been
added to implement the global initializer constraint, as it cannot be
defined recursively (without a global context).

since `__init__()` functions can now be called from other `__init__()`
functions (which is not allowed for normal `@external` functions!), a
new `@deploy` visibility has been added to vyper's visibility system.
`@deploy` functions can be called from other `@deploy` functions, and
never from `@external` or `@internal` functions. they also have special
treatment in the ABI relative to other `@external` functions.

`initializes:` is useful since it also serves the purpose of being a
storage allocator directive. wherever `initializes:` is placed, is where
the module will be placed in storage (and code, transient storage, or
any other future storage locations).

this commit refactors the storage allocator so that it recurses into
child modules whenever it sees an `initializes:` statement. it refactors
several data structures surrounding the storage allocator, including
removing inheritance on the `DataPosition` data structure (which has
also been renamed to `VarOffset`). some utility functions have been
added for calculating the size of a given variable, which also get used
in codegen (`get_element_ptr()`).

additional work/refactoring in this commit:
- new analysis machinery for detecting reads/writes for all `ExprInfo`s
- dynamic programming on the `get_expr_info()` routine
- refactoring of `visit_Expr`, which fixes call mutability analysis
- move `StringEnum` back to vyper/utils.py
- remove the "TYPE_DEFINITION" kludge in certain builtins, replace with
  usage of `TYPE_T`
- improve `tag_exceptions()` formatting
- remove `Context.globals`, as we rely on the results of the front-end
  analyser now.
- remove dead variable: `Context.in_assertion`
- refactor `generate_ir_for_function` into
  `generate_ir_for_external_function` and
  `generate_ir_for_internal_function`
- move `get_nonreentrant_lock` to `function_definitions/common.py`
- simplify layout allocation across locations into single function
- add `VyperType.get_size_in()` and `VarInfo.get_size()` helper
  functions so we don't need to do as much switch/case in implementation
  functions
- refactor `codegen/core.py` functions to use `VyperType.get_size()`
- fix interfaces access from `.vyi` files

Description for the changelog

Cute Animal Picture

image

vyper/ast/nodes.py Fixed Show fixed Hide fixed
refactor:
- refactor generate_ir_for_function into generate_ir_for_external_function
  and generate_ir_for_internal_function
- move get_nonreentrant_lock to function-definitions/common.py
vyper/builtins/_utils.py Dismissed Show dismissed Hide dismissed
hint += "a top-level statement to your contract"
raise ImmutableViolation(msg, hint=hint)

# the leftmost- referenced module
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# the leftmost- referenced module
# the leftmost referenced module

nit

Copy link
Collaborator

@pcaversaccio pcaversaccio left a comment

Choose a reason for hiding this comment

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

Alright, I tested the new design on a couple of contracts. Some further feedback, but overall lgtm:

  • Maybe I missed them, but there are no tests on event imports? I did some local tests and it should work (at least it compiled for me 😄).
  • It will be important to test the full design in combination with export bundles to ensure the correct behaviour of implements.
  • The implements keyword doesn't work properly with module imports yet:
# mod3.vy

# Assume a folder `interfaces` at root directory with a file `IAccessControl.vy`
import interfaces.IAccessControl as IAccessControl

# mod2.vy

import mod3

# mod1.vy

import mod2 as acl

implements: acl.mod3.IAccessControl

@deploy
def __init__():
    pass

This will throw with:

vyper.exceptions.InvalidType: 'acl.mod3.IAccessControl' is not a type!

Which is not the correct error first (it should state that the interface is not implemented), and second shows that the implements feature doesn't work yet with modules.

  • Nit: the error vyper.exceptions.CallViolation: Cannot call external functions via 'self' or via library can be split into the self and library case since it's easy to analyse this

vyper/ast/grammar.lark Show resolved Hide resolved
@charles-cooper
Copy link
Member Author

vyper.exceptions.InvalidType: 'acl.mod3.IAccessControl' is not a type!

this seems like an outstanding issue, it should already work on master (but doesn't)

@pcaversaccio
Copy link
Collaborator

vyper.exceptions.InvalidType: 'acl.mod3.IAccessControl' is not a type!

this seems like an outstanding issue, it should already work on master (but doesn't)

Created an issue here #3766

@charles-cooper
Copy link
Member Author

Maybe I missed them, but there are no tests on event imports? I did some local tests and it should work (at least it compiled for me 😄).

it's a planned feature to auto-export (in the ABI) events that are used from imported modules. right now they won't show up in the ABI, so they can't be tested via web3.

@charles-cooper
Copy link
Member Author

the interfaces issue happens only when accessing an interface defined via .vyi. this should fix (adding a test shortly): 5c30bf4

@pcaversaccio
Copy link
Collaborator

the interfaces issue happens only when accessing an interface defined via .vyi. this should fix (adding a test shortly): 5c30bf4

Can confirm. Works now
image

@charles-cooper
Copy link
Member Author

note: there is a fuzz failure in https://github.com/vyperlang/vyper/actions/runs/7845945261/job/21411529442?pr=3729 which doesn't appear to be related to this PR (that is, it is reproducible on master).

tests/functional/builtins/folding/test_bitwise.py:69: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/functional/builtins/folding/test_bitwise.py:84: in test_bitwise_shift_signed
    validate_expected_type(new_node, INT256_T)  # force bounds check
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

node = vyper.ast.nodes.Int:
---> 1 128 << 248
-------^
expected_type = (int256,)

...
...
...

E           vyper.exceptions.TypeMismatch: Expected int256 but literal can only be cast as uint256.
E           
E             line 1:0 
E             ---> 1 128 << 248
E             -------^

vyper/semantics/analysis/utils.py:615: TypeMismatch
---------------------------------- Hypothesis ----------------------------------
Falsifying example: test_bitwise_shift_signed(
    get_contract=<function tests.conftest.get_contract.<locals>.fn>,
    a=128,
    b=248,
    op='<<',
)

@charles-cooper
Copy link
Member Author

note: there is a fuzz failure in https://github.com/vyperlang/vyper/actions/runs/7845945261/job/21411529442?pr=3729

fixed here: #3768

@charles-cooper charles-cooper changed the title feat: singleton modules with ownership hierarchy feat[lang]: singleton modules with ownership hierarchy Feb 10, 2024
@charles-cooper charles-cooper merged commit 8ccacb3 into vyperlang:master Feb 10, 2024
84 checks passed
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.

implements does not allow for imported interfaces Calls to functions with higher mutability not prevented
6 participants