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

Not all method descriptors are callable #64508

Closed
larryhastings opened this issue Jan 20, 2014 · 16 comments
Closed

Not all method descriptors are callable #64508

larryhastings opened this issue Jan 20, 2014 · 16 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@larryhastings
Copy link
Contributor

BPO 20309
Nosy @warsaw, @arigo, @rhettinger, @ncoghlan, @pitrou, @vstinner, @larryhastings, @bitdancer, @ericsnowcurrently, @jdemeyer, @eryksun, @sruggier
Files
  • curious.py
  • descr_v1.diff: In progress version of the patch (not finished!)
  • descr_v2.diff
  • descr_v3.diff
  • example_use_case.py: Example use case for callable static methods.
  • 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 = 'https://github.com/rhettinger'
    closed_at = <Date 2015-04-14.14:53:36.344>
    created_at = <Date 2014-01-20.06:43:37.890>
    labels = ['interpreter-core', 'type-bug', 'invalid']
    title = 'Not all method descriptors are callable'
    updated_at = <Date 2021-03-31.14:45:13.131>
    user = 'https://github.com/larryhastings'

    bugs.python.org fields:

    activity = <Date 2021-03-31.14:45:13.131>
    actor = 'vstinner'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2015-04-14.14:53:36.344>
    closer = 'ncoghlan'
    components = ['Interpreter Core']
    creation = <Date 2014-01-20.06:43:37.890>
    creator = 'larry'
    dependencies = []
    files = ['33559', '34928', '34959', '38942', '47940']
    hgrepos = []
    issue_num = 20309
    keywords = ['patch']
    message_count = 16.0
    messages = ['208524', '208525', '208531', '208537', '208538', '208544', '216593', '216676', '216769', '240686', '240843', '240894', '240898', '330227', '338473', '389908']
    nosy_count = 15.0
    nosy_names = ['barry', 'arigo', 'rhettinger', 'ncoghlan', 'pitrou', 'vstinner', 'chrish42', 'larry', 'Arfrever', 'r.david.murray', 'eric.snow', 'jdemeyer', 'eryksun', 'ozialien', 'sruggier']
    pr_nums = []
    priority = 'low'
    resolution = 'not a bug'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue20309'
    versions = ['Python 3.5']

    @larryhastings
    Copy link
    Contributor Author

    I found something curious while experimenting with types tonight.

    Everybody knows, if you access a descriptor in a class through the normal attribute-getting methods, you actually get the result of calling its '__get__' method. If you want to get your hands on the descriptor itself, you can be sneaky and pull it out through the class's __dict__.

    I did an experiment tonight, accessing a bunch of methods in a bunch of different ways.

    The methods I tried were

    • an instance method,
    • a class method, and
    • a static method.

    I accessed them

    • as an instance attribute,
    • as a class attribute, and
    • through the class __dict__.

    And for each of the above, I tried

    • one user method and
    • one built-in method.

    That's a total of eighteen methods tried (3x3x2).

    For each one, I printed out whether or not it was callable. I found the results startling: fifteen were, but three (!) were not.

    All three that weren't were accessed through the class dict. They were:

    • Class method, from builtin class ( dict.__dict__['fromkeys'] )
    • Static method, from user class
    • Static method, from builtin class ( str.__dict__['maketrans'] )

    I find it strange that the static method descriptors aren't callable.
    I find it *even stranger* that *one* of the class method descriptors isn't callable, but the other one is.

    I guess a foolish consistency is the hobgoblin of my small mind, but... shouldn't all eighteen of these objects be callable?

    Attached is the test harness I wrote.

    (p.s. I attached you guys as nosy because I thought you might be interested.)

    @larryhastings larryhastings added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Jan 20, 2014
    @larryhastings
    Copy link
    Contributor Author

    I should add, I see the same results with current trunk and with my handy Python 3 (currently 3.3.1rc1, huh guess I'm behind).

    @larryhastings larryhastings changed the title Class method descriptors are different between builtin and user classes Not all descriptors are callable Jan 20, 2014
    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Jan 20, 2014

    Instances of 'staticmethod' or 'classmethod' are not callable. I'm unsure why not, but it's good enough, as you need to go through various unexpected hops in order to try to call one.

    'dict.fromkeys' is not a classmethod, as seen by "dict.__dict__['fromkeys']". It's an internal object usually never seen by Python code. I believe this is only because implementing a regular classmethod in C would be rather messy.

    @ncoghlan
    Copy link
    Contributor

    I believe it's just a matter of pattern of use - applying staticmethod
    outside a class (or retrieving the descriptor directly from the dict,
    bypassing the descriptor protocol), so nobody every noticed.

    Ditto for the C wrapper vs the Python wrapper for a classmethod - it's
    really rare to access those without going through the descriptor
    machinery, so it's likely just an accident of implementation that one
    of them isn't callable.

    I don't see any specific reason for them to be non-callable - I
    suspect it's just a case of never adding the code to make it happen.

    @ncoghlan ncoghlan changed the title Not all descriptors are callable Class method descriptors are different between builtin and user classes Jan 20, 2014
    @ncoghlan
    Copy link
    Contributor

    Oh, so that's how that happens :)

    (the title reverted because I replied via email from a part of the thread before Larry changed the issue name)

    @ncoghlan ncoghlan changed the title Class method descriptors are different between builtin and user classes Not all method descriptors are callable Jan 20, 2014
    @bitdancer
    Copy link
    Member

    See also bpo-19072 for previous report of the classmethod problem.

    @chrish42
    Copy link
    Mannequin

    chrish42 mannequin commented Apr 16, 2014

    Work in progress for fixing this bug. (See descr_v1.diff) I converted the "curious.py" file into additional testcases. I started writing the functions for the tp_call slots for class and static methods.

    To do: add tests to make sure that the code works for more than what's accepted by function_call(), then switch to using PyObject_Call() (which is the right function to use here, thanks to ncoghlan for catching that). Will finish this in the next few days.

    @eryksun
    Copy link
    Contributor

    eryksun commented Apr 17, 2014

    classmethod_descriptor instances such as vars(dict)['fromkeys'] are callable. The first argument has to be a subclass of __objclass__:

        >>> vars(dict)['fromkeys'].__objclass__
        <class 'dict'>

    Calling the descriptor creates a bound built-in method; slices the args to remove the class; and calls it with the args and kwds.

        >>> vars(dict)['fromkeys'](dict, 'abc')
        {'a': None, 'b': None, 'c': None}

    source: classmethoddescr_call
    http://hg.python.org/cpython/file/04f714765c13/Objects/descrobject.c#l256

    While the classmethod and staticmethod types that are defined in funcobject.c aren't callable, they do expose a __func__ member.

    @rhettinger rhettinger self-assigned this Apr 17, 2014
    @chrish42
    Copy link
    Mannequin

    chrish42 mannequin commented Apr 18, 2014

    Here is the (first?) complete version of the patch. All tests pass. Note that I had to remove a test that was checking that the object returned from staticmethod was not callable.

    Let me know if I should add more tests, or if there are any other problems with the patch. Thanks!

    @chrish42
    Copy link
    Mannequin

    chrish42 mannequin commented Apr 13, 2015

    Updated patch to work with current state of repository. Tests still pass (including the new ones).

    @rhettinger
    Copy link
    Contributor

    I don't agree that this is a bug that should be fixed. It adds code that will likely never get called or needed (i.e. there has never been a request for this in the decade long history of desciptors and it seems like a made up requirement to me.

    I concur with Amrin's comment that this "is good enough" and Nick's comment that "it's just a matter of pattern of use'. I recommend that this be closed as "not-a-bug" and that we avoid gratuitous API expansion for what Larry states is likely "foolish consistency".

    If someone object to recommendation to close and really wants to push for this, I recommend making a business case for acceptance and then assigning this issue to Guido for a decision. This is his code and AFAICT he intentionally didn't go down a number of possible paths for descriptors simply because there weren't motivating use cases. To the extent that __call__ was omitted on some descriptors, I don't think it was an accident (most of this code was developed as the same time by the same person with deliberate differences between each of them). His write-ups for descriptors when they were introduced make no mention of an implied requirement for callability.

    FWIW, bpo-19072 isn't really related to this one. That issue is a feature request to support descriptor chainging and it isn't clear whether that should be supported or left as-in.

    @chrish42
    Copy link
    Mannequin

    chrish42 mannequin commented Apr 14, 2015

    As a newbie to the CPython source code (and as someone who started working on this bug because it was on the lists of easy bugs for PyCon 2014), I don't have a strong attachment either way, as long as some kind of decision is reached, and I can check this off my list.

    If forced to take a stance, I would probably agree that this might be reaching into "foolish consistency" territory, as I just don't see myself ever using the new possibilities that this added code would allow.

    If the decision is made to fix this, I'll improve the tests to actually call these new callables (and check that the result of the call is correct). I'll wait until the "we should fix this" decision is made to work on the patch again, though. But if this is closed as not-a-bug, I'll be a happy camper too (as I've learned some stuff about CPython internals in the process).

    @ncoghlan
    Copy link
    Contributor

    I agree with Raymond's recommendation - actually supporting this would mean adding code that would need to be maintained indefinitely without providing a compensating practical benefit, so I'm flagging this as "not a bug". Thanks Christian for nudging us to make a decision one way or the other.

    If another implementation requests clarification, we might want to document that "directly callable-or-not" for these descriptors is formally an interpreter implementation detail, but in the absence of such a request, I think this issue serves as an adequate reference.

    @sruggier
    Copy link
    Mannequin

    sruggier mannequin commented Nov 22, 2018

    I hit this problem today with what I'd consider a valid use case: I wanted to use a static method as a default argument to a function on the same class. Within the class definition context, automatic unwrapping of the staticmethod object doesn't occur, so the default value ends up not being callable unless the staticmethod object is manually, explicitly unwrapped via __func__.

    For those who prefer code to prose, I've attached example Python 2/3 code demonstrating what I mean.

    The workaround is pretty easy, but on the other hand, it also looks like it would also be pretty straightforward to facilitate this use case without forcing the author to learn about this distinction and manually unwrap the static method, and I think it makes sense to do that.

    @jdemeyer
    Copy link
    Contributor

    See also PEP-579 (issue 11) and the thread https://mail.python.org/pipermail/python-ideas/2018-June/051572.html

    @vstinner
    Copy link
    Member

    I proposed again a similar idea, but only for @staticmethod: bpo-43682.

    @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
    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

    7 participants