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

set_display evaluation order doesn't match documented behaviour #70208

Closed
HamishCampbell mannequin opened this issue Jan 6, 2016 · 17 comments
Closed

set_display evaluation order doesn't match documented behaviour #70208

HamishCampbell mannequin opened this issue Jan 6, 2016 · 17 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@HamishCampbell
Copy link
Mannequin

HamishCampbell mannequin commented Jan 6, 2016

BPO 26020
Nosy @rhettinger, @bitdancer, @serhiy-storchaka, @NeilGirdhar, @Vgr255
Files
  • build_set.diff: Rough first draft patch needs test and more thought
  • build_set2.diff: Add tests
  • 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 2016-09-08.22:32:50.933>
    created_at = <Date 2016-01-06.02:42:21.697>
    labels = ['interpreter-core', 'type-bug']
    title = "set_display evaluation order doesn't match documented behaviour"
    updated_at = <Date 2016-09-08.22:32:50.932>
    user = 'https://bugs.python.org/HamishCampbell'

    bugs.python.org fields:

    activity = <Date 2016-09-08.22:32:50.932>
    actor = 'rhettinger'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2016-09-08.22:32:50.933>
    closer = 'rhettinger'
    components = ['Interpreter Core']
    creation = <Date 2016-01-06.02:42:21.697>
    creator = 'Hamish Campbell'
    dependencies = []
    files = ['41514', '41518']
    hgrepos = []
    issue_num = 26020
    keywords = ['patch']
    message_count = 17.0
    messages = ['257579', '257580', '257582', '257584', '257585', '257590', '257691', '264307', '264308', '264309', '264312', '264314', '270111', '270132', '275178', '275186', '275187']
    nosy_count = 8.0
    nosy_names = ['rhettinger', 'r.david.murray', 'docs@python', 'python-dev', 'serhiy.storchaka', 'NeilGirdhar', 'abarry', 'Hamish Campbell']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'commit review'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26020'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @HamishCampbell
    Copy link
    Mannequin Author

    HamishCampbell mannequin commented Jan 6, 2016

    It looks like the behaviour of set displays do not match behaviour in some cases. The documentation states:

    "A set display yields a new mutable set object, the contents being specified by either a sequence of expressions or a comprehension. When a comma-separated list of expressions is supplied, its elements are evaluated from left to right and added to the set object. When a comprehension is supplied, the set is constructed from the elements resulting from the comprehension."

    Note the following:

       >>> foo = { True, 1 }
       >>> print(foo)
       {1}

    However, if we add elements 'left to right':

       >>> foo = set()
       >>> foo.add(True)
       >>> foo.add(1)
       >>> print(foo)
       {True}

    Note that similar documentation for dict displays produces the expected result.

    "If a comma-separated sequence of key/datum pairs is given, they are evaluated from left to right to define the entries of the dictionary: each key object is used as a key into the dictionary to store the corresponding datum. This means that you can specify the same key multiple times in the key/datum list, and the final dictionary’s value for that key will be the last one given."

       >>> foo = {}
       >>> foo[True] = 'bar'
       >>> foo[1] = 'baz'
       >>> print(foo)
       {True: 'baz'}

    Which matches the dict display construction:

       >>> foo = { True: 'bar', 1: 'baz'}
       >>> print(foo)
       {True: 'baz'}

    Note that I've tagged this as a documentation bug, but it seems like the documentation might be the preferred implementation.

    @HamishCampbell HamishCampbell mannequin added the docs Documentation in the Doc dir label Jan 6, 2016
    @HamishCampbell
    Copy link
    Mannequin Author

    HamishCampbell mannequin commented Jan 6, 2016

    Apologies, that first line should read "It looks like the documentation of set displays do not match behaviour in some cases".

    @HamishCampbell
    Copy link
    Mannequin Author

    HamishCampbell mannequin commented Jan 6, 2016

    Note also the differences here:

       >>> print(set([True, 1]))
       {True}
       >>> print({True, 1})
       {1}
       >>> print({x for x in [True, 1]})
       {True}

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Jan 6, 2016

    Set displays appear to be the culprit here:

    >>> class A:
    ...   count = 0
    ...   def __init__(self):
    ...     self.cnt = self.count
    ...     type(self).count += 1
    ...   def __eq__(self, other):
    ...     return type(self) is type(other)
    ...   def __hash__(self):
    ...     return id(type(self))
    ...
    >>> e={A(), A(), A(), A(), A()}
    >>> e
    {<__main__.A object at 0x002BB2B0>}
    >>> list(e)[0].cnt
    4
    >>> list(e)[0].count
    5
    >>> A.count = 0
    >>> q=set([A(), A(), A(), A(), A()])
    >>> q
    {<__main__.A object at 0x002BB310>}
    >>> list(q)[0].cnt
    0
    >>> list(q)[0].count
    5

    I'm guessing this is an optimization and/or set displays just don't keep ordering at definition time.

    Do you have a use case where x == y/hash(x) == hash(y) does not mean that x and y should be interchangeable? True and 1 are 100% interchangeable, minus their str() output, and my example is very unlikely to ever appear in actual code.

    Probably just better to fix the docs to specify that sets literals don't check ordering.

    @rhettinger
    Copy link
    Contributor

    The culprit is the BUILD_SET opcode in Python/ceval.c which unnecessarily loops backwards (it looks like it was copied from the BUILD_TUPLE opcode).

    @rhettinger rhettinger assigned rhettinger and unassigned docspython Jan 6, 2016
    @HamishCampbell
    Copy link
    Mannequin Author

    HamishCampbell mannequin commented Jan 6, 2016

    Do you have a use case where x == y/hash(x) == hash(y) does not mean that x and y should be interchangeable? True and 1 are 100% interchangeable, minus their str() output, and my example is very unlikely to ever appear in actual code.

    No I don't have a use case :)

    The culprit is the BUILD_SET opcode in Python/ceval.c which unnecessarily loops backwards (it looks like it was copied from the BUILD_TUPLE opcode).

    Incidentally, pypy seems to behave the same as reported here.

    @NeilGirdhar
    Copy link
    Mannequin

    NeilGirdhar mannequin commented Jan 7, 2016

    If BUIlD_SET is going to be changed, it would probably be could to keep BUILD_SET_UNPACK consistent.

    @serhiy-storchaka
    Copy link
    Member

    LGTM.

    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error and removed docs Documentation in the Doc dir labels Apr 26, 2016
    @NeilGirdhar
    Copy link
    Mannequin

    NeilGirdhar mannequin commented Apr 26, 2016

    Please don't forget to fix BUILD_SET_UNPACK to match.

    @NeilGirdhar
    Copy link
    Mannequin

    NeilGirdhar mannequin commented Apr 26, 2016

    Also, please add the following test: "{*{True, 1}}"

    Should be True.

    @serhiy-storchaka
    Copy link
    Member

    Please don't forget to fix BUILD_SET_UNPACK to match.

    Isn't it already correct?

    Also, please add the following test: "{*{True, 1}}"

    Did you meant "{*{True}, *{1}}"?

    @NeilGirdhar
    Copy link
    Mannequin

    NeilGirdhar mannequin commented Apr 26, 2016

    Ah, sorry, I somehow went cross-eyed. Not sure offhand which test would test the BUILD_TUPLE_UNPACK, but I think you're right Serhiy. Could just add both?

    @bitdancer
    Copy link
    Member

    Ping. (Raymond, if you are OK with someone else committing this, you could un-assign it.)

    @rhettinger
    Copy link
    Contributor

    I've got it.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 8, 2016

    New changeset 0967531fc762 by Raymond Hettinger in branch '3.5':
    Issue bpo-26020: Fix evaluation order for set literals
    https://hg.python.org/cpython/rev/0967531fc762

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 8, 2016

    New changeset a8d504600c18 by Raymond Hettinger in branch '2.7':
    Issue bpo-26020: Fix evaluation order for set literals
    https://hg.python.org/cpython/rev/a8d504600c18

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 8, 2016

    New changeset aa2bf603a9bd by Raymond Hettinger in branch '2.7':
    Issue bpo-26020: Add news entry
    https://hg.python.org/cpython/rev/aa2bf603a9bd

    @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

    3 participants