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: reorder folding after semantics pass #3616

Closed
wants to merge 209 commits into from

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Sep 23, 2023

What I did

How I did it

How to verify it

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-->

vyper/semantics/namespace.py Outdated Show resolved Hide resolved
@charles-cooper
Copy link
Member

charles-cooper commented Oct 30, 2023

some high-level review:

  • pre_typecheck.py should be a semantics pass, e.g. vyper/semantics/analysis/pre_typecheck.py
  • i think this is going to be cleaner if prefold() operates by annotating the ast instead of returning values. ex.
    node._metadata["folded_value"] = value
    instead of
    return value
    this way the ast does not need to be destroyed/mutated, and we can guarantee a single pass (since it's dynamic programming), instead of potential blowup with repeated visiting of ast nodes
  • let's try to minimize the amount of stuff that prefold() does. for instance, can we avoid 'prefold'ing structs?

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

for the change to infer_arg_types() API, it would be better to not be so generic, but look like:

def infer_arg_types(self, args, expected_return_type = None):
    ...

otherwise it's too difficult to reason about what kind of generic arguments could go into the *args slot.

self.visit(node.args[0])
node._metadata["folded_value"] = node.args[0]._metadata.get("folded_value")

from vyper.builtins.functions import DISPATCH_TABLE

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.builtins.functions
begins an import cycle.
@@ -16,6 +16,7 @@
)
from vyper.semantics.analysis.base import VarInfo
from vyper.semantics.analysis.levenshtein_utils import get_levenshtein_error_suggestions
from vyper.semantics.analysis.pre_typecheck import pre_typecheck

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.semantics.analysis.pre_typecheck
begins an import cycle.
tests/ast/test_folding.py Fixed Show fixed Hide fixed
tests/ast/test_folding.py Fixed Show fixed Hide fixed
vyper/builtins/functions.py Fixed Show fixed Hide fixed
from vyper import ast as vy_ast


def get_folded_value(node: vy_ast.VyperNode) -> Optional[vy_ast.VyperNode]:
Copy link
Member

@charles-cooper charles-cooper Nov 2, 2023

Choose a reason for hiding this comment

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

get_folded_value should probably tag the node with the return value if it doesn't find it, that way it's dynamic programming and we don't revisit nodes in the traversal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have moved the tagging of folded_value in a node's metadata into the pre-typecheck visitor pass. get_folded_value is now a simple wrapper to either get a literal node's value or the literal value of the folded_value metadata.

if all(isinstance(v, vy_ast.NameConstant) for v in values):
try:
node._metadata["folded_value"] = node.evaluate(values)
except UnfoldableNode:

Check notice

Code scanning / CodeQL

Empty except Note

'except' clause does nothing but pass and there is no explanatory comment.
right = get_folded_value(node.right)
try:
node._metadata["folded_value"] = node.evaluate(left, right)
except UnfoldableNode:

Check notice

Code scanning / CodeQL

Empty except Note

'except' clause does nothing but pass and there is no explanatory comment.
@tserg tserg closed this Nov 19, 2023
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.

4 participants