Issue45791
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2021-11-12 05:01 by rhettinger, last changed 2022-04-11 14:59 by admin.
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 29540 | open | serhiy.storchaka, 2021-11-12 17:57 |
Messages (11) | |||
---|---|---|---|
msg406187 - (view) | Author: Raymond Hettinger (rhettinger) * ![]() |
Date: 2021-11-12 05:01 | |
Per PEP 3119, "Overloading works as follows: The call isinstance(x, C) first checks whether C.__instancecheck__ exists, and if so, calls C.__instancecheck__(x) instead of its normal implementation." However, this doesn't work because the isinstance() has a bug introduced in Python 3.1 and no one ever noticed. In abstract.c::object_recursive_isinstance(), we have: PyObject *checker = _PyObject_LookupSpecial(cls, &PyId___instancecheck__); However, that function expects an instance as an argument rather than a class. Calling typeobject.c::_PyObject_LookupSpecial() runs: res = _PyType_LookupId(Py_TYPE(self), attrid); Note, the Py_TYPE(self) is intended to move up from an instance to a class, but we're already started with a class, so it moves to the metaclass instead. This code was correct when originally implemented but it did not have tests: if (name == NULL) { name = PyString_InternFromString("__instancecheck__"); if (name == NULL) return -1; } checker = PyObject_GetAttr(cls, name); if (checker == NULL && PyErr_Occurred()) PyErr_Clear(); ------- Demonstration code --------------- class C: def __instancecheck__(self, inst): raise RuntimeError(f'{self=} {inst=}') class P: def __instancecheck__(self, inst): raise RuntimeError(f'{self=} {inst=}') class C(P): pass >>> isinstance(C(), P) # Incorrectly fails to invoke __instancecheck__ True >>> isinstance(C(), P()) # Incorrectly invokes __instancecheck__ Traceback (most recent call last): File "<pyshell#10>", line 1, in <module> isinstance(C(), P()) File "<pyshell#5>", line 3, in __instancecheck__ raise RuntimeError(f'{self=} {inst=}') RuntimeError: self=<__main__.P object at 0x107586c80> inst=<__main__.C object at 0x107587100> |
|||
msg406191 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * ![]() |
Date: 2021-11-12 07:19 | |
I think that the code is correct, and the documentation is not complete. As in most (but not all) cases of dunder methods it is looked up in a class, ignoring instance attributes. |
|||
msg406193 - (view) | Author: Raymond Hettinger (rhettinger) * ![]() |
Date: 2021-11-12 08:07 | |
> As in most (but not all) cases of dunder methods it > is looked up in a class, ignoring instance attributes. That is true, but the starting point in this case is a class so the attribute lookup should be in that class, not its metaclass. Guido's original code was correct and it became broken in 3.1 in the switch from PyObject_GetAttr(cls, name) to _PyObject_LookupSpecial(cls, &PyId___instancecheck__). Also, the demonstration code I gave (that matches the documentation) used to work in 3.0 and prior. It broken in 3.1. |
|||
msg406219 - (view) | Author: Guido van Rossum (gvanrossum) * ![]() |
Date: 2021-11-12 15:47 | |
I'm torn. It's been like this for a very long time and Serhiy's reasoning makes some sense. OTOH it *does* make more sense as it's written in the PEP. The basic idea is "the class has control over what is considered an instance of it" and it would be nice if this didn't require writing a metaclass. (Metaclasses are often problematic.) As an experiment, could you implement the proposed behavior and see what fails in e.g. the stdlib or the test suites of the 100 most popular packages? |
|||
msg406222 - (view) | Author: Raymond Hettinger (rhettinger) * ![]() |
Date: 2021-11-12 17:10 | |
FWIW, I discovered the issue when experimenting with ways to use the class pattern in structural pattern matching. --- Code that should work but doesn't ----------- class Warm: def __instancecheck__(cls, inst): return inst in {'red', 'orange', 'blue'} match 'red': case Warm(): # This doesn't match but should print('Glowing') --- What you have to do to get it to work ------- class MetaWarm(type): def __instancecheck__(cls, inst): return inst in {'red', 'orange', 'blue'} class Warm(metaclass=MetaWarm): pass match 'red': case Warm(): # This matches print('Hot') |
|||
msg406226 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * ![]() |
Date: 2021-11-12 18:00 | |
Failed tests: ====================================================================== FAIL: test_isinstance_with_or_union (test.test_isinstance.TestIsInstanceIsSubclass) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/serhiy/py/cpython/Lib/test/test_isinstance.py", line 223, in test_isinstance_with_or_union with self.assertRaises(TypeError): ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: TypeError not raised ====================================================================== FAIL: test_isinstance (test.test_genericalias.BaseTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/serhiy/py/cpython/Lib/test/test_genericalias.py", line 249, in test_isinstance with self.assertRaises(TypeError): ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: TypeError not raised ====================================================================== FAIL: test_issubclass (test.test_genericalias.BaseTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/serhiy/py/cpython/Lib/test/test_genericalias.py", line 255, in test_issubclass with self.assertRaises(TypeError): ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: TypeError not raised ====================================================================== FAIL: test_callable_instance_type_error (test.test_typing.CollectionsCallableTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/serhiy/py/cpython/Lib/test/test_typing.py", line 490, in test_callable_instance_type_error with self.assertRaises(TypeError): ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: TypeError not raised ====================================================================== FAIL: test_self_subclass (test.test_typing.CollectionsCallableTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/serhiy/py/cpython/Lib/test/test_typing.py", line 446, in test_self_subclass with self.assertRaises(TypeError): ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: TypeError not raised ====================================================================== FAIL: test_special_method_lookup (test.test_descr.ClassPropertiesAndMethods) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/serhiy/py/cpython/Lib/test/test_descr.py", line 2097, in test_special_method_lookup runner(X()) ^^^^^^^^^^^ File "/home/serhiy/py/cpython/Lib/test/test_descr.py", line 2032, in do_isinstance return isinstance(int, obj) ^^^^^^^^^^^^^^^^^^^^ File "/home/serhiy/py/cpython/Lib/test/test_descr.py", line 2077, in __getattribute__ test.fail("__getattribute__ called with {0}".format(attr)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: __getattribute__ called with __instancecheck__ ---------------------------------------------------------------------- |
|||
msg406227 - (view) | Author: Raymond Hettinger (rhettinger) * ![]() |
Date: 2021-11-12 18:01 | |
I did a scan of the standard library and code in the wild. It looks like almost all uses are in metaclasses (which makes sense because that matches the AppendableSequence example in PEP 3119). However, the typing module does have some cases of __instancecheck__ being used in regular classes. AFAICT that code is completely inoperative and there is no way for it to ever get called by isinstance(). I think people who __instancecheck__ in regular classes aren't noticing that their code isn't being called at all. The likely reasons are that isinstance() has a fast path for exact type matches and type() defines an __instancecheck__ in a reasonable way. So unless someone tests a case not already covered by those two paths, they won't notice the dead code. The two cases in typing.py are _Final and _BaseGenericAlias. The __instancecheck__ methods in those were written by Ivan and by GvR likely with the expectation that that code would by called by isinstance(). |
|||
msg406229 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * ![]() |
Date: 2021-11-12 18:10 | |
Most of failed tests are related to typing, but there is a purposed test for __instancecheck__ and __subclasscheck__ bypassing __getattr__ and __getattribute__ (test.test_descr.ClassPropertiesAndMethods.test_special_method_lookup). |
|||
msg406230 - (view) | Author: Raymond Hettinger (rhettinger) * ![]() |
Date: 2021-11-12 18:43 | |
Here's an example in PEP 3119 that appears to work but actually bypasses the method: from abc import ABCMeta, abstractmethod class Sized(metaclass=ABCMeta): @abstractmethod def __hash__(self): return 0 @classmethod def __instancecheck__(cls, x): print('Called') return hasattr(x, "__len__") @classmethod def __subclasscheck__(cls, C): return hasattr(C, "__bases__") and hasattr(C, "__len__") $ python3.10 -i demo_3119.py >>> isinstance([], Sized) True |
|||
msg406237 - (view) | Author: Guido van Rossum (gvanrossum) * ![]() |
Date: 2021-11-12 20:35 | |
I believe that the PEP 3119 example doesn't work (I've confirmed something simpler) but I have a feeling that the uses of __instancecheck__ in typing.py are actually okay. For example, isinstance(42, typing.Any) correctly calls _SpecialForm.__instancecheck__ -- because Any is not a type, it's an instance of typing._SpecialForm so it all works out correctly. (I haven't checked out the other -- @Serhiy do you have time to look into that one?) |
|||
msg406245 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * ![]() |
Date: 2021-11-12 22:08 | |
All typing related tests are fixed by updating types.GenericAlias.__getattribute__. So the only thing is left to make all tests passed is removing some special methods tests. The lookup of __instancecheck__ and __subclasscheck__ was changed by Benjamin in fb6fb062e8f677dd63943f3a4b8a45c6665b3418. It looks like deliberate change. Benjamin, do you remember the reasons of the original change? |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:59:52 | admin | set | github: 89949 |
2021-11-13 15:44:36 | brandtbucher | set | nosy:
+ brandtbucher |
2021-11-12 22:08:57 | serhiy.storchaka | set | nosy:
+ benjamin.peterson messages: + msg406245 |
2021-11-12 20:35:44 | gvanrossum | set | messages: + msg406237 |
2021-11-12 18:43:26 | rhettinger | set | messages: + msg406230 |
2021-11-12 18:10:36 | serhiy.storchaka | set | messages: + msg406229 |
2021-11-12 18:01:26 | rhettinger | set | messages: + msg406227 |
2021-11-12 18:00:34 | serhiy.storchaka | set | messages: + msg406226 |
2021-11-12 17:57:10 | serhiy.storchaka | set | keywords:
+ patch stage: patch review pull_requests: + pull_request27789 |
2021-11-12 17:10:57 | rhettinger | set | messages: + msg406222 |
2021-11-12 16:26:52 | rhettinger | set | title: __instancecheck__ being checked of type(cls) instead of cls -> __instancecheck__ being checked on type(cls) instead of cls |
2021-11-12 15:47:41 | gvanrossum | set | messages: + msg406219 |
2021-11-12 08:07:36 | rhettinger | set | nosy:
+ gvanrossum messages: + msg406193 |
2021-11-12 07:19:57 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages: + msg406191 |
2021-11-12 05:01:11 | rhettinger | create |