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

Creating dict from OrderedDict doesn't preserve order #78501

Closed
serhiy-storchaka opened this issue Aug 2, 2018 · 9 comments
Closed

Creating dict from OrderedDict doesn't preserve order #78501

serhiy-storchaka opened this issue Aug 2, 2018 · 9 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 34320
Nosy @rhettinger, @methane, @ericsnowcurrently, @Rosuav, @serhiy-storchaka, @zooba, @miss-islington
PRs
  • bpo-34320: Fix dict(o) didn't copy order of dict subclass #8624
  • [3.7] bpo-34320: Fix dict(o) didn't copy order of dict subclass (GH-8624) #9582
  • [3.6] bpo-34320: Fix dict(o) didn't copy order of dict subclass (GH-8624) #9583
  • 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 2018-09-26.06:40:18.588>
    created_at = <Date 2018-08-02.13:12:23.864>
    labels = ['interpreter-core', '3.8', 'type-bug', '3.7']
    title = "Creating dict from OrderedDict doesn't preserve order"
    updated_at = <Date 2018-09-26.06:40:18.587>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2018-09-26.06:40:18.587>
    actor = 'methane'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-09-26.06:40:18.588>
    closer = 'methane'
    components = ['Interpreter Core']
    creation = <Date 2018-08-02.13:12:23.864>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34320
    keywords = ['patch']
    message_count = 9.0
    messages = ['322951', '325898', '326340', '326356', '326357', '326365', '326416', '326417', '326421']
    nosy_count = 7.0
    nosy_names = ['rhettinger', 'methane', 'eric.snow', 'Rosuav', 'serhiy.storchaka', 'steve.dower', 'miss-islington']
    pr_nums = ['8624', '9582', '9583']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue34320'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @serhiy-storchaka
    Copy link
    Member Author

    >>> from collections import OrderedDict
    >>> od = OrderedDict([('a', 1), ('b', 2)])
    >>> od.move_to_end('a')
    >>> od
    OrderedDict([('b', 2), ('a', 1)])
    >>> dict(od)
    {'a': 1, 'b': 2}

    This affects also PEP-468.

    >>> def f(**kwargs): return kwargs
    ... 
    >>> f(**od)
    {'a': 1, 'b': 2}

    And PEP-520.

    >>> type('A', (), od).__dict__
    mappingproxy({'a': 1, 'b': 2, '__module__': '__main__', '__dict__': <attribute '__dict__' of 'A' objects>, '__weakref__': <attribute '__weakref__' of 'A' objects>, '__doc__': None})

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Aug 2, 2018
    @ericsnowcurrently
    Copy link
    Member

    FWIW, the PEP-520 example isn't exactly right. PEP-520 is about the class definition namespace, not the namespace object passed to type(). That always does the right thing, since the default class definition namespace is dict (which happens to be ordered in 3.6+).

    That said, a test that explicitly calls type() with an OrderedDict, as you have shown, is still a good idea since type() always changes the namespace to a dict.

    Perhaps another good test of the same thing would be with a metaclass that returns an OrderedDict from __prepare__():

      class Meta(type):
          @classmethod
          def __prepare__(meta, name, bases, **kwargs):
              return OrderedDict()
    
      class Spam(metaclass=Meta):
          a = 1
          b = 2
          locals().move_to_end('a')

    Spam.__dict__

    Just keep in mind that neither of those is specific to PEP-520.

    @methane
    Copy link
    Member

    methane commented Sep 25, 2018

    Perhaps another good test of the same thing would be with a metaclass that returns an OrderedDict from __prepare__():

    It is not relating to this issue: there are no conversion happened.
    Should I add the test in this PR?

    @ericsnowcurrently
    Copy link
    Member

    There is a conversion. See builtin___build_class__ in Python/bltinmodule.c (and the LOAD_BUILD_CLASS target in Python/ceval.c). Specifically, the metaclass (e.g. the builtin type) is instantiated using the namespace from the class definition. The metaclass copies that namespace into a new dict. So the following two bits of code are equivalent:

    # using a class definition

      ns = OrderedDict()
    
      class Meta(type):
          def __prepare__(*args, **kwargs):
              return ns
    
      class Spam(metaclass=Meta):
          a = 1
          b = 2
          ns.move_to_end('a')

    # using the metaclass directly

      ns = OrderedDict()
      ns['a'] = 1
      ns['b'] = 2
      ns.move_to_end('a')
      Spam = Meta('Spam', (object,), ns)

    In both cases Spam.__dict__ will be a proxy for a new dict copied from ns.

    @methane
    Copy link
    Member

    methane commented Sep 25, 2018

    Conversion is happens when type() (or metaclass()) is called.
    And I added test for it already.
    https://github.com/python/cpython/pull/8624/files#diff-7ba610fbe5686a1d67c5504312be4817R1977

    Why regression test for this bugfix need to use __prepare__?

    @ericsnowcurrently
    Copy link
    Member

    Conversion is happens when type() (or metaclass()) is called.

    Right. So an extra test to cover the __prepare__ case is somewhat redundant. I only suggested it because type() is called implicitly and the test would make the conversion case clear. However, it was only a suggestion for you to consider, not something I considered necessary. :) I'm fine with not adding a test that uses __prepare__.

    @miss-islington
    Copy link
    Contributor

    New changeset 2aaf98c by Miss Islington (bot) (INADA Naoki) in branch 'master':
    bpo-34320: Fix dict(o) didn't copy order of dict subclass (GH-8624)
    2aaf98c

    @miss-islington
    Copy link
    Contributor

    New changeset 12e3e80 by Miss Islington (bot) in branch '3.7':
    bpo-34320: Fix dict(o) didn't copy order of dict subclass (GH-8624)
    12e3e80

    @miss-islington
    Copy link
    Contributor

    New changeset d45a961 by Miss Islington (bot) in branch '3.6':
    [3.6] bpo-34320: Fix dict(o) didn't copy order of dict subclass (GH-8624) (GH-9583)
    d45a961

    @methane methane closed this as completed Sep 26, 2018
    @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.7 (EOL) end of life 3.8 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

    4 participants