python / cpython

The Python programming language
https://www.python.org
Other
62.63k stars 30.05k forks source link

`isinstance` on multiple-inheritance class erases type hints #124840

Open coretl opened 2 hours ago

coretl commented 2 hours ago

Bug report

Bug description:

Steps to reproduce:

from abc import abstractmethod
from typing import Protocol, get_type_hints, runtime_checkable

@runtime_checkable
class Hello(Protocol):
    @abstractmethod
    def hello(self) -> str: ...

class Base:
    a: str

class Base2:
    def hello(self) -> str:
        return "hello"

class Thing(Base, Base2): ...

def test_hints():
    t = Thing()
    assert get_type_hints(t) == {"a": str}
    assert isinstance(t, Hello)
    assert get_type_hints(t) == {"a": str}

CPython versions tested on:

3.11, 3.13

Operating systems tested on:

Linux

coretl commented 2 hours ago

Note that if you move the hello method from Base2 into Thing then the problem goes away

Eclips4 commented 2 hours ago

Hello! Thanks for the report. That's interesting issue because on current main it raises an AssertionError on 20th line:

Traceback (most recent call last):
  File "/Users/admin/Projects/cpython/example.py", line 25, in <module>
    test_hints()
    ~~~~~~~~~~^^
  File "/Users/admin/Projects/cpython/example.py", line 20, in test_hints
    assert get_type_hints(t) == {"a": str}
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
AlexWaygood commented 1 hour ago

Right -- I can reproduce the issue exactly as reported on Python 3.11, 3.12 and 3.13, but as @Eclips4 says, on Python 3.14 both assertions fail (get_type_hints(t) is {} before and after the isinstance() call). I'm guessing the change on 3.14 is due to some annotationlib-related refactoring? (Cc. @JelleZijlstra.)

The <=3.13 behaviour is pretty weird, regardless of whether the 3.14+ change is a feature or a bug.

sobolevn commented 1 hour ago

Probably this is related to this paragraph in https://peps.python.org/pep-0649/#backwards-compatibility-with-stock-semantics

The original implementation of class annotations had what can only be called a bug: 
if a class didn’t define any annotations of its own, but one of its base classes did define annotations, the class would “inherit” those annotations.
This behavior was never desirable, so user code found a workaround: instead of accessing the annotations on the class directly via cls.__annotations__,
code would access the class’s annotations via its dict as in cls.__dict__.get("__annotations__", {}).
This idiom worked because classes stored their annotations in their __dict__, and accessing them this way avoided the lookups in the base classes.
JelleZijlstra commented 1 hour ago

This is definitely a bug. I will take a look.

@sobolevn that paragraph is about accessing .__annotations__ directly, not about get_type_hints(). get_type_hints() is expected to gather annotations from all base classes.

AlexWaygood commented 1 hour ago

On Python <3.14, this is why the isinstance() call affects the output of get_type_hints:

>>> from abc import abstractmethod
>>> from typing import *
>>> @runtime_checkable
... class Hello(Protocol):
...     @abstractmethod
...     def hello(self) -> str: ...
... 
>>> class Base:
...     a: str
... 
>>> class Base2:
...     def hello(self) -> str:
...         return "hello"
... 
>>> class Thing(Base, Base2): ...
... 
>>> import pprint
>>> pprint.pp({cls.__name__: cls.__dict__.get("__annotations__") for cls in Thing.__mro__})
{'Thing': None, 'Base': {'a': <class 'str'>}, 'Base2': None, 'object': None}
>>> {cls.__name__: cls.__dict__.get("__annotations__") for cls in Thing.__mro__}
{'Thing': None, 'Base': {'a': <class 'str'>}, 'Base2': None, 'object': None}
>>> Thing().__annotations__
{'a': <class 'str'>}
>>> isinstance(Thing(), Hello)
True
>>> {cls.__name__: cls.__dict__.get("__annotations__") for cls in Thing.__mro__}
{'Thing': {}, 'Base': {'a': <class 'str'>}, 'Base2': None, 'object': None}
>>> Thing().__annotations__
{}

Here's a simpler repro of the <py314 behaviour, that doesn't involve runtime_checkable protocols or multiple inheritance:

>>> from typing import get_type_hints
>>> class A:
...     a: str
... 
>>> class B(A): pass
... 
>>> print(B.__dict__.get("__annotations__"))
None
>>> get_type_hints(B())
{'a': <class 'str'>}
>>> B.__annotations__  # this attribute access mutates B's __dict__
{}
>>> print(B.__dict__.get("__annotations__"))
{}
>>> get_type_hints(B())
{}
AlexWaygood commented 1 hour ago

The inconsistency on Python <3.14 seems like it's because get_type_hints isn't really trying at all to differentiate between instance attributes and class attributes. If an instance has an instance attribute __annotations__ set on it, then I think it's correct for get_type_hints to just naively believe it, like it currently does. But if accessing __annotations__ on an instance ends up resolving it to an attribute on the class, then we should probably iterate through the mro and combine the __annotations__ dictionaries, similar to how we do for class objects.

Put differently: for a given class Foo, it seems wrong that get_type_hints(Foo()) should ever give a different result to get_type_hints(Foo), unless instances of Foo have per-instance __annotations__ attributes that override any __annotations__ attributes on the class Foo or any of Foo's superclasses.

JelleZijlstra commented 1 hour ago

@AlexWaygood found the main thing that makes this snippet behave oddly: you're calling get_type_hints() on an instance instead of a class.

That's also why it behaves yet differently on Python 3.14. Now, classes don't have an __annotations__ key in their class namespace until you access the __annotations__ attribute.

I'm not sure it's feasible to change this behavior.

coretl commented 1 hour ago

@AlexWaygood found the main thing that makes this snippet behave oddly: you're calling get_type_hints() on an instance instead of a class.

Thanks, doing get_type_hints(type(t)) works for me, happy to close this.