classification
Title: classmethod doesn't honour descriptor protocol of wrapped callable
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: berker.peksag, daniel.urban, eriknw, grahamd, gvanrossum, ionelmc, iritkatriel, lukasz.langa, miss-islington, pitrou, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-09-22 13:22 by grahamd, last changed 2021-11-19 19:12 by lukasz.langa. This issue is now closed.

Files
File name Uploaded Description Edit
funcobject.c.diff grahamd, 2013-09-22 13:22 Patch to classmethod tp_descr_get to do descriptor binding on wrapped object.
issue19072.diff berker.peksag, 2016-05-05 17:51 review
Pull Requests
URL Status Linked Edit
PR 8405 merged berker.peksag, 2018-07-23 10:30
PR 22757 closed eriknw, 2020-10-18 22:25
PR 22934 merged rhettinger, 2020-10-24 00:59
PR 22935 merged miss-islington, 2020-10-24 01:37
PR 27115 merged lukasz.langa, 2021-07-13 15:45
PR 27162 merged miss-islington, 2021-07-15 13:19
PR 29634 merged rhettinger, 2021-11-19 06:36
PR 29643 merged miss-islington, 2021-11-19 18:44
Messages (23)
msg198274 - (view) Author: Graham Dumpleton (grahamd) Date: 2013-09-22 13:22
The classmethod decorator when applied to a function of a class, does not honour the descriptor binding protocol for whatever it wraps. This means it will fail when applied around a function which has a decorator already applied to it and where that decorator expects that the descriptor binding protocol is executed in order to properly bind the function to the class.

A decorator may want to do this where it is implemented so as to be able to determine automatically the context it is used in. That is, one magic decorator that can work around functions, instance methods, class methods and classes, thereby avoiding the need to have multiple distinct decorator implementations for the different use case.

So in the following example code:

class BoundWrapper(object):
    def __init__(self, wrapped):
        self.__wrapped__ = wrapped
    def __call__(self, *args, **kwargs):
        print('BoundWrapper.__call__()', args, kwargs)
        print('__wrapped__.__self__', self.__wrapped__.__self__)
        return self.__wrapped__(*args, **kwargs)

class Wrapper(object):
    def __init__(self, wrapped):
        self.__wrapped__ = wrapped
    def __get__(self, instance, owner):
        bound_function = self.__wrapped__.__get__(instance, owner)
        return BoundWrapper(bound_function)

def decorator(wrapped):
    return Wrapper(wrapped)

class Class(object):
    @decorator
    def function_im(self):
        print('Class.function_im()', self)

    @decorator
    @classmethod
    def function_cm_inner(cls):
        print('Class.function_cm_inner()', cls)

    @classmethod
    @decorator
    def function_cm_outer(cls):
        print('Class.function_cm_outer()', cls)

c = Class()

c.function_im()
print()
Class.function_cm_inner()
print()
Class.function_cm_outer()

A failure is encountered of:

$ python3.3 cmgettest.py
BoundWrapper.__call__() () {}
__wrapped__.__self__ <__main__.Class object at 0x1029fc150>
Class.function_im() <__main__.Class object at 0x1029fc150>

BoundWrapper.__call__() () {}
__wrapped__.__self__ <class '__main__.Class'>
Class.function_cm_inner() <class '__main__.Class'>

Traceback (most recent call last):
  File "cmgettest.py", line 40, in <module>
    Class.function_cm_outer()
TypeError: 'Wrapper' object is not callable

IOW, everything is fine when the decorator is applied around the classmethod, but when it is placed inside of the classmethod, a failure occurs because the decorator object is not callable.

One could argue that the error is easily avoided by adding a __call__() method to the Wrapper class, but that defeats the purpose of what is trying to be achieved in using this pattern. That is that one can within the bound wrapper after binding occurs, determine from the __self__ of the bound function, the fact that it was a class method. This can be inferred from the fact that __self__ is a class type.

If the classmethod decorator tp_descr_get implementation is changed so as to properly apply the descriptor binding protocol to the wrapped object, then what is being described is possible.

Having it honour the descriptor binding protocol also seems to make application of the Python object model more consistent.

A patch is attached which does exactly this.

The result for the above test after the patch is applied is:

BoundWrapper.__call__() () {}
__wrapped__.__self__ <__main__.Class object at 0x10ad237d0>
Class.function_im() <__main__.Class object at 0x10ad237d0>

BoundWrapper.__call__() () {}
__wrapped__.__self__ <class '__main__.Class'>
Class.function_cm_inner() <class '__main__.Class'>

BoundWrapper.__call__() () {}
__wrapped__.__self__ <class '__main__.Class'>
Class.function_cm_outer() <class '__main__.Class'>

That is, the decorator whether it is inside or outside now sees things in the same way.

If one also tests for calling of the classmethod via the instance:

print()
c.function_cm_inner()
print()
c.function_cm_outer()

Everything again also works out how want it:

BoundWrapper.__call__() () {}
__wrapped__.__self__ <class '__main__.Class'>
Class.function_cm_inner() <class '__main__.Class'>

BoundWrapper.__call__() () {}
__wrapped__.__self__ <class '__main__.Class'>
Class.function_cm_outer() <class '__main__.Class'>

FWIW, the shortcoming of classmethod not applying the descriptor binding protocol to the wrapped object, was found in writing a new object proxy and decorator library called 'wrapt'. This issue in the classmethod implementation is the one thing that has prevented wrapt having a system of writing decorators that can magically work out the context it is used in all the time. Would be nice to see it fixed. :-)

The wrapt library can be found at:

https://github.com/GrahamDumpleton/wrapt
http://wrapt.readthedocs.org

The limitation in the classmethod implementation is noted in the wrapt documentation at:

http://wrapt.readthedocs.org/en/v1.1.2/issues.html#classmethod-get
msg198672 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-09-30 03:27
I don't think it was ever intended that decorators be chained together.  

The whole point is to control binding behavior during dotted look-up (when __getattribute__ is called) and not in other circumstances (such as a direct lookup in a class dictionary).

Note that classmethods typically wrap regular functions which have both __call__ and __get__ methods.   The classmethod object intentionally invokes the former instead of the latter which would unhelpfully create an inner bound or unbound method.
msg198673 - (view) Author: Graham Dumpleton (grahamd) Date: 2013-09-30 03:43
The classmethod __get__() method does:

static PyObject *
cm_descr_get(PyObject *self, PyObject *obj, PyObject *type)
{
    classmethod *cm = (classmethod *)self;

    if (cm->cm_callable == NULL) {
        PyErr_SetString(PyExc_RuntimeError,
                        "uninitialized classmethod object");
        return NULL;
    }
    if (type == NULL)
        type = (PyObject *)(Py_TYPE(obj));
    return PyMethod_New(cm->cm_callable,
                        type, (PyObject *)(Py_TYPE(type)));
}

So it isn't intentionally calling __call__(). If it still doing binding, but doing it by calling PyMethod_New() rather than using __get__() on the wrapped function. Where it wraps a regular function the result is same as if __get__() was called as __get__() for a regular function internally calls PyMethod_New() in the same way.

static PyObject *
func_descr_get(PyObject *func, PyObject *obj, PyObject *type)
{
    if (obj == Py_None)
        obj = NULL;
    return PyMethod_New(func, obj, type);
}

By not using __get__(), you deny the ability to have chained decorators that want/need the knowledge of the fact that binding was being done. The result for stacking multiple decorators which use regular functions (closures) is exactly the same, but you open up other possibilities of smarter decorators.
msg198679 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-09-30 06:11
I'll take a look at this in more detail in the next week or so.
msg198682 - (view) Author: Graham Dumpleton (grahamd) Date: 2013-09-30 07:17
If you have the time, would be great if you can have a quick look at my wrapt package. That will give you an idea of where I am coming from in suggesting this change.

http://wrapt.readthedocs.org/en/latest/
http://wrapt.readthedocs.org/en/latest/issues.html
http://wrapt.readthedocs.org/en/latest/decorators.html
http://wrapt.readthedocs.org/en/latest/examples.html

In short, aiming to be able to write decorators which are properly transparent and aware of the context they are used in, so we don't have this silly situation at the moment where it is necessary to write distinct decorators for regular functions and instance methods. A classmethod around another decorator was the one place things will not work as would like to see them work.

I even did a talk about writing better decorators at PyCon NZ. Slides with notes at:

http://lanyrd.com/2013/kiwipycon/scpkbk/

Thanks.
msg201512 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-10-28 05:38
Antoine, do you have any thoughts on this proposal?
msg201523 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-28 10:50
Well... I've not written enough descriptor-implementing code to have a clear opinion on this, but this looks quite obscure. I have personally never needed anything like the wrapt library (I've also never used the PyPI "decorator" module, FWIW).
msg201572 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-10-28 19:25
@grahamd: I occasionally have felt the pain of wrapping @classmethod (or @staticmethod). Never enough though to think of how to fix it. I really don't have the stomach to review your wrapt library, but your code looks okay except for style and missing tests. I'd also recommend adding a few words to the docs. (And yes, all of this is your responsibility -- nobody has time to do all that stuff for you.)

Style-wise:
- the continuation line in your patch is not properly formatted;
- either the else block should also use { } or the else clause should be omitted.
msg201713 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-10-30 06:31
Graham, do we have a contributor agreement from you?
msg201714 - (view) Author: Graham Dumpleton (grahamd) Date: 2013-10-30 06:36
I don't believe so.
msg264911 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-05-05 17:51
Here is an updated patch with a test (adapted from msg198274.)
msg264916 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-05 19:28
With the patch class properties work:

>>> class A:
...     @classmethod
...     @property
...     def __doc__(cls):
...         return 'A doc for %r' % cls.__name__
... 
>>> A.__doc__
"A doc for 'A'"

This is worth to be explicitly documented.
msg321217 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-07 12:29
Berker, do you mind to create a PR?

Supporting class properties looks good rationale to me. But we need to check how this change affects performance.
msg321222 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-07-07 16:11
> Berker, do you mind to create a PR?

I will submit a PR tomorrow.

Do you have specific ideas for a micro-benchmark in mind or do you want to me just run the Python benchmark suite against the patch?
msg321227 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-07 18:03
I think it is impossible to get significant impact on the Python benchmark suite from this patch. But mickrobenchmarks can expose the regression if it exists. Something like:

    ./python -m perf timeit -s 'class A:' -s ' @classmethod' -s ' def cm(cls): pass' -- 'A.cm()'
    ./python -m perf timeit -s 'class A:' -s ' @classmethod' -s ' def cm(cls): pass' -s 'f = A.cm' -- 'f()'
msg350404 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-08-24 22:37
New changeset 805f8f9afea116c5d4d000570e3d02ae84502f43 by Raymond Hettinger (Berker Peksag) in branch 'master':
bpo-19072: Make @classmethod support chained decorators (GH-8405)
https://github.com/python/cpython/commit/805f8f9afea116c5d4d000570e3d02ae84502f43
msg379509 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-10-24 01:37
New changeset 8e5b0fdce337ef0a1f4f38b31a8c6b66c56b16d2 by Raymond Hettinger in branch 'master':
bpo-19072: Update descriptor howto for decorator chaining (GH-22934)
https://github.com/python/cpython/commit/8e5b0fdce337ef0a1f4f38b31a8c6b66c56b16d2
msg379510 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-10-24 02:01
New changeset c17f63fae57dc02e78bd6931b8fb6c6c2f9d4d81 by Miss Skeleton (bot) in branch '3.9':
bpo-19072: Update descriptor howto for decorator chaining (GH-22934) (GH-22935)
https://github.com/python/cpython/commit/c17f63fae57dc02e78bd6931b8fb6c6c2f9d4d81
msg384405 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2021-01-05 15:13
I've created a followup issue re documentation of this change: issue42832
msg397547 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-07-15 13:16
New changeset b83861f0265e07207a6ae2c49c40fa8f447893f2 by Łukasz Langa in branch 'main':
bpo-42073: allow classmethod to wrap other classmethod-like descriptors (#27115)
https://github.com/python/cpython/commit/b83861f0265e07207a6ae2c49c40fa8f447893f2
msg397551 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-07-15 13:42
New changeset 2ce8af3cbcb368a35a05a5a9f97a09405124f239 by Miss Islington (bot) in branch '3.10':
bpo-42073: allow classmethod to wrap other classmethod-like descriptors (GH-27115) (GH-27162)
https://github.com/python/cpython/commit/2ce8af3cbcb368a35a05a5a9f97a09405124f239
msg406607 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-11-19 18:43
New changeset e34809e1c2a09478f4e0651d551c9c12d3c556ab by Raymond Hettinger in branch 'main':
bpo-19072: Classmethod can wrap other classmethod like descriptors (GH-29634)
https://github.com/python/cpython/commit/e34809e1c2a09478f4e0651d551c9c12d3c556ab
msg406617 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-11-19 19:12
New changeset bbe3c57c865439f2194eb760a4362b5506d221a7 by Miss Islington (bot) in branch '3.10':
bpo-19072: Classmethod can wrap other classmethod like descriptors (GH-29634) (GH-29643)
https://github.com/python/cpython/commit/bbe3c57c865439f2194eb760a4362b5506d221a7
History
Date User Action Args
2021-11-19 19:12:24lukasz.langasetmessages: + msg406617
2021-11-19 18:44:20miss-islingtonsetpull_requests: + pull_request27875
2021-11-19 18:43:59lukasz.langasetmessages: + msg406607
2021-11-19 06:36:28rhettingersetpull_requests: + pull_request27866
2021-07-15 13:42:15lukasz.langasetmessages: + msg397551
2021-07-15 13:19:06miss-islingtonsetpull_requests: + pull_request25699
2021-07-15 13:16:28lukasz.langasetmessages: + msg397547
2021-07-13 15:45:30lukasz.langasetnosy: + lukasz.langa

pull_requests: + pull_request25663
2021-01-05 15:13:15iritkatrielsetnosy: + iritkatriel
messages: + msg384405
2020-10-24 02:01:42rhettingersetmessages: + msg379510
2020-10-24 01:37:43miss-islingtonsetnosy: + miss-islington

pull_requests: + pull_request21855
2020-10-24 01:37:32rhettingersetmessages: + msg379509
2020-10-24 00:59:17rhettingersetpull_requests: + pull_request21854
2020-10-18 22:25:59eriknwsetnosy: + eriknw

pull_requests: + pull_request21721
2019-08-24 22:37:58rhettingersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-08-24 22:37:27rhettingersetmessages: + msg350404
2018-07-23 10:30:37berker.peksagsetpull_requests: + pull_request7931
2018-07-07 18:03:59serhiy.storchakasetmessages: + msg321227
2018-07-07 16:11:43berker.peksagsetmessages: + msg321222
2018-07-07 12:29:50serhiy.storchakasetversions: + Python 3.8, - Python 3.6
2018-07-07 12:29:41serhiy.storchakasetmessages: + msg321217
2016-05-05 19:28:39serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg264916
2016-05-05 17:51:59berker.peksagsetfiles: + issue19072.diff
versions: + Python 3.6, - Python 3.4
nosy: + berker.peksag

messages: + msg264911

stage: patch review
2016-01-13 10:38:41ionelmcsetnosy: + ionelmc
2013-10-30 06:36:21grahamdsetmessages: + msg201714
2013-10-30 06:31:48rhettingersetmessages: + msg201713
2013-10-28 19:25:40gvanrossumsetmessages: + msg201572
2013-10-28 10:50:08pitrousetnosy: + gvanrossum
messages: + msg201523
2013-10-28 05:38:54rhettingersetnosy: + pitrou
messages: + msg201512
2013-09-30 07:17:57grahamdsetmessages: + msg198682
2013-09-30 06:11:37rhettingersetmessages: + msg198679
2013-09-30 03:43:31grahamdsetmessages: + msg198673
2013-09-30 03:27:04rhettingersetmessages: + msg198672
2013-09-30 03:19:32rhettingersetassignee: rhettinger

nosy: + rhettinger
2013-09-29 10:13:38daniel.urbansetnosy: + daniel.urban
2013-09-28 20:42:41terry.reedysetversions: + Python 3.4, - Python 3.3
2013-09-22 13:22:44grahamdcreate