Repository navigation
fix Support for Django Manager/QuerySet #3648 - #3700
asukaminato0721 wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
|
@grievejia has imported this pull request. If you are a Meta employee, you can view this in D111289442. |
There was a problem hiding this comment.
Pull request overview
This PR improves Pyrefly’s Django integration by recognizing QuerySetSubclass.as_manager() assignments on Django models so custom queryset methods are available via Model.objects and Model.objects.all(), and by reducing override-check noise around manager reassignment.
Changes:
- Add a Django-specific helper to detect
QuerySet.as_manager()patterns and preserve the custom queryset type. - Thread this helper into class-field type postprocessing and adjust inference/override behavior for Django manager assignments.
- Add a Django regression test covering custom queryset methods exposed through
as_manager().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pyrefly/lib/test/django/fields.rs | Adds a regression test verifying custom queryset methods work via objects and objects.all() when using as_manager(). |
| pyrefly/lib/alt/class/django.rs | Introduces detection for QuerySetSubclass.as_manager() and returns a type derived from the queryset class. |
| pyrefly/lib/alt/class/class_field.rs | Integrates the new helper into class-field inference and suppresses override-consistency checks for Django objects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6f9809e to
30dd9b2
Compare
This comment has been minimized.
This comment has been minimized.
30dd9b2 to
7539251
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
stroxler
left a comment
There was a problem hiding this comment.
Thanks! I think there are some problems with this that we need to patch up before we can merge
1. The manager is given the wrong type
pyrefly/pyrefly/lib/alt/class/django.rs
Lines 382 to 387 in 71f952b
as_manager() creates a manager, but the new code gives it the queryset’s type. That makes Pyrefly reject valid code and accept invalid code:
- Valid calls such as
objects.db_manager()andobjects.get_queryset()are rejected. Passingobjectswhere aManager[Model]is expected also fails. - Calls such as
objects.delete(), private queryset methods, anditer(objects)are accepted, even though they fail at runtime.
Suggested fix: keep the manager’s type and add the queryset methods that Django actually copies onto it. Methods such as .all() should return the custom queryset, but the manager itself should not be treated as that queryset.
2. The manager loses track of its model
pyrefly/pyrefly/lib/alt/class/django.rs
Lines 382 to 387 in 71f952b
The new code uses the queryset’s generic type parameters without connecting them to the model that owns the manager. As a result, objects.get() can return:
Unknownfor an unparameterized queryset subclass.- A generic variable such as
Tor_Row, instead of the actual model. Basewhen the manager is inherited byChildfrom an abstract base model.
These calls previously retained the correct model type.
Suggested fix: connect the manager and queryset types to the owning model, including subclasses. For cases the new code cannot handle yet, retain the existing behavior.
3. The override exception is too broad in one place and too narrow in another
pyrefly/pyrefly/lib/alt/class/class_field.rs
Lines 4174 to 4179 in 71f952b
The new exception skips override checks for anything named objects, even when it is not a manager. This hides real errors, such as changing an explicitly annotated inherited attribute from int to str.
But a valid manager replacement under another name—such as records = Q2.as_manager()—still produces an override error against the parent’s Q1 manager.
Suggested fix: base the exception on whether the attribute is actually a manager replacement, not just whether it is named objects.
This comment has been minimized.
This comment has been minimized.
stroxler
left a comment
There was a problem hiding this comment.
I think this one is going to be interesting. I was originally thinking we might be able to land this, but based on what happened when I tried to have some agents tie up loose ends there seem to be structural problems.
I think the real issue is that the layout of the data in Pyrefly may need to be rethought. I would suggest starting the PR from scratch, with the current code as a reference for some of the desired behavior but considering composability of the logic carefully.
Here's a more detailed diagnosis from my coding agent of what seems to cause maintainability issues...
The main concern: the metadata is not consistently compositional
The new implementation stores queryset information in a KwCall inside an intersection, then interprets it through an early hook in type_of_attr_get.
That makes direct attribute calls work, but other operations either do not recognize the carrier or interpret its underlying type differently. There are concrete examples in this revision:
- Indexing breaks:
M.objects.all()[0]reports that the queryset is not subscriptable. - Annotation information is lost:
M.objects.annotate(extra=models.Value(1)).get().extrareports a missing attribute. - Explicit row types get overwritten:
M.objects.all().values_list("id", flat=True).first()becomesM | None, rather than retaining the stub’sAny | None. - Explicitly different models get overwritten: if a custom method returns
QuerySet[Other], a subsequent.get()or.create()is typed as the model owning the original manager.
The first two arise because existing Pyrefly operations do not compose with the new wrapper. The latter two arise because the implementation treats later queryset results as though they still need the original manager’s model binding.
Separate construction-time binding from subsequent queryset operations
Binding an initially generic queryset to the model receiving its manager is different from interpreting a method’s return type.
Once that initial binding is established:
- A
Selfreturn should retain the custom queryset and its effective arguments. - An explicit
QuerySet[Other]return should retainOther. - An operation that produces a different row type should retain that row type—even when the stub deliberately represents it as
Any. - Inherited model binding should behave consistently across
.get(), indexing, and iteration, rather than being repaired only for selected method names.
This would avoid repeatedly guessing model/row bindings during attribute lookup.
Preserve manager-owned methods separately from forwarded methods
There is also an important distinction between methods Django forwards to the queryset and methods the manager implements itself.
For example, BaseManager.all() does not forward to a custom QuerySet.all() override. If the queryset override adds an optional keyword, the manager still cannot accept it. The current implementation substitutes the queryset’s entire signature when it returns Self, which accepts calls that fail at runtime.
For manager-owned methods, we should preserve their parameters and refine only the return type where appropriate.
Suggested next step
We don't need any particular representation or for a new general-purpose type system mechanism. But we probably want a coherent approach in which generated members participate in shared member resolution, and the carrier’s behavior through substitution, simplification, and ordinary queryset operations is explicit.
A short explanation of that contract, backed by regression tests for the examples above, would make this much easier to review than adding further special cases to the direct attribute-read hook.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Fixes #3648
recognizes QuerySetSubclass.as_manager() on Django models and preserves the custom queryset type, and routes that Django manager assignment through the new helper and avoids inherited Model.objects assignment/override noise.
Test Plan
add test