Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class attribute inheritance regression in 1.1.324 #5792

Closed
bellini666 opened this issue Aug 23, 2023 · 2 comments
Closed

Class attribute inheritance regression in 1.1.324 #5792

bellini666 opened this issue Aug 23, 2023 · 2 comments
Labels
as designed Not a bug, working as intended bug Something isn't working

Comments

@bellini666
Copy link

This is a minimal reproduction to the issue I'm seeing in my django projects:

from django.db import models


class SomeModel(models.Model):
    ...


reveal_type(SomeModel.objects)


class OtherModelManager(models.Manager["OtherModel"]):
    ...


class OtherModel(models.Model):
    objects: OtherModelManager = OtherModelManager()


reveal_type(OtherModel.objects)

pip install django django-types to test this code

Before 1.1.323 this, this would be the output of pyright for this code:

❯ pyright test.py         
/tmp/test/test.py
  /tmp/test/test.py:8:13 - information: Type of "SomeModel.objects" is "BaseManager[SomeModel]"
  /tmp/test/test.py:19:13 - information: Type of "OtherModel.objects" is "OtherModelManager"
0 errors, 0 warnings, 2 informations

On 1.1.324 this is the output:

/tmp/test/test.py
  /tmp/test/test.py:8:13 - information: Type of "SomeModel.objects" is "BaseManager[SomeModel]"
  /tmp/test/test.py:19:13 - information: Type of "OtherModel.objects" is "BaseManager[OtherModel]"
0 errors, 0 warnings, 2 informations

OtherModel.objects is ignoring the objects override in it and using its parent one, that comes from its metaclass: https://github.com/sbdchd/django-types/blob/main/django-stubs/db/models/base.pyi#L39

A smaller reproduction example would be this, which doesn't rely on django but mimics what is happening in the previous example:

class Meta(type):
    @property
    def some_attr(cls) -> str:
        ...


class Foo(metaclass=Meta):
    ...


reveal_type(Foo.some_attr)


class Bar(Foo):
    some_attr: int


reveal_type(Bar.some_attr)

Before 1.1.323 this, this would be the output of pyright for this code:

/tmp/test/test.py
  /tmp/test/test.py:11:13 - information: Type of "Foo.some_attr" is "str"
  /tmp/test/test.py:18:13 - information: Type of "Bar.some_attr" is "int"
0 errors, 0 warnings, 2 informations

On 1.1.324 this is the output:

❯ pyright test.py        
/tmp/test/test.py
  /tmp/test/test.py:11:13 - information: Type of "Foo.some_attr" is "str"
  /tmp/test/test.py:18:13 - information: Type of "Bar.some_attr" is "str"
0 errors, 0 warnings, 2 informations
@bellini666 bellini666 added the bug Something isn't working label Aug 23, 2023
@erictraut
Copy link
Collaborator

This was an intentional change, so I don't consider it a bug or a regression. It is a bug fix for #5686. This is also an open bug in the mypy issue tracker.

When the Python runtime processes an attribute access on a class (such as OtherModel.objects), it first checks to see if the metaclass has an attribute by that name and whether that attribute is a descriptor object. If it is, it calls that descriptor object to fetch the value. If the metaclass does not have an attribute by that name or the attribute is not a descriptor object, it then looks up the attribute on the class. This is surprising behavior, but it's what the CPython code does. Here's a modified version of your code that demonstrates this:

class Meta(type):
    @property
    def some_attr(cls) -> str:
        return "Meta"

    other_attr = "Meta"

class Foo(metaclass=Meta):
    other_attr = 0

print(Foo.some_attr)  # This prints "Meta"
print(Foo.other_attr)  # This prints 0

class Bar(Foo):
    some_attr = 1
    other_attr = 2

print(Bar.some_attr)  # This prints "Meta"
print(Bar.other_attr)  # This prints 2

Prior to 1.1.324, pyright was not correctly modeling the runtime behavior. Now it does.

@erictraut erictraut added the as designed Not a bug, working as intended label Aug 23, 2023
@erictraut erictraut closed this as not planned Won't fix, can't repro, duplicate, stale Aug 23, 2023
@bellini666
Copy link
Author

This is surprising behavior, but it's what the CPython code does. Here's a modified version of your code that demonstrates this:

Oh, this is indeed surprising. Just tested here and you are correct regarding how the runtime behaves.

So, on Django itself this is not actually a descriptor, objects is set in the class by its metaclass during creation if one is not already defined.

The reason the stub is using a @property is to be able to return a specialized version of the generic manager, i.e. Foo.objects -> Manager[Foo], Bar.objects -> Manager[Bar]

Doing some tests here I think the stubs can be typed as objects: ClassVar[BaseManager[Self]] directly on the class, and it will work correctly for both pyright and mypy. It wasn't possible before because Self did not exist until recently. I'll try to make that change and contribute it to the stubs.

Thanks for the explanation about this!

kodiakhq bot pushed a commit to sbdchd/django-types that referenced this issue Aug 25, 2023
The way CPython works at runtime is to give a higher priority to the metaclass descriptors than the ones in the class itself.

Pyright 1.1.324 adjusted that but made our `.objects` typing break when using our own custom managers.

Nowadays we can use `Self` for that, which not only is more elegant but also solves this issue in a way that both mypy and pyright can resolve correctly.

Related issues:
- microsoft/pyright#5792
- microsoft/pyright#5686
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants