return in finally considered harmful

Irit Katriel Nov 9 2024

Discussion link.

TL;DR

The semantics of return, break and continue in a finally block are surprising. This document describes an analysis of their use in real world code, which was conducted in order to assess the cost and benefit of blocking these features in future versions of Python. The results show that

1. they are not used often.
2. when used, they are usually used incorrectly.
3. code authors are typically receptive and quick to fix the code when the error is pointed out to them.

My conclusion is that it is both desireable and feasible to make this a SyntaxWarning, and later a SyntaxError.

Introduction

The semantics of return, break and continue are surprising for many developers. The documentation mentions that

• If the finally clause executes a break, continue or return statement, exceptions are not re-raised.
• If a finally clause includes a return statement, the returned value will be the one from the finally clause’s return statement, not the value from the try clause’s return statement.

Both of these behaviours cause confusion, but the first is particularly dangerous because a swallowed exception is more likely to slip through testing, than an incorrect return value.

In 2019, PEP 601 proposed to change Python to emit a SyntaxWarning for a few releases and then turn it into a SyntaxError. The PEP was rejected in favour of viewing this as a programming style issue, to be handled by linters and PEP8. Indeed, PEP8 now recommends not to used control flow statements in a finally block, and linters such as pylint, ruff and flake8-bugbear flag them as a problem.

It is not disputed that return, break and continue in a finally clause should be avoided, the question is whether changing Python to forbid it now is worth the churn. What was missing at the time that PEP 601 was rejected, was an understanding of how this issue manifests in practice. Are people using it? How often are they using it incorrectly? How much churn would the proposed change create?

The purpose here is to bridge that gap by evaluating real code (the 8000 most popular packages on PyPI) to answer these questions. I believe that the results show that PEP 601 should now be implemented.

Method

The analysis is based on the 8000 most popular PyPI packages, in terms of number of downloads in the last 30 days. They were downloaded on the 17th-18th of October, using a script written by Guido van Rossum, which in turn relies on Hugo van Kemenade's tool that creates a list of the most popular packages.

Once downloaded, a second script was used to construct an AST for each file, and traverse it to identify break, continue and return statements which are directly inside a finally block. By directly, I mean that the script looks for things like

    finally:
        return 42  / break / continue

and not situations where a return exits a function defined in the finally block:

    finally:
        def f():
            return 42
        ...

or a break or continue relates to a loop which is nested in the finally block:

    finally:
        for i in ...:
              break / continue

I then found the current source code for each occurrence, and categorized it. For cases where the code seems incorrect, I created an issue in the project's bug tracker. The responses to these issues are also part of the data collected in this investigation.

Results

I decided not to include a list of the incorrect usages, out of concern that it would make this report look like a shaming exercise. Instead I will describe the results in general terms, but will mention that some of the problems I found appear in very popular libraries, including a cloud security application. For those so inclined, it should not be hard to replicate my analysis, as I provided links to the scripts I used in the Method section.

The projects examined contained a total of 120,964,221 lines of Python code, and among them the script found 203 instances of control flow instructions in a finally block. Most were return, a handful were break, and none were continue. Of these:

• 46 are correct, and appear in tests that target this pattern as a feature (e.g., tests for linters that detect it).
• 8 seem like they could be correct - either intentionally swallowing exceptions or appearing where an active exception cannot occur. Despite being correct, it is not hard to rewrite them to avoid the bad pattern, and it would make the code clearer: deliberately swallowing exceptions can be more explicitly done with except BaseException:, and return which doesn't swallow exceptions can be moved after the finally block.
• 149 were clearly incorrect, and can lead to unintended swallowing of exceptions. These are analyzed in the next section.

The Error Cases

Many of the error cases followed this pattern:

    try:
        ...
    except SomeSpecificError:
        ...
    except Exception:
        logger.log(...)
    finally:
        return some_value

Code like this is obviously incorrect because it deliberately logs and swallows Exception subclasses, while silently swallowing BaseExceptions. The intention here is either to allow BaseExceptions to propagate on, or (if the author is unaware of the BaseException issue), to log and swallow all exceptions. However, even if the except Exception was changed to except BaseException, this code would still have the problem that the finally block swallows all exceptions raised from within the except block, and this is probably not the intention (if it is, that can be made explicit with another try-except BaseException).

Another variation on the issue found in real code looks like this:

        try:
            ...
        except:
            return NotImplemented
        finally:
            return some_value

Here the intention seems to be to return NotImplemented when an exception is raised, but the return in the finally block would override the one in the except block.

Note

Following the discussion, I repeated the analysis on a random selection of PyPI packages (to analyze code written by average programmers). The sample contained in total 77,398,892 lines of code with 316 instances of return/break/continue in finally. So about 4 instances per million lines of code.

Author reactions

Of the 149 incorrect instances of return or break in a finally clause, 27 were out of date, in the sense that they do not appear in the main/master branch of the library, as the code has been deleted or fixed by now. The remaining 122 are in 73 different packages, and I created an issue in each one to alert the authors to the problems. Within two weeks, 40 of the 73 issues received a reaction from the code maintainers:

• 15 issues had a PR opened to fix the problem.
• 20 received reactions acknowledging the problem as one worth looking into.
• 3 replied that the code is no longer maintained so this won't be fixed.
• 2 closed the issue as "works as intended", one said that they intend to swallow all exceptions, but the other seemed unaware of the distinction between Exception and BaseException.

One issue was linked to a pre-existing open issue about non-responsiveness to Ctrl-C, conjecturing a connection.

Two of the issue were labelled as "good first issue".

The correct usages

The 8 cases where the feature appears to be used correctly (in non-test code) also deserve attention. These represent the "churn" that would be caused by blocking the feature, because this is where working code will need to change. I did not contact the authors in these cases, so we will need to assess the difficulty of making these changes ourselves.

• In mosaicml there is a return in a finally at the end of the main function, after an except: clause which swallows all exceptions. The return in the finally would swallow an exception raised from within the except: clause, but this seems to be the intention. A possible fix would be to assign the return value to a variable in the finally clause, dedent the return statement and wrap the body of the except: clause by another try-except that would swallow exceptions from it.

• In webtest there is a finally block that contains only return False. It could be replaced by

    except BaseException:
        pass
    return False

• In kivy there is a finally that contains only a return statement. Since there is also a bare except just before it, in this case the fix will be to just remove the finally: block and dedent the return statement.

• In logilab-common there is, once again, a finally clause that can be replace by an except BaseException with the return dedented one level.

• In pluggy there is a lengthy finally with two return statements (the second on line 182). Here the return value can be assigned to a variable, and the return itself can appear after we've exited the finally clause.

• In aws-sam-cli there is a conditional return at the end of the block. From reading the code, it seems that the condition only holds when the exception has been handled. The conditional block can just move outside of the finally block and achieve the same effect.

• In scrappy there is a finally that contains only a break instruction. Assuming that it was the intention to swallow all exceptions, it can be replaced by

    except BaseException:
         pass
    break

Discussion

The first thing to note is that return/break/continue in a finally block is not something we see often: 203 instance in over 120 million lines of code. This is, possibly, thanks to the linters that warn about this.

The second observation is that most of the usages were incorrect: 73% in our sample (149 of 203).

Finally, the author responses were overwhelmingly positive. Of the 40 responses received within two weeks, 35 acknowledged the issue, 15 of which also created a PR to fix it. Only two thought that the code is fine as it is, and three stated that the code is no longer maintained so they will not look into it.

The 8 instances where the code seems to work as intended, are not hard to rewrite.

Conclusion

The results indicate that return, break and continue in a finally block

• are rarely used.
• when they are used, they are usually used incorrectly.
• code authors are receptive to changing their code, and tend to find it easy to do.

This indicates that it is both desireable and feasible to change Python to emit a SyntaxWarning, and in a few years a SyntaxError for these patterns.

Acknowledgements

I thank:

• Alyssa Coghlan for bringing this issue my attention.

• Guido van Rossum and Hugo van Kemenade for the script that downloads the most popular PyPI packages.

• The many code authors I contacted for their responsiveness and grace.

© 2024 Irit Katriel