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

Test identity first in membership operation of ItemsView, ValuesView and Sequence in collections.abc #71102

Closed
zhangyangyu opened this issue May 3, 2016 · 19 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@zhangyangyu
Copy link
Member

BPO 26915
Nosy @rhettinger, @serhiy-storchaka, @MojoVampire, @zhangyangyu
PRs
  • bpo-26915: Test identity first in membership operation #503
  • [3.6] bpo-26915: Test identity first in membership operation  #553
  • [Do Not Merge] Sample of CPython life with blurb. #703
  • Files
  • issue26915.patch
  • issue26915_v2.patch
  • issue26915_s2.patch
  • issue26915_v3.patch
  • issue26915_v4.patch
  • 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 2017-03-08.03:44:58.702>
    created_at = <Date 2016-05-03.06:19:16.847>
    labels = ['type-bug', 'library']
    title = 'Test identity first in membership operation of ItemsView, ValuesView and Sequence in collections.abc'
    updated_at = <Date 2017-03-24.22:43:22.115>
    user = 'https://github.com/zhangyangyu'

    bugs.python.org fields:

    activity = <Date 2017-03-24.22:43:22.115>
    actor = 'xiang.zhang'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-03-08.03:44:58.702>
    closer = 'xiang.zhang'
    components = ['Library (Lib)']
    creation = <Date 2016-05-03.06:19:16.847>
    creator = 'xiang.zhang'
    dependencies = []
    files = ['42687', '42688', '42734', '43189', '43190']
    hgrepos = []
    issue_num = 26915
    keywords = ['patch']
    message_count = 19.0
    messages = ['264688', '264689', '264764', '264773', '264891', '264892', '264896', '264898', '264900', '264902', '266151', '267106', '267124', '267247', '267248', '269877', '271151', '290275', '290277']
    nosy_count = 6.0
    nosy_names = ['rhettinger', 'stutzbach', 'python-dev', 'serhiy.storchaka', 'josh.r', 'xiang.zhang']
    pr_nums = ['503', '553', '703']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26915'
    versions = ['Python 3.6']

    @zhangyangyu
    Copy link
    Member Author

    __contains__ operation in ItemsView, ValuesView and Sequence in collections.abc simply test equality with ==. This introduces inconsistent behaviour with built-in containers when encountering objects like NaN, which does not equal to itself. I asked something about this on core-mentorship, see https://mail.python.org/mailman/private/core-mentorship/2016-April/003543.html.

    @zhangyangyu zhangyangyu added the stdlib Python modules in the Lib dir label May 3, 2016
    @8977102d-e623-4805-b309-cd0ad35b0d72 SilentGhost mannequin added the type-bug An unexpected behavior, bug, or error label May 3, 2016
    @zhangyangyu
    Copy link
    Member Author

    Add issue number to test method name.

    @99ffcaa5-b43b-4e8e-a35e-9c890007b9cd
    Copy link
    Mannequin

    MojoVampire mannequin commented May 3, 2016

    At some point someone really needs to decide if the C layer behavior of performing an identity test before full equality checking is something that should be emulated at the Python layer or not. The current state seems ridiculous, where C containers check identity first simply by using the easier RichCompareBool function, while Python containers have to have the identity-then-equality check rewritten explicitly, which feels like a DRY violation.

    Makes it harder for non-CPython implementations too, since they end up either not matching CPython behavior, or writing extra code to match the CPython quirks.

    I have nothing against this patch, but between PyObject_RichCompareBool and the various slightly strange behaviors in the argument parsing format codes (which leads to silly workarounds like _check_int_field in bpo-20858), I feel like the Python code base is getting cluttered with hacks to emulate the hacky C layer.

    @zhangyangyu
    Copy link
    Member Author

    I agree with you josh. Actually that's what I want to know, consistency. But I don't mention it in my post, so Guido only gives what to do in this case. In this thread, it means does Python code have to keep the invariant mentioned in msg75735?

    @1762cc99-3127-4a62-9baf-30c3d0f51ef7
    Copy link
    Mannequin

    python-dev mannequin commented May 5, 2016

    New changeset 1c6cf4010df3 by Raymond Hettinger in branch 'default':
    bpo-26915: Add identity checks to the collections ABC __contains__ methods.
    https://hg.python.org/cpython/rev/1c6cf4010df3

    @rhettinger
    Copy link
    Contributor

    Xiang, hanks for the patch and for the link to Guido's opinion.

    Josh, I concur with your sentiments. In this case the burden is light and is likely to prevent future difficulties by keeping the container invariants intact.

    @serhiy-storchaka
    Copy link
    Member

    Is it worth to add a function in the operator module that tests arguments for identity or equality?

    @zhangyangyu
    Copy link
    Member Author

    In my opinion it's not worth. If there is such an operator in stdlib, I would expect more, exposing PyObject_RichCompareBool in Python level, providing a new operator like ===.

    @serhiy-storchaka
    Copy link
    Member

    What about Sequence.index and Sequence.count?

    @zhangyangyu
    Copy link
    Member Author

    Oops, I forgot about them. I think they should. Raymond mentioned count in msg75735. Attach patch to take them in.

    @zhangyangyu
    Copy link
    Member Author

    Reopen this since the solution is not complete pointed out by Serhiy and the new patch has already been attached(s2).

    @zhangyangyu zhangyangyu reopened this May 23, 2016
    @rhettinger rhettinger removed their assignment May 24, 2016
    @zhangyangyu
    Copy link
    Member Author

    Serhiy, actually the patch(issue26915_s2.patch) I uploaded early has tried to make Sequence.index and Sequence.count consistent. Maybe the issue stage is not right.

    @serhiy-storchaka
    Copy link
    Member

    Oh, sorry, I was absentminded. In general the patch LGTM. But I think that using list subclass for tests is not a good idea. If we make a typo in CustomSequence method name (contain, inedx, coumt), the test is still passed. It would be better to use a class that doesn't have any sequence-related methods besides explicitly defined.

    @zhangyangyu
    Copy link
    Member Author

    Thanks for review Serhiy and nice thoughts. I make the class inherited directly from Sequence. So if the methods in Sequence are OK, the test class is OK.

    @zhangyangyu
    Copy link
    Member Author

    Oh, sorry. Make the __len__ signature wrong. Change it.

    @zhangyangyu
    Copy link
    Member Author

    ping

    @zhangyangyu
    Copy link
    Member Author

    Ping again.

    @zhangyangyu
    Copy link
    Member Author

    New changeset 78ad039 by Xiang Zhang in branch '3.6':
    bpo-26915: Test identity first in index() and count() of collections.abc.Sequence (GH-553)
    78ad039

    @zhangyangyu
    Copy link
    Member Author

    New changeset d5d3249 by Xiang Zhang in branch 'master':
    bpo-26915: Test identity first in membership operation in index() and count() methods of collections.abc.Sequence (GH-503)
    d5d3249

    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants