Created on 2015-12-26 22:26 by abarnert, last changed 2016-08-18 19:16 by gvanrossum. This issue is now closed.
Serhiy Storchaka raised an issue () with static type hints on #25864 (), 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 (), 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`.`)?
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.
Hash.
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. :)
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).
> I.
> Also,__.)
I believe that currently, except for the minor problem in #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.
>.
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 #25864 patch:
* collections.abc.Reversible added (as in #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).
I like all of that. Thanks!...)
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..
> A Mercurial export patch should work with the Reitveld review thing if it is a single revision (and based on a public revision).
Well, it's 7 separate commits on a work branch, so I could check in each piece and test it separately, and then I just compared the branch to its parent (default) for the export. But, as I said, I can flatten it into a single diff if the review system can't handle this way.
Sorry for making the first patch hard to review, and thanks for reviewing it anyway; I'll fix that with the next one.
> You have a stray print() call, probably in Hashable.__subclasshook__().
Thanks; fixed for the next patch.
> For a the do-nothing __reversed__() implementation, do you think “yield from ()” would be clearer than “while False”? Or even “return iter(())”?
Maybe, but this way is identical to the definition for the do-nothing Iterable.__iter__.
> What’s the advantage of having Reversed inherit Iterable?
This was discussed briefly... somewhere, maybe the -ideas thread?
Pro: If they were independent types, more people would probably register with/inherit from Reversible but not Iterable in error than intentionally.
Pro: One less thing for Sequence to inherit from. This was mentioned in the context of adding both Reversible and HalfSequence (or whatever it ends up being called): "class Sequence(Iterable, Reversible, HalfSequence, Sized, Container) seems a little much; "class Sequence(Reversible, HalfSequence, Sized, Container)" is slightly better.
Con: It's not completely impossible that a type could be reversible but not iterable. For example, you might have a reversible, and even indexable-with-negative-indicies-only, infinite collection of all the negative numbers. (But nobody could think of an actual _useful_ example of such a type.)
Those are all pretty trivial points. When Guido said someone needs to work out where Reversible fits into the hierarchy, and there'd been enough talk without a patch. So I looked at where the discussion seemed to be leaning, couldn't find any additional problems with it when I looked into it further, and did it that way. Guido said he liked it on the Reversible bug, so I did it the same way here.
So, no strong arguments or even feelings from anyone; if you've got one, definitely chime in.
> 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.
Exactly the same way Iterator's handles a class that defines __next__ but not __iter__.
I'll double-check that it actually is implemented right before the next patch (and verify that the tests run), but it should be "return _check_methods(C, "__reversed__", "__iter__").
> Should the new Reversed class be excluded from collections.__all__, or is it not worth it?
It's Reversible, not Reversed. It's in __all__, and I'm pretty sure it should be there--if it's not worth exporting, it's not worth creating in the first place, or documenting, right?
> 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.
This is taken from an identical test in test_Iterable, in the same file.
> It would be good to have a versionadded notice for Reversible.
Definitely; thanks for the catch. I'll add that for the next version of the patch.
> I think there should also be one for allowing None for all special methods.
I'm assuming you mean in collections.abc, not in the data model, right?
The problem is where to put it. The collections.abc docs don't even mention that some of the ABCs have subclass hooks that detect "implicit subclasses", much less tell you which ones do and don't, much less tell you which among those that do treat None specially. Until we add that documentation, there's really nothing to contrast with.
Maybe this means we need another bug where we rewrite the collections.abc docs to include all that information, and in the new documentation we can note what prior versions of Python did (or maybe prior versions of CPython--I doubt any major implementations were different, but without checking them all I wouldn't want to guarantee that)?
> Instead of self.assertEqual(list(reversed(R())), list(reversed([]))), why not check it is equal to an empty list directly?
This might be a case of following the parallelism with test_Iterable too far; I can change it.
> In test_contains.py, I would either write lambda: 0 in bc, or use the “with self.assertRaises()” form.
That makes sense; I'll do it.
>.
I definitely don't want to change the implementation unless there's a very good reason.
Also, if you think it through, from either angle, the way it's always worked makes perfect sense. None (in fact, anything that raises a TypeError when called) always blocks fallback, so it blocks fallback to other.__radd__. Alternatively, the rules for when + looks for __radd__ are very specific and reasonably simple, and TypeError is not one of the things that triggers it.
Making None trigger __radd__ would require making one of those two rules more complicated. And what would the benefit be? If you want to trigger fallback, there's already a simple, well-documented, readable way to do that; we don't need another one.
And I think the note would be more likely to confuse someone who didn't need it (and maybe make him think he should be defining __add__ = None where he really shouldn't) than to help someone who did. Actual reasonable use cases for __add__ = None are rare enough that I think it's OK that someone has to understand what they're doing and be able to think through the rules and/or know what combinations to test via trial and error before they can use it.
But maybe a footnote would work? For example, in the sentence "These functions are only called if the left operand does not support the corresponding operation and the operands are of different types. [2]", we could add another footnote after "does not support the corresponding operation", something like this:
> [2] "Does not support" here means has no such method, or has a method that returns NotImplemented, as described above. Do not assign the method to None if you want to force fallback to the right operand's reflected method--that will instead have the opposite effect of explicitly _blocking_ such fallback.
Again, thanks for all the notes.
I'm regenerating the patch in the hope that it will trigger the code review hook.
Note that setting some special methods (such as __copy__, __deepcopy__, __reduce_ex__, __reduce__, __setstate__, etc) to None has different meaning. The value None is just ignored and corresponding operation is fall back to other methods. For example copy.deepcopy() uses __reduce_ex__ if __deepcopy__ is None, __reduce__ if __reduce_ex__ is None.
May be this should be changed.
FWIW, Martin's review was much more extensive. I'm looking forward to the flat version of your next patch, Andrew!
In response to Serhiy's comment regarding __copy__ etc.: while the
distinction is somewhat unfortunate, I think it's too late to make this
more consistent. I think it's fine that the special methods used by copy
and pickle protocols behave somewhat differently -- that's a totally
different area anyways (and not directly supported by the core language).
In contrast, __hash__, __iter__, __contains__, __reversed__, __iadd__ etc.
are much more core to the language (representing either builtin functions
or operations). Plus here we really need a way to signal the difference
between "not defined here so fall back on either a superclass or a
different protocol" and "defined here as not existing so cause an error
when used". So I don't think there's anything actionable here.
There is already a precedent for None, but it would have been nicer to use NotImplemented.
The idea of using NotImplemented was already discussed (IIR in
python-ideas). I really don't like it.
Needed tests for Hashable, Awaitable, Coroutine, AsyncIterable, AsyncIterator, Iterator, Generator, Sized, Container, Callable.
The patch adds some tests for __iadd__ and __eq__. I think needed tests for a number of other special methods. In particular should be tested a classes without __iadd__, but with __add__ but to None; with __radd__, but with __add__ set to None; without __add__, but with __radd__ set to None; with __eq__, but with __ne__ set to None, etc.
Added other comments on Rietveld.
What if use None and NotImplemented as different signals: "not defined here so fall back on either a superclass or a different protocol", and "not defined here so fail without falling back"?
No. Simply No. Nobody will be able to remember which means which.
True, even I were not sure which should mean which. ;)
> When I manually trigger the code in typeobject.c:5827, I get a segfault;
I'm surprised no test triggered that.
I think this triggered one of Victor's guards, added to catch such sort of errors. In this case the function both raises an exception and returns a result. If not catch such sort of error, it can cause unexpected errors or very strange results during executing unrelated code, so crashing as early as possible is lesser evil.
The second patch takes into account all the issues raised by Martin and Guido, as well as some other changes that didn't make it into the first patch because Windows hates me. And it should be flattened into a single commit, and therefore should hopefully work with Rietveld out of the box. It passes the test suite on Windows and Cygwin (and on OS X, but that's a slightly older version of the changes than what's in this flattened patch).
I think it's still an open question whether Reversible should inherit Iterable. In the current patch, as in the first, it does.
I'll go over Serhiy's Reitveld comments to see if there's anything I missed, and, if so, address it in a third attempt.
On Serhiy's test suggestions:
* I don't think we need to repeat the same tests on every ABC. That seems more likely to introduce copy-paste bugs than to catch anything, especially since the ABCs now all share the same code, but the tests couldn't.
* Likewise for adding repetitive tests on more __spam__, __ispam__, and __rspam__ methods.
* However, he's definitely right that there are other kinds of fallback worth testing, like __ne__ -> __eq__. I'll review the different kinds of fallback more carefully and make sure we have tests for each case.
Without tests you can't be sure that there is no special case in some abstract classes or operators, or that it can't be introduced in future. To decrease the copy-pasting you can use code generation for testing.
But I don't know what is the best place for tests. Tests for special methods are scattered though different files: test_binop, test_class, test_compare, test_descr, test_richcmp, test_augassign, test_contains, test_iter, etc.
The. :)
Uploading a flattened version of patch3.diff.
FWIW, it looks fine to me -- but I'm hoping to get Serhiy's agreement first. Serhiy: feel free to commit when you're happy.
Added few stylistic nitpicks and asked few questions on Rietveld.
I don’t have strong opinions about the Reversible class because I don’t imagine needing it. My instinct was __reverse__() is independent of __iter__(), so it should not be a subclass. But I don’t really mind either way.. Imagine someone using an Orderable virtual base class that tested for __gt__() etc.
If you need, you can write repetitive tests without copying and pasting or generated code:
for [param, result1, result2] in parameters:
with self.subTest(param=param):
...
Maybe it is okay to add a test to ABCTestCase.validate_isinstance() to check that None cancels isinstance().
I also added some Reitveld comments for patch4a.
Style changes based on Martin's review
>.
Are you suggesting that in Python 3.5, it's not defined what happens if you set __reversed__ = None on a sequence-like object and then call reversed()? I think any implementation has to raise a TypeError; the only difference is that we're now documenting the behavior directly, rather than forcing you to infer it from reading a half-dozen different parts of the docs and tracing through the implied behavior. It's already the best way to do it in 3.5, or even 2.5; the problem is that it isn't _obviously_ the best way. And I think a version-changed would send the wrong message: someone might think, "Oh, I can't use None here because I can't require 3.6, so what should I do? Write a method that raises TypeError when called?" (Which will work, but will then make it very hard to write a Reversible implicit ABC if they later want to.)
> Imagine someone using an Orderable virtual base class that tested for __gt__() etc.
That's part of what we're trying to solve. Do you test that with hasattr, like Sized, or do you test for getattr with default None, like Hashable?. Somewhere I think the documentation says you are only allowed to use these names as documented.
>.
As Guido pointed out on the -ideas thread, defining __spam__ = None to block inheritance of a superclass implementation has long been the standard way to mark a class unhashable. So, any Python that raised such an error would break a lot of code.
> Somewhere I think the documentation says you are only allowed to use these names as documented.
I can't find anything that says that. Any idea where to look? That might be worth adding, but if we add it at the same time as (or after) we explicitly document the None behavior, that's not a problem. :)
By the way, did you not review my last patch because you didn't get an email for it? I think Rietveld #439 () may be causing issues to get stalled in the patch stage because people are expecting to get flagged when a new patch goes up but never see it (unless they're on the same mail domain as the patcher).
>..)
The.)
What?
I have manually "rebased" the patch taking into account that part of the work has been done in
Serhiy, Martin could you please review the latest patch and check that everything is OK?
Andrew did a really good job and I would like this to land in 3.6
Oops, sorry, forgot one import, all tests pass with the corrected patch.
The patch LGTM. Do all the tests pass? Who should be attributed in the commit message and the Misc/NEWS item?
New changeset 72b9f195569c by Guido van Rossum in branch 'default':
Anti-registration of various ABC methods.
Pushed, let's see what the buildbots say.
On my machine 8 tests are always skipped:
test_devpoll test_kqueue test_msilib test_ossaudiodev
test_startfile test_winreg test_winsound test_zipfile64
All others tests pass OK.
The main part of the work is by Andrew Barnert, I only introduced small changes due to previous changes in Reversible and few minor things in response to previous comments.
Guido, thank you for applying the patch!