python / cpython

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

`_GenericAlias` caches origin's `__module__` #123250

Open A5rocks opened 3 weeks ago

A5rocks commented 3 weeks ago

Bug report

Bug description:

When subscripting a class, __module__ gets saved. This prevents fixing it up later:

https://github.com/python/cpython/blob/4c3f0cbeaec0d49212d305618743fabb0e74a696/Lib/typing.py#L1411

from typing import Generic, TypeVar
T = TypeVar("T")

class A(Generic[T]):
  pass

x = A[T]  # this originally was in ... somewhere, but is here for minimization
A.__module__ = "new_module"
assert x.__module__ == "new_module"  # this is going to raise AssertionError

This came up when updating the Sphinx version in trio due to our handling of private/public. We update __module__ for these classes in __init__ (to make Sphinx work, among other things) but this doesn't end up updating any stored _GenericAliass' __module__s.

CPython versions tested on:

3.11

Operating systems tested on:

Windows

sobolevn commented 3 weeks ago

The same happens with types.GenericAlias:

>>> class A[T]:
...     ...

>>> x = A[int]
>>> A.__module__ = 'example'
>>> x.__module__
'__main__'

My idea was to add something like this to typing._GenericAlias:

    @property
    def __module__(self):
        if not self._name:
            return self.__origin__.__module__
        return self.__class__.__module__

    @__module__.setter
    def __module__(self, module):
        self.__class__.__module__ = module

It actually passes all tests. But, we also need to change types.GenericAlias in this case.

We can try adding __module__ to the list of exceptions:

static const char* const attr_exceptions[] = {
    "__class__",
    "__bases__",
    "__origin__",
    "__module__", // ...

All tests pass. I am not sure if that's 100% correct.

Let's evaluate ideas, and I can send a PR with the fix.

A5rocks commented 3 weeks ago

The setter should probably update the origin's __module__ if it reads from there? (Kind of weird but probably more appropriate behavior)


No other comments though!

sobolevn commented 3 weeks ago

I tested this more, it turns out to be not that easy. There are lots of corner cases that I didn't account for. For example, some cases involve using cls.__module__ checks, which would resolve in <property ...>

A5rocks commented 2 weeks ago

What specifically uses cls.__module__ checks? Wouldn't that be kinda weird cause cls is _GenericAlias?