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.

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

classmethod does not pass "type/owner" when invoking wrapped __get__ #86239

Closed
eriknw mannequin opened this issue Oct 18, 2020 · 4 comments
Closed

classmethod does not pass "type/owner" when invoking wrapped __get__ #86239

eriknw mannequin opened this issue Oct 18, 2020 · 4 comments
Labels
3.10 only security fixes 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@eriknw
Copy link
Mannequin

eriknw mannequin commented Oct 18, 2020

BPO 42073
Nosy @rhettinger, @ambv, @berkerpeksag, @serhiy-storchaka, @eriknw, @miss-islington
PRs
  • [3.9] bpo-42073: allow classmethod to wrap other classmethod-like descriptors. #22757
  • bpo-42073: allow classmethod to wrap other classmethod-like descriptors #27115
  • [3.10] bpo-42073: allow classmethod to wrap other classmethod-like descriptors (GH-27115) #27162
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-07-15.10:53:50.542>
    created_at = <Date 2020-10-18.20:22:44.119>
    labels = ['interpreter-core', 'type-bug', '3.10', '3.11']
    title = 'classmethod does not pass "type/owner" when invoking wrapped __get__'
    updated_at = <Date 2021-07-15.13:42:15.372>
    user = 'https://github.com/eriknw'

    bugs.python.org fields:

    activity = <Date 2021-07-15.13:42:15.372>
    actor = 'lukasz.langa'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-07-15.10:53:50.542>
    closer = 'lukasz.langa'
    components = ['Interpreter Core']
    creation = <Date 2020-10-18.20:22:44.119>
    creator = 'eriknw'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42073
    keywords = ['patch']
    message_count = 4.0
    messages = ['378893', '397538', '397548', '397552']
    nosy_count = 6.0
    nosy_names = ['rhettinger', 'lukasz.langa', 'berker.peksag', 'serhiy.storchaka', 'eriknw', 'miss-islington']
    pr_nums = ['22757', '27115', '27162']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue42073'
    versions = ['Python 3.10', 'Python 3.11']

    @eriknw
    Copy link
    Mannequin Author

    eriknw mannequin commented Oct 18, 2020

    The following is new to Python 3.9, and I consider the implementation incomplete. I have code that works for Python 3.8 and before, but not for Python 3.9:

    "Class methods can now wrap other :term:`descriptors <descriptor>` such as :func:`property`."

    #8405
    https://bugs.python.org/issue19072

    As implemented, classmethod does not correctly wrap descriptors that mimic classmethod. Previously, __get__of wrapped objects wasn't invoked by classmethod, so it was safe to have an object with both call and get behave like a classmethod. Now, classmethod calling get first gives incorrect results.

    Here is a minimal example:

    from types import MethodType
    
    
    class myclassmethod:
        def __init__(self, func):
            self.func = func
    
        def __call__(self, cls):
            return self.func(cls)
    
        def __get__(self, instance, owner=None):
            if owner is None:
                owner = type(instance)
            return MethodType(self, owner)
    
    
    class A:
        @myclassmethod
        def f1(cls):
            return cls
    
        @classmethod
        @myclassmethod
        def f2(cls):
            return cls
    
    
    assert A.f1() is A
    assert A.f2() is A  # <-- fails in 3.9, works in 3.8 and before
    

    This pattern would typically be used to do something extra in __call__.

    For the sake of discussion, let's call the two arguments to __get__ "instance" and "owner". Typically, "instance" is an instance of "owner", or, equivalently, "owner" is the type of "instance". If "owner" is None, it is generally assumed to be the type of "instance".

    In bpo-19072 (and gh8405), classmethod was changed to call obj.__get__(owner) if the wrapped object "obj" has get. Notice that only the "instance" argument is provided. Moreover, the type owner is passed as the "instance" argument. This means that the "owner" argument (which is None) will be assumed to be the type of the "instance" argument, which is the type of the owner type. This is wrong. The "owner" argument should be owner.

    I believe it would be better for classmethod to call obj.__get__(owner, owner) if "obj" has get.

    This is kind of difficult to explain. I will make a PR with more informative tests shortly. Here is the simple diff to make the above example pass:

    diff --git a/Objects/funcobject.c b/Objects/funcobject.c
    index bd24f67b97..74f9167566 100644
    --- a/Objects/funcobject.c
    +++ b/Objects/funcobject.c
    @@ -739,7 +739,7 @@ cm_descr_get(PyObject *self, PyObject *obj, PyObject *type)
             type = (PyObject *)(Py_TYPE(obj));
         if (Py_TYPE(cm->cm_callable)->tp_descr_get != NULL) {
             return Py_TYPE(cm->cm_callable)->tp_descr_get(cm->cm_callable, type,
    -                                                      NULL);
    +                                                      type);
         }
         return PyMethod_New(cm->cm_callable, type);
     }
    
    

    Since I consider the new behavior to have introduced a regression, I think this change should be applied to both 3.9 and 3.10.

    Cheers!

    @eriknw eriknw mannequin added 3.9 only security fixes 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Oct 18, 2020
    @ambv
    Copy link
    Contributor

    ambv commented Jul 15, 2021

    Thanks! ✨ 🍰 ✨

    @ambv ambv closed this as completed Jul 15, 2021
    @ambv
    Copy link
    Contributor

    ambv commented Jul 15, 2021

    New changeset b83861f by Łukasz Langa in branch 'main':
    bpo-42073: allow classmethod to wrap other classmethod-like descriptors (bpo-27115)
    b83861f

    @ambv ambv added 3.11 only security fixes and removed 3.9 only security fixes labels Jul 15, 2021
    @ambv
    Copy link
    Contributor

    ambv commented Jul 15, 2021

    New changeset 2ce8af3 by Miss Islington (bot) in branch '3.10':
    bpo-42073: allow classmethod to wrap other classmethod-like descriptors (GH-27115) (GH-27162)
    2ce8af3

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant