Problems with a decorator used for both methods and functions?
Closed this issue · 5 comments
In this PR @ncoghlan suggests that a single singledispatch
decorator is not likely to work because of the way the descriptor protocol works.
Currently, what we're doing to support method variants is this, where we simply implement __get__
on VariantFunction
so that it spawns a new VariantMethod
if necessary. There is a problem that this implementation ends up eagerly binding many variants every time a new instance is created (see #6), but I don't think this necessitates an entirely new decorator.
I don't totally understand why a separate staticdispatchmethod
is suggested rather than using the approach we've taken, so I'm opening this issue to track that to see if we've missed something.
If it does turn out that a single implementation can't work for both functions and methods, the options I see are (in order of how much I like them):
- A
method
variant onvariant
itself, e.g.@variants.primary.method
- A separate
@variants.primarymethod
decorator used only with methods. - A parameter passed to
primary
, e.g.@variants.primary(method=True)
Not sure what effect (if any) this will have on the VariantFunction.variant
decorator.
The more detailed comment is the one in python/cpython#4987 (diff)
On its own, class-definition-time wrapper generation works, but things can start to misbehave once you start composing the descriptor with other descriptors , so partialmethod
and singledispatchmethod
ended up as separate types that work differently as descriptors from the way partial
and singledispatch
do.
That said, if partial.__get__
and singledispatch.__get__
had been written this way from the beginning, I expect we wouldn't have needed dedicated *method
versions.
@ncoghlan Do you know why singledispatch
and partial
were not just rewritten to add the __get__
behavior seen in variants? It seems like it could be done without a change in user-facing behavior. I'm still having trouble assessing exactly what problems I'm buying with this approach.
Also, I don't think that what I'm doing here is class-definition-time wrapper generation, right? These wrappers are actually generated whenever the method is accessed, see:
class FuncWrapper:
def __init__(self, func):
self._func = func
def __call__(self, *args, **kwargs):
return self._func(*args, **kwargs)
def __get__(self, obj, objtype=None):
print('Wrapper descriptor called')
if obj is not None:
print('Method wrapper created')
return MethodWrapper(self, obj)
return self
class MethodWrapper:
def __init__(self, func_wrapper, parent):
self._parent = parent
self._func = func_wrapper
print('Creating MethodWrapper')
def __call__(self, *args, **kwargs):
return self._func(self._parent, *args, **kwargs)
print('Defining func1')
@FuncWrapper
def func1():
pass
print('Defining class1')
class class1:
@FuncWrapper
def func2(self):
print(self)
print('\nCalling func1')
func1()
print('\nCreating instance1')
instance1 = class1()
print('\nAccessing func2')
instance1.func2
print('\nCalling func2')
instance1.func2()
print('\nCalling func2 again')
instance1.func2()
Running this:
$ python test_descriptor.py
Defining func1
Defining class1
Calling func1
Creating instance1
Accessing func2
Wrapper descriptor called
Method wrapper created
Creating MethodWrapper
Calling func2
Wrapper descriptor called
Method wrapper created
Creating MethodWrapper
<__main__.class1 object at 0x7ff8b67e1588>
Calling func2 again
Wrapper descriptor called
Method wrapper created
Creating MethodWrapper
<__main__.class1 object at 0x7ff8b67e1588>
Given this, the wrapper generation should be lazier, cheaper and probably cached. If I kinda squint I can imagine a scenario where something funky happens when you stack a registration decorator on the outside of one of these things, but I'm not sure I can come up with any obvious concrete examples, so it's hard to know how seriously to take it (e.g. how easy would the problems this causes be to work around?).
Ah, yeah - what you're doing is essentially how the method variants in the standard library work.
The compatibility issue we had/have in the standard library is that partial and singledispatch historically implied staticmethod type behaviour, and there isn't any straightforward way to deprecate that and ask folks to add an explicit static method instead.
Sounds good to me. Closing this issue as my concerns are alleviated.
And clarifying for future readers: the class definition time problem I mention above is one that the closed PR against CPython suffered from - I just misinterpreted Paul's link to that as suggesting that python-variants also worked that way (it doesn't - python-variants works the same way as the accepted version of the standard library PR)