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

typing.Reversible doesn't fully support the reversing protocol #170

Open
serhiy-storchaka opened this issue Dec 26, 2015 · 8 comments

Comments

Projects
None yet
4 participants
@serhiy-storchaka
Copy link
Member

commented Dec 26, 2015

typing.Reversible doesn't work with types that are supported by reserved() but don't have the __reversed__ method. E.g. tuple, str, bytes, bytearray, and array.

>>> issubclass(tuple, typing.Reversible)
False
>>> issubclass(str, typing.Reversible)
False
>>> issubclass(bytes, typing.Reversible)
False
>>> issubclass(bytearray, typing.Reversible)
False
>>> issubclass(array.array, typing.Reversible)
False 

The reversing protocol as well as the iterating protocol is supported not only by special method, but implicitly if the class has implemented __getitem__ and __len__ methods.

>>> class Counter(int):
...   def __getitem__(s, i): return i
...   def __len__(s): return s
... 
>>> list(reversed(Counter(10)))
[9, 8, 7, 6, 5, 4, 3, 2, 1, 0]
>>> issubclass(Counter, typing.Reversible)
False
@gvanrossum

This comment has been minimized.

Copy link
Member

commented Dec 26, 2015

Yeah, the _ProtocolMeta metaclass that implements the structural check is pretty simple-minded. However, the BDFL-delegate for PEP 484 decreed that the classes in typing should not support isinstance() or issubclass() at all. I ripped out isinstance() support, but ripping out issubclass() support is still on the TODO list (see #136). So I'm not sure it's worth adding this.

The place where this really should be implemented is in the type checker, e.g. mypy (https://github.com/JukkaL/mypy). I don't know whether mypy currently supports this, but it's worth trying and searching its issue tracker. (I would, but I'm home sick and can't concentrate very long.)

@abarnert

This comment has been minimized.

Copy link

commented Dec 26, 2015

This is the same problem as Iterable being defined as "has __iter__" instead of "has __iter__ or implements the old-style sequence protocol". As far as I can tell, the only reason Iterable passes such types today is that they're all registered with collections.abc.Sequence, which is a subclass of collections.abc.Iterable, and typing.Iterable has extra=collections.abc.Iterable attached and defers to the ABC to do the check. (I'm not sure if/how #136 will affect that.) So the obvious way to fix this problem seems to be doing the equivalent thing that makes Iterable work:

  • Add collections.abc.Reversible, which is a subtype of Iterable, and has a __subclasshook__ that checks for __reversed__.
  • Change collections.abc.Sequence to be a subtype of Reversible instead of Iterable, so that everything registered as a Sequence or MutableSequence is now Reversible as well as Iterable as far as the ABCs are concerned.
  • Change typing.Reversible to add extra=collections.abc.Reversible.
  • Any third-party types that want to be Reversible based on the old-style sequence protocol despite not being Sequences could call collections.abc.Reversible.register.

Alternatively, is Reversible even useful in practice? If the only thing it's used for is as a type hint for reverse, and it doesn't get that right, and can't get that right without adding a lot of complexity, maybe it just shouldn't be there.

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Dec 26, 2015

The type checker (e.g. mypy) is independent from the issubclass()
implementation in typing.py. As I wrote in a different thread we shouldn't
even have the latter, per decree from the BDFL-delegate for PEP 484. AFAICT
mypy does the right thing for Reversible and those builtin types.

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Dec 26, 2015

IOW Reversible does get things right in practice (in the type checker).

FWIW, maybe we should try to deprecate supporting iteration using the old-style protocol? It's really a very old backwards compatibility measure (from when iterators were first introduced). Then eventually we could do the same for reversing using the old-style protocol.

@abarnert

This comment has been minimized.

Copy link

commented Dec 27, 2015

The last time deprecating the old-style protocol came up, Stephen d'Aprano strongly objected (http://article.gmane.org/gmane.comp.python.ideas/23369/match=old+sequence+protocol), citing an earlier objection by Raymond Hettinger (which I don't have a link for).

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Dec 27, 2015

On Sat, Dec 26, 2015 at 6:24 PM, Andrew Barnert notifications@github.com
wrote:

The last time deprecating the old-style protocol came up, Stephen d'Aprano
strongly objected (
http://article.gmane.org/gmane.comp.python.ideas/23369/match=old+sequence+protocol),
citing an earlier objection by Raymond Hettinger (which I don't have a link
for).

Hm. The example given there is a toy though. Something with a getitem
that maps its argument to its square might as well be a mapping. I really
think it's time to slowly let go of this (no need to rush into removing
support, but we could still frown upon its use).

--Guido van Rossum (python.org/~guido)

@serhiy-storchaka

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2017

The issue is fixed for tuple, str, bytes and bytearray. But the issubclass() check still doesn't work for array.array and sample Counter class.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2017

@serhiy-storchaka Yes, I also noticed this some time ago. But now typing.Reversible just defers all checks to collections.abc.Reversible, so that if we fix the latter, then this will be automatically fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.