Skip to content

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

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

Closed
serhiy-storchaka opened this issue Dec 26, 2015 · 9 comments
Closed
Labels
resolution: out of scope The idea or discussion was out of scope for this repository

Comments

@serhiy-storchaka
Copy link
Member

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
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

@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.

@srittau srittau added the resolution: out of scope The idea or discussion was out of scope for this repository label Nov 4, 2021
@srittau
Copy link
Collaborator

srittau commented Nov 4, 2021

If this is still an issue, it should be reported to bpo.

@srittau srittau closed this as completed Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution: out of scope The idea or discussion was out of scope for this repository
Projects
None yet
Development

No branches or pull requests

5 participants