classification
Title: Creating dict from OrderedDict doesn't preserve order
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Rosuav, eric.snow, inada.naoki, miss-islington, rhettinger, serhiy.storchaka, steve.dower
Priority: normal Keywords: patch

Created on 2018-08-02 13:12 by serhiy.storchaka, last changed 2018-09-26 06:40 by inada.naoki. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 8624 merged inada.naoki, 2018-08-02 13:36
PR 9582 merged miss-islington, 2018-09-26 03:59
PR 9583 merged miss-islington, 2018-09-26 03:59
Messages (9)
msg322951 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-08-02 13:12
>>> 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})
msg325898 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2018-09-20 16:17
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.
msg326340 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2018-09-25 11:11
> 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?
msg326356 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2018-09-25 14:26
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.
msg326357 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2018-09-25 14:44
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__?
msg326365 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2018-09-25 15:46
> 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__.
msg326416 - (view) Author: miss-islington (miss-islington) Date: 2018-09-26 03:59
New changeset 2aaf98c16ae3070378de523a173e29644037d8bd by Miss Islington (bot) (INADA Naoki) in branch 'master':
bpo-34320: Fix dict(o) didn't copy order of dict subclass (GH-8624)
https://github.com/python/cpython/commit/2aaf98c16ae3070378de523a173e29644037d8bd
msg326417 - (view) Author: miss-islington (miss-islington) Date: 2018-09-26 04:17
New changeset 12e3e80241e91df79811f53ff372e28e9371bf9b by Miss Islington (bot) in branch '3.7':
bpo-34320: Fix dict(o) didn't copy order of dict subclass (GH-8624)
https://github.com/python/cpython/commit/12e3e80241e91df79811f53ff372e28e9371bf9b
msg326421 - (view) Author: miss-islington (miss-islington) Date: 2018-09-26 06:38
New changeset d45a9613402b686f8afd3dd5b6acf8141f14d711 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)
https://github.com/python/cpython/commit/d45a9613402b686f8afd3dd5b6acf8141f14d711
History
Date User Action Args
2018-09-26 06:40:18inada.naokisetstatus: open -> closed
dependencies: - Add tests for PEP 468 and PEP 520
resolution: fixed
stage: patch review -> resolved
2018-09-26 06:38:40miss-islingtonsetmessages: + msg326421
2018-09-26 04:17:55miss-islingtonsetmessages: + msg326417
2018-09-26 03:59:19miss-islingtonsetpull_requests: + pull_request8984
2018-09-26 03:59:13miss-islingtonsetpull_requests: + pull_request8983
2018-09-26 03:59:05miss-islingtonsetnosy: + miss-islington
messages: + msg326416
2018-09-25 15:46:22eric.snowsetmessages: + msg326365
2018-09-25 14:44:52inada.naokisetmessages: + msg326357
2018-09-25 14:26:59eric.snowsetmessages: + msg326356
2018-09-25 11:11:57inada.naokisetmessages: + msg326340
2018-09-20 16:17:17eric.snowsetmessages: + msg325898
2018-09-20 16:16:49eric.snowsetmessages: - msg325897
2018-09-20 16:15:08eric.snowsetmessages: + msg325897
2018-08-06 12:08:50serhiy.storchakasetdependencies: + Add tests for PEP 468 and PEP 520
2018-08-02 13:36:05inada.naokisetkeywords: + patch
stage: patch review
pull_requests: + pull_request8129
2018-08-02 13:12:23serhiy.storchakacreate