Skip to content

gh-105736: Sync pure python version of OrderedDict with the C version #108098

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

Merged
merged 8 commits into from
Aug 21, 2023

Conversation

rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Aug 17, 2023

This solves a problem noticed in the referenced issue. When __init__ is overridden, the C version is still usable but the pure Python version fails. The solution is to have the internal invariants sets up in __new__ rather than __init__.

The original copy/pickle issue still remains.

@rhettinger rhettinger added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 17, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @rhettinger for commit 4f04b7e 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 17, 2023
@rhettinger rhettinger added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 17, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @rhettinger for commit d3179bc 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 17, 2023
@ambv
Copy link
Contributor

ambv commented Aug 18, 2023

I can confirm the new tests fail without the fix 👍🏻

One thing I'm trying to find is why we needed the AttributeError check for self.__root before. Since this is an double-underscore attribute, intuition tells me the only case when this can happen is unpickling. But when we provide self.__class__ as an "empty object factory" in the result of self.reduce, that will call __init__() first before setting the state, so self.__root won't exist at pickling and unpickling time either. And in fact, I can reproduce that it isn't ever present in those times, for protocols 3 - 5.

So I guess skipping the try:except: is harmless. But I wonder if I'm missing something.

@rhettinger
Copy link
Contributor Author

rhettinger commented Aug 18, 2023

@ambv

One thing I'm trying to find is why we needed the AttributeError check for self.__root before.

The __init__ method can be called more than once for an instance and it must add on to existing state rather than reset it from scratch. Here's an example with both regular dictionaries and OrderedDicts:

>>> d = dict(a=1, b=2)
>>> d.__init__(b=3, c=4)
>>> d
{'a': 1, 'b': 3, 'c': 4}

>>> d = OrderedDict(a=1, b=2)
>>> d.__init__(b=3, c=4)
>>> d
OrderedDict({'a': 1, 'b': 3, 'c': 4})

In contrast, the __new__ method can only be called once because it creates the actual instance. Hence, the try/except is no longer needed — the attribute cannot already exist.

@ambv
Copy link
Contributor

ambv commented Aug 19, 2023

By whom can it be called multiple times? I mean, yes, theoretically you can explicitly grab __init__() from an existing instance and call it again but is this a pattern in actual use anywhere? I haven't seen this before.

(yes, I agree that __new__() can skip the try:except:.)

@serhiy-storchaka serhiy-storchaka changed the title GH-105736 Sync pure python version of OrderedDict with the C version gh-105736: Sync pure python version of OrderedDict with the C version Aug 21, 2023
@serhiy-storchaka serhiy-storchaka self-requested a review August 21, 2023 09:39
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. And the resulting code looks clearer.

Moving the code from __init__ to __new__ can break pickling, but OrderedDict uses full constructor instead of __new__ in pickling, so it is safe from this side.

@ambv ambv merged commit 20cc90c into python:main Aug 21, 2023
@miss-islington
Copy link
Contributor

Thanks @rhettinger for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-108200 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Aug 21, 2023
@bedevere-bot
Copy link

GH-108201 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 21, 2023
…ersion (pythonGH-108098)

(cherry picked from commit 20cc90c)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 21, 2023
…ersion (pythonGH-108098)

(cherry picked from commit 20cc90c)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 21, 2023
@serhiy-storchaka
Copy link
Member

Side note. "gh-" in the title should be in lower case, and the issue number should be separated by a colon from the rest of the text. I think this is the reason why other Raymond's PRs were not automatically linked from the corresponding issue.

serhiy-storchaka pushed a commit that referenced this pull request Aug 21, 2023
…version (GH-108098) (GH-108201)

(cherry picked from commit 20cc90c)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
Yhg1s pushed a commit that referenced this pull request Aug 21, 2023
…version (GH-108098) (#108200)

gh-105736: Sync pure python version of OrderedDict with the C version (GH-108098)
(cherry picked from commit 20cc90c)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants