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

bpo-36144: Dictionary Union (PEP 584) #12088

Merged
merged 39 commits into from Feb 25, 2020
Merged

Conversation

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Feb 28, 2019

`d1 + d2` is semantically identical to `d3 = d1.copy(); d3.update(d2); d3`, and `d1 += d2` is semantically identical to `d1.update(d2)`.

Isn't that sweet?
It feels so weird to sum dicts like this... cool.
This gets the collections tests passing again.
This adds one test with 5 new assertions.
In case this gets pulled. Very cool!
Whoops.
brandtbucher and others added 2 commits Feb 28, 2019
This whole patch adds 3 new objects with docstrings: dict.__add__, dict.__radd__, dict.__iadd__. On some builds, this puts us over the threshold, so bump the count!
@rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Feb 28, 2019

It usually isn't an effective use to time to go straight to a PR before a proposal has been discussed and accepted.

@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Feb 28, 2019

I figured it would help with decision-making. The person who started the thread asked for an implementation, and I had the free time to make one. I'm not very familiar with how the PEP process usually works, but I saw in PEP 1:

Standards Track PEPs consist of two parts, a design document and a reference implementation. It is generally recommended that at least a prototype implementation be co-developed with the PEP, as ideas that sound good in principle sometimes turn out to be impractical when subjected to the test of implementation.

Sorry if this is a distraction. It's not my intention to disrupt the normal process!

Replaces two places where a failed PyDict_Update call returns NotImplemented instead of raising the error.
@rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Feb 28, 2019

It looks like this may yet have a chance :-)

brandtbucher and others added 6 commits Mar 2, 2019
This brings this reference implementation in line with PEP 584's spec.

Separately, allows RHS of += to be any valid arg to dict.update.
These are implemented as they are currently laid out in PEP 584.
This should get tests passing again.
And... we're officially implemented!
@brandtbucher brandtbucher changed the title bpo-36144: Dictionary addition. bpo-36144: Dictionary Addition/Subtraction (PEP 584) Mar 2, 2019
@methane methane added the DO-NOT-MERGE label Mar 3, 2019
brandtbucher added 5 commits Mar 3, 2019
This fixes issues where iteration errors could be swallowed, and also stops a possible reference leak.
This also implements the complex keys/__iter__ dance that update uses, as an example of compatibility with dict.update.
dict.__iadd__ can take any mapping (i.e., a list).
This is pretty much copied-and-pasted from UserDict, but it's better than what we had before. They may need some tweaking.
This was requested by Steven for comparison purposes. The final implementation will only keep one!
@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Feb 6, 2020

I've added subclass tests.

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Feb 6, 2020

Because list and set do not call copy() and extend()/update().

It would make dict very different from other builtin types.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Feb 6, 2020

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Feb 6, 2020

Since it is a principal point of the PEP, I suggest you to create two or more separate PRs: one does not depend on subclasses, and the other calls copy() and update().

Yet one argument against calling copy() and update() is that it would be the only case when operator depends on non-dunder methods.

@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Feb 6, 2020

@serhiy-storchaka Yes, and in the earlier stages of the proposal I felt as you did:

https://mail.python.org/archives/list/python-ideas@python.org/message/HBWBRRGXUGPT2NQLVNM3XLBHZSHKWBGI/

But Steven was strongly for this, and over time his arguments were enough to shift my view.

https://mail.python.org/archives/list/python-ideas@python.org/message/SIPJZPEUTIMZ7ETCCDUKWDIFNRN7YHMH/

Also:

Possibly relevant: I've always been frustrated and annoyed at classes
that hardcode their own type into methods... This gets really annoying really quickly. Try subclassing int, for example, where you have to override something like 30+ methods and do nothing but wrap calls to super.

It's more common to subclass dict than either list or set, so I see this as more important issue than just blindly following what seems like a less-than-ideal precedent without a really good reason to.

dict doesn't have any operators yet, so we're not bound by any prior design constraints in this area (rather, we're setting them). My example above with defaultdict shows that this has immediate benefits for both usability and maintenance.

@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Feb 6, 2020

@gvanrossum, is this enough of an issue that you would consider rejecting the proposal over it? I know that Steven and I have somewhat strongly formed opinions on this from all of the endless mailing-list threads, so it might be a good idea to continue this discussion on the python-dev thread where he can participate, if so.

@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Feb 6, 2020

Also, @serhiy-storchaka it would be pretty easy to just strip the last 5 commits at this point. I can make another PR if the two designs deviate more substantially.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Feb 6, 2020

@brandtbucher This won't be an issue unless you make it one. The proposal is a good one regardless of what it does with subclasses. Does the PEP explicitly address this (either in the spec or in the text about rejected alternatives or elsewhere)? If so can you point me to the text?

A general problem with honoring subclasses is that it's worse when it's done inconsistently than not at all. For immutable objects (e.g. Steven's int subclassing example) the problem is that you won't know the arguments to the subclass constructor, and you can't mutate int objects after they've been created. So you just have to suck it up. For dict, in this case copy()+update() will work. But that's just luck. C code that inserts a new key into a dict shouldn't have to look for an overridden __setitem__, it should be able to just use PyDict_SetItem.

@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Feb 6, 2020

I don't believe this was ever explicitly outlined in the PEP (probably because we didn't get much pushback relative to other decisions 😉), but the Reference Implementation section is clear - it uses bound self methods rather than unbound dict methods. We also mention the desire of subclass-preserving behavior in the Motivation section, although that is less relevant because the operators could still be manually overridden.

Thinking out loud, if dict were to grow more operators, they would likely be &, ^, and - (with the associated in-place versions, which is important). Continuing our subclass-friendliness started here would probably require, at the very least, also respecting __delitem__ and __setitem__ as well...

How do we feel about keeping the copy call and dropping the update one? I know Steven was open to that. New subclasses just have to implement copy to get fast, working versions of everything, and we get to keep a working defaultdict, etc...

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Feb 6, 2020

I still think it's a doubtful choice (and the PEP is clear that the code there is not exactly equivalent). Can you bring this up in the python-dev thread? So far you've convinced 0 out of 2 core devs -- but you still could get the Steering Council behind you.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Feb 6, 2020

@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Feb 6, 2020

Can you bring this up in the python-dev thread?

Done.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Feb 6, 2020

@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Feb 7, 2020

I am going to go ahead and remove the update calls, since we've come to an agreement on that. Jury's still out on copy.

Copy link
Member

@gvanrossum gvanrossum left a comment

LGTM. Obviously let's wait until PEP 584 is accepted.

@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Feb 17, 2020

Thanks for all of the feedback @gvanrossum and @serhiy-storchaka (especially for your insightful comments on the subclass issue).

@brandtbucher
Copy link
Member Author

@brandtbucher brandtbucher commented Feb 25, 2020

Guido just informed us that the Steering Council accepted his recommendation!

@gvanrossum gvanrossum merged commit eb8ac57 into python:master Feb 25, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200217.14 succeeded
Details
bedevere/issue-number Issue number 36144 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Feb 25, 2020

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

chrisburr added a commit to chrisburr/cpython that referenced this pull request Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants