Skip to content
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.

Sign up for GitHub

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

gh-127750: Fix singledispatchmethod caching (v2) #128648

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+59 −13
Open

gh-127750: Fix singledispatchmethod caching (v2) #128648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b9a05dc
add singledispatchmethod test for objects with equal hash and ==
eendebakpt Dec 11, 2024
0714c06
add singledispatchmethod test for dangling references
eendebakpt Dec 11, 2024
ef03f2d
use id for object cache in singledispatchmethod
eendebakpt Dec 11, 2024
6c96b9d
📜🤖 Added by blurb_it.
blurb-it[bot] Dec 11, 2024
f282ab8
rework using __dict__
eendebakpt Jan 8, 2025
89a4a5c
remove comments
eendebakpt Jan 8, 2025
ecc3e82
avoid exception
eendebakpt Jan 8, 2025
efd4034
whitespace
eendebakpt Jan 8, 2025
88b5e90
whitespace
eendebakpt Jan 8, 2025
1885278
Merge branch 'main' into singledispatchmethod_caching_v2
eendebakpt Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions Lib/functools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1026,8 +1026,8 @@ def __init__(self, func):
self.dispatcher = singledispatch(func)
self.func = func

import weakref # see comment in singledispatch function
self._method_cache = weakref.WeakKeyDictionary()
def __set_name__(self, obj, name):
self.attrname = name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check cached_property.__set_name__, it has some more stuff in it - might be needed here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. The additions there prevent something like this:

@dataclass(frozen=True)
class A:
    value: int

    @singledispatchmethod
    def dispatch(self, x):
        return id(self)

    renamed_dispatch = dispatch # allowed? if so, how should it behave

The corresponding test for the cached_property for this is

cpython/Lib/test/test_functools.py

Line 3315 in 34e840f

def test_reuse_different_names(self):
.

But on current main renaming is allowed for the singledispatchmethod.

I am not sure here what the desired behavior is (and why)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this implementation is desirable, maybe later someone who knows more about this can comment.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, the only reason cached properties can't be renamed is because the cache is keyed by the attribute's name.

Allowing a rebind would disconnect the cached property from it's cached value.

Copy link

@vodik vodik Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think you might want to either ignore renames or do something along these lines (ignoring error handling):

    if self.attrname:
        cache[name] = cache.pop(self.attrname)
    self.attrname = name

As far as I know, each binding shares the same instance of the descriptor, so as long as the cache key is constant, it should work no matter how many times it's been renamed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing a rebind would disconnect the cached property from it's cached value.

This is kind of the same situation.

If rename is allowed, then it would simply cache to the last attrname. Drawback is that there is a small risk for unused cached methods.

I think it might be most straight forward to copy+paste cached_property.__set_name__. It does seem a sensible restriction. It comes at expense of flexibility, but personally, I have never run into that TypeError.

Also, it will be easier to address changes/improvements when 2 implementations that use the same caching approach are aligned.


def register(self, cls, method=None):
"""generic_method.register(cls, func) -> func
Expand All @@ -1037,15 +1037,13 @@ def register(self, cls, method=None):
return self.dispatcher.register(cls, func=method)

def __get__(self, obj, cls=None):
if self._method_cache is not None:
try:
_method = self._method_cache[obj]
except TypeError:
self._method_cache = None
except KeyError:
pass
else:
return _method
if hasattr(obj, '__dict__'):
cache = obj.__dict__
method = cache.get(self.attrname)
if method is not None:
return method
else:
cache = None

dispatch = self.dispatcher.dispatch
funcname = getattr(self.func, '__name__', 'singledispatchmethod method')
Expand All @@ -1059,8 +1057,8 @@ def _method(*args, **kwargs):
_method.register = self.register
update_wrapper(_method, self.func)

if self._method_cache is not None:
self._method_cache[obj] = _method
if cache is not None:
cache[self.attrname] = _method

return _method

Expand Down
47 changes: 47 additions & 0 deletions Lib/test/test_functools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import collections
import collections.abc
import copy
import gc
from itertools import permutations
import pickle
from random import choice
Expand Down Expand Up @@ -3240,6 +3241,52 @@ def f(arg):
def _(arg: undefined):
return "forward reference"

def test_singledispatchmethod_hash_comparision_equal(self):
from dataclasses import dataclass

@dataclass(frozen=True)
class A:
value: int

@functools.singledispatchmethod
def dispatch(self, x):
return id(self)

t1 = A(1)
t2 = A(1)

assert t1 == t2
assert id(t1) == t1.dispatch(2)
assert id(t2) == t2.dispatch(2) # gh-127750

def test_singledispatchmethod_object_references(self):
class ReferenceTest:
instance_counter = 0

def __init__(self):
ReferenceTest.instance_counter = ReferenceTest.instance_counter + 1

def __del__(self):
ReferenceTest.instance_counter = ReferenceTest.instance_counter - 1

@functools.singledispatchmethod
def go(self, item):
pass

assert ReferenceTest.instance_counter == 0
t=ReferenceTest()
assert ReferenceTest.instance_counter == 1
x = []
for ii in range(1000):
t = ReferenceTest()
t.go(ii)
x.append(t)
del t
del x
gc.collect()
assert ReferenceTest.instance_counter == 0


class CachedCostItem:
_cost = 1

Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2024-12-11-20-26-44.gh-issue-127750.8witaH.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Update caching of :class:`functools.singledispatchmethod` to avoid collisions of objects which compare equal.
Loading