feat[lang]: add abstract methods#4875
Conversation
Seems to have been always empty anyways
The result from the original was never used, and should not have been anyways, as it only extracts a single terminating node, when there can be multiple
It seems correct, but I am not familiar enough with these complicated classes to be sure there are no mistakes
…space) pair of classes
…e into a ContextVar
…order Now functions in CompilerData only call methods before them, so you can have a clearer idea of what has and hasn't been executed Note: This does not guarantee every method before the one you are on will have been called!
vyper/semantics/analysis/local.py
Outdated
|
|
||
| if self.func.return_type and not _is_terminated: | ||
| raise FunctionDeclarationException( | ||
| f"Missing return statement in function '{self.fn_node.name}'", self.fn_node |
There was a problem hiding this comment.
now i see why it was originally find_terminating_node, i guess the intent was to have a hint pointing to exactly which branch was not terminated
There was a problem hiding this comment.
Should I refactor it to do that ? (Or add a TODO)
The issue is that it's always possible to add a return at the very end of a block, so there's not really a location to point to
vyper/semantics/analysis/local.py
Outdated
| if varaccess is not None: | ||
| self.loop_variables.append(varaccess) | ||
| try: | ||
| yield | ||
| finally: | ||
| self.loop_variables.pop() | ||
|
|
||
| def visit(self, node): | ||
| super().visit(node) | ||
| if varaccess is not None: | ||
| self.loop_variables.pop() |
There was a problem hiding this comment.
at this point i would rewrite the entire block so that the branching is clear
| if varaccess is not None: | |
| self.loop_variables.append(varaccess) | |
| try: | |
| yield | |
| finally: | |
| self.loop_variables.pop() | |
| def visit(self, node): | |
| super().visit(node) | |
| if varaccess is not None: | |
| self.loop_variables.pop() | |
| if varaccess is None: | |
| yield | |
| return # if this is not possible then use an else branch | |
| self.loop_variables.append(varaccess) | |
| try: | |
| yield | |
| finally: | |
| self.loop_variables.pop() |
vyper/semantics/analysis/local.py
Outdated
| ) | ||
| is_abstract = func._metadata["func_type"].is_abstract | ||
|
|
||
| if is_abstract: |
There was a problem hiding this comment.
do we visit the bodies of abstract functions?
| if root_module_info is not None: | ||
| self.func.mark_used_module(root_module_info) | ||
|
|
||
| # Note: We don't check what the override touches, as this would severely hurt |
There was a problem hiding this comment.
we can't just not validate it lol, in that case the overriding module is responsible for declaring uses!
There was a problem hiding this comment.
The concrete methods in an abstract module are validated !
What we don't do is look at the override of abstract methods (who live in another module), and use their body to add requirements to the abstract module
See test_overriding_module_can_initialize_state for an example of what would happen otherwise:
# abstract module:
# if the following was needed, it would severely limit the usefulness of the feature
# uses: stateful
@abstract
def process() -> uint256: ...
# override module:
import stateful
import b_module
initializes: stateful
initializes: b_module
@override(b_module)
def process() -> uint256:
stateful.increment()
return stateful.get_counter()
vyper/semantics/analysis/module.py
Outdated
| The root module (compilation target) must be the last element in `modules`. | ||
| """ | ||
| return _analyze_module_r(module_ast, module_ast.is_interface) | ||
| # The root module is always the last element, since dependencies are sorted |
There was a problem hiding this comment.
this comment contradicts the docstring
There was a problem hiding this comment.
Both updated, what's going on should be clearer now
Let me know if it's still confusing
vyper/semantics/analysis/module.py
Outdated
| Analyze Vyper module ASTs, add all module-level objects to the namespace, | ||
| type-check/validate semantics and annotate with type and analysis info. | ||
|
|
||
| The root module (compilation target) must be the last element in `modules`. |
There was a problem hiding this comment.
kind of weird, can we enforce this in a type safe way?
There was a problem hiding this comment.
Not really, but it is a logical consequence of modules being sorted such that dependencies always come before dependents
|
|
||
|
|
||
| def analyze_module(module_ast: vy_ast.Module) -> ModuleT: | ||
| def analyze_modules(modules: OrderedSet[vy_ast.Module]) -> ModuleT: |
There was a problem hiding this comment.
why is the api changing?
There was a problem hiding this comment.
Because analysis is now done in multiple passes (no way around it), so we need to have the list of modules (in post-order)
And it's already computed as part of another phase
vyper/semantics/analysis/module.py
Outdated
| raise CallViolation(msg, func, g.ast_def) | ||
|
|
||
| def _compute_reachable_sets(module_ast: vy_ast.Module): | ||
| with override_global_namespace(module_ast._metadata["namespace"]): |
There was a problem hiding this comment.
Removed, was unnecessary
| # note that we don't just copy the namespace because | ||
| # there are constructor issues. | ||
| _ns.update({k: self.namespace[k] for k in self.namespace._scopes[-1]}) # type: ignore | ||
| _ns._scopes = self.namespace._scopes.copy() |
There was a problem hiding this comment.
Because it was broken before
Using a module namespace would instantly fail with "self is already a member" (or something like that) because self gets added when scopes is empty
Semantically I also don't see why we wouldn't copy the scopes as part of a copy of a namespace
vyper/semantics/analysis/module.py
Outdated
|
|
||
| # Return type validation | ||
|
|
||
| if return_type_abstract: |
There was a problem hiding this comment.
this is a lot of nesting to just output some strings
There was a problem hiding this comment.
Heavily simplified, should be much clearer now
vyper/semantics/analysis/module.py
Outdated
| action = "add a" if abstract_t.nonreentrant else "remove the" | ||
| discrepancies.append( | ||
| FunctionDeclarationException( | ||
| f"Override reentrancy mismatch: Override {_is(override_t.nonreentrant)} " |
There was a problem hiding this comment.
maybe " is [reentrant/nonreentrant] but is [nonreentrant/reentrant]"
like the error message is also wrong because we can have @reentrant decorator
There was a problem hiding this comment.
Done
I also removed the hint for simplicity, let me know if you want me to add one back
| ) | ||
|
|
||
|
|
||
| def _structurally_equivalent_any_r(v1: Any, v2: Any) -> bool: |
There was a problem hiding this comment.
For the list elements, nodes can contain lists which contain things that are not nodes
Or do you mean I should inline _structurally_equivalent_node_r so there's a single method ?
Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>
Since `"bar" in "barr"`, it can pass for the wrong reasons
This reverts commit cd3ba42.
What I did
Implement "initializes-based proposal v2" of #4730
With the change that calling an abstract method requires
usesing its module.This forces the abstract module to be
initializes, and hence overridden, somewhere.Note that this PR does not add docs for this feature !
It does not either change venom in any way !
(not even assertions to make sure it crashes on abstract methods)
How I did it
How to verify it
Commit message
Description for the changelog
(Not sure what the format is for these, let me know if this can be improved)
@abstractand@overridedecorators to make modules overridableCute Animal Picture