◐ Shell
clean mode source ↗

bpo-37830: Optimize return statement bytecode with fixing segfaults by isidentical · Pull Request #15247 · python/cpython

pablogsal

Choose a reason for hiding this comment

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

Thanks, @isidentical for your PR.

Apart from the failures that you already see in the CI, I think this approach is not the correct one
as aside from other implementation issues is backwards-incompatible as the left side of the return statement is not executed anymore. For example:

def f():
    print("Side effect")

def g():
    for _ in range(3):
        try:
            return f()
        finally:
            break

g()

In Python3 this prints "Side effect" while with this approach it does not print anything.

@bedevere-bot

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.

@isidentical

Hey @pablogsal ,
I am aware of that (because it no longer generates instructions for return's value). Do we really want that side affect for just backwards-compatibility?

@pablogsal

Do we really want that side affect for just backwards-compatibility?

Backwards compatibility is essential in CPython, so yes, we want backwards compatibility as there may be code out there that is using this (there are users even relying on compiler optimizations). Additionally, this approach fails with nested blocks:

def g():
    for _ in range(3):
        try:
            return _
        finally:
            try:
                return_
            finally:
                break
g()

@isidentical

Backwards compatibility is essential in CPython, so yes, we want backwards compatibility as there may be code out there that is using this

What about reverting #15230 and then adding allow continue with a new future flag called finally_continue (with a PEP). When that flag turned on we dont have to evaluate return value and it can be the default behavior when 4.0 arrives?

Additionally, this approach fails with nested blocks

Thanks, i'll look into them By the way, currently i am on a vacation (muslim holiday) so maybe i can't be very productive but i'm going to return in 2 days.

@pablogsal

What about reverting #15230 and then adding allow continue with a new future flag called finally_continue (with a PEP).

I (personally) don't think to allow continue in finally is that important to start adding that complexity (especially future flags). Also, as Damien George indicates in the other PR, other Python implementations are having troubles with this for little gain.

I think we need to think of other solutions that do not involve backward-incompatible changes or notable changes in the compiler state. Especially we should focus on solving the problem for 'break' cleanly at least for 3.8.

@serhiy-storchaka

This does not work because break and continue can be conditional. For example:

def simple(x):
    for number in range(2):
        try:
            return number
        finally:
            if x:
                continue

print(simple(0))
print(simple(1))

It should print

@isidentical

Hmm, i dont see a way to make this solution work with conditions so i believe it is best to close this too :/