-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
PEP 765: Disallow return/break/continue that exit a finally block #4119
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
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
5ba270a
first draft
iritkatriel 66238e1
apply Alissa's comments
iritkatriel c895bb8
add python version
iritkatriel e975cd3
fix lint errors. Add Rationale headline
iritkatriel ea35c82
typo
iritkatriel 0450fe4
emails as elsewhere
iritkatriel 8971a6f
assign pep number. Fix build errors
iritkatriel b06a4b6
fix date
iritkatriel 48b787b
typo
iritkatriel 7ec364a
Apply suggestions from code review
iritkatriel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| PEP: 765 | ||
| Title: Disallow return/break/continue that exit a finally block | ||
| Author: Irit Katriel <irit@python.org>, Alyssa Coghlan <ncoghlan@gmail.com> | ||
| Discussions-To: https://discuss.python.org/t/an-analysis-of-return-in-finally-in-the-wild/70633 | ||
| Status: Draft | ||
| Type: Standards Track | ||
| Created: 15-Nov-2024 | ||
| Python-Version: 3.14 | ||
| Post-History: `09-Nov-2024 <https://discuss.python.org/t/an-analysis-of-return-in-finally-in-the-wild/70633>`__, | ||
|
|
||
| Abstract | ||
| ======== | ||
|
|
||
| This PEP proposes to withdraw support for ``return``, ``break`` and | ||
| ``continue`` statements that break out of a ``finally`` block. | ||
| This was proposed in the past by :pep:`601`. The current PEP | ||
| is based on empirical evidence regarding the cost/benefit of | ||
| this change, which did not exist at the time that :pep:`601` | ||
| was rejected. It also proposes a slightly different solution | ||
| than that which was proposed by :pep:`601`. | ||
|
|
||
| Motivation | ||
| ========== | ||
|
|
||
| The semantics of ``return``, ``break`` and ``continue`` in a | ||
| finally block are surprising for many developers. | ||
| The :ref:`documentation <python:tut-cleanup>` 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``. It was rejected in favour of viewing this | ||
| as a programming style issue, to be handled by linters and :pep:`8`. | ||
| Indeed, :pep:`8` now recommends not to use control flow statements | ||
| in a ``finally`` block, and linters such as | ||
| `Pylint <https://pylint.readthedocs.io/en/stable/>`__, | ||
| `Ruff <https://docs.astral.sh/ruff/>`__ and | ||
| `flake8-bugbear <https://github.com/PyCQA/flake8-bugbear>`__ | ||
| flag them as a problem. | ||
|
|
||
| Rationale | ||
| ========= | ||
|
|
||
| A recent `analysis of real world code | ||
| <https://github.com/iritkatriel/finally/blob/main/README.md>`_ shows that: | ||
|
|
||
| - These features are rare (2 per million LOC in the top 8,000 PyPI | ||
| packages, 4 per million LOC in a random selection of packages). | ||
| This could be thanks to the linters that flag this pattern. | ||
| - Most of the usages are incorrect, and introduce unintended | ||
| exception-swallowing bugs. | ||
| - Code owners are typically receptive to fixing the bugs, and | ||
| find that easy to do. | ||
|
|
||
| This new data indicates that it would benefit Python's users if | ||
| Python itself moved them away from this harmful feature. | ||
|
|
||
| `One of the arguments brought up | ||
| <https://discuss.python.org/t/pep-601-forbid-return-break-continue-breaking-out-of-finally/2239/24>`__ | ||
| was that language features should be orthogonal, and combine without | ||
| context-based restrictions. However, in the meantime :pep:`654` has | ||
| been implemented, and it forbids ``return``, ``break`` and ``continue`` | ||
| in an ``except*`` clause because the semantics of that would violate | ||
| the property that ``except*`` clauses operate *in parallel*, so the | ||
| code of one clause should not suppress the invocation of another. | ||
| In that case we accepted that a combination of features can be | ||
| harmful enough that it makes sense to disallow it. | ||
|
|
||
|
|
||
| Specification | ||
| ============= | ||
|
|
||
| The change is to specify as part of the language spec that | ||
| Python's compiler may emit a ``SyntaxWarning`` or ``SyntaxError`` | ||
| when a ``return``, ``break`` or ``continue`` would transfer | ||
| control flow from within a ``finally`` block to a location outside | ||
| of it. | ||
|
|
||
| This includes the following examples: | ||
|
|
||
| .. code-block:: | ||
| :class: bad | ||
|
|
||
| def f(): | ||
| try: | ||
| ... | ||
| finally: | ||
| return 42 | ||
|
|
||
| for x in o: | ||
| try: | ||
| ... | ||
| finally: | ||
| break # (or continue) | ||
|
|
||
| But excludes these: | ||
|
|
||
| .. code-block:: | ||
iritkatriel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| :class: good | ||
|
|
||
| try: | ||
| ... | ||
| finally: | ||
| def f(): | ||
| return 42 | ||
|
|
||
| try: | ||
| ... | ||
| finally: | ||
| for x in o: | ||
| break # (or continue) | ||
|
|
||
|
|
||
| CPython will emit a ``SyntaxWarning`` in version 3.14, and we leave | ||
| it open whether, and when, this will become a ``SyntaxError``. | ||
| However, we specify here that a ``SyntaxError`` is permitted by | ||
| the language spec, so that other Python implementations can choose | ||
| to implement that. | ||
|
|
||
| Backwards Compatibility | ||
| ======================= | ||
|
|
||
| For backwards compatibility reasons, we are proposing that CPython | ||
| emit only a ``SyntaxWarning``, with no concrete plan to upgrade that | ||
| to an error. Code running with ``-We`` may stop working once this | ||
| is introduced. | ||
|
|
||
| Security Implications | ||
| ===================== | ||
|
|
||
| The warning/error will help programmers avoid some hard to find bugs, | ||
| so will have a security benefit. We are not aware of security issues | ||
| related to raising a new ``SyntaxWarning`` or ``SyntaxError``. | ||
|
|
||
| How to Teach This | ||
| ================= | ||
|
|
||
| The change will be documented in the language spec and in the | ||
| What's New documentation. The ``SyntaxWarning`` will alert users | ||
| that their code needs to change. The `empirical evidence | ||
| <https://github.com/iritkatriel/finally/blob/main/README.md>`__ | ||
| shows that the changes necessary are typically quite | ||
| straightforward. | ||
|
|
||
| Copyright | ||
| ========= | ||
|
|
||
| This document is placed in the public domain or under the | ||
| CC0-1.0-Universal license, whichever is more permissive. | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.