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.

classification
Title: __instancecheck__ being checked on type(cls) instead of cls
Type: Stage: patch review
Components: Interpreter Core Versions: Python 3.11, Python 3.10, Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, brandtbucher, gvanrossum, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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:52adminsetgithub: 89949
2021-11-13 15:44:36brandtbuchersetnosy: + brandtbucher
2021-11-12 22:08:57serhiy.storchakasetnosy: + benjamin.peterson
messages: + msg406245
2021-11-12 20:35:44gvanrossumsetmessages: + msg406237
2021-11-12 18:43:26rhettingersetmessages: + msg406230
2021-11-12 18:10:36serhiy.storchakasetmessages: + msg406229
2021-11-12 18:01:26rhettingersetmessages: + msg406227
2021-11-12 18:00:34serhiy.storchakasetmessages: + msg406226
2021-11-12 17:57:10serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request27789
2021-11-12 17:10:57rhettingersetmessages: + msg406222
2021-11-12 16:26:52rhettingersettitle: __instancecheck__ being checked of type(cls) instead of cls -> __instancecheck__ being checked on type(cls) instead of cls
2021-11-12 15:47:41gvanrossumsetmessages: + msg406219
2021-11-12 08:07:36rhettingersetnosy: + gvanrossum
messages: + msg406193
2021-11-12 07:19:57serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg406191
2021-11-12 05:01:11rhettingercreate