Implicit ABCs have no means of "anti-registration"

[abarnert]

BPO	25958
Nosy	@gvanrossum, @rhettinger, @ncoghlan, @ethanfurman, @vadmium, @serhiy-storchaka, @ilevkivskyi, @Mariatta
Files	patch.diff patch-regenerated.diff patch2.diff patch3.diff patch3-regenerated.diff patch4a.diff patch5.diff abarnert_rebased.diff abarnert_rebased2.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-08-18.19:16:27.926>
created_at = <Date 2015-12-26.22:26:42.809>
labels = ['type-feature', 'library']
title = 'Implicit ABCs have no means of "anti-registration"'
updated_at = <Date 2017-06-10.03:39:41.507>
user = 'https://bugs.python.org/abarnert'

bugs.python.org fields:

activity = <Date 2017-06-10.03:39:41.507>
actor = 'Mariatta'
assignee = 'none'
closed = True
closed_date = <Date 2016-08-18.19:16:27.926>
closer = 'gvanrossum'
components = ['Library (Lib)']
creation = <Date 2015-12-26.22:26:42.809>
creator = 'abarnert'
dependencies = []
files = ['41502', '41508', '41509', '41511', '41519', '41527', '41651', '44122', '44123']
hgrepos = []
issue_num = 25958
keywords = ['patch']
message_count = 46.0
messages = ['257052', '257056', '257059', '257129', '257277', '257295', '257303', '257305', '257481', '257490', '257491', '257512', '257518', '257523', '257538', '257542', '257543', '257544', '257546', '257548', '257549', '257550', '257551', '257555', '257559', '257564', '257576', '257636', '257639', '257641', '257790', '258539', '259346', '259350', '259351', '259355', '259357', '270439', '272800', '272801', '273041', '273047', '273048', '273051', '295540', '295602']
nosy_count = 10.0
nosy_names = ['gvanrossum', 'rhettinger', 'ncoghlan', 'ethan.furman', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'abarnert', 'levkivskyi', 'Mariatta']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue25958'
versions = ['Python 3.6']

[abarnert]

Serhiy Storchaka raised an issue (http://bugs.python.org/msg256910) with static type hints on bpo-25864 (http://bugs.python.org/issue25864), which I believe also applies to runtime ABCs.

Consider this class, which uses [] in a way similar to generic types:

    class Thing:
        def __getitem__(self, specialization):
            return type(self)._specialize(specialization)(self)
        def __len__(self):
            return len(self._specializations)

Because this type looks like it supports the old-style sequence protocol, calling either iter or reversed on an instance will successfully return a useless iterator. (You may get an error when you start trying to iterate it, but you may not even then.) You don't want that, so you add either this:

        __iter__ = None
        __reversed__ = None

... or this:

        def __iter__(self): raise TypeError('not iterable')
        def __reversed__(self): raise TypeError('not iterable')

Unfortunately, doing either means that issubclass(Thing, collections.abc.Iterable) now returns true. Which is the exact opposite of the intention of that check. (The same is true for typing.Iterable and typing.Reversible.) So, fixing the problem for duck typing creates the equivalent problem for explicit typing.

There are a few possible solutions here:

1. Maybe document it, otherwise do nothing.

2. Change the ABCs to check that the dunder method exists and is not None (or is callable, or is a non-data descriptor). Then, the one way to opt out is to assign __iter__ = __reversed__ = None.

3. Add an ABC.unregister method that can be used to explicitly state that this type does not support that ABC, regardless of what its __subclasshook__ says.

Possible argument for #1: Iterable rarely has a problem. (Types that use __getitem__ for something completely un-sequence-like, like typing's generic types, usually don't have __len__. Types that have both __getitem__ and __len__, like mappings, usually have a reasonable alternative __iter__ to offer.) Reversible would have a problem if there was such an ABC, but there isn't. Off the top of my head, I can't think of any of the other implicit ABCs that are susceptible to this problem.

The counter-argument is that static typehinting definitely does have this problem (python/typing#170), and, depending on how that's solved, it may well make sense to use the same solution here.

If we do need a solution, #2 seems better than #3 (or anything else I could think up). The only problem there is that def __iter__(self): raise TypeError('not iterable') gives you a nicer error than __iter__ = None.

[abarnert]

As Guido pointed out on -ideas, hashing already uses the convention of __hash__ is None to declare a type unhashable, and collections.abc.Hashable.__subclasshook__ already checks for that.

Meanwhile, setting __iter__ and __reversed__ to None already raises a TypeError on iter and reversed (although not with the most helpful description).

So, maybe that should be documented as the standard way to unimplement __iter__ and __reversed__, and collections.abc.Iterable should check that the looked-up __iter__ is not None (and presumably typecheckers doing the equivalent for both Iterable and Reversible)?

[abarnert]

As Serhiy pointed out on -ideas, there's no reason reversed couldn't add special handling for __reversed__ is None, akin to hash's special handling for __hash__ is None, to produce whatever error message we wanted in the TypeError, instead of "'NoneType' object is not callable". So, retract that argument against #2.

[abarnert]

Hashable and Awaitable already treat None (actually any falsey value) as not implementing the special method, and blocking any superclass implementation, in their __subclasshook__. (This only blocks implicit subclassing--anything that actually directly or indirectly inherits from or registers with Hashable is still Hashable even if it sets __hash__ = None.)

Coroutine, AsyncIterable, AsyncIterator, Iterable, Iterator, Generator, Sized, Container, and Callable treat None as an implementation.

http://bugs.python.org/file41440/cpython-iter-patch.diff for bpo-25864 changes Iterable to work like Hashable, but doesn't touch any of the other types.

To fix this in general, just apply the same change to the other implicit ABCs (and probably factor out the logic instead of repeating it that many more times).

But I'm not sure a general fix is needed here. The special methods for those other types don't have a base class and/or fallback implementation to block the way __hash__, __iter__, and __reversed__ do (except for __call__ on metaclasses, but I can't imagine why you'd want a metaclass that isn't callable), so there's no need to set them to None to block that, so the problem should never come up.

[vadmium]

I don’t think you need to define __len__() to get an iterable, only __getitem__().

Also, if I understand your problem, Container would also be susceptible in theory, because membership testing via “in” and “not in” falls back to iteration. If you only define __getitem__(), your class is both iterable and a container, but it is neither an Iterable nor a Container. :)

[bitdancer]

Absolutely you do not need to define __len__ to get an iterable. Length is not a property of an iterable (iterables can be indefinite in length or infinite in length).

[abarnert]

I don’t think you need to define __len__() to get an iterable, only __getitem__().

The "old-style sequence protocol" means having a __getitem__ that works for values from 0 to __len__() and raises IndexError at __len__(). You don't need to be a complete old-style sequence to be iterable; just having __getitem__ makes you iterable (without being a collections.abc.Iterable or a typing.Iterable), and having __getitem__ and __len__ makes you reversible (without being a typing.Reversible). At any rate, this bug isn't about avoiding false negatives for the implicit ABCs, but false positives: defining __iter__ = None blocks the old-style sequence protocol, but makes isinstance(Iterable) true.

[abarnert]

Also, if I understand your problem, Container would also be susceptible in theory

You're right, but not in the details.

Being iterable (whether via __iter__ or via the old-style sequence protocol) makes you a container. But, again, false negatives for Container aren't a problem.

But blocking that by setting __contains__ = None makes you a Container but not a container, the same kind of false positive as bpo-25864. That's exactly why I split off this bug from that one--that one only fixes __iter__ and __reversed__, but it's possible that a more general solution is needed. (Or, of course, maybe we don't need anything more general, we just need to expand it to __iter__, __reversed__, and __contains__.)

[gvanrossum]

I propose to solve the narrow problem by indeed supporting the setting of certain special methods to None similar to __hash__. This should be limited to those methods for which this adds value, e.g. where the complete absence of the method causes a fall-back to a different API (e.g. __getitem__+len in the case of __reversed__) -- the None value should just block the fallback. This will require some changes in the ABCs that test for the presence of a given method (Iterable/iter, Container/contains, Reversible/reversed -- the latter ABC should be added) and in implementations such as reversed(), iter() and 'in'. Maybe we should just do this for every ABC in collections.abc (plus Reversible) that has a __subclasshook__ that tests for the presence of the special method.

Oh, probably some more, like Iterator/next, Sized/len and Callable/call. But for those there isn't a fallback in the corresponding bytecode, so I'm only +0 for those -- it doesn't seem to harm anything to be consistent with those for which it does matter, but it doesn't cost much either, and the consistency is slightly useful -- it provides a pattern to follow for future ABCs.

[abarnert]

I propose to solve the narrow problem by indeed supporting the setting of certain special methods to None similar to __hash__. This should be limited to those methods for which this adds value, e.g. where the complete absence of the method causes a fall-back to a different API (e.g. __getitem__+len in the case of __reversed__) -- the None value should just block the fallback

I believe that currently, except for the minor problem in bpo-25864, you can set any special method to None and it _will_ block any fallback (and also block inheritance, of course, the reason it's used with __hash__):

• __iter__ and __reversed__ directly fall back to another method.

• __contains__ falls back to iterating, which will indirectly fall back on __iter__ (or on __getitem__, of course).

• __iadd__ and friends fall back on __add__ and friends.

• __add__ and friends fall back on __radd__ (on the other operand, but typically it will be of the same type).

• __lt__ and friends are similar to __add__.

• __getattr__ and friends are a pile of special cases.

• I think everything else doesn't have a fallback, and would just raise a TypeError anyway--although setting to None is still useful as a way to block inheritance, as used for __hash__.

In all cases, if the primary method exists but is None, the implementation will try to call it, and let the TypeError from NoneType.__call__ escape, which blocks the fallback. (And, of course, it blocks the MRO search for an inherited method.)

Of course the message is the useless and cryptic "'NoneType' object is not callable" rather than the nice one you get from hash. But technically, it's all already working as desired.

We probably need to document that setting special methods to None blocks any fallback implementation (both for people who write such classes, and to ensure that other Pythons do the same thing as CPython).

Practically, I think we need to improve the error message for the ones that are likely to come up, but probably not for all of them. I think this means either just reversed, or reversed+iter, or maybe reversed+iter+in. (Plus hash, of course, but that code is already there.)

Also, I suppose we should have tests for this behavior.

This will require some changes in the ABCs that test for the presence of a given method (Iterable/iter, Container/contains, Reversible/reversed -- the latter ABC should be added) and in implementations such as reversed(), iter() and 'in'. Maybe we should just do this for every ABC in collections.abc (plus Reversible) that has a __subclasshook__ that tests for the presence of the special method.

Now that I think about it, I'm +0.5 on changing all of them. Imagine, as a reader, trying to guess why some do the check and some don't.

Also, I think they should test for None instead of falsiness like Hashable does. (Hopefully nobody ever writes something that's falsey but callable, but why tempt fate?)

---

I'll write a patch that expands the bpo-25864 patch:

• collections.abc.Reversible added (as in bpo-25987).

• All collections.abc.* subclass hooks check for None.

• Mapping.__reversed__ = None.

• One paragraph added to datamodel docs.

• iter, reversed, and 'in' operator all raise a custom TypeError when the special method is None the same way as hash (but everything else will continue to implicitly raise the ugly TypeError).

• Unit tests for None blocking fallback on all special methods I can think of (treating all the __ispam__, add the __rspam__, all inheritance except for __hash__, etc. as a single method each, rather than copying-pasting a half-dozen times).

[gvanrossum]

I like all of that. Thanks!

[abarnert]

Is an hg export patch usable? If not, let me know and I'll flatten it.

Anyway, the attached patch fixes bpo-25987 and bpo-25864 as well as this one, as follows:

• Every ABC in collections.abc that has a subclass hook now treats None as blocking, instead of a few checking falsiness and others not checking anything. (And all the hooks are now identical, which eliminates any unintended inconsistencies.)

• collections.abc.Mapping.__reversed__ = None (fixes bpo-25864).

• collections.abc.Reversible added (fixes bpo-25987).

• iter(), reversed(), and in operator all have custom text in their TypeError instead of the generic "'NoneType' object is not callable".

• Unit tests for all of the above, plus that None blocking also works appropriately (i.e., same as in CPython 2.3-3.6) for __spam__ -> __rspam__, __ispam__ -> __spam__.

• I didn't try to write tests for _everything_ with fallback. For example, blocking fallback from __str__ to __repr__, or anything to do with __attr__, does work, but I can't think of any conceivable case where you'd ever do such a thing. (The fact that you can write a class that isn't repr-able isn't a feature of Python we want to guarantee, it's just a side-effect of other stuff that will hopefully never come up, so it seems wrong to write a unit test to guarantee it.)

• Data Model docs document that you can set a special method to None to block behavior (including blocking fallback to old-style sequence protocol, inheritance from superclass, etc.).

(Note that this patch is not yet fully tested, because my Windows and Cygwin builds are for some reason taking forever. But if I don't post an update tomorrow, that will mean they passed the overnight tests. I doubt anyone was going to commit this tonight anyway, but, just in case...)

[vadmium]

A Mercurial export patch should work with the Reitveld review thing if it is a single revision (and based on a public revision). Or you could try getting a plain diff from the base revision. Or for changes that are independent, separate patches based off a common public revision. Please at least fold your fixup commits into the original commits; that would make it a lot easier to review.

You have a stray print() call, probably in Hashable.__subclasshook__().

For a the do-nothing __reversed__() implementation, do you think “yield from ()” would be clearer than “while False”? Or even “return iter(())”?

What’s the advantage of having Reversed inherit Iterable? How does the subclass hook interact with classes that implement __reversed__() but not __iter__()? I don’t see how the self.assertFalse(issubclass(K, Reversible)) test could pass.

Should the new Reversed class be excluded from collections.__all__, or is it not worth it?

I find the lambda generator a bit quirky in test_Reversible(). Maybe it would be better as an ordinary non-lambda generator with a yield statement.

It would be good to have a versionadded notice for Reversible. I think there should also be one for allowing None for all special methods.

Instead of self.assertEqual(list(reversed(R())), list(reversed([]))), why not check it is equal to an empty list directly?

In test_contains.py, I would either write lambda: 0 in bc, or use the “with self.assertRaises()” form.

Finally, if setting a binary operator method to None means that operation is not available, I find it a bit surprising that doing this prevent the other operand’s method from being tried. I guess you don’t want to change the implementation, but perhaps the documentation of the binary operator methods could be clarified.