feat[lang]!: strengthen init checks#5103
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| UnfoldableNode, | ||
| ) | ||
| from vyper.semantics.analysis.base import Modifiability | ||
| from vyper.semantics.analysis.base import InitializesInfo, Modifiability |
| UnfoldableNode, | ||
| ) | ||
| from vyper.semantics.analysis.base import Modifiability | ||
| from vyper.semantics.analysis.base import InitializesInfo, Modifiability |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 597fdbe6cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if isinstance(node, vy_ast.Raise): | ||
| # If we raise, return the wildcard | ||
| return None |
There was a problem hiding this comment.
When a constructor branch exits with a bare return, this walker treats it like a normal statement and continues accumulating initializer calls from later statements. For example, if cond: return; lib.__init__() is accepted as initializing lib, but the cond path deploys after skipping lib.__init__(). Since visit_Return allows value-less returns in functions without a return type and constructor codegen jumps to the deploy epilogue, Return needs to terminate the path here without acting like the reverting wildcard used for raise.
Useful? React with 👍 / 👎.
Gas ChangesNo changes detected. Summary
|
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5103 +/- ##
==========================================
+ Coverage 92.77% 92.96% +0.18%
==========================================
Files 188 188
Lines 27729 27911 +182
Branches 4802 4852 +50
==========================================
+ Hits 25726 25947 +221
+ Misses 1351 1312 -39
Partials 652 652 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
TODO: Handle initializes: lib
@deploy
def __init__():
return
lib.__init__()and initializes: lib
@deploy
def __init__():
if True:
return
lib.__init__() |
|
Note: @deploy
def __init__():
if <cond1>:
lib.__init__()
if <cond2 such that it's true iff cond1 is false>:
lib.__init__()Or: @deploy
def __init__():
for i in <seq1 such that it can never be empty>:
lib.__init__()But I was not able to find a case where this would actually be a problem, for example the two examples above can be rewritten in a cleaner way as follows: @deploy
def __init__():
if <cond1>:
lib.__init__()
else:
#optional: assert <cond2>
lib.__init__()@deploy
def __init__():
#optional: assert len(<seq1>) > 0
lib.__init__()And remember the previous condition was "exactly one call to |
| """ | ||
| Check a block for wether it calls all required __init__() methods, and in the right order. | ||
| (Dependencies must be initialized before dependents.) | ||
| Reverting acts as a wildcard with respects to initialization: when a branch contains a revert, |
| # Return or Raise, terminates block with wildcard | ||
| return None | ||
| for module_info, calls in node_init_calls.items(): | ||
| init_calls[module_info] += calls |
There was a problem hiding this comment.
iiuc this modifies the input init_calls but returns something else? that's kind of confusing.. why is init_calls passed as an argument instead of just self.init_calls? since it is being modified by callees
There was a problem hiding this comment.
It doesn't modify the input init_calls, we make a copy of it, should I name the copy something different ?
The reason we can't mutate init_calls is because of branching:
if cond:
a.__init__()
else:
a.__init__()Would do the same sequence of mutations (and observe the same sequence of states) as:
if cond:
a.__init__()
a.__init__()
else:
passBasically init_calls (the copy) = init_calls (the parameter) + local_init_calls, but mutating both is simpler than combining them every time.
The reason we return local_init_calls is to preserve the semantics of visit_:
It returns the init calls of the given expression (and not given expression + above)
This simplifies the logic of branches/loops (since otherwise we would have to check there whether the calls are from the expression, or from the preceding nodes)
| # def __init__(xs): | ||
| # for x in xs: | ||
| # if is_valid(x): | ||
| # lib.__init__(x) | ||
| # return | ||
| # raise "No valid x in xs" |
There was a problem hiding this comment.
It's an example for the text above
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b1ef08793
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if then_nodes is None or else_nodes is None: | ||
| # If either branch reverts/returns, return the other | ||
| # (if both revert, will return None) | ||
| return then_nodes or else_nodes |
There was a problem hiding this comment.
When the else branch raises and the then branch falls through without local initializer calls, then_nodes is an empty dict and else_nodes is None, so this truthiness check returns None and makes visit_block treat the whole if as terminating. A constructor with if cond: pass else: raise "x" is then accepted for an initializes: module even though the cond path deploys without calling lib.__init__(); choose the non-None branch with explicit is None checks instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
truthiness was a mistake
| dependent_init_calls = init_calls[dependent] | ||
| if len(dependent_init_calls) == 0: |
There was a problem hiding this comment.
This lookup only recognizes a direct dep.__init__() call in the current constructor, so it rejects dependencies that were initialized transitively by an already-called module constructor. For example, when owner initializes dep and user uses dep, a parent can declare initializes: user[dep := owner.dep] and run owner.__init__(); user.__init__(), but init_calls[dep] remains empty even though owner.__init__() is required to initialize dep; account for modules initialized by already-called constructors before reporting the dependency as missing.
Useful? React with 👍 / 👎.
| def visit_VyperNode( | ||
| self, node: vy_ast.VyperNode, init_calls: dict[ModuleInfo, list[vy_ast.VyperNode]] | ||
| ): |
There was a problem hiding this comment.
Expression statements that terminate without a Raise node, such as raw_revert(b""), fall through this generic visitor as ordinary statements because it only scans for constructor calls. With if cond: raw_revert(b"") else: lib.__init__(), the checker reports that lib.__init__() is present in only one branch even though the cond path cannot deploy; treat revert-like terminus expressions the same way as raise.
Useful? React with 👍 / 👎.
3 participants
What I did
Close #4126
Close #3779
Close #4855
Now we check:
B.__init__()must be called beforeA.__init__()This allows complex and statically checked initialization logic:
How I did it
Traverse the
__init__method, keeping track of which modules get initialized, and:The restriction from 1. is what allows us to have a concept of "modules which have already been initialized", since branches must be similar, we don't need to reason about execution paths.
What I didn't do (aka future work)
lib1.fooshould only be valid afterlib1.__init__())How to verify it
Commit message
Description for the changelog
Cute Animal Picture