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: add support for modules with variables #3707

Closed

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Dec 22, 2023

What I did

add support for modules with variables. modules are instantiated separately from import statements, decoupling code and storage layout concerns.

How I did it

  • change the allocator to handle modules as variables
  • add analysis support for touched locations
  • change the calling convention to include pointers to storage/immutables/whatever

note that this feature probably requires dropping support for storage layout overrides (at least in its current form)

How to verify it

still incomplete. need to make sure support for immutables is correct, and add tests.

  • unit tests for the analysis
  • various combinations of importing modules with storage and/or immutables
  • tests for the storage allocator
  • handling of __init__() functions

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

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

kill ModuleInfo, can just use VarInfo now
…rType

storage_size_in_words -> storage_slots_required
immutable_section_bytes -> immutable_bytes_required

this establishes a convention to find the size of a type:
`typ.{location}_{slots_or_bytes}_required`

this maybe indicates that a better API will be
`size_in(location: DataLocation)` but we will see how the design evolves
- rename module_ctx to compilation_target
Comment on lines +2 to +8
from vyper.codegen.core import (
_freshname,
data_location_to_addr_space,
eval_once_check,
get_element_ptr,
make_setter,
)

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.codegen.core
begins an import cycle.
@@ -94,6 +92,24 @@
# either the constructor, or called from the constructor
self.is_ctor_context = is_ctor_context

def self_ptr(self, location):

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns Note

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
vyper/codegen/self_call.py Fixed Show fixed Hide fixed
Comment on lines 70 to 77
if s == DataLocation.STORAGE:
return STORAGE
if s == DataLocation.MEMORY:
return MEMORY
if s == DataLocation.CODE:
return IMMUTABLES

raise CompilerPanic("unreachable") # pragma: nocover
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if s == DataLocation.STORAGE:
return STORAGE
if s == DataLocation.MEMORY:
return MEMORY
if s == DataLocation.CODE:
return IMMUTABLES
raise CompilerPanic("unreachable") # pragma: nocover
return {
DataLocation.STORAGE: STORAGE,
DataLocation.MEMORY: MEMORY,
DataLocation.CODE: IMMUTABLES,
}[s]

Comment on lines 476 to 477
for i in range(index):
ofst += module_t.variables[attrs[i]].typ.storage_slots_required
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for i in range(index):
ofst += module_t.variables[attrs[i]].typ.storage_slots_required
offset_from_parent = sum(
module_t.variables[var].typ.storage_slots_required
for var in attrs[:index]
)

module_t = func_t.ast_def._parent._metadata["type"]
if module_t == context.compilation_target:
# we don't need to pass a pointer
return ret
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return ret
return []

Comment on lines +120 to +126
for r in self._variable_reads:
if r._metadata["variable_access"].location == location:
return True
for w in self._variable_writes:
if w._metadata["variable_write"].location == location:
return True
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for r in self._variable_reads:
if r._metadata["variable_access"].location == location:
return True
for w in self._variable_writes:
if w._metadata["variable_write"].location == location:
return True
return False
from itertools import concat
return any(
v._metadata["variable_access"].location == location
for v in concat(self._variable_reads, self._variable_writes)
)

Comment on lines 131 to 136
ret = []
possible_locations = [DataLocation.STORAGE, DataLocation.CODE]
for location in possible_locations:
if self.touches_location(location):
ret.append(location)
return ret
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ret = []
possible_locations = [DataLocation.STORAGE, DataLocation.CODE]
for location in possible_locations:
if self.touches_location(location):
ret.append(location)
return ret
possible_locations = (DataLocation.STORAGE, DataLocation.CODE)
return [
location for location in possible_locations
if self.touches_location(location)
]

AlbertoCentonze and others added 5 commits December 27, 2023 09:40
per title, replace `enum` with `flag` as it more closely models
https://docs.python.org/3/library/enum.html#enum.IntFlag than regular
enums. allow `enum` for now (for backwards compatibility) but convert to
`flag` internally and issue a warning
rename `assert_tx_failed` to `tx_failed` and change it into a context
manager which has a similar API to `pytest.raises()`.

---------

Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>
- allow range where both start and end arguments are variables, so long
  as a bound is supplied

- ban range expressions of the form `range(x, x + N)` since the new form
  is cleaner and supersedes it.

- also do a bit of refactoring of the codegen for range

---------

Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>
* feat: improve panics in IR generation

this QOL commit improves on 9165926 by passing through the
`__traceback__` field when the exception is modified (instead of using
`__cause__` - cf. PEP-3134 regarding the difference) and improves error
messages when an IRnode is not returned properly. using `__traceback__`
generally results in a better experience because the immediate cause of
the exception is displayed when running `vyper -v` instead of needing to
scroll up through the exception chain (if the exception chain is
reproduced correctly at all in the first place).

---------

Co-authored-by: Harry Kalogirou <harkal@nlogn.eu>
StructT,
TupleT,
_BytestringT,
)
from vyper.semantics.types.shortcuts import BYTES32_T, INT256_T, UINT256_T
from vyper.semantics.types.subscriptable import SArrayT
from vyper.semantics.types.user import EnumT
from vyper.semantics.types.user import FlagT

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.semantics.types.user
begins an import cycle.
@@ -248,13 +245,13 @@
# comparisons, e.g. `x < y`

# TODO fixme circular import
from vyper.semantics.types.user import EnumT
from vyper.semantics.types.user import FlagT

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.semantics.types.user
begins an import cycle.
- split VarInfo positions API from get_position/set_position to
  get_storage_position/set_storage_position and ditto for immutables
- only one of these is valid for regular VarInfos, but modules get
  allocated in both storage and immutables.
- rewrite set_code_offsets to share allocator code
- rename set_data_positions to allocate_module_variables
vyper/semantics/analysis/utils.py Fixed Show fixed Hide fixed
def __post_init__(self):
super().__post_init__()
# hmm
from vyper.semantics.types.module import ModuleT

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.semantics.types.module
begins an import cycle.
@@ -36,6 +36,7 @@
VyperException,
tag_exceptions,
)
from vyper.semantics.analysis.base import VarInfo

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.semantics.analysis.base
begins an import cycle.
@@ -17,7 +17,7 @@
ZeroDivisionException,
)
from vyper.semantics import types
from vyper.semantics.analysis.base import ExprInfo, Modifiability, ModuleInfo, VarInfo
from vyper.semantics.analysis.base import ExprInfo, Modifiability, ModuleVarInfo, VarInfo

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'ModuleVarInfo' is not used.
@@ -17,7 +17,7 @@
ZeroDivisionException,
)
from vyper.semantics import types
from vyper.semantics.analysis.base import ExprInfo, Modifiability, ModuleInfo, VarInfo
from vyper.semantics.analysis.base import ExprInfo, Modifiability, ModuleVarInfo, VarInfo

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.semantics.analysis.base
begins an import cycle.
@charles-cooper
Copy link
Member Author

superseded by #3729

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