Skip to content

fix Support for Django Manager/QuerySet #3648 - #3700

Open
asukaminato0721 wants to merge 2 commits into
meta:mainfrom
asukaminato0721:3648
Open

asukaminato0721 wants to merge 2 commits into
meta:mainfrom
asukaminato0721:3648

Conversation

@asukaminato0721

Copy link
Copy Markdown
Collaborator

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

@meta-cla meta-cla Bot added the cla signed label Jun 5, 2026
@asukaminato0721 asukaminato0721 changed the title fix Support for Django Manager/QuerySet #3648c fix Support for Django Manager/QuerySet #3648 Jun 5, 2026
@github-actions github-actions Bot added the size/m label Jun 5, 2026
@github-actions

This comment has been minimized.

@asukaminato0721
asukaminato0721 marked this pull request as ready for review June 5, 2026 10:09
@meta-codesync

meta-codesync Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@grievejia has imported this pull request. If you are a Meta employee, you can view this in D111289442.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread pyrefly/lib/alt/class/class_field.rs Resolved
Comment thread pyrefly/lib/alt/class/django.rs Resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@stroxler stroxler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

[django.rs:382–387](

match self.expr_infer(&attr.value, &self.error_swallower()) {
Type::ClassDef(queryset_cls) if self.inherits_from_django_queryset(&queryset_cls) => {
Some(self.instantiate(&queryset_cls))
}
_ => None,
}
)

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() and objects.get_queryset() are rejected. Passing objects where a Manager[Model] is expected also fails.
  • Calls such as objects.delete(), private queryset methods, and iter(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

[Same return expression](

match self.expr_infer(&attr.value, &self.error_swallower()) {
Type::ClassDef(queryset_cls) if self.inherits_from_django_queryset(&queryset_cls) => {
Some(self.instantiate(&queryset_cls))
}
_ => None,
}
)

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:

  • Unknown for an unparameterized queryset subclass.
  • A generic variable such as T or _Row, instead of the actual model.
  • Base when the manager is inherited by Child from 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

[class_field.rs:4174–4179](

// Django models commonly replace `Model.objects` with a model-specific
// manager, which should not be checked as a normal class override.
if class_metadata.is_django_model() && field_name.as_str() == "objects" {
return false;
}
)

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.

@github-actions

This comment has been minimized.

@stroxler stroxler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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().extra reports a missing attribute.
  • Explicit row types get overwritten: M.objects.all().values_list("id", flat=True).first() becomes M | None, rather than retaining the stub’s Any | 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 Self return should retain the custom queryset and its effective arguments.
  • An explicit QuerySet[Other] return should retain Other.
  • 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.

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for Django Manager/QuerySet

5 participants