-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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
gh-126835: Disable tuple folding in the AST optimizer #128802
Changes from 22 commits
2f475e1
7a96d47
6d93343
02a38a3
5c05d69
05f3e62
a3d9790
9f1feb4
ac50aad
2b39a13
17dffaa
46845f0
e9631d8
53dfa93
9754820
477c784
6731dfb
aad9fb3
be40093
5aec965
ee69f0f
0801463
a268315
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,22 +153,6 @@ def test_optimization_levels__debug__(self): | |
self.assertIsInstance(res.body[0].value, ast.Name) | ||
self.assertEqual(res.body[0].value.id, expected) | ||
|
||
def test_optimization_levels_const_folding(self): | ||
folded = ('Expr', (1, 0, 1, 6), ('Constant', (1, 0, 1, 6), (1, 2), None)) | ||
not_folded = ('Expr', (1, 0, 1, 6), | ||
('Tuple', (1, 0, 1, 6), | ||
[('Constant', (1, 1, 1, 2), 1, None), | ||
('Constant', (1, 4, 1, 5), 2, None)], ('Load',))) | ||
|
||
cases = [(-1, not_folded), (0, not_folded), (1, folded), (2, folded)] | ||
for (optval, expected) in cases: | ||
with self.subTest(optval=optval): | ||
tree1 = ast.parse("(1, 2)", optimize=optval) | ||
tree2 = ast.parse(ast.parse("(1, 2)"), optimize=optval) | ||
for tree in [tree1, tree2]: | ||
res = to_tuple(tree.body[0]) | ||
self.assertEqual(res, expected) | ||
|
||
def test_invalid_position_information(self): | ||
invalid_linenos = [ | ||
(10, 1), (-10, -11), (10, -11), (-5, -2), (-5, 1) | ||
|
@@ -3138,101 +3122,6 @@ def test_folding_format(self): | |
|
||
self.assert_ast(code, non_optimized_target, optimized_target) | ||
|
||
|
||
def test_folding_tuple(self): | ||
code = "(1,)" | ||
|
||
non_optimized_target = self.wrap_expr(ast.Tuple(elts=[ast.Constant(1)])) | ||
optimized_target = self.wrap_expr(ast.Constant(value=(1,))) | ||
|
||
self.assert_ast(code, non_optimized_target, optimized_target) | ||
|
||
def test_folding_type_param_in_function_def(self): | ||
code = "def foo[%s = (1, 2)](): pass" | ||
|
||
unoptimized_tuple = ast.Tuple(elts=[ast.Constant(1), ast.Constant(2)]) | ||
unoptimized_type_params = [ | ||
("T", "T", ast.TypeVar), | ||
("**P", "P", ast.ParamSpec), | ||
("*Ts", "Ts", ast.TypeVarTuple), | ||
] | ||
|
||
for type, name, type_param in unoptimized_type_params: | ||
result_code = code % type | ||
optimized_target = self.wrap_statement( | ||
ast.FunctionDef( | ||
name='foo', | ||
args=ast.arguments(), | ||
body=[ast.Pass()], | ||
type_params=[type_param(name=name, default_value=ast.Constant((1, 2)))] | ||
) | ||
) | ||
non_optimized_target = self.wrap_statement( | ||
ast.FunctionDef( | ||
name='foo', | ||
args=ast.arguments(), | ||
body=[ast.Pass()], | ||
type_params=[type_param(name=name, default_value=unoptimized_tuple)] | ||
) | ||
) | ||
self.assert_ast(result_code, non_optimized_target, optimized_target) | ||
|
||
def test_folding_type_param_in_class_def(self): | ||
code = "class foo[%s = (1, 2)]: pass" | ||
|
||
unoptimized_tuple = ast.Tuple(elts=[ast.Constant(1), ast.Constant(2)]) | ||
unoptimized_type_params = [ | ||
("T", "T", ast.TypeVar), | ||
("**P", "P", ast.ParamSpec), | ||
("*Ts", "Ts", ast.TypeVarTuple), | ||
] | ||
|
||
for type, name, type_param in unoptimized_type_params: | ||
result_code = code % type | ||
optimized_target = self.wrap_statement( | ||
ast.ClassDef( | ||
name='foo', | ||
body=[ast.Pass()], | ||
type_params=[type_param(name=name, default_value=ast.Constant((1, 2)))] | ||
) | ||
) | ||
non_optimized_target = self.wrap_statement( | ||
ast.ClassDef( | ||
name='foo', | ||
body=[ast.Pass()], | ||
type_params=[type_param(name=name, default_value=unoptimized_tuple)] | ||
) | ||
) | ||
self.assert_ast(result_code, non_optimized_target, optimized_target) | ||
|
||
def test_folding_type_param_in_type_alias(self): | ||
code = "type foo[%s = (1, 2)] = 1" | ||
|
||
unoptimized_tuple = ast.Tuple(elts=[ast.Constant(1), ast.Constant(2)]) | ||
unoptimized_type_params = [ | ||
("T", "T", ast.TypeVar), | ||
("**P", "P", ast.ParamSpec), | ||
("*Ts", "Ts", ast.TypeVarTuple), | ||
] | ||
|
||
for type, name, type_param in unoptimized_type_params: | ||
result_code = code % type | ||
optimized_target = self.wrap_statement( | ||
ast.TypeAlias( | ||
name=ast.Name(id='foo', ctx=ast.Store()), | ||
type_params=[type_param(name=name, default_value=ast.Constant((1, 2)))], | ||
value=ast.Constant(value=1), | ||
) | ||
) | ||
non_optimized_target = self.wrap_statement( | ||
ast.TypeAlias( | ||
name=ast.Name(id='foo', ctx=ast.Store()), | ||
type_params=[type_param(name=name, default_value=unoptimized_tuple)], | ||
value=ast.Constant(value=1), | ||
) | ||
) | ||
self.assert_ast(result_code, non_optimized_target, optimized_target) | ||
Comment on lines
-3150
to
-3234
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as with |
||
|
||
def test_folding_match_case_allowed_expressions(self): | ||
def get_match_case_values(node): | ||
result = [] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -570,11 +570,6 @@ def test_compile_ast(self): | |
self.assertIsInstance(raw_right, ast.Tuple) | ||
self.assertListEqual([elt.value for elt in raw_right.elts], [1, 2]) | ||
|
||
for opt in [opt1, opt2]: | ||
opt_right = opt.value.right # expect Constant((1,2)) | ||
self.assertIsInstance(opt_right, ast.Constant) | ||
self.assertEqual(opt_right.value, (1, 2)) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test tests whether we optimize ast when passed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's ok to modify this one, and remove others from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe you're right. They are testing const folding which no longer is there and |
||
def test_delattr(self): | ||
sys.spam = 1 | ||
delattr(sys, 'spam') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -793,14 +793,6 @@ def check_same_constant(const): | |
self.check_constant(f1, Ellipsis) | ||
self.assertEqual(repr(f1()), repr(Ellipsis)) | ||
|
||
# Merge constants in tuple or frozenset | ||
f1, f2 = lambda: "not a name", lambda: ("not a name",) | ||
f3 = lambda x: x in {("not a name",)} | ||
self.assertIs(f1.__code__.co_consts[0], | ||
f2.__code__.co_consts[0][0]) | ||
self.assertIs(next(iter(f3.__code__.co_consts[1])), | ||
f2.__code__.co_consts[0]) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these tests removed? They are not related. I understand this is failing, but its enough to update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also bring us back to #130016. We wouldn't need to touch this if we were to merge that. @iritkatriel are we planning to? |
||
# {0} is converted to a constant frozenset({0}) by the peephole | ||
# optimizer | ||
f1, f2 = lambda x: x in {0}, lambda x: x in {0} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -154,7 +154,7 @@ def test_folding_of_tuples_of_constants(self): | |||||
for line, elem in ( | ||||||
('a = 1,2,3', (1, 2, 3)), | ||||||
('("a","b","c")', ('a', 'b', 'c')), | ||||||
('a,b,c = 1,2,3', (1, 2, 3)), | ||||||
('a,b,c,d = 1,2,3,4', (1, 2, 3, 4)), | ||||||
('(None, 1, None)', (None, 1, None)), | ||||||
('((1, 2), 3, 4)', ((1, 2), 3, 4)), | ||||||
): | ||||||
|
@@ -164,8 +164,9 @@ def test_folding_of_tuples_of_constants(self): | |||||
self.assertNotInBytecode(code, 'BUILD_TUPLE') | ||||||
self.check_lnotab(code) | ||||||
|
||||||
# Long tuples should be folded too. | ||||||
code = compile(repr(tuple(range(10000))),'','single') | ||||||
# Long tuples should be folded too, but their length should not | ||||||
# exceed the `STACK_USE_GUIDELINE` | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we perhaps add a test that tuples longer than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about it. At the moment, for constant tuples which are longer than
Shall we assert that this intrinsic is presented in bytecode? |
||||||
code = compile(repr(tuple(range(30))),'','single') | ||||||
Comment on lines
+181
to
+183
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How much of a concern is that we can no longer create constant tuples beyond length of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's probably not ideal in case you have some kind of large constant lookup table for instance. As Kirill pointed out, this would get compiled to a bunch of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not a hard pattern to detect, right? Maybe we could fold it anyway? @Eclips4 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems quite predictable: >>> def foo():
... return (1,2,3, ... ,31)
...
>>> dis.dis(foo)
1 RESUME 0
2 BUILD_LIST 0
LOAD_SMALL_INT 1
LIST_APPEND 1
LOAD_SMALL_INT 2
LIST_APPEND 1
...
LOAD_SMALL_INT 31
LIST_APPEND 1
CALL_INTRINSIC_1 6 (INTRINSIC_LIST_TO_TUPLE) (Could also be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. I think we can easily fold it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we could check with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's a good idea. We would be creating dependency between both. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if we are going to fold constant tuple beyond |
||||||
self.assertNotInBytecode(code, 'BUILD_TUPLE') | ||||||
# One LOAD_CONST for the tuple, one for the None return value | ||||||
load_consts = [instr for instr in dis.get_instructions(code) | ||||||
|
@@ -345,6 +346,28 @@ def negzero(): | |||||
self.assertInBytecode(code, opname) | ||||||
self.check_lnotab(code) | ||||||
|
||||||
def test_folding_of_tuples_on_constants(self): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And we actually, already have |
||||||
tests =[ | ||||||
('()', True, 0), | ||||||
('(1, 2, 3)', True, 3), | ||||||
('("a", "b", "c")', True, 3), | ||||||
('(1, a)', False, 2), | ||||||
('(a, b, c)', False, 3), | ||||||
('(1, (2, 3))', True, 2), | ||||||
('(a, (b, c))', False, 2), | ||||||
('(1, [], {})', False, 3), | ||||||
(repr(tuple(range(30))), True, 30), | ||||||
('(1, (2, (3, (4, (5)))))', True, 2) | ||||||
] | ||||||
for expr, is_const, length in tests: | ||||||
with self.subTest(expr=expr, is_const=is_const, length=length): | ||||||
code = compile(expr, '', 'eval') | ||||||
if is_const: | ||||||
self.assertNotInBytecode(code, 'BUILD_TUPLE', length) | ||||||
self.assertInBytecode(code, 'LOAD_CONST', eval(expr)) | ||||||
else: | ||||||
self.assertInBytecode(code, 'BUILD_TUPLE', length) | ||||||
|
||||||
def test_elim_extra_return(self): | ||||||
# RETURN LOAD_CONST None RETURN --> RETURN | ||||||
def f(x): | ||||||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with
test_compile_ast
. Maybe add test for__debug__
instead of removing test entirely?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there is
test_optimization_levels__debug__
right above this one, so this one can indeed be gone.