msg374378 - (view) |
Author: Akuli (Akuli) |
Date: 2020-07-27 14:18 |
The pop method of collections.deque can't be used like deque.pop(index), even though `del deque[index]` or deque.pop() without an argument works. This breaks the Liskov substitution principle because collections.abc.MutableMapping supports the .pop(index) usage. Is this intentional?
related typeshed issue: https://github.com/python/typeshed/issues/4364
|
msg374380 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2020-07-27 14:28 |
deque is not a subclass of MutableMapping, so the Liskov substitution principle is not related here.
deque is not registered as a virtual subclass of MutableMapping and it lacks a number of MutableMapping methods.
>>> import collections.abc
>>> issubclass(collections.deque, collections.abc.MutableMapping)
False
>>> sorted(set(dir(collections.abc.MutableMapping)) - set(dir(collections.deque)))
['_MutableMapping__marker', '__abstractmethods__', '__module__', '__slots__', '_abc_impl', 'get', 'items', 'keys', 'popitem', 'setdefault', 'update', 'values']
|
msg374381 - (view) |
Author: Akuli (Akuli) |
Date: 2020-07-27 14:30 |
I meant MutableSequence instead of MutableMapping. Oops.
|
msg374382 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2020-07-27 14:38 |
>>> issubclass(collections.deque, collections.abc.MutableSequence)
True
>>> sorted(set(dir(collections.abc.MutableSequence)) - set(dir(collections.deque)))
['__abstractmethods__', '__module__', '__slots__', '_abc_impl']
Well, it is a bug.
|
msg374384 - (view) |
Author: Yuan (Yuan) |
Date: 2020-07-27 14:58 |
Same status as slicing support from MutableSequence.
|
msg374386 - (view) |
Author: Shubham Kumar Jha (ShubhamKJha) * |
Date: 2020-07-27 15:26 |
Hi, I want to start contributing to CPython. Can I take up this issue?
|
msg374415 - (view) |
Author: Pablo Galindo Salgado (pablogsal) * |
Date: 2020-07-27 18:58 |
> Hi, I want to start contributing to CPython. Can I take up this issue?
Sure, go ahead. Feel free to ping one of us in the PR for review
|
msg374432 - (view) |
Author: Jim Jewett (Jim.Jewett) * |
Date: 2020-07-27 22:32 |
It may well have been intentional, as deques should normally be mutated only at the ends. But Raymond did make changes to conform to the ABC, so this should probably be supported too. Go ahead and include docstrings and/or discouraging it, though, except for i=0 and i=-1
|
msg374458 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2020-07-28 03:30 |
This was an intentional decision. Deques designed for fast access at the end points. Also, pop() is a core deque operation that needs to be fast. Altering its signature with an optional argument would slow it down.
Thank you for the suggestion, but we really shouldn't do this.
|
msg374464 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2020-07-28 05:14 |
FWIW, the relationship between a concrete class and an ABC is normative with respect to core capabilities but isn't 100% strict. Concrete classes can vary in small respects when it makes sense. For example, the __or__ and __and__ methods for collections.Set will accept any iterable, but the concrete does not.
We use the Liskov substitution principle as a guide, not as a law. In a number of cases, we choose practicality-beats-purity and allow subclasses to differ in ways than their parents. Counter() doesn't support fromkeys(). OrderedDict.popitem() has different signature than dict.popitem().
Hope that insight was helpful.
|
msg374472 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2020-07-28 07:11 |
If deque does not fully support the MutableSequence interface, should not it be un-regitered as MutableSequence? Maybe we need other abstract class which would be parent of MutableSequence and deque?
|
msg374483 - (view) |
Author: Akuli (Akuli) |
Date: 2020-07-28 10:43 |
I don't think it's very common to write code that needs to work with any MutableSequence but not with any Sequence. I think that's the only situation where missing support for deque.pop(index) is a problem.
Maybe deque should be a Sequence but not a MutableSequence. Or maybe there should be a way to say "MutableSequence does not require support for .pop(index) even though you get it by inheriting from MutableSequence".
|
msg374493 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2020-07-28 14:44 |
> If deque does not fully support the MutableSequence interface,
> should not it be un-regitered as MutableSequence?
You could unregister it, but for such a minor variance, it isn't worth it. We're not unregistering sets and frozenset from Set even though there are minor variances in the operators.
The ABCs are quite limited in what they do. The underlying machinery is mostly about determining whether a method is present or not. Concrete classes are free to override, extend or raise NotImplemented. For example, a Counter is still a mapping even though fromkeys() raises NotImplemented and update() has different semantics than Mapping.
Jelle has already adapted TypeShed to match the concrete class, so no actual user issue remains.
|
msg374514 - (view) |
Author: Pablo Galindo Salgado (pablogsal) * |
Date: 2020-07-28 17:48 |
I am convinced by Raymond's argument, it seems that with the current state of the ABC classes and semantics the most pragmatical solution is the status quo. It would be a weird if deque is a Sequence but not a MutableSequence because it can clearly be mutated.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:34 | admin | set | github: 85581 |
2020-07-28 17:48:46 | pablogsal | set | messages:
+ msg374514 |
2020-07-28 14:44:05 | rhettinger | set | messages:
+ msg374493 |
2020-07-28 10:43:25 | Akuli | set | messages:
+ msg374483 |
2020-07-28 07:11:50 | serhiy.storchaka | set | nosy:
+ stutzbach messages:
+ msg374472
|
2020-07-28 05:14:44 | rhettinger | set | messages:
+ msg374464 |
2020-07-28 03:30:22 | rhettinger | set | status: open -> closed versions:
+ Python 3.10 messages:
+ msg374458
resolution: rejected stage: resolved |
2020-07-27 22:32:18 | Jim.Jewett | set | nosy:
+ Jim.Jewett messages:
+ msg374432
|
2020-07-27 18:58:26 | pablogsal | set | nosy:
+ pablogsal messages:
+ msg374415
|
2020-07-27 15:26:20 | ShubhamKJha | set | nosy:
+ ShubhamKJha messages:
+ msg374386
|
2020-07-27 14:58:14 | Yuan | set | nosy:
+ Yuan messages:
+ msg374384
|
2020-07-27 14:38:52 | serhiy.storchaka | set | type: behavior messages:
+ msg374382 |
2020-07-27 14:30:22 | Akuli | set | messages:
+ msg374381 |
2020-07-27 14:28:37 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg374380
|
2020-07-27 14:26:57 | xtreak | set | nosy:
+ rhettinger
|
2020-07-27 14:18:55 | Akuli | create | |