bpo-34320: Fix dict(o) didn't copy order of dict subclass by methane · Pull Request #8624 · python/cpython

methane · 2018-08-02T13:36:04Z

When dict subclass overrides order (__iter__(), keys(), and items()), dict(o)
should use it instead of dict ordering.

https://bugs.python.org/issue34320

serhiy-storchaka

Would be nice to add tests for PEP 468 and PEP 520. These tests should be backported to 3.6, but the test for the dict constructor can not.

serhiy-storchaka · 2018-08-02T14:02:17Z

This may be too restrictive. Most dict subclasses don't change the order. Maybe check also that __iter__ is not overridden?

You mean, "Maybe check instead", right? So something like the following.

if (PyDict_Check(b) && Py_TYPE(b)->tp_iter == PyDict_Type.tp_iter) {

Adding the tp_iter checks seems like the most reasonable way to go. That would preserve the fast conversion speed of defaultdict.

serhiy-storchaka · 2018-08-03T05:40:51Z

Please add tests.

methane · 2018-08-03T06:01:29Z

@serhiy-storchaka Which type of test?
New test_dict_copy_order is minimal test for the regression; it fails on master branch.

serhiy-storchaka · 2018-08-03T06:05:32Z

Tests for for PEP 468 and PEP 520. test_dict_copy_order can't be backported to 3.6.

serhiy-storchaka · 2018-08-04T14:53:46Z

+    # Dict preserves order by language spec from 3.7.
+    # Dict preserves order by implementation from 3.6.
+    if sys.version_info < (3, 7):
+        test_dict_copy_order = support.cpython_only(test_dict_copy_order)


In 3.7+ this is not needed. And I think that in <3.7 this test should be backported.

Sorry, what do you mean concretely?

Tests are ran with concrete Python version. sys.version_info is a constant in every branch. In branches master and 3.7 these two lines should be removed. In branch 3.6 the decorator should be added unconditionally. But the backport to 3.6 needs editing in any case (only the test should be backported), so I don't think this will add additional work.

serhiy-storchaka · 2018-08-04T14:57:10Z

Perhaps there are better places for tests for PEP 468 and PEP 520. I would try to break the order of all kwargs dicts and class dicts and see what tests will fail.

serhiy-storchaka · 2018-08-06T11:48:14Z

I have made dict_merge() reverting the order for all mappings. The only failed tests are test.test_collections.TestChainMap.test_order_preservation, test.test_dict.DictTest.test_merge_and_mutate and several tests in test_ordered_dict. Nothing specific for PEP 468 and PEP 520. @ericsnowcurrently, do you know good places for testing these PEPs?

methane · 2018-08-06T11:58:03Z

FYI, PEP 520 is reverted because of dict preserves order.
type.__new__ should preserve order of passed namespace mapping. But it's not PEP 520.
It's just an expected behavior of "dict preserves insertion order".

methane · 2018-09-20T07:47:41Z

We missed 3.7.1, maybe.
I want to merge this simple bugfix without waiting long time to determine where we
should put tests for PEP 468 and 520.

May I add comment like "# TODO: Move this test to better place" and merge this?
@serhiy-storchaka

serhiy-storchaka · 2018-09-20T10:09:00Z

Would be nice to mention PEP 468 and 520 in comments for corresponding tests.

serhiy-storchaka

Thank you @methane!

ericsnowcurrently

Aside from moving the tests you already have, there should be at least 3 more tests:

with a metaclass.__prepare__ that returns an OrderedDict (see https://bugs.python.org/issue34320#msg325898) -- this would be next to either other tests related to __prepare__() or other tests related to type()
dict(obj) where obj is an instance of some non-OrderedDict subclass of dict without a custom __iter__ -- this would be in test_dict.py
dict(obj) where obj is an instance of some subclass of dict with a custom __iter__ -- this would be in test_dict.py

ericsnowcurrently · 2018-09-20T16:22:52Z


+    # TODO: This test is relating to PEP 468.
+    # Move this test to somewhere more appropriate.
+    def test_kwargs_order(self):


This test is about kwargs ordering rather than OrderedDict, so I'd expect it to be next to other tests for kwargs, not here in test_ordered_dict.py.

Yes, but problem is I can't find other tests for kwargs.

I added new test class for this, in test_call.py

ericsnowcurrently · 2018-09-20T16:24:27Z

+        self.assertIsInstance(res, dict)
+        self.assertEqual(list(res.items()), expected)
+
+    # TODO: This test is relating to PEP 520.


This test is not specific to PEP 520, which only relates directly to the class definition namespace. So you should drop this comment.

ericsnowcurrently · 2018-09-20T16:24:57Z

+
+    # TODO: This test is relating to PEP 520.
+    # Move this test to somewhere more appropriate.
+    def test_type_namespace_order(self):


This test should be next to other tests of the type() builtin.

ericsnowcurrently · 2018-09-20T16:25:55Z

+        C = type('C', (), od)
+        self.assertEqual(list(C.__dict__.items())[:2], [('b', 2), ('a', 1)])
+
+    def test_dict_copy_order(self):


This test should be in test_dict.py.

bedevere-bot · 2018-09-20T16:46:20Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

serhiy-storchaka · 2018-09-22T14:20:56Z

Thank you for your suggestions @ericsnowcurrently. They all LGTM.

miss-islington · 2018-09-26T03:59:03Z

Thanks @methane for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-09-26T03:59:13Z

GH-9582 is a backport of this pull request to the 3.7 branch.

) When dict subclass overrides order (`__iter__()`, `keys()`, and `items()`), `dict(o)` should use it instead of dict ordering. https://bugs.python.org/issue34320 (cherry picked from commit 2aaf98c) Co-authored-by: INADA Naoki <methane@users.noreply.github.com>

bedevere-bot · 2018-09-26T03:59:19Z

GH-9583 is a backport of this pull request to the 3.6 branch.

) When dict subclass overrides order (`__iter__()`, `keys()`, and `items()`), `dict(o)` should use it instead of dict ordering. https://bugs.python.org/issue34320 (cherry picked from commit 2aaf98c) Co-authored-by: INADA Naoki <methane@users.noreply.github.com>

When dict subclass overrides order (`__iter__()`, `keys()`, and `items()`), `dict(o)` should use it instead of dict ordering. https://bugs.python.org/issue34320 (cherry picked from commit 2aaf98c) Co-authored-by: INADA Naoki <methane@users.noreply.github.com>

) (GH-9583) When dict subclass overrides order (`__iter__()`, `keys()`, and `items()`), `dict(o)` should use it instead of dict ordering. https://bugs.python.org/issue34320 (cherry picked from commit 2aaf98c) Co-authored-by: INADA Naoki <methane@users.noreply.github.com> https://bugs.python.org/issue34320

the-knights-who-say-ni added the CLA signed label Aug 2, 2018

bedevere-bot added the awaiting merge label Aug 2, 2018

methane added type-bug An unexpected behavior, bug, or error awaiting core review needs backport to 3.7 and removed awaiting merge labels Aug 2, 2018

serhiy-storchaka reviewed Aug 2, 2018

View reviewed changes

rhettinger self-assigned this Aug 2, 2018

rhettinger approved these changes Aug 3, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 3, 2018

serhiy-storchaka added the needs backport to 3.6 label Aug 3, 2018

bpo-34320: Fix dict(od) didn't copy order

253f5db

methane force-pushed the dict-copy-order branch from af63eda to 253f5db Compare August 3, 2018 12:08

serhiy-storchaka reviewed Aug 4, 2018

View reviewed changes

methane closed this Sep 20, 2018

methane reopened this Sep 20, 2018

methane added 3 commits September 20, 2018 19:13

Remove unnecessary version check

e0bf46c

Update comment

26ef93f

Merge remote-tracking branch 'origin/master' into dict-copy-order

5b6502d

serhiy-storchaka approved these changes Sep 20, 2018

View reviewed changes

ericsnowcurrently requested changes Sep 20, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 20, 2018

methane force-pushed the dict-copy-order branch 3 times, most recently from f117479 to 603bc6c Compare September 25, 2018 14:27

Move tests

ee8696d

methane force-pushed the dict-copy-order branch from 603bc6c to ee8696d Compare September 25, 2018 15:01

methane changed the title ~~bpo-34320: Fix dict(od) didn't copy order~~ bpo-34320: Fix dict(o) didn't copy order of dict subclass Sep 26, 2018

methane added awaiting merge 🤖 automerge and removed awaiting changes labels Sep 26, 2018

miss-islington merged commit 2aaf98c into python:master Sep 26, 2018

bedevere-bot removed the awaiting merge label Sep 26, 2018

bedevere-bot removed the needs backport to 3.7 label Sep 26, 2018

bedevere-bot removed the needs backport to 3.6 label Sep 26, 2018

methane deleted the dict-copy-order branch November 29, 2018 07:54

dequis mentioned this pull request Feb 11, 2019

ImmutableMultiDict expansion breaks on Python 3.7.1 pallets/werkzeug#1379

Closed

Uh oh!

Conversation

methane commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Aug 3, 2018

Uh oh!

methane commented Aug 3, 2018

Uh oh!

serhiy-storchaka commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Aug 4, 2018

Uh oh!

serhiy-storchaka commented Aug 6, 2018

Uh oh!

methane commented Aug 6, 2018

Uh oh!

methane commented Sep 20, 2018

Uh oh!

serhiy-storchaka commented Sep 20, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 20, 2018

Uh oh!

serhiy-storchaka commented Sep 22, 2018

Uh oh!

miss-islington commented Sep 26, 2018

Uh oh!

bedevere-bot commented Sep 26, 2018

Uh oh!

bedevere-bot commented Sep 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

methane commented Aug 2, 2018 •

edited

Loading

serhiy-storchaka left a comment •

edited

Loading

ericsnowcurrently left a comment •

edited

Loading