New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve issubclass() error checking and message #77199
Comments
Creating this issue by request of INADA Naoki to discuss my proposed patch in #5944. Copy/pasting from that PR: 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. |
Why But we use "duck typing". In this case, if the object (a) supports weakref and (2) has __mro__ and it is tuple, we treat the object as a class. And when raising TypeError, information about which assumption was broken may be useful. So I'm -1 on adding such strong check or removing internal information without meta programming expert advice. |
ABC.register() has an explicit check, and it is mentioned in PEP-3119. The point here is not to change issubclass(), but to change ABC.__subclasscheck__(). It may conceivably have stricter requirements than issubclass() has. But certainly an advice from actual ABC users would be nice. |
issubclass(class-like, class-like) is allowed. |
Hmm, normal class doesn't support issubclass(class-like. class).
OK, problem is ABC should support it or not. |
I do not see any point in allowing non-types in ABCMeta.__subclasscheck__. Currently, ABCs are clearly not designed to support non-types:
|
Yes. While ABC.register() and issubclass() have different users (e.g. ABC.register() will be used by framework author, and issubclass will be used by framework users), it's positive reason to remove non-type support.
Isn't it just a limitation?
What "by OP" means?
Yes, as I commented above.
Of course. Personally speaking, I dislike magics including ABC, __subclasscheck__, overwriting __class__ with dummy object. So I'm OK to limit the ability of it. But I don't know much about how mages use ABC. I need mages comment before merging the pull request. BTW, do you think it should be backported to 3.7, or even 3.6? |
Formally, there is no implication. It is the abc module authors who know the truth. But I can't imagine why anybody would impose such a limitation by design, because while instances of user-defined classes support weakrefs, built-in classes used by everybody like tuple, list and dict don't. That's why I guessed that non-types were not meant to be supported.
Sorry for being unclear. There is no explicit check as you say, but __mro__ is directly accessed (see msg313376). But it may probably be considered "duck typing" too.
For 3.6, I'd listen to ABC users/experts. Might raising a TypeError instead of returning False from issubclass(user_defined_obj, ABC) break something important? Personally, I think it would mostly expose bugs and not hinder reasonable usage.
Seems like it can, but the test should survive in some form :) |
Even if subclass() will check explicitly that its first argument is a type, ABC.__subclasscheck__() can be called directly, and it shouldn't crash when called with non-type. |
PR 5944 changes ABC.__subclasscheck__ (not issubclass) to check its first argument, so if it's merged there will be no crash even with the revert. |
Of course, issubclass(42, AnyABC) must raise TypeError. They aren't class-like object. I didn't discuss on it. I talked about class-like objects. import typing
import collections.abc as cabc
print(issubclass(typing.MutableMapping, cabc.Mapping)) # Python 3.7 raises TypeError I don't think it's real problem. But if someone claims it's real issue, we can make typing.MutableMapping more "class-like" by adding __mro__. diff --git a/Lib/typing.py b/Lib/typing.py
index 7ca080402e..2edaa3f868 100644
--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -619,6 +619,7 @@ def __init__(self, origin, params, *, inst=True, special=False, name=None):
a for a in params)
self.__parameters__ = _collect_type_vars(params)
self.__slots__ = None # This is not documented.
+ self.__mro__ = (origin,) + getattr(origin, '__mro__', (object,))
if not name:
self.__module__ = origin.__module__ Again, I don't think it's a real problem. |
To be honest I am still undecided on this. In principle, I am OK with status quo, but I am also OK, with the PR that will prohibit non-classes. I am a bit worried that it may break some existing code, so it is probably not for 3.7. |
Regarding status quo (expanding the examples of @inada.naoki and @jab): >>> import typing
>>> import collections.abc as cabc
>>> issubclass(typing.Mapping, cabc.Mapping)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/izbyshev/workspace/cpython/Lib/abc.py", line 143, in __subclasscheck__
return _abc_subclasscheck(cls, subclass)
TypeError: issubclass() arg 1 must be a class
>>> from abc import ABC
>>> issubclass(typing.Mapping, ABC)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/izbyshev/workspace/cpython/Lib/abc.py", line 143, in __subclasscheck__
return _abc_subclasscheck(cls, subclass)
File "/home/izbyshev/workspace/cpython/Lib/abc.py", line 143, in __subclasscheck__
return _abc_subclasscheck(cls, subclass)
File "/home/izbyshev/workspace/cpython/Lib/contextlib.py", line 30, in __subclasshook__
return _collections_abc._check_methods(C, "__enter__", "__exit__")
File "/home/izbyshev/workspace/cpython/Lib/_collections_abc.py", line 73, in _check_methods
mro = C.__mro__
File "/home/izbyshev/workspace/cpython/Lib/typing.py", line 706, in __getattr__
raise AttributeError(attr)
AttributeError: __mro__
>>> ABC.register(int)
<class 'int'>
>>> issubclass(typing.Mapping, ABC)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/izbyshev/workspace/cpython/Lib/abc.py", line 143, in __subclasscheck__
return _abc_subclasscheck(cls, subclass)
TypeError: issubclass() arg 1 must be a class
>>> typing.Mapping.__mro__ = ()
>>> issubclass(typing.Mapping, ABC)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/izbyshev/workspace/cpython/Lib/abc.py", line 143, in __subclasscheck__
return _abc_subclasscheck(cls, subclass)
TypeError: issubclass() arg 1 must be a class
>>> typing.Mapping.__bases__ = ()
>>> issubclass(typing.Mapping, ABC)
False Can't say that I'm OK with it :) I'm for forbidding non-types in ABCMeta.__subclasscheck__, but if we are to add clean support for "class-likes" instead, I think that "class-like" objects should be clearly defined, for example, that they must have __mro__ and __bases__ (and probably support weakrefs unless we want to skip caching if they don't). ABCMeta.__subclasscheck__ is not standalone: it relies on issubclass() and __subclasshook__, and both of them have some expectations in practice. |
My current opinion is:
|
I agree except that I'd like to see it in 3.7 too. |
I'll share the use case that prompted me to submit this PR in the first place. I am the author of bidict (https://pypi.python.org/pypi/bidict), which provides a bidirectional dict class. A bidict is just like a dict, except it automatically maintains its inverse bidict, which is accessible via its .inv attribute. To prevent a bidict and its inverse from creating a strong reference cycle, a weak ref is used to store the reference one direction. bidicts implement my BidirectionalMapping ABC, which extends collections.abc.Mapping to include the .inv abstractproperty. BidirectionalMapping overrides __subclasshook__ so that outside implementations that don't subclass it explicitly may still be considered subclasses. Recently, I tried something like I experimented more and noticed that The exceptions that are raised in these cases seem like an abstraction leak. The error messages do not help users immediately realize what they did wrong and how they can fix it; more knowledge of internals is required to make sense of what's going on than should be needed. The inconsistency in these errors is a further problem. The same mistake should not cause three different errors unless there is some really good reason. This seems unintentional. Can any of the original authors say whether this is working as intended or if this is in fact an oversight? The current behavior causes confusion for both less experienced and more experienced Python users alike. (Would anyone else here have correctly predicted all of the different errors that the examples above cause? How many other Python experts could have?) For less experienced users, Python's general consistency and predictability, lack of gotchas, and good errors are some of its best features. This is such an exception that it seems like a bug. I'm happy for some other patch than the one I submitted in #5944 to land if necessary, as long as something fixes this. And fwiw, +1 for 3.7, unless anyone can demonstrate any credible risk. Thanks for your consideration :) |
I am still -1 on changing this in Python 3.7, unless Guido wants this in 3.7, if yes, then we can go ahead. Otherwise, I think we can consider just merging this into master, in this case I would switch to the PR to discuss the details. |
While we're in feature freeze mode, we're not in release candidate mode |
If there are some code which depend on ABC.__subclasscheck__() allow class-like object, we'll have to revert it in 3.7b4 or rc. |
Hmm... That is actually a not entirely imaginary risk, right? Can you come |
OTOH if we don't do this now, it's not going to be any easier to make this |
OK, I played with this a bit and it looks good. There is however a merge conflict now, and a NEWS item is missing. I will leave a comment in the PR. |
I am closing this for now. We can re-open it later if problems will appear in 3.7. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: