Skip to content

Commit 4095f16

Browse files
asukaminato0721meta-codesync[bot]
authored andcommitted
fix What's up with django-models-utils #2328 (#4695)
Summary: Fixes #2328 now preserves explicitly assigned Django manager types, so django-model-utils APIs such as `InheritanceManager.select_subclasses()` remain available and correctly specialize to the model type. Pull Request resolved: #4695 Test Plan: add test Reviewed By: stroxler Differential Revision: D128046245 fbshipit-source-id: cb2125e8076c6309ec0f7ac793f6fbb33b08e88c
1 parent 8793ce3 commit 4095f16

6 files changed

Lines changed: 256 additions & 18 deletions

File tree

‎crates/pyrefly_types/src/class.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -467,22 +467,22 @@ pub struct ClassType(Class, TArgs, Option<Arc<DjangoClassMetadata>>);
467467

468468
/// Construction-time Django specializations on nominal class instances.
469469
///
470-
/// The manager resolves generated members against its queryset. A specialized queryset uses
470+
/// The manager resolves generated members against its queryset. A specialized class uses
471471
/// its effective base for inherited members and nominal upcasts. Type traversal and substitution
472472
/// include this data, and instance binding preserves it when replacing `Self`. Explicit method
473473
/// return types stand alone: neither their model nor their row is rebound to the manager's model.
474474
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
475475
#[derive(TypeEq)]
476476
pub enum DjangoClassMetadata {
477477
Manager { queryset: ClassType },
478-
QuerySet { base: ClassType },
478+
SpecializedBase { base: ClassType },
479479
}
480480

481481
impl Visit<Type> for Arc<DjangoClassMetadata> {
482482
fn recurse<'a>(&'a self, f: &mut dyn FnMut(&'a Type)) {
483483
match self.as_ref() {
484484
DjangoClassMetadata::Manager { queryset } => queryset.visit(f),
485-
DjangoClassMetadata::QuerySet { base } => base.visit(f),
485+
DjangoClassMetadata::SpecializedBase { base } => base.visit(f),
486486
}
487487
}
488488
}
@@ -491,7 +491,7 @@ impl VisitMut<Type> for Arc<DjangoClassMetadata> {
491491
fn recurse_mut(&mut self, f: &mut dyn FnMut(&mut Type)) {
492492
match Arc::make_mut(self) {
493493
DjangoClassMetadata::Manager { queryset } => queryset.visit_mut(f),
494-
DjangoClassMetadata::QuerySet { base } => base.visit_mut(f),
494+
DjangoClassMetadata::SpecializedBase { base } => base.visit_mut(f),
495495
}
496496
}
497497
}

‎pyrefly/lib/alt/class/class_field.rs‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3378,7 +3378,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
33783378
|| cls.has_qname("django.db.models.manager", "BaseManager"))
33793379
})
33803380
&& let ExprOrBinding::Expr(e) = value
3381-
&& let Some(manager) = self.get_django_manager_from_queryset_type(class, e)
3381+
&& let Some(manager) = self.get_django_manager_assignment_type(class, e)
33823382
{
33833383
// Standard manager interfaces allow model rebinding. Explicit custom interfaces
33843384
// retain their declared type and use ordinary assignment checking below.
@@ -4580,13 +4580,18 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
45804580
}
45814581
}
45824582
if metadata.is_django_model()
4583-
&& !class_field.has_explicit_annotation()
4583+
&& !cls_fields.is_field_annotated(field_name)
45844584
&& self.is_django_manager_type(&class_field.ty())
4585-
&& matches!(want_class_field.ty(), Type::ClassType(cls) if
4585+
&& self.is_django_manager_type(&want_class_field.ty())
4586+
&& (matches!(want_class_field.ty(), Type::ClassType(cls) if
45864587
cls.has_qname("django.db.models.manager", "Manager")
45874588
|| cls.has_qname("django.db.models.manager", "BaseManager"))
4589+
|| self
4590+
.get_class_fields(&want_field.defining_class)
4591+
.is_some_and(|fields| !fields.is_field_annotated(field_name)))
45884592
{
4589-
// Standard manager interfaces allow rebinding to the receiving model.
4593+
// Django permits replacing inferred managers, including sibling manager classes.
4594+
// Explicit custom interfaces still require ordinary override checks.
45904595
continue;
45914596
}
45924597
let want_is_static = want_class_field.is_staticmethod();

‎pyrefly/lib/alt/class/classdef.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
259259
pub fn as_superclass(&self, class: &ClassType, want: &Class) -> Option<ClassType> {
260260
if class.class_object() == want {
261261
Some(class.clone())
262-
} else if let Some(DjangoClassMetadata::QuerySet { base }) = class.django_metadata()
262+
} else if let Some(DjangoClassMetadata::SpecializedBase { base }) = class.django_metadata()
263263
&& self.has_superclass(base.class_object(), want)
264264
{
265265
self.as_superclass(base, want)

‎pyrefly/lib/alt/class/django.rs‎

Lines changed: 61 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8+
use std::iter;
9+
810
use pyrefly_python::module_name::ModuleName;
911
use pyrefly_python::module_name::is_python_identifier;
1012
use pyrefly_types::callable::Callable;
@@ -201,7 +203,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
201203
if queryset.has_qname("django.db.models.query", "QuerySet") {
202204
queryset = base;
203205
} else {
204-
queryset = queryset.with_django_metadata(DjangoClassMetadata::QuerySet { base });
206+
queryset = queryset.with_django_metadata(DjangoClassMetadata::SpecializedBase { base });
205207
}
206208
self.heap.mk_class_type(queryset)
207209
}
@@ -462,7 +464,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
462464
let queryset = if queryset.has_qname("django.db.models.query", "QuerySet") {
463465
base
464466
} else {
465-
queryset.with_django_metadata(DjangoClassMetadata::QuerySet { base })
467+
queryset.with_django_metadata(DjangoClassMetadata::SpecializedBase { base })
466468
};
467469
let Type::ClassDef(manager) = self.try_get_from_export(
468470
ModuleName::from_str("django.db.models.manager"),
@@ -484,9 +486,64 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
484486
)
485487
}
486488

489+
/// Bind a constructed manager to its model without changing its nominal class.
490+
pub(crate) fn get_django_manager_assignment_type(
491+
&self,
492+
model: &Class,
493+
initial_value_expr: &Expr,
494+
) -> Option<Type> {
495+
if let Some(manager) = self.get_django_manager_from_queryset_type(model, initial_value_expr)
496+
{
497+
return Some(manager);
498+
}
499+
let call = initial_value_expr.as_call_expr()?;
500+
let callee = self.expr_infer(&call.func, &self.error_swallower());
501+
let (_, constructor) = self.unwrap_class_object_silently(&callee)?;
502+
let Type::ClassType(constructor) = constructor else {
503+
return None;
504+
};
505+
let mut base = iter::once(&constructor)
506+
.chain(
507+
self.get_mro_for_class(constructor.class_object())
508+
.ancestors(self.stdlib),
509+
)
510+
.find(|base| {
511+
base.has_qname("django.db.models.manager", "Manager")
512+
|| base.has_qname("django.db.models.manager", "BaseManager")
513+
})?
514+
.clone();
515+
let Type::ClassType(mut manager) =
516+
self.expr_infer(initial_value_expr, &self.error_swallower())
517+
else {
518+
return None;
519+
};
520+
if manager.class_object() != constructor.class_object() {
521+
return None;
522+
}
523+
let [original_model] = base.targs().as_slice() else {
524+
return None;
525+
};
526+
let model = self.heap.mk_self_type(self.as_class_type_unchecked(model));
527+
// The model parameter can occur at any position in a custom manager's parameters.
528+
// Preserve other arguments inferred from the constructor.
529+
let parameters = manager.tparams().clone();
530+
for (parameter, argument) in parameters.iter().zip(manager.targs_mut().as_mut()) {
531+
if matches!(original_model, Type::Quantified(q) if q.as_ref() == parameter) {
532+
*argument = model.clone();
533+
}
534+
}
535+
base.targs_mut().as_mut()[0] = model;
536+
let manager = if manager.class_object() == base.class_object() {
537+
base
538+
} else {
539+
manager.with_django_metadata(DjangoClassMetadata::SpecializedBase { base })
540+
};
541+
Some(self.heap.mk_class_type(manager))
542+
}
543+
487544
/// Return the effective QuerySet base, including a construction-time specialization.
488545
fn django_queryset_base(&self, cls: &ClassType) -> Option<ClassType> {
489-
if let Some(DjangoClassMetadata::QuerySet { base }) = cls.django_metadata() {
546+
if let Some(DjangoClassMetadata::SpecializedBase { base }) = cls.django_metadata() {
490547
Some(base.clone())
491548
} else if cls.has_qname("django.db.models.query", "QuerySet") {
492549
Some(cls.clone())
@@ -572,7 +629,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
572629
}
573630
self.get_instance_attribute(queryset, name)
574631
}
575-
DjangoClassMetadata::QuerySet { base } => {
632+
DjangoClassMetadata::SpecializedBase { base } => {
576633
let member = self.get_class_member_with_defining_class(cls.class_object(), name)?;
577634
if !self.has_superclass(base.class_object(), &member.defining_class) {
578635
return None;

‎pyrefly/lib/test/django/fields.rs‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -778,7 +778,7 @@ assert_type(Ordinary.records.get(), Base)
778778
);
779779

780780
django_testcase!(
781-
test_queryset_manager_inferred_custom_interface,
781+
test_queryset_manager_inferred_manager_replacements,
782782
r#"
783783
from typing import assert_type
784784
from django.db import models
@@ -797,11 +797,14 @@ class Base(ModelBase):
797797
standard = models.Manager[ModelBase]()
798798
base_manager = BaseManager[ModelBase]()
799799
800-
class InvalidManager(Base):
801-
records = models.Manager() # E: overrides parent class `Base` in an inconsistent manner
800+
class PlainManager(Base):
801+
records = models.Manager()
802802
803-
class InvalidQuerySet(Base):
804-
records = Q.as_manager() # E: overrides parent class `Base` in an inconsistent manner
803+
class GeneratedManager(Base):
804+
records = Q.as_manager()
805+
806+
assert_type(PlainManager.records.get(), PlainManager)
807+
assert_type(GeneratedManager.records.get(), GeneratedManager)
805808
806809
class Valid(Base):
807810
records = SpecificManager()

‎pyrefly/lib/test/django/model.rs‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,179 @@
66
*/
77

88
use crate::django_testcase;
9+
use crate::test::django::util::django_env;
10+
use crate::test::util::TestEnv;
11+
use crate::testcase;
12+
13+
fn django_model_utils_env() -> TestEnv {
14+
let mut env = django_env();
15+
env.add(
16+
"model_utils.managers",
17+
r#"
18+
from typing import TypeVar
19+
20+
from django.db import models
21+
22+
ModelT = TypeVar("ModelT", bound=models.Model, covariant=True)
23+
24+
class InheritanceQuerySet(models.QuerySet[ModelT]): ...
25+
26+
class InheritanceManager(models.Manager[ModelT]):
27+
def select_subclasses(self) -> InheritanceQuerySet[ModelT]: ...
28+
"#,
29+
);
30+
env
31+
}
32+
33+
testcase!(
34+
test_django_model_utils_custom_manager,
35+
django_model_utils_env(),
36+
r#"
37+
from typing import assert_type
38+
39+
from django.db import models
40+
from model_utils.managers import InheritanceManager, InheritanceQuerySet
41+
42+
class Place(models.Model):
43+
objects = InheritanceManager()
44+
45+
assert_type(Place.objects, InheritanceManager[Place])
46+
assert_type(Place.objects.select_subclasses(), InheritanceQuerySet[Place])
47+
assert_type(Place.objects.get(), Place)
48+
assert_type(Place.objects.create(), Place)
49+
assert_type(Place.objects.filter(), models.QuerySet[Place])
50+
51+
class Child(Place):
52+
objects = models.Manager()
53+
54+
assert_type(Child.objects, models.Manager[Child])
55+
assert_type(Child.objects.get(), Child)
56+
57+
class SiblingManager(models.Manager):
58+
def custom(self) -> int:
59+
return 1
60+
61+
class OtherChild(Place):
62+
objects = SiblingManager()
63+
64+
assert_type(OtherChild.objects, SiblingManager)
65+
assert_type(OtherChild.objects.get(), OtherChild)
66+
assert_type(OtherChild.objects.custom(), int)
67+
"#,
68+
);
69+
70+
django_testcase!(
71+
test_bare_custom_manager_model_binding,
72+
r#"
73+
from typing import assert_type
74+
from django.db import models
75+
76+
class PersonManager(models.Manager):
77+
def custom(self) -> int:
78+
return 1
79+
80+
class Person(models.Model):
81+
objects = PersonManager()
82+
83+
class Child(Person): ...
84+
85+
assert_type(Person.objects, PersonManager)
86+
assert_type(Person.objects.get(), Person)
87+
assert_type(Person.objects.create(), Person)
88+
assert_type(Person.objects.filter(), models.QuerySet[Person])
89+
assert_type(Person.objects.model, type[Person])
90+
assert_type(Person.objects.custom(), int)
91+
assert_type(Person.objects.db_manager().get(), Person)
92+
assert_type(Child.objects.get(), Child)
93+
assert_type(Child.objects.filter(), models.QuerySet[Child])
94+
95+
def accept_manager(manager: models.Manager[Person]) -> None: ...
96+
accept_manager(Person.objects)
97+
"#,
98+
);
99+
100+
django_testcase!(
101+
test_custom_manager_inherited_qualifiers,
102+
r#"
103+
from typing import Any, ClassVar, Final, Protocol, assert_type
104+
from django.db import models
105+
106+
class CustomManager(models.Manager): ...
107+
108+
class Base(models.Model):
109+
records: ClassVar[models.Manager["Base"]] = models.Manager()
110+
locked: Final[models.Manager["Base"]] = models.Manager()
111+
112+
class Person(Base):
113+
objects = CustomManager()
114+
records = CustomManager()
115+
locked = CustomManager() # E: `locked` is declared as final in parent class `Base`
116+
117+
class HasManager(Protocol):
118+
objects: models.Manager[Any]
119+
120+
def accept_model(model: HasManager) -> None: ...
121+
122+
class HasCustomManager(Protocol):
123+
objects: CustomManager
124+
125+
def accept_custom_model(model: HasCustomManager) -> None: ...
126+
accept_custom_model(Person)
127+
128+
person = Person()
129+
person.objects = Person.objects # E: Cannot set field `objects`
130+
person.records = Person.records # E: Cannot set field `records`
131+
Person.locked = Person.locked # E: Cannot set field `locked`
132+
133+
class Child(Person):
134+
objects = models.Manager()
135+
records = models.Manager()
136+
137+
assert_type(Child.objects.get(), Child)
138+
assert_type(Child.records.get(), Child)
139+
accept_model(Child)
140+
child = Child()
141+
child.objects = Child.objects # E: Cannot set field `objects`
142+
child.records = Child.records # E: Cannot set field `records`
143+
144+
class Plain(models.Model):
145+
objects = models.Manager()
146+
147+
accept_model(Plain)
148+
plain = Plain()
149+
plain.objects = Plain.objects # E: Cannot set field `objects`
150+
"#,
151+
);
152+
153+
django_testcase!(
154+
test_custom_manager_constructor_arguments,
155+
r#"
156+
from typing import Generic, TypeVar, assert_type
157+
from django.db import models
158+
159+
T = TypeVar("T")
160+
M = TypeVar("M", bound=models.Model)
161+
162+
class CustomManager(models.Manager[M], Generic[T, M]):
163+
def __init__(self, value: T) -> None:
164+
self.value = value
165+
166+
def custom(self) -> T:
167+
return self.value
168+
169+
class Person(models.Model):
170+
objects = CustomManager("label")
171+
172+
assert_type(Person.objects, CustomManager[str, Person])
173+
assert_type(Person.objects.custom(), str)
174+
assert_type(Person.objects.get(), Person)
175+
176+
class Ordinary:
177+
objects = models.Manager[Person]()
178+
179+
assert_type(Ordinary.objects.get(), Person)
180+
"#,
181+
);
9182

10183
django_testcase!(
11184
test_model,

0 commit comments

Comments
 (0)