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

Model.object and djangos @classproperties not working since 1.13.1 #1285

Closed
djbrown opened this issue Dec 8, 2022 · 12 comments · Fixed by #1287
Closed

Model.object and djangos @classproperties not working since 1.13.1 #1285

djbrown opened this issue Dec 8, 2022 · 12 comments · Fixed by #1287
Labels
bug Something isn't working regression Behavior has changed for worse with a release

Comments

@djbrown
Copy link
Contributor

djbrown commented Dec 8, 2022

Bug report

Hello :-) first of all thank you very much for releasing the new version 1.13.1 so quickly after I asked for it!
But now I am facing two new problems, which might be related so I'm posting them together.

Steps to reproduce

See my separately crafted repo with minimal steps to reproduce:
https://github.com/djbrown/mypy-django-bugs

But basically it's:

  • pip install django mypy django-stubs==1.13.0
  • generate simple django project and include above bug snippets
  • execute mypy check -> ✔
  • pip install django-stubs==1.13.1
  • execute mypy check -> ❌
  • pip install django-stubs==1.13.0
  • execute mypy check -> ✔

So it's definitely the new version..
I can also add a failing test or workflow so you don't have to reproduce manually.

[Collapsed: This part of the report was resolved]

no attribute Model.objects

from django.db.models import Model
def invalid(model: Model):
    model.objects

Results in error: "Model" has no attribute "objects" [attr-defined]
I already tried to fix by enabling monkey-patching in settings.py or module via django_stubs_ext or use ModelBase or TypeVar instead of Model.
Because in #1150 the object property was moved from Model to ModelBase - but that doesn't work for me.

Maybe also related issues:

cannot access classproperties

from django.test import LiveServerTestCase
class ServerTestCase(LiveServerTestCase):
    def invalid(self, arg: str):
        self.live_server_url

Results in:

error: Argument 1 to "__get__" of "classproperty" has incompatible type "ServerTestCase"; expected "Optional[classproperty[Any]]"  [arg-type]
error: Argument 2 to "__get__" of "classproperty" has incompatible type "Type[ServerTestCase]"; expected "Type[classproperty[Any]]"  [arg-type]

I already tried to fix by enabling monkey-patching in settings.py or module via django_stubs_ext, or access classproperty via class or super class.
Accessing normal class attributes works fine though.

System information

  • OS: ubuntu
  • python version: 3.10.4
  • django version: 4.1.4
  • mypy version: 0.991
  • django-stubs version: 1.13.1
  • django-stubs-ext version: 0.7.0
@djbrown djbrown added the bug Something isn't working label Dec 8, 2022
@djbrown djbrown changed the title classproperties and models.object not working since 1.13.1 models.object and classproperties not working since 1.13.1 Dec 8, 2022
@djbrown djbrown changed the title models.object and classproperties not working since 1.13.1 models.object and @classproperties not working since 1.13.1 Dec 8, 2022
@djbrown djbrown changed the title models.object and @classproperties not working since 1.13.1 Model.object and djangos @classproperties not working since 1.13.1 Dec 8, 2022
djbrown added a commit to djbrown/hbscorez that referenced this issue Dec 8, 2022
@djbrown
Copy link
Contributor Author

djbrown commented Dec 8, 2022

A possible workaround would be to to remove the type hints. That probably signals mypy to leave out the function.
I decided to comment-ignore the problematic lines until this is resolved.

@intgr
Copy link
Collaborator

intgr commented Dec 8, 2022

from django.db.models import Model
def invalid(model: Model):
    model.objects

Results in error: "Model" has no attribute "objects" [attr-defined]

This error is correct. model: Model tells mypy that the variable is an instance of Model.

Only Model classes have this attribute, not instances.

Try def invalid(model: Type[Model]):

@intgr
Copy link
Collaborator

intgr commented Dec 8, 2022

As for the classproperty case, I am not sure what is happening there.

djbrown added a commit to djbrown/hbscorez that referenced this issue Dec 8, 2022
@djbrown
Copy link
Contributor Author

djbrown commented Dec 8, 2022

ahhh thank you, that's correct! the Model.object problem is fixed now :-)

@dlesbre

This comment was marked as off-topic.

@intgr
Copy link
Collaborator

intgr commented Dec 8, 2022

@dlesbre Please open a separate issue, tracking multiple different problems in one issue gets messy.

@intgr intgr added the regression Behavior has changed for worse with a release label Dec 8, 2022
@dlesbre
Copy link
Contributor

dlesbre commented Dec 8, 2022

@dlesbre Please open a separate issue, tracking multiple different problems in one issue gets messy.

Done #1286

@djbrown
Copy link
Contributor Author

djbrown commented Dec 8, 2022

The stub for django.test.testcases.LiveServerTestCase looks currenty look like this:

class LiveServerTestCase(TransactionTestCase):
host: str
port: int
server_thread_class: type[Any]
server_thread: Any
static_handler: Any
@classproperty
def live_server_url(cls: Any) -> str: ...
@classproperty
def allowed_host(cls: Any) -> str: ...

mypy complains about the accessed classproperties e.g. live_server_url or allowed_host,
but the normal class attributes can be used just fine e.g. port, host ...

I tried to set some other type hints instead of cls: Any (e.g. type, type[Any] or type[Self]) or use TypeVar or remove the annotation in the local testcases.pyi stub file, but only got slightly different mypy error: ... expected "Optional[classproperty[<nothing>]]"

What kind of did work, was to replace @classproperty with @classmethod but then I had to add parenthesis to my actual source code (live_server_url()) and that doesn't match the django implementation.
The code wont actually work that way, so this is not a solution, but maybe someone can find a proper solution with that.
Maybe djangos custom @classproperty decorator has to be typed differently? It's not from the standard python library..

The python typing docs say:

Do not annotate self and cls in method definitions, except when referencing a type variable.

So I guess the cls: type[Self] annotation in other classmethods should be removed too, since they don't use type variables.

@djbrown
Copy link
Contributor Author

djbrown commented Dec 8, 2022

What finally did work for me (based on my previous comments last concerns) was to just set the classproperties as simple attributes in the type stub without any decorators:

class LiveServerTestCase(TransactionTestCase): 
    host: str 
    port: int 
    ...
    live_server_url: str
    allowed_host: str

I will create a pull request for that and hope you agree to this solution.
Alternatively someone will have to think about how to properly annotate djangos classproperties..

@intgr
Copy link
Collaborator

intgr commented Dec 8, 2022

The LiveServerTestCase and classproperty classes don't seem to have changed between 0.13.0...1.13.1. Are you sure this regressed in 1.13.1?

Edit: Oh, sorry, I had some old commit checked out. I think I see the bug now.

@intgr
Copy link
Collaborator

intgr commented Dec 8, 2022

Indeed, classproperty was broken, fixed in #1287.

@djbrown
Copy link
Contributor Author

djbrown commented Dec 8, 2022

thanks :) I reviewed your PR and applied the patch locally. that fixes the problem too - and more correctly :)

intgr added a commit that referenced this issue Dec 9, 2022
Removed unnecessary `Self` hints on classproperty methods.

The removal of `self: Self` fixes #1285. Regressed in #1253.
djbrown added a commit to djbrown/hbscorez that referenced this issue Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression Behavior has changed for worse with a release
Development

Successfully merging a pull request may close this issue.

3 participants