Skip to content

Commit

Permalink
Fix a crash when accessing locals on Python 3.13.
Browse files Browse the repository at this point in the history
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/
  • Loading branch information
chipx86 committed Sep 13, 2024
1 parent 9c18e19 commit d1c01a9
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 3 deletions.
8 changes: 6 additions & 2 deletions kgb/spies.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
18 changes: 18 additions & 0 deletions kgb/tests/test_function_spy.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
2 changes: 1 addition & 1 deletion tox.ini
Original file line number Diff line number Diff line change
@@ -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]
Expand Down

0 comments on commit d1c01a9

Please sign in to comment.