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

Contradiction in definition of "data descriptor" between (dotted lookup behavior/datamodel documentation) and (inspect lib/descriptor how-to) #70291

Closed
aaronchall mannequin opened this issue Jan 13, 2016 · 13 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes docs Documentation in the Doc dir stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@aaronchall
Copy link
Mannequin

aaronchall mannequin commented Jan 13, 2016

BPO 26103
Nosy @rhettinger, @serhiy-storchaka, @1st1, @aaronchall, @Mariatta, @csabella, @chnlior
PRs
  • bpo-26103 resolve Data descriptor contradiction #1959
  • bpo-26103: Update the description of isdatadescriptor in inspect.rst #16645
  • [3.8] bpo-26103: Fix the description of isdatadescriptor in inspect.rst (GH-16645) #17180
  • 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 2019-03-20.20:16:37.395>
    created_at = <Date 2016-01-13.20:56:52.079>
    labels = ['3.7', '3.8', 'type-bug', 'library', 'docs']
    title = 'Contradiction in definition of "data descriptor" between (dotted lookup behavior/datamodel documentation) and (inspect lib/descriptor how-to)'
    updated_at = <Date 2019-11-15.21:49:05.856>
    user = 'https://github.com/aaronchall'

    bugs.python.org fields:

    activity = <Date 2019-11-15.21:49:05.856>
    actor = 'miss-islington'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-03-20.20:16:37.395>
    closer = 'Aaron Hall'
    components = ['Documentation', 'Library (Lib)']
    creation = <Date 2016-01-13.20:56:52.079>
    creator = 'Aaron Hall'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 26103
    keywords = []
    message_count = 12.0
    messages = ['258168', '295212', '295243', '295287', '295288', '295307', '295378', '301206', '302721', '308078', '317211', '338426']
    nosy_count = 8.0
    nosy_names = ['rhettinger', 'docs@python', 'serhiy.storchaka', 'yselivanov', 'Aaron Hall', 'Mariatta', 'cheryl.sabella', 'chnlior']
    pr_nums = ['1959', '16645', '17180']
    priority = 'normal'
    resolution = None
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26103'
    versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']

    @aaronchall
    Copy link
    Mannequin Author

    aaronchall mannequin commented Jan 13, 2016

    Based on the data-model documentation (https://docs.python.org/2/reference/datamodel.html#invoking-descriptors) and the dotted lookup behavior, the follow definitions are correct:

    "If the descriptor defines __set__() and/or __delete__(), it is a data descriptor; if it defines neither, it is a non-data descriptor."

    def has_data_descriptor_attrs(obj):
        return set(['__set__', '__delete__']) & set(dir(obj))
    
    def is_data_descriptor(obj):
        return bool(has_data_descriptor_attrs(obj))

    However, the inspect module has the following, which is also reflected in the descriptor how-to (https://docs.python.org/2/howto/descriptor.html#descriptor-protocol):

    "If an object defines both __get__() and __set__(), it is considered a data descriptor."

    def isdatadescriptor(object):
        """Return true if the object is a data descriptor.
    Data descriptors have both a __get__ and a __set__ attribute..."""
    if isclass(object) or ismethod(object) or isfunction(object):
        # mutual exclusion
        return False
    tp = type(object)
    return hasattr(tp, "__set__") and hasattr(tp, "__get__")
    

    I'm willing to sign a contributor release and fix myself.

    @aaronchall aaronchall mannequin added the type-bug An unexpected behavior, bug, or error label Jan 13, 2016
    @SilentGhost SilentGhost mannequin added docs Documentation in the Doc dir stdlib Python modules in the Lib dir labels Jan 14, 2016
    @SilentGhost SilentGhost mannequin assigned docspython Jan 14, 2016
    @aaronchall
    Copy link
    Mannequin Author

    aaronchall mannequin commented Jun 5, 2017

    Bumping this - I intend to work on this next, if no objections.

    @serhiy-storchaka
    Copy link
    Member

    This isn't just a documentation issue since it fixes inspect.isdatadescriptor(). I confirm that the new implementation better matches the C code. LGTM, but needed tests for inspect.isdatadescriptor() and a Misc/NEWS entry.

    @aaronchall
    Copy link
    Mannequin Author

    aaronchall mannequin commented Jun 6, 2017

    Added news, working on tests

    @Mariatta
    Copy link
    Sponsor Member

    Mariatta commented Jun 6, 2017

    Please also add yourself to Misc/ACKS.

    @aaronchall
    Copy link
    Mannequin Author

    aaronchall mannequin commented Jun 6, 2017

    Please also add yourself to Misc/ACKS.

    Done!

    @aaronchall
    Copy link
    Mannequin Author

    aaronchall mannequin commented Jun 8, 2017

    I tweaked the docs a little more this morning, but I believe I am done making any further changes unless so requested.

    This issue doesn't say it's assigned to anyone. Is there anything else that needs to happen here?

    @aaronchall
    Copy link
    Mannequin Author

    aaronchall mannequin commented Sep 4, 2017

    Serhiy,

    Not sure what else needs to be done to wrap this up. All checks are passing on the pull request.

    Thoughts?

    @rhettinger rhettinger self-assigned this Sep 4, 2017
    @rhettinger rhettinger removed their assignment Sep 12, 2017
    @serhiy-storchaka
    Copy link
    Member

    The only question is remained -- should *data descriptors* be *descriptors*? I.e. is the __get__ method required for data descriptors?

    @chnlior
    Copy link
    Mannequin

    chnlior mannequin commented Dec 11, 2017

    Joining @serhiy Storchaka last question.
    Is the __get__ method existance is a must be a data descriptor?

    According to the C implementation in descrobject.h

    #define PyDescr_IsData(d) (Py_TYPE(d)->tp_descr_set != NULL)
    #endif
    

    the answer is No.
    Does this C code reflect the true definition?

    @serhiy-storchaka serhiy-storchaka added the 3.8 only security fixes label May 14, 2018
    @serhiy-storchaka
    Copy link
    Member

    New changeset 4054b17 by Serhiy Storchaka (Aaron Hall, MBA) in branch 'master':
    bpo-26103: Fix inspect.isdatadescriptor() and a data descriptor definition. (GH-1959)
    4054b17

    @csabella
    Copy link
    Contributor

    It looks like this issue can be closed now that it's merged?

    @aaronchall aaronchall mannequin closed this as completed Mar 20, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @carljm
    Copy link
    Contributor

    carljm commented Apr 6, 2023

    I suspect that this issue was resolved in the wrong direction. It is meaningless and not useful to define something as a "data descriptor" that does not have a __get__ method, since the only implication of something being a "data descriptor" is that its __get__ method takes precedence over the instance dict. But obviously this "taking precedence" cannot occur when the descriptor has no __get__ method. So I think it is more useful to clarify that data descriptors must have both __get__ and either or both of __set__ and __delete__.

    The reason PyDescr_IsData does not explicitly check for tp_descr_get is that, in every case where it's used, the accompanying step (either immediately before or after the PyDescr_IsData check) is to actually fetch tp_descr_get from the descriptor; if it is NULL, then attribute-getting always proceeds exactly as if the descriptor were not a data descriptor. (In fact, in 3/4 call sites the check is literally (descr_get != NULL && PyDescr_IsData(descr).) So also checking existence of tp_descr_get within PyDescr_IsData would be redundant in every place it is used. But the end-to-end behavior in all these cases is that descriptors without tp_descr_get behave exactly the same as non-data descriptors. I don't think we want PyDescr_IsData to do the redundant check (these are hot code paths; though tbh compilers might be smart enough to eliminate it anyway), maybe it should be renamed for better clarity?

    I'm not sure whether this is important enough to file a new issue and fix. The main case where I can see it might cause confusion is that e.g. if inspect.getattr_static hewed blindly to the currently-documented definition of what makes a data descriptor, it would wrongly return the descriptor object instead of the instance dict value for descriptors without __get__. (Fortunately it doesn't follow the documented definition, but also checks for __get__.) This came up in #75367.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes docs Documentation in the Doc dir stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants