PEP 765: add report as appendix

Author: iritkatriel

peps/pep-0765.rst

@@ -54,7 +54,7 @@ Rationale
 =========
 
 A recent `analysis of real world code
-<https://github.com/iritkatriel/finally/blob/main/README.md>`_ shows that:
+<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).
@@ -64,6 +64,8 @@ A recent `analysis of real world code
 - Code owners are typically receptive to fixing the bugs, and
   find that easy to do.
 
+See `the appendix<#appendix>`__ for more details.
+
 This new data indicates that it would benefit Python's users if
 Python itself moved them away from this harmful feature.
 
@@ -154,6 +156,235 @@ that their code needs to change. The `empirical evidence
 shows that the changes necessary are typically quite
 straightforward.
 
+Appendix
+========
+
+Below is an abridged version of a
+`research report<https://github.com/iritkatriel/finally/commits/main/README.md>`__
+by Irit Katriel, which was posted on 9 Nov 2024.
+
+``return`` in ``finally`` considered harmful
+--------------------------------------------
+
+I describe an investigation of usage of ``return``, ``break`` and ``continue``
+in a ``finally`` clause in real world code. It addresses the
+questions: Are people using it? How often are they using it incorrectly?
+How much churn would the proposed change create?
+
+Method
+^^^^^^
+
+The analysis is based on the 8,000 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<https://github.com/faster-cpython/tools/blob/main/scripts/download_packages.py>`__
+written by Guido van Rossum, which in turn relies on Hugo van Kemenade's
+`tool<https://hugovk.github.io/top-pypi-packages/>`__ that creates a list of the
+most popular packages.
+
+Once downloaded, a
+`second script<https://github.com/iritkatriel/finally/blob/main/scripts/ast_analysis.py>`__
+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.
+
+I then found the current source code for each occurence, 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:
+
+.. code-block::
+    :class: bad
+
+    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:
+
+.. code-block::
+    :class: bad
+
+        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<https://discuss.python.org/t/an-analysis-of-return-in-finally-in-the-wild/70633/15>`__,
+> 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 acknowleding 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<https://github.com/mosaicml/composer/blob/694e72159cf026b838ba00333ddf413185b4fb4f/composer/cli/launcher.py#L590>`__
+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 in this code. 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`` to swallow any
+exceptions from within it.
+
+- In `webtest<https://github.com/Pylons/webtest/blob/617a2b823c60e8d7c5f6e12a220affbc72e09d7d/webtest/http.py#L131>`__
+there is a ``finally`` block that contains only ``return False``. It could be replaced
+by
+
+.. code-block::
+
+    except BaseException:
+        pass
+    return False
+
+- In `kivy<https://github.com/kivy/kivy/blob/3b744c7ed274c1a99bd013fc55a5191ebd8a6a40/kivy/uix/codeinput.py#L204>`__
+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<https://forge.extranet.logilab.fr/open-source/logilab-common/-/blob/branch/default/test/data/module.py?ref_type=heads#L60>`__
+there is, once again, a ``finally`` clause that can be replace by an ``except BaseException``
+with the ``return`` dedented one level.
+
+- In `pluggy<https://github.com/pytest-dev/pluggy/blob/c760a77e17d512c3572d54d368fe6c6f9a7ac810/src/pluggy/_callers.py#L141>`__
+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<https://github.com/aws/aws-sam-cli/blob/97e63dcc2738529eded8eecfef4b875abc3a476f/samcli/local/apigw/local_apigw_service.py#L721>`__
+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<https://github.com/scrapy/scrapy/blob/52c072640aa61884de05214cb1bdda07c2a87bef/scrapy/utils/gz.py#L27>`__
+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
+
+.. code-block::
+
+    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.
+
+## Acknowledgements
+
+I thank:
+
+- Alyssa Coghlan for
+`bringing this issue my attention<https://discuss.python.org/t/pep-760-no-more-bare-excepts/67182/97>`__.
+
+- Guido van Rossum and Hugo van Kemenade for the
+`script<https://github.com/faster-cpython/tools/blob/main/scripts/download_packages.py>`__
+that downloads the most popular PyPI packages.
+
+- The many code authors I contacted for their responsiveness and grace.
+
 Copyright
 =========