Make Model.objects a ClassVar

[davidhalter]

Model.objects is only available on classes and while we cannot force this, without ClassVar it's kind of only available on instances.

I have noticed this when I was writing a Jedi plugin that makes django-stubs work properly.

[davidhalter]

The test suite is failing, but master is also failing on Travis, so I'm not sure if that's my fault.

I also don't really understand why this one line change would cause so many test failures.

[sobolevn]

@davidhalter that's totally not your fault. We have some unexpected failures from our build system.

[sobolevn]

This is a great addition! But, this does raise an important topic to discuss.

Currently, we do not treat properties as ClassVars. I guess that we need to fix that. And mark ones that apply as ClassVars.

@danifus can you please open a new issue?

[danifus]

Hi, was this comment intended for me or @davidhalter?

[sobolevn]

Oups! 😆 My bad!

But, feel free to participate!

[davidhalter]

@sobolevn I don't really understand what needs to be fixed. I see that the CI is still failing even after I force pushed an amended commit. I'm guessing that has something to do with the Mypy plugin. But since I'm not using Mypy, I'm only interested in correct type annotations and I would appreciate if you guys could take it from here.

I obviously understand that this is more work, but it's not my forte. As a temporary measure, I might just ship a patched version in Jedi until this gets fixed.

Still thanks for looking at it @sobolevn!

[kszmigiel]

@sobolevn Failing tests are not a matter of the bug I've fixed, I've just checked that. Introducing ClassVars to our stubs it quite a big change. It'd be awesome @davidhalter if you could create an issue on this one. As @sobolevn wrote, marking properties that apply as ClassVar would be a lot of help too. Thank you for your work, I can take it from here if you want me to 😄

[davidhalter]

@kszmigiel Yeah feel free to take it from here, I really have no idea :)

I can of course create an issue, but I'd rather not, since I really don't understand what the issue is. The issue would probably have the content "ClassVar for Model.objects doesn't work". If that has value for you, I'm fine doing that :).

[kszmigiel]

@davidhalter Sure thing, I'll create an issue for that later, thanks for pointing this out 😄

[sobolevn]

Thanks a lot for your work! 👍

[sobolevn]

The CI is fixed.

[adamchainz]

I’ve rebase this PR - @sobolevn want to double check and merge?

[sobolevn]

Looks like this needs a bit more work:

 django-stubs/contrib/sites/models.pyi:14: error: Cannot override class variable (previously declared on base class "Model") with instance variable
django-stubs/contrib/contenttypes/models.pyi:18: error: Cannot override class variable (previously declared on base class "Model") with instance variable
django-stubs/contrib/auth/models.pyi:22: error: Cannot override class variable (previously declared on base class "Model") with instance variable
django-stubs/contrib/auth/models.pyi:33: error: Cannot override class variable (previously declared on base class "Model") with instance variable
django-stubs/contrib/admin/models.pyi:32: error: Cannot override class variable (previously declared on base class "Model") with instance variable
django-stubs/contrib/sessions/base_session.pyi:17: error: Cannot override class variable (previously declared on base class "Model") with instance variable

[adamchainz]

Yeah I was just looking at this. We can add objects: ClassVar[models.Manager] = ... in many places, but that will erase some of the inferred types from the Mypy plugin. I think the plugin would need adjusting to wrap its return types with ClassVar[...], if that’s possible.

It might also annoying to users if they have to add ClassVar everywhere they override a manager.

[sobolevn]

ClassVar is only really helpful when there are classmethods. Otherwise, overriding class-level variables with instance-level ones is not great, but not problematic.

This is what we do in typeshed at least.

[adamchainz]

So you think we can close this one? It does seem quite disruptive.

[sobolevn]

Yes, I think so.

@davidhalter thank you for your proposal!

If anyone wants to submit a full-featured solution, please provide real-world use-cases where this can be useful first.

[PeterJCLaw]

Cross linking: it looks like #1150 may have resolved this.

[davidhalter]

I feel like I also changed the default behavior of non-ClassVar variables on classes to be available on classes as well. This is something that Mypy allows as well, so I figured we are probably not wrong to allow that.