bpo-33018: Improve issubclass() error checking and message.

[jab]

If you try something like issubclass('not a class', str), you get a helpful error message that immediately clues you in on what you did wrong:

>>> issubclass('not a class', str)
TypeError: issubclass() arg 1 must be a class

("AHA! I meant isinstance there. Thanks, friendly error message!")

But if you try this with some ABC, the error message is much less friendly!

>>> from some_library import SomeAbc
>>> issubclass('not a class', SomeAbc)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/Cellar/python3/3.6.4_2/Frameworks/Python.framework/Versions/3.6/lib/python3.6/abc.py", line 230, in __subclasscheck__
    cls._abc_negative_cache.add(subclass)
  File "/usr/local/Cellar/python3/3.6.4_2/Frameworks/Python.framework/Versions/3.6/lib/python3.6/_weakrefset.py", line 84, in add
    self.data.add(ref(item, self._remove))
TypeError: cannot create weak reference to 'str' object

("WTF just went wrong?" Several more minutes of head-scratching ensues. Maybe a less experienced Python programmer who hits this hasn't seen weakrefs before and gets overwhelmed, maybe needlessly proceeding down a deep rabbit hole before realizing no knowledge of weakrefs was required to understand what they did wrong.)

Or how about this example:

>>> from collections import Reversible
>>> issubclass([1, 2, 3], Reversible)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/Cellar/python3/3.6.4_2/Frameworks/Python.framework/Versions/3.6/lib/python3.6/abc.py", line 207, in __subclasscheck__
    ok = cls.__subclasshook__(subclass)
  File "/usr/local/Cellar/python3/3.6.4_2/Frameworks/Python.framework/Versions/3.6/lib/python3.6/_collections_abc.py", line 305, in __subclasshook__
    return _check_methods(C, "__reversed__", "__iter__")
  File "/usr/local/Cellar/python3/3.6.4_2/Frameworks/Python.framework/Versions/3.6/lib/python3.6/_collections_abc.py", line 73, in _check_methods
    mro = C.__mro__
AttributeError: 'list' object has no attribute '__mro__'

Here you don't even get the same type of error (AttributeError rather than TypeError), which seems unintentionally inconsistent.

This trivial patch fixes this, and will hopefully save untold numbers of future Python programmers some time and headache.

Let me know if any further changes are required, and thanks in advance for reviewing.

https://bugs.python.org/issue33018

[jab]

Here's a build with this patch applied showing the fix in action:

Python 3.8.0a0 (heads/master-dirty:f0daa880a4, Mar  1 2018, 10:46:05)
[Clang 9.0.0 (clang-900.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from collections.abc import Reversible
>>> issubclass([1, 2, 3], Reversible)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jab/src/cpython/Lib/abc.py", line 143, in __subclasscheck__
    return _abc_subclasscheck(cls, subclass)
TypeError: issubclass() arg 1 must be a class

[zhangyangyu]

Check https://bugs.python.org/issue32999 please, some core devs don't want to limit the first arg only to type objects.

[methane]

Please create new issue and discuss on it.

[jab]

https://bugs.python.org/issue33018

[ilevkivskyi]

Thanks! I think this looks good. Could you please add a NEWS item?

[methane]

Would you revert fc7df0e too?

((PyTypeObject *)subclass)->tp_mro must be valid tuple.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[ilevkivskyi]

Would you revert fc7df0e too?

I don't think we need to revert it. First of all the tests should stay, so I would just make another PR in addition to this one. Or you want to add the change to ->tp_mro to this PR?

[methane]

I don't think we need to revert it. First of all the tests should stay, so I would just make another PR in addition to this one. Or you want to add the change to ->tp_mro to this PR?

Yes. I didn't mean revert adding test.
I want to minimize diff between 3.7 and 3.8 as possible at this time.
It's OK to make another change (before 3.7b3) to use ->tp_mro if it will be merged into 3.7 branch too.

[jab]

Just added the NEWS entry: I have made the requested changes; please review again.

First time (getting a patch accepted (🎉!) and) using blurb. Hopefully that turned out ok.

Thanks for helping to get this landed!

[bedevere-bot]

Thanks for making the requested changes!

@methane, @ilevkivskyi: please review the changes made to this pull request.

[methane]

OK, I will create another PR later.

[ilevkivskyi]

@methane

OK, I will create another PR later.

Thanks! Don't forget to backport your PR to 3.7, as I am backporting this one to 3.7 as @gvanrossum suggested.

[ilevkivskyi]

LGTM, thanks!

[miss-islington]

Thanks @jab for the PR, and @ilevkivskyi for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

[bedevere-bot]

GH-6188 is a backport of this pull request to the 3.7 branch.

[ilevkivskyi]

@jab Congrats on your first contribution!

[jab]

Thanks for making it possible!