GeezerVison

Why have tests been removed from test_compile and test_peepholer?
Those test the code after the CFG optimizer has run, so should be unchanged.

The tests in test_opcache should not be removed, but should be changed to keep the unpacking operation by moving the tuple out of the loop.
Instead of:

for ...
    a, b = 1, 2

use

t = 1, 2
for ...
    a, b = t

Why have tests been removed from test_compile and test_peepholer? Those test the code after the CFG optimizer has run, so should be unchanged.

test_compile was changed because the test was fundamentally wrong. The name of the test suggests that it tests constant merging, but actually, all the work was done by AST optimizer. Constant merging has never had any relation to this test. In particular, it tested that co_consts for lambda: (257,) would look like a ((257,),) instead of (257, (257,)). Do you think it's worth fixing?

test_peepholer only changed where it tries to fold set into frozenset where one of the elements is constant tuple. Since folding of constant tuples are handled by the CFG, set folding during the AST optimization can't work for such case. We could add folding for sets (not in all cases, only with in operator and where set is a target for for loop) in the follow-up PR.

The tests in test_opcache should not be removed, but should be changed to keep the unpacking operation by moving the tuple out of the loop.

Thank you, for some reason I forgot about non-constant case...

Constant merging has never had any relation to this test. In particular, it tested that co_consts for lambda: (257,) would look like a ((257,),) instead of (257, (257,)). Do you think it's worth fixing?

No. You can remove that test. Try to keep any tests that test behavior, but any that just test artifacts (like the test for lambda: (257,) ) can be removed.

We could add folding for sets (not in all cases, only with in operator and where set is a target for for loop) in the follow-up PR.

Yes, leave that for another PR.

As an aside:
From a quick experiment I did a while ago, I think the only folding that needs to be done before the CFG optimizer is for negative numbers, converting -(1) into -1. I think that can be done in the codegen pass.

As an aside: From a quick experiment I did a while ago, I think the only folding that needs to be done before the CFG optimizer is for negative numbers, converting -(1) into -1. I think that can be done in the codegen pass.

I think we could do that in the follow-up PR.
Though, I'm not an parser expert, but when I understood that parser generates ast.UnaryOp node instead of ast.Constant for simple constants like -1, I was confused.
@pablogsal as parser expert, is it hard to do this in the parser?

@markshannon do you think it's ok to merge this PR? This PR only removes parts of test_ast which are basically tests AST optimizations, and incorrect test in test_compile. Any other changes are simply commenting some parts of tests, which we can uncomment later when other optimizations will take place in CFG.

@pablogsal as parser expert, is it hard to do this in the parser?

This normally should not be done in the parser as the general idea is that the parser gives the most pure AST to the next step in the pipeline and every modification and optimisations happens there. There are many reasons for this but the classical one is to not make the life of formatters and similar tools hard and not to make unparsing harder.

There are many reasons for this but the classical one is to not make the life of formatters and similar tools easy

I guess you mean to use the word "hard" here. Otherwise, do the parser team make someone's life harder on purpose? 🤣

I guess you mean to use the word "hard" here.

Haha, indeed! 😆

Otherwise, do the parser team make someone's life harder on purpose? 🤣

Maybe just our own life 😉

FYI, I plan to merge this tomorrow, as this PR is quite small and I don't see any points here which need further discussion. Mark's comments have already been addressed.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

cc @iritkatriel @WolframAlph for review

@tomasr8 I believe this one can be closed in favour of #130769