[issue25958] Implicit ABCs have no means of "anti-registration"

[Mariatta Wijaya]

Changes by Mariatta Wijaya :


--
pull_requests:  -2123

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Mariatta Wijaya]

Mariatta Wijaya added the comment:


New changeset 753422f6e32e13d96319b090788f0474f1e21fc4 by Mariatta in branch 
'3.6':
bpo-30266: support "= None" pattern in AbstractContextManager (GH-1448) 
(GH-2054)
https://github.com/python/cpython/commit/753422f6e32e13d96319b090788f0474f1e21fc4


--
nosy: +Mariatta

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Mariatta Wijaya]

Changes by Mariatta Wijaya :


--
pull_requests: +2123

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Mariatta Wijaya]

Changes by Mariatta Wijaya :


--
pull_requests:  -2115

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Mariatta Wijaya]

Changes by Mariatta Wijaya :


--
pull_requests: +2115

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:


New changeset 57161aac5eb9bcb0b43e551a1937ff0a84c1ec52 by Guido van Rossum 
(Jelle Zijlstra) in branch 'master':
bpo-30266: support "= None" pattern in AbstractContextManager (#1448)
https://github.com/python/cpython/commit/57161aac5eb9bcb0b43e551a1937ff0a84c1ec52


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Changes by Guido van Rossum :


--
stage: commit review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Ivan Levkivskyi]

Ivan Levkivskyi added the comment:

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

Pushed, let's see what the buildbots say.

--
resolution:  -> fixed
stage: patch review -> commit review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Roundup Robot]

Roundup Robot added the comment:

New changeset 72b9f195569c by Guido van Rossum in branch 'default':
Anti-registration of various ABC methods.
https://hg.python.org/cpython/rev/72b9f195569c

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

The patch LGTM. Do all the tests pass? Who should be attributed in the commit 
message and the Misc/NEWS item?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Ivan Levkivskyi]

Ivan Levkivskyi added the comment:

Oops, sorry, forgot one import, all tests pass with the corrected patch.

--
Added file: http://bugs.python.org/file44123/abarnert_rebased2.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Ivan Levkivskyi]

Ivan Levkivskyi added the comment:

I have manually "rebased" the patch taking into account that part of the work 
has been done in http://bugs.python.org/issue25987

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

--
Added file: http://bugs.python.org/file44122/abarnert_rebased.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Ivan Levkivskyi]

Ivan Levkivskyi added the comment:

What holds back this issue now?

It looks like Andrew did a great job.
When I was making a patch for #25987 following Andrew's ideas, I was surprised 
how many different idioms has been used for __subclasshook__.
I was going to open an issue on this, when I noticed a week ago that there is 
already this issue.

Is any help needed here?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Ivan Levkivskyi]

Changes by Ivan Levkivskyi :


--
nosy: +levkivskyi

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Ethan Furman]

Changes by Ethan Furman :


--
nosy: +ethan.furman

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Martin Panter]

Martin Panter added the comment:

The documentation about double-underscore names: 
.
 It says any undocumented usage may be broken. The technique with __hash__() is 
already documented: 
.

I have seen your 5th patch, and do appreciate the new improvements. (I think I 
did see an email for it. But a while ago G Mail started treating a lot of stuff 
as spam until I made a special rule for bug tracker emails.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

> This is not really my area of expertise, but I would have thought if you 
> defined a __special__ method to something illegal (non-callable, or wrong 
> signature) it would be reasonable for Python to raise an error at class 
> definition (or assignment) time, not just later when you try to use it.

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 
(http://psf.upfronthosting.co.za/roundup/meta/issue439) 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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

> This is not really my area of expertise, but I would have thought if you 
> defined a __special__ method to something illegal (non-callable, or wrong 
> signature) it would be reasonable for Python to raise an error at class 
> definition (or assignment) time, not just later when you try to use it.

No, that's not the intention.

> Somewhere I think the documentation says you are only allowed to use these 
> names as documented.

Indeed, but it's not enforced. What it means is that when the next
release of Python (or a different implementation) changes the meaning
of a __special__ name, you can't complain that your code broke.

(And please don't go suggesting that we start enforcing it.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Martin Panter]

Martin Panter added the comment:

This is not really my area of expertise, but I would have thought if you 
defined a __special__ method to something illegal (non-callable, or wrong 
signature) it would be reasonable for Python to raise an error at class 
definition (or assignment) time, not just later when you try to use it. 
Somewhere I think the documentation says you are only allowed to use these 
names as documented.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

> I did actually mean a version changed notice for the data model change. I see 
> this as a small expansion of the Python object API. Previously, 
> __reversed__() had to be a function, now you are also allowed to set it to 
> None. The collections ABCs are just catching up with the API change.

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Changes by Andrew Barnert :


Added file: http://bugs.python.org/file41651/patch5.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Changes by Andrew Barnert :


Removed file: http://bugs.python.org/file41650/patch5.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

Style changes based on Martin's review

--
Added file: http://bugs.python.org/file41650/patch5.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Martin Panter]

Martin Panter added the comment:

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.

I did actually mean a version changed notice for the data model change. I see 
this as a small expansion of the Python object API. Previously, __reversed__() 
had to be a function, now you are also allowed to set it to None. The 
collections ABCs are just catching up with the API change. 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Changes by Andrew Barnert :


Added file: http://bugs.python.org/file41527/patch4a.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[R. David Murray]

Changes by R. David Murray :


--
nosy:  -r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Added few stylistic nitpicks and asked few questions on Rietveld.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

Uploading a flattened version of patch3.diff.

--
Added file: http://bugs.python.org/file41519/patch3-regenerated.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

The attached patch fixes the one wording change from the patch2 review, and 
makes all the error messages consistent as suggested by Serhiy, and also adds a 
lot more tests: every way that a special method can fall back should now be 
tested. (Of course it would be good if someone else went through and made sure 
I didn't miss anything.) However:

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

Yes, but there's clearly a cost to maintaining and running 13 copies of each 
__ispam__ test, and 20 copies of each __rspam__ test. And the benefit is 
minuscule--if all 13 methods share the same code; it's far more likely that 
someone will break the __isub__ test than that they'll break the __isub__ code.

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

Yeah, they pretty much have to be scattered around the code. As they are in the 
attached diff. But I don't think that's a huge problem, except for the poor sap 
who has to review the patch. :)

--
Added file: http://bugs.python.org/file41511/patch3.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

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.

--
Added file: http://bugs.python.org/file41509/patch2.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

No. Simply No. Nobody will be able to remember which means which.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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"?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

The idea of using NotImplemented was already discussed (IIR in
python-ideas). I really don't like it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Raymond Hettinger]

Raymond Hettinger added the comment:

There is already a precedent for None, but it would have been nicer to use 
NotImplemented.

--
nosy: +rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

FWIW, Martin's review was much more extensive. I'm looking forward to the flat 
version of your next patch, Andrew!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

I'm regenerating the patch in the hope that it will trigger the code review 
hook.

--
Added file: http://bugs.python.org/file41508/abarnert-patch-regenerated.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

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

> 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 
> co

[issue25958] Implicit ABCs have no means of "anti-registration"

[Martin Panter]

Martin Panter added the comment:

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.

--
stage:  -> patch review
type:  -> enhancement

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

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

Anyway, the attached patch fixes #25987 and #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 #25864).

 * collections.abc.Reversible added (fixes #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...)

--
keywords: +patch
Added file: http://bugs.python.org/file41502/patch.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

I like all of that. Thanks!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

> 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 #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 #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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Guido van Rossum]

Guido van Rossum added the comment:

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.

--
nosy: +gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

> 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 #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__.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[R. David Murray]

R. David Murray added the comment:

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

--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Martin Panter]

Martin Panter added the comment:

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. :)

--
nosy: +martin.panter

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Nick Coghlan]

Changes by Nick Coghlan :


--
nosy: +ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

Andrew Barnert added the comment:

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`)?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25958] Implicit ABCs have no means of "anti-registration"

[Andrew Barnert]

New submission from Andrew Barnert:

Serhiy Storchaka raised an issue (http://bugs.python.org/msg256910) with static 
type hints on #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 (https://github.com/ambv/typehinting/issues/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`.

--
components: Library (Lib)
messages: 257052
nosy: abarnert
priority: normal
severity: normal
status: open
title: Implicit ABCs have no means of "anti-registration"
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com