From d1c01a97dcbe9f28c5bcb09309a2113cccc8684e Mon Sep 17 00:00:00 2001 From: Christian Hammond Date: Fri, 13 Sep 2024 13:25:53 -0700 Subject: [PATCH] Fix a crash when accessing locals on Python 3.13. Python 3.13 is introducing some behind-the-scenes changes for how local variables are accessed within a function, with an eye toward performance and reliability. This normally wouldn't impact users in any way, but it does impact kgb, given the work required to dynamically patch functions. The problem was that when spying on closures where the inner function references an argument from a parent function, any attempt to access `locals()` would fail. Since we need to store `locals()` in order to access variables in the function, this broke us. The reason it happens is a bit unclear, but the fix is not. Historically, when creating a new `CodeType` for a patched function, we would retain the `co_freevars` and `co_cellvars` of the old function, which was at some point necessary for closures. Best I can tell, this was needed when creating a new merged `CodeType` on these older releases, but does not seem to be important anymore (at least when using `CodeType.replace()`). Removing the overridden `co_freevars` and `co_cellvars` and using the new values only ended up fixing the `locals()` access. There doesn't seem to be a need to override these. I've tested with running the kgb, Review Board, Djblets, and RBTools test suites for all affected Python versions (3.8 through 3.13) without issue. Testing Done: Unit tests pass in kgb, Djblets, and Review Board with all supported versions of Python. Reviewed at https://reviews.reviewboard.org/r/14168/ --- kgb/spies.py | 8 ++++++-- kgb/tests/test_function_spy.py | 18 ++++++++++++++++++ tox.ini | 2 +- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/kgb/spies.py b/kgb/spies.py index f85834e..8ca1d27 100644 --- a/kgb/spies.py +++ b/kgb/spies.py @@ -993,10 +993,14 @@ def _build_spy_code(self, func, forwarding_call): # one when possible. On Python 3.11, this will ensure that # state needed for exceptions (co_positions()) will be set # correctly. + # + # NOTE: Prior to kgb 7.2, we had set co_freevars and co_vellvars + # here. This caused crashes with Python 3.13 beta 2 (the + # latest release as of this writing -- June 19, 2024). We + # don't appear to actually need or want to set these on + # Python 3, so we removed this. replace_kwargs = { 'co_name': old_code.co_name, - 'co_freevars': old_code.co_freevars, - 'co_cellvars': old_code.co_cellvars, } if pyver >= (3, 11): diff --git a/kgb/tests/test_function_spy.py b/kgb/tests/test_function_spy.py index 9ec3119..ab223e7 100644 --- a/kgb/tests/test_function_spy.py +++ b/kgb/tests/test_function_spy.py @@ -1188,6 +1188,24 @@ def inline_func(): self.assertTrue(func.called) self.assertEqual(d, {'called': True}) + def test_call_with_function_providing_closure_access_args(self): + """Testing FunctionSpy calls with an inline function accessing + parent's arguments + """ + # NOTE: This originally caused a crash on Python 3.13 beta 2. + # See: https://github.com/beanbaginc/kgb/issues/11 + def func(arg): + def inline_func(): + print(arg) + + return 123 + + self.agency.spy_on(func) + + d = func(42) + self.assertEqual(d, 123) + self.assertTrue(func.called) + def test_call_with_bound_method_with_list_comprehension_and_self(self): """Testing FunctionSpy calls for bound method using a list comprehension referencing 'self' diff --git a/tox.ini b/tox.ini index 42de234..ae8da09 100644 --- a/tox.ini +++ b/tox.ini @@ -1,5 +1,5 @@ [tox] -envlist = py{27,36,37,38,39,310,311},pypy{37,38} +envlist = py{27,36,37,38,39,310,311,312,313},pypy{37,38} skipsdist = True [testenv]