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 more unary and binary expressions #15202

Merged
merged 15 commits into from Jun 25, 2023

Conversation

ichard26
Copy link
Collaborator

@ichard26 ichard26 commented May 6, 2023

(#14838 had a lot of conflicts and it was easier to fix them in a new PR)

Now mypy can constant fold these additional operations:

  • Float arithmetic
  • Mixed int and float arithmetic
  • String multiplication
  • Complex plus or minus a literal real (eg. 1+j2)

While this can be useful with literal types, the main goal is to improve constant folding in mypyc (mypyc/mypyc#772).

mypyc can also fold bytes addition and multiplication, but mypy cannot as byte values can't be easily stored anywhere.

Comment on lines +170 to +173
try:
ret = left**right
except OverflowError:
return None
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Digging through the CPython source, only power ops can raise an OverflowError.

@@ -3356,7 +3356,7 @@ def analyze_simple_literal_type(self, rvalue: Expression, is_final: bool) -> Typ
return None

value = constant_fold_expr(rvalue, self.cur_mod_id)
if value is None:
if value is None or isinstance(value, complex):
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 no idea whether complex literals make any sense in mypy. PTAL.

value = node.final_value
if isinstance(value, (CONST_TYPES)):
return value
final_value = node.final_value
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 value local name should support ConstantValue | None, but Var.final_value does not support bytes. To avoid causing type errors later in the function from this assignment implicitly setting value's type, these variables were renamed.

Comment on lines +1175 to +1176
real = 1
return 5j+real
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 this change is to preserve the old test case behavior where constant folding wasn't performed.

div = 2.0 / 0.0
floor_div = 2.0 // 0.0
power_imag = (-2.0)**0.5
power_overflow = 2.0**10000.0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While floats usually don't raise an error on overflow, this is the one exception I know of. This is why there's a guard against OverflowError in mypy/constant_fold.py.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A good catch! We could also generate a compile-time error, but it's not important.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Great, thanks! Finally got around to reviewing this. Looks good, just a few minor comments.

With these changes we probably have pretty much all the constant folding we need in the medium term at least. math constant support in final expressions might still be a good thing to have at some point, if it doesn't work yet (e.g. X: Final = math.pi * 2, see #15324).

if right != 0:
return left % right
elif op == "**":
if (left < 0 and right >= 1 or right == 0) or (left >= 0 and right >= 0):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could return a complex number, but the return type doesn't include complex (not sure if supporting complex results is worth it):

>>> (-1.2) ** 1.5
(-2.414760036730213e-16-1.3145341380123985j)

I used this fragment to look for other interesting cases, maybe it's helpful here:

values = -1.5, -1.0, -0.5, 0.0, 0.5, 1.0, 1.5
for x in values:
    for y in values:
        try:
            print(x, y, x**y)
        except Exception:
            print(f'error: {x} ** {y}')

add_mul = (1.5 + 3.5) * 5.0
sub = 7.0 - 7.5
div = 3.0 / 2.0
floor_div = 3.0 // 2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test with negative operands as well?

div = 2.0 / 0.0
floor_div = 2.0 // 0.0
power_imag = (-2.0)**0.5
power_overflow = 2.0**10000.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

A good catch! We could also generate a compile-time error, but it's not important.

from typing_extensions import Final

N: Final = 1
FLOAT_N: Final = 1.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a test case specific to a final float? If not, it would be good to add. Also test complex initializers such as N: Final = 1.5 * 2.

@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! LGTM

@JukkaL JukkaL merged commit cee0030 into python:master Jun 25, 2023
20 checks passed
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.

None yet

2 participants