Hi Steven.

Thanks for the clarifications. I've pushed a complete working patch (with tests) to GitHub. It's linked to the bpo issue.

Branch: https://github.com/brandtbucher/cpython/tree/addiction
PR: https://github.com/python/cpython/pull/12088

Right now, it's pretty much a straight reimplementation of your Python examples. I plan to update it periodically to keep it in sync with any changes, and to make a few optimizations (for example, when operands are identical or empty).

Let me know if you have any questions/suggestions. Stoked to learn and help out with this process! :)

Brandt

On Fri, Mar 1, 2019 at 7:57 PM Steven D'Aprano <steve@pearwood.info> wrote:
Executive summary:

- I'm going to argue for subclass-preserving behaviour;

- I'm not wedded to the idea that dict += should actually call the
update method, so long as it has the same behaviour;

- __iadd__ has no need to return NotImplemented or type-check its
argument.

Details below.


On Fri, Mar 01, 2019 at 04:10:44PM -0800, Brandt Bucher wrote:

[...]
> In your Python implementation samples from the PEP, dict subclasses will
> behave differently from how list subclasses do. List subclasses, without
> overrides, return *list* objects for bare "+" operations

Right -- and I think they are wrong to do so, for reasons I explained
here:

https://mail.python.org/pipermail/python-ideas/2019-March/055547.html

I think the standard handling of subclasses in Python builtins is wrong,
and I don't wish to emulate that wrong behaviour without a really good
reason. Or at least a better reason than "other methods break
subclassing unless explicitly overloaded, so this should do so too".

Or at least not without a fight :-)



> (and "+=" won't call an overridden "extend" method).

I'm slightly less opinionated about that. Looking more closely into the
docs, I see that they don't actually say that += calls list.extend:

s.extend(t)       extends s with the contents of t (for
or s += t         the most part the same as s[len(s):len(s)] = t)

https://docs.python.org/3/library/stdtypes.html#mutable-sequence-types

only that they have the same effect. So the wording re lists calling
extend certainly needs to be changed. But that doesn't mean that we must
change the implementation. We have a choice:

- regardless of what lists do, we define += for dicts as literally
  calling dict.update; the more I think about it, the less I like this.

- Or we say that += behaves similarly to update, without actually
  calling the method. I think I prefer this.

(The second implies either that += either contains a duplicate of the
update logic, or that += and update both delegate to a private, C-level
function that does most of the work.)

I think that the second approach (define += as having the equivalent
semantics of update but without actually calling the update method) is
probably better. That decouples the two methods, allows subclasses to
change one without necessarily changing the other.


> So a more analogous
> pseudo-implementation (if that's what we seek) would look like:
>
> def __add__(self, other):
>     if isinstance(other, dict):
>         new = dict.copy(self)
>         dict.update(new, other)
>         return new
>     return NotImplemented

We should not require the copy method.

The PEP should be more explicit that the approximate implementation does
not imply the copy() and update() methods are actually called.


> def __iadd__(self, other):
>     if isinstance(other, dict):
>         dict.update(self, other)
>         return self
>     return NotImplemented

I don't agree with that implementation.

According to PEP 203, which introduced augmented assignment, the
sequence of calls in ``d += e`` is:

1. Try to call ``d.__iadd__(e)``.

2. If __iadd__ is not present, try ``d.__add__(e)``.

3. If __add__ is missing too, try ``e.__radd__(d)``.

but my tests suggest this is inaccurate. I think the correct behaviour
is this:

1. Try to call ``d.__iadd__(e)``.

2. If __iadd__ is not present, or if it returns NotImplemented,
   try ``d.__add__(e)``.

3. If __add__ is missing too, or if it returns NotImplemented,
   fail with TypeError.

In other words, e.__radd__ is not used.

We don't want dict.__iadd__ to try calling __add__, since the later is
more restrictive and less efficient than the in-place merge. So there is
no need for __iadd__ to return NotImplemented. It should either succeed
on its own, or fail hard:

def __iadd__(self, other):
    self.update(other)
    return self

Except that the actual C implementation won't call the update method
itself, but will follow the same semantics.

See the docstring for dict.update for details of what is accepted by
update.


--
Steven
_______________________________________________
Python-ideas mailing list
Python-ideas@python.org
https://mail.python.org/mailman/listinfo/python-ideas
Code of Conduct: http://python.org/psf/codeofconduct/