PEP 765: Disallow return/break/continue that exit a finally block

I abandoned this thread for a while, but I will point out that if an inner “finally” is exited this way, an outer “finally” still runs!

I still have mixed feelings and wish the next SC good luck figuring it out. I honestly don’t know what I would have done when I was BDFL.

3 Likes

So the return/break/continue statement in that finally block would be silently ignored?

You may argue that this is the “correct” behavior, but that doesn’t seem obvious to me. The problem is we don’t know what the original developer intended. Python’s usual stance on this is to “avoid the temptation to guess”.

3 Likes

Only in that it doesn’t exit the function (which is already exiting due to an exception) or repeat the loop (which is already broken due to an exception).

Any later statements in the finally block would not be executed. And any outer finally blocks would still be executed (thanks Guido!).

Worth noting that raising an exception from a finally block when an exception is already being raised will chain the exception, which means we already acknowledge that the original exception is still relevant. Applying that logic to all ways to early-exit the finally block seems pretty consistent to me (though making that change is nearly as hard as making the proposed one, hence my preference for status quo).

1 Like

I don’t like this change. Either make it an error or don’t make it a warning. This halfway-inbetween stance is targeting a small number of users and telling them they are wrong, but without the conviction to actually make it an error, and instead doing it in a way that turns it into “lets annoy your users to shame you into changing it”

For what it’s worth, I don’t think this should be an error or a warning, and this would make some code I maintain much more convoluted where finally is intentionally used to silence errors and return a value, and there is exception handling involved, requiring code duplication in branches to actually get the desired ordering.

2 Likes

I don’t see how that combination of desiderata is achievable. The only way to remove it is to break stuff; if you raise a warning you don’t break stuff but also don’t remove it.

Regardless of “should”, I don’t see how there can be a way. :slight_smile:

This is something I’ve seen in other discussions about backwards compatibility, and I think it kind of misses the point. In my view, the important inflection point in terms of backwards compatibility is between “this code can still run with zero changes” and “this code can’t run without changes”. It doesn’t matter whether code that gets broken by a change is “easy to rewrite”; the point of backwards compatibility is to ensure that old code can run exactly as is.

As I mentioned in an earlier post, it might sometimes be worthwhile to break backwards compatibility, but I don’t see it as useful to try to break it “just a little bit” to fix minor things like this.

So if we don’t want to break things, we can’t change this behavior. If we can’t change the behavior, I don’t think it’s a great idea to just add a warning. There are endless dubious coding practices we could warn about, but I’d rather not go down that slippery slope. It would be better to encourage linters to add a check for this, and perhaps to add a warning note to the docs.

5 Likes

The proposal moves the construct into the “may not work on all implementations” category. Allowing new Python implementations to skip implementing this feature was the primary motivation in PEP 601 (the MicroPython devs didn’t want to support it), and that aspect is still part of the motivation for this PEP.

Essentially, we want to demote this behaviour from “Python language feature that all compliant implementations offer” to “CPython implementation detail that other implementations may or may not support”. That can be done without adding even the syntax warning to CPython, but if the demotion happens, a syntax warning makes the portability issue explicit.

I don’t know if this is the best place to chip in specific feedback on wording in the PEP, since the discussion is still fairly high-level. But in reading the PEP for the first time, just one small clarity thing.

In the Specifications section, it’s not super clear at first glance which examples have new behavior and which don’t, just based on the verbs “include”/“exclude”. Perhaps the headings could change to:

“This includes the following examples” → "These examples may emit a SyntaxWarning or SyntaxError

and

“But excludes these” → “These examples would not emit the warning or error”

3 Likes

I’ve been mulling this proposal over for a few days now since first noticing this thread.

Personally I have been bitten by this gotcha many times, and IMO being able to exit a finally block in this way should never have been allowed in the first place.

Although I don’t like the idea of adding a SyntaxWarning for something without a plan to eventually make it a SyntaxError - it doesn’t feel like the right way to do things - I suppose it still brings the benefit of being alerted to the problem, and doesn’t really have a downside in practice, only in philosophy. So I guess I can say I’m -0 on this PEP.

What I’d love to see is for PEP 601, which would have made this a SyntaxError, to be revived, reconsidered, and accepted. I suppose if PEP 765 gets accepted, and that causes PEP 601 to become acceptable in future, then it will have been worth it.

Linters and static analysis tools should definitely gain a rule to forbid this facility if they don’t already have one, even if it’s optional and off by default; at least people could then opt in to forbidding it from their own code.

Forbidding it in your own code doesn’t help you if it’s one of your dependencies that is eating the exception. All linter based mitigations for this have that problem: they don’t tell you if one of your dependencies is at risk of making exceptions unexpectedly vanish.

By contrast, if the compiler is doing it, then app developers get notified no matter if it’s their own code or code in one of their dependencies which has the dubious construct.

I’m thoroughly sympathetic to folks favouring eventual escalation to a full SyntaxError (I was the core dev sponsor for PEP 601), but the following numbers from the project author responses in Irit’s report concern me:

  • “3 replied that the code is no longer maintained so this won’t be fixed.”
  • 33 did not respond (at least in the time frame covered by the report)

When CPython is only emitting SyntaxWarning, app developers have options for attempting to suppress that warning (this can be done programmatically for implicit runtime compilation, but can be a bit trickier for installation time pre-compilation).

If CPython escalates to a full SyntaxError, the problem becomes much harder to work around without outright forking the affected dependency.

4 Likes