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

Constant fold initializers of final variables #14283

Merged
merged 20 commits into from
Dec 15, 2022
Merged

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Dec 11, 2022

Now mypy can figure out the values of final variables even if the initializer has some operations on constant values:

A: Final = 2  # This has always worked
A: Final = -(1 << 2)  # This is now supported
B: Final = 'x' + 'y'  # This also now works

Currently we support integer arithmetic and bitwise operations, and string concatenation.

This can be useful with literal types, but my main goal was to improve constant folding in mypyc. In particular, this helps constant folding with native ints in cases like these:

FLAG1: Final = 1 << 4
FLAG2: Final = 1 << 5

def f() -> i64:
    return FLAG1 | FLAG2  # Can now be constant folded

We still have another constant folding pass in mypyc, since it does some things more aggressively (e.g. it constant folds some member expression references).

Work on mypyc/mypyc#772.

Also helps with mypyc/mypyc#862.

@@ -3273,7 +3273,7 @@ L2:
[case testFinalStaticInt]
from typing import Final

x: Final = 1 + 1
x: Final = 1 + int()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The intent of changes like these is to preserve the old test case behavior where constant folding wasn't performed.

L2:
r2 = CPyTagged_Add(r0, 2)
a = r2
a = 14
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This illustrates how much we can simplify the generated code. The simpler code is also significantly faster, since there is no memory read any more.

@@ -8,8 +8,9 @@ x
[out]
MypyFile:1(
AssignmentStmt:1(
NameExpr(x* [__main__.x])
IntExpr(1))
NameExpr(x [__main__.x])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changes such as these are a side effect of making semantic analyzer tests use the constant folding logic. Previously it was disabled due to historical reasons. The actual behavior outside tests wasn't changed (for most of these test changes).

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Comment on lines +36 to +46
if isinstance(expr, IntExpr):
return expr.value
if isinstance(expr, StrExpr):
return expr.value
if isinstance(expr, FloatExpr):
return expr.value
elif isinstance(expr, NameExpr):
if expr.name == "True":
return True
elif expr.name == "False":
return False
Copy link
Member

@AlexWaygood AlexWaygood Dec 11, 2022

Choose a reason for hiding this comment

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

Interesting, this is very similar logic to the code @JelleZijlstra added in evalexpr.py in a9c62c5. I wonder if that logic could be reused here?

(Apologies if this comment makes no sense; I don't know much about mypyc internals!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, there is definitely some overlap. Right now the benefits of sharing code don't seem big enough to me to make it worthwhile to increase coupling, but if we add support for more things, it may make sense to refactor and share parts of the implementations. Note that mypy and mypyc already share some of the constant folding code.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a visitor?

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'm not using a visitor, since only relatively few AST node types are handled here, and others have shared default behavior. I generally use a visitor when I need to implement logic for many node types. However, I think that in this case both using and not using a visitor would have been reasonable.

@JukkaL JukkaL merged commit df6e828 into master Dec 15, 2022
@JukkaL JukkaL deleted the mypyc-constant-fold branch December 15, 2022 10:18
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