-
-
Notifications
You must be signed in to change notification settings - Fork 821
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[lang]: stateless modules should not be initialized #4347
base: master
Are you sure you want to change the base?
Changes from 23 commits
1b1ac45
3a9e29d
5527244
3c695da
5000efc
e3d7582
c92e1ad
cf91784
1ab6433
28c9be7
0c5f645
47efc8e
5fe6c84
fececb7
4cc01bb
ba19783
b668f41
6c7a30f
948d047
3dbbeb6
1f4a2d6
be5aa0b
7b2f38b
bb4944f
b41bc28
2bcba89
7f2f452
f64109f
bf7307c
a075979
b67ab3f
ef0c49d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -218,6 +218,8 @@ def bar(x: {type}): | |
|
||
def test_exports_abi(make_input_bundle): | ||
lib1 = """ | ||
phony: uint32 | ||
|
||
@external | ||
def foo(): | ||
pass | ||
|
@@ -361,6 +363,8 @@ def __init__(): | |
def test_event_export_from_function_export(make_input_bundle): | ||
# test events used in exported functions are exported | ||
lib1 = """ | ||
phony: uint32 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we shouldn't add phony here -- the code change is telling us to remove |
||
|
||
event MyEvent: | ||
pass | ||
|
||
|
@@ -396,6 +400,8 @@ def foo(): | |
def test_event_export_unused_function(make_input_bundle): | ||
# test events in unused functions are not exported | ||
lib1 = """ | ||
phony: uint32 | ||
|
||
event MyEvent: | ||
pass | ||
|
||
|
@@ -639,6 +645,8 @@ def update_counter(): | |
import lib1 | ||
uses: lib1 | ||
|
||
phony: uint32 | ||
|
||
@internal | ||
def use_lib1(): | ||
lib1.update_counter() | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -444,19 +444,19 @@ def find_module_info(self, needle: "ModuleT") -> Optional["ModuleInfo"]: | |||||||||
return s | ||||||||||
return None | ||||||||||
|
||||||||||
@property | ||||||||||
@cached_property | ||||||||||
def variable_decls(self): | ||||||||||
return self._module.get_children(vy_ast.VariableDecl) | ||||||||||
|
||||||||||
@property | ||||||||||
@cached_property | ||||||||||
def uses_decls(self): | ||||||||||
return self._module.get_children(vy_ast.UsesDecl) | ||||||||||
|
||||||||||
@property | ||||||||||
@cached_property | ||||||||||
def initializes_decls(self): | ||||||||||
return self._module.get_children(vy_ast.InitializesDecl) | ||||||||||
|
||||||||||
@property | ||||||||||
@cached_property | ||||||||||
def exports_decls(self): | ||||||||||
return self._module.get_children(vy_ast.ExportsDecl) | ||||||||||
|
||||||||||
|
@@ -469,7 +469,7 @@ def used_modules(self): | |||||||||
ret.append(used_module) | ||||||||||
return ret | ||||||||||
|
||||||||||
@property | ||||||||||
@cached_property | ||||||||||
def initialized_modules(self): | ||||||||||
# modules which are initialized to | ||||||||||
ret = [] | ||||||||||
|
@@ -559,3 +559,24 @@ def immutable_section_bytes(self): | |||||||||
@cached_property | ||||||||||
def interface(self): | ||||||||||
return InterfaceT.from_ModuleT(self) | ||||||||||
|
||||||||||
@cached_property | ||||||||||
def is_not_initializable(self): | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we can flip the logic and rename to |
||||||||||
""" | ||||||||||
Determine whether ModuleT is stateless by examining its top-level | ||||||||||
declarations. A module has state if it contains storage variables, | ||||||||||
transient variables, or immutables, or if it includes a "initializes" | ||||||||||
declaration, or any nonreentrancy locks. | ||||||||||
""" | ||||||||||
if len(self.initializes_decls) > 0: | ||||||||||
return False | ||||||||||
if any(not v.is_constant for v in self.variable_decls): | ||||||||||
return False | ||||||||||
if self.init_function is not None: | ||||||||||
charles-cooper marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
return False | ||||||||||
|
||||||||||
for fun in self.functions.values(): | ||||||||||
if fun.nonreentrant: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. convention: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually maybe this should be a loop over exposed functions (per @cyberthirst )? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that would skip internal functions with nonreentrancy locks - is that intentional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, not intentional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, maybe the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup, i think as you said - we dont need to recurse in the function as the state of the imported modules is already handled. So the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes i think so, unless @cyberthirst thinks otherwise. |
||||||||||
return False | ||||||||||
|
||||||||||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's please add a comment here that we're testing the initialization and how it relates to state