__instancecheck__ being checked on type(cls) instead of cls

[rhettinger]

BPO	45791
Nosy	@gvanrossum, @rhettinger, @benjaminp, @serhiy-storchaka, @brandtbucher
PRs	gh-89949: Look up __instancecheck__ and __subclasscheck__ in instance #29540

^{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:

assignee = None
closed_at = None
created_at = <Date 2021-11-12.05:01:11.036>
labels = ['interpreter-core', '3.9', '3.10', '3.11']
title = '__instancecheck__ being checked on type(cls) instead of cls'
updated_at = <Date 2021-11-13.15:44:36.246>
user = 'https://github.com/rhettinger'

bugs.python.org fields:

activity = <Date 2021-11-13.15:44:36.246>
actor = 'brandtbucher'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2021-11-12.05:01:11.036>
creator = 'rhettinger'
dependencies = []
files = []
hgrepos = []
issue_num = 45791
keywords = ['patch']
message_count = 11.0
messages = ['406187', '406191', '406193', '406219', '406222', '406226', '406227', '406229', '406230', '406237', '406245']
nosy_count = 5.0
nosy_names = ['gvanrossum', 'rhettinger', 'benjamin.peterson', 'serhiy.storchaka', 'brandtbucher']
pr_nums = ['29540']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue45791'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

[rhettinger]

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>

[serhiy-storchaka]

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.

[rhettinger]

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.

[gvanrossum]

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?

[rhettinger]

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