Skip to content

bpo-37830: Optimize return statement bytecode with fixing segfaults #15247

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

Closed
wants to merge 5 commits into from

Conversation

isidentical
Copy link
Member

@isidentical isidentical commented Aug 13, 2019

I think this can help continue/break to stay inside finally with optimizing return bytecode, also merging #15230 .

https://bugs.python.org/issue37830

Copy link
Member

@pablogsal pablogsal left a comment

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
Copy link

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
Copy link
Member Author

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
Copy link
Member

pablogsal commented Aug 13, 2019

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
Copy link
Member Author

isidentical commented Aug 13, 2019

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
Copy link
Member

pablogsal commented Aug 13, 2019

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
Copy link
Member

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

0
None

@isidentical
Copy link
Member Author

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

@isidentical isidentical deleted the donotemit branch December 7, 2019 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants