Skip to content

Preserve descriptor binding in weak proxies for decorated callables - #356

Merged
GrahamDumpleton merged 4 commits into
GrahamDumpleton:developfrom
agammann:fix-weak-proxy-descriptor-binding
Sep 22, 2026
Merged

GrahamDumpleton merged 4 commits into
GrahamDumpleton:developfrom
agammann:fix-weak-proxy-descriptor-binding

Conversation

@agammann

Copy link
Copy Markdown
Contributor

WeakFunctionProxy raises TypeError for decorated free functions and class-accessed descriptors because their wrapper instance can be None. Simply avoiding weakref.ref(None) is insufficient: a descriptor accessed through a class still needs its original owner when rebound, including when inherited by a subclass.

Keep the instance optional and retain the descriptor owner through a weak reference. Rebind with that owner when calling the proxy, preserving classmethod, staticmethod, and unbound instance-method behavior without keeping the class alive.

Add regressions for decorated free functions, descriptor access through classes and instances (including subclasses), and garbage collection. The tests also check that the expiration callback runs once and expired calls raise ReferenceError. The new free-function and descriptor tests fail on the original code.

Validation: the full just test matrix passes on Linux across Python 3.9 through 3.15, including 3.14t and 3.15t. Each of the nine interpreter configurations runs the full suite with a pure-Python installation, the C extension enabled, and the C extension disabled at runtime. The targeted weak-proxy tests also pass on Windows/Python 3.12.

@GrahamDumpleton

Copy link
Copy Markdown
Owner

Do you have example code which resulted in you finding this so I have a better idea of use cases that cause a problem. Thanks.

@agammann

Copy link
Copy Markdown
Contributor Author

Thanks for taking a look. I found this while reviewing WeakFunctionProxy's handling of decorated callables and exercising the supported function/method forms. The concrete pattern is a callback registry that stores weak proxies to callbacks which have already been decorated. Here is a reduced example:

import wrapt


@wrapt.decorator
def traced(wrapped, instance, args, kwargs):
    return wrapped(*args, **kwargs)


@traced
def on_event(value):
    return value + 1


class Handler:
    @traced
    @classmethod
    def on_event(cls, value):
        return cls.__name__, value


class Child(Handler):
    pass


for callback in (on_event, Handler.on_event, Child.on_event):
    try:
        registered = wrapt.WeakFunctionProxy(callback)
        print(registered(41))
    except TypeError as error:
        print(type(error).__name__ + ': ' + str(error))

I ran this against the base commit (24bd836) and the PR. On the base, each proxy construction raises TypeError: cannot create weak reference to 'NoneType' object. With the patch it prints:

42
('Handler', 41)
('Child', 41)

The free function has no bound instance. The class-accessed methods likewise have a wrapper instance of None, but carry an owner needed to bind the original descriptor again. Preserving that owner is what keeps Child.on_event bound to Child when called through the weak proxy. The tests also cover static methods and class-accessed instance methods.

I also investigated the Python 3.13 free-threaded CI failure. The new owner-lifetime test incorrectly assumed classes could be collected there after threads had started. I reproduced the same retention with a plain class and no wrapt import; it matches CPython 3.13's documented class immortalization.

The follow-up skips only that owner-collection test on free-threaded 3.13. The callable binding and function-expiration tests still run there, and the owner-collection test remains active on other interpreter configurations. No runtime implementation change was needed for this CI failure.

Local verification after the test correction: the full suite passes with pure Python, C extensions enabled, and C extensions disabled at runtime on each of Python 3.13t, 3.13, and 3.14t. For 3.13t, the three results were 1,265 passed / 9 skipped, 1,267 passed / 7 skipped, and 1,265 passed / 9 skipped. The 3.13t run used the pinned mypy 1.20.1 from a separate standard-Python environment because its librt dependency could not build against my local 3.13t interpreter; the runtime tests themselves ran under 3.13t. The 3.13 and 3.14t runs used the normal Justfile recipes. The correction is in commit 07db6cd.

@GrahamDumpleton

Copy link
Copy Markdown
Owner

They have started saying that 3.13t should be effectively ignored since it was the experimental version. I already ignore it when doing local tests because of issues with it, although that may be macOS specific and related to shared library availability (can't remember). So I should perhaps consider just dropping 3.13t testing from the GitHub actions workflow. I'll decide that when I properly review the PR. Thanks for the report.

The comments in weakrefs.py described the weak reference as being held
only against the instance and the function, so extend them to cover the
owner which is now also retained for function wrappers, and why it is
needed. Add a release note under 2.4.2 for the fix.
…criptor-binding

# Conflicts:
#	docs/changes.rst
@GrahamDumpleton

Copy link
Copy Markdown
Owner

Thanks for this, the analysis is right and the fix is the correct one. I confirmed that only avoiding the weakref.ref(None) isn't enough: without the owner, a decorated classmethod accessed via the class fails with 'classmethod' object is not callable, and a decorated instance method accessed via the class and called with an instance as the first argument runs but the decorator sees instance=None. Retaining the owner addresses both, and your tests pin down each case.

I've pushed one extra commit on top of your branch, with no changes to your code or tests:

  • A release note in docs/changes.rst under 2.4.2 describing the problem and the fix.
  • Expanded comments in weakrefs.py. The module comment and __init__ comment still described the weak reference as being held only against the instance and the function, so they now also cover the owner and why it's needed. The comment in __call__ that had ended up above the new owner branch described the original instance path, so I split it into one for each branch.

I also merged develop into the branch to resolve a conflict in docs/changes.rst with the release note from #357, which landed at the same spot.

Nothing else is needed from your side. Will merge once CI is through.

@GrahamDumpleton
GrahamDumpleton merged commit 167944d into GrahamDumpleton:develop Sep 22, 2026
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants