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

Improve typing for unresolved managers #1044

Merged

Conversation

ljodal
Copy link
Contributor

@ljodal ljodal commented Jul 2, 2022

This changes the logic when encountering an unresolvable manager class. Instead of adding it as a Manager we create a subclass of Manager that has fallback_to_any=True set. Similarly a QuerySet class is created that also has fallbacks to Any. This allows calling custom methods on the manager and querysets without getting type errors.

Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add a test case having two different unresolvable managers, in the same module, that'll also reveal their model attribute type?

mypy_django_plugin/transformers/models.py Show resolved Hide resolved
tests/typecheck/fields/test_related.yml Outdated Show resolved Hide resolved
mypy_django_plugin/transformers/models.py Outdated Show resolved Hide resolved
@ljodal
Copy link
Contributor Author

ljodal commented Jul 3, 2022

@flaeppe isn't that what this test is checking?

- case: test_emits_error_for_unresolved_managers
main: |
from myapp import models
installed_apps:
- myapp
files:
- path: myapp/__init__.py
- path: myapp/models.py
content: |
from django.db import models
def LocalManager() -> models.Manager:
"""
Returns a manager instance of an inlined manager type that can't
be resolved.
"""
class InnerManager(models.Manager):
...
return InnerManager()
class User(models.Model):
name = models.TextField()
class Booking(models.Model):
renter = models.ForeignKey(User, on_delete=models.PROTECT)
owner = models.ForeignKey(User, on_delete=models.PROTECT, related_name='bookingowner_set')
objects = LocalManager()
class TwoUnresolvable(models.Model):
objects = LocalManager()
second_objects = LocalManager()
class AbstractUnresolvable(models.Model):
objects = LocalManager()
class Meta:
abstract = True
class InvisibleUnresolvable(AbstractUnresolvable):
text = models.TextField()
def process_booking(user: User):
reveal_type(User.objects)
reveal_type(User._default_manager)
reveal_type(Booking.objects)
reveal_type(Booking._default_manager)
reveal_type(TwoUnresolvable.objects)
reveal_type(TwoUnresolvable.second_objects)
reveal_type(TwoUnresolvable._default_manager)
reveal_type(InvisibleUnresolvable.objects)
reveal_type(InvisibleUnresolvable._default_manager)
reveal_type(user.bookingowner_set)
reveal_type(user.booking_set)
# Check QuerySet methods on UnknownManager
reveal_type(Booking.objects.all)
reveal_type(Booking.objects.custom)
reveal_type(Booking.objects.all().filter)
reveal_type(Booking.objects.all().custom)
# Check QuerySet methods on UnknownRelatedManager
reveal_type(user.booking_set.all)
reveal_type(user.booking_set.custom)
reveal_type(user.booking_set.all().filter)
reveal_type(user.booking_set.all().custom)
out: |
myapp/models:13: error: Couldn't resolve related manager for relation 'booking' (from myapp.models.Booking.myapp.Booking.renter).
myapp/models:13: error: Couldn't resolve related manager for relation 'bookingowner_set' (from myapp.models.Booking.myapp.Booking.owner).
myapp/models:20: error: Could not resolve manager type for "myapp.models.Booking.objects"
myapp/models:23: error: Could not resolve manager type for "myapp.models.TwoUnresolvable.objects"
myapp/models:24: error: Could not resolve manager type for "myapp.models.TwoUnresolvable.second_objects"
myapp/models:27: error: Could not resolve manager type for "myapp.models.AbstractUnresolvable.objects"
myapp/models:32: error: Could not resolve manager type for "myapp.models.InvisibleUnresolvable.objects"
myapp/models:36: note: Revealed type is "django.db.models.manager.Manager[myapp.models.User]"
myapp/models:37: note: Revealed type is "django.db.models.manager.Manager[myapp.models.User]"
myapp/models:39: note: Revealed type is "myapp.models.UnknownManager[myapp.models.Booking]"
myapp/models:40: note: Revealed type is "django.db.models.manager.BaseManager[myapp.models.Booking]"
myapp/models:42: note: Revealed type is "myapp.models.UnknownManager[myapp.models.TwoUnresolvable]"
myapp/models:43: note: Revealed type is "myapp.models.UnknownManager[myapp.models.TwoUnresolvable]"
myapp/models:44: note: Revealed type is "django.db.models.manager.BaseManager[myapp.models.TwoUnresolvable]"
myapp/models:46: note: Revealed type is "myapp.models.UnknownManager[myapp.models.InvisibleUnresolvable]"
myapp/models:47: note: Revealed type is "django.db.models.manager.BaseManager[myapp.models.InvisibleUnresolvable]"
myapp/models:49: note: Revealed type is "myapp.models.UnknownRelatedManager[myapp.models.Booking]"
myapp/models:50: note: Revealed type is "myapp.models.UnknownRelatedManager[myapp.models.Booking]"
myapp/models:53: note: Revealed type is "def () -> myapp.models.UnknownQuerySet[myapp.models.Booking]"
myapp/models:54: note: Revealed type is "Any"
myapp/models:55: note: Revealed type is "def (*args: Any, **kwargs: Any) -> myapp.models.UnknownQuerySet[myapp.models.Booking]"
myapp/models:56: note: Revealed type is "Any"
myapp/models:59: note: Revealed type is "def () -> myapp.models.UnknownQuerySet[myapp.models.Booking]"
myapp/models:60: note: Revealed type is "Any"
myapp/models:61: note: Revealed type is "def (*args: Any, **kwargs: Any) -> myapp.models.UnknownQuerySet[myapp.models.Booking]"
myapp/models:62: note: Revealed type is "Any"

@ljodal
Copy link
Contributor Author

ljodal commented Jul 4, 2022

I've found a bug with this implementation, so hold off on merging until I have that under control :)

Edit: Fixed now ✅

@ljodal
Copy link
Contributor Author

ljodal commented Jul 7, 2022

@sobolevn Any plans to merge this and the few other PRs that should be ready? We're currently running with this branch at work and it's been working fine (both 1.11 and 1.12 is unusable for us due to phantom type errors :().

I also have a few other things on my list to fix (like #960), but I'll get too many merge conflicts so I'm holding off for now

ljodal added 3 commits July 13, 2022 09:39
This changes the logic when encountering an unresolvable manager class.
Instead of adding it as a `Manager` we create a subclass of `Manager`
that has `fallback_to_any=True` set. Similarly a `QuerySet` class is
created that also has fallbacks to `Any`. This allows calling custom
methods on the manager and querysets without getting type errors.
Because this inherits from _QuerySet, not QuerySet, it needs to have two
parameters
@sobolevn
Copy link
Member

sobolevn commented Jul 13, 2022

Sorry for the delay, I was on vacation. Can you please resolve merge conflicts?

@ljodal ljodal force-pushed the improve-typing-of-unresolved-managers branch from d89701d to 8035abb Compare July 13, 2022 07:58
@ljodal
Copy link
Contributor Author

ljodal commented Jul 13, 2022

Sorry for the delay, I was on vacation. Can you please resolve merge conflicts?

No worries, I assumed as much. The conflicts should be resolved now :)

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@sobolevn sobolevn merged commit e7a89f7 into typeddjango:master Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants