Repository navigation
fix Django: assigning a query expression (F(), Value()) to a model field is rejected #5322 - #5335
asukaminato0721 wants to merge 2 commits into
Conversation
|
This pull request has been imported. If you are a Meta employee, you can view this in D127050295. (Because this pull request was imported automatically, there will not be any future comments.) |
This comment has been minimized.
This comment has been minimized.
stroxler
left a comment
There was a problem hiding this comment.
Thanks! I think this causes a couple of behavior regressions based on a round of review:
Ordinary assignments lose nullable-field narrowing
The new setter returns narrow: false for every Django-field assignment—not just expressions. For an IntegerField(null=True):
book.pages = 1
return book.pages + 1This now reports an unsupported operation involving None. Nullable strings and foreign keys regress similarly. It also breaks the common “if missing, assign a default, then use it” pattern, consistent with the added nullable-access errors in the primer report.
Fix: preserve assignment narrowing when the RHS fits the read type; suppress it for expressions or coercible values outside that type.
Choice-restricted fields accept values contradicting their read type
Only the getter is narrowed by narrow_charfield_choices. The setter remains str | int | Combinable, so this becomes accepted:
# suit has choices "A" and "B"
card.suit = "INVALID"
assert_type(card.suit, Literal["A", "B"])Previously the assignment was rejected. This makes Pyrefly’s existing finite-choice guarantee inconsistent; it is not a claim that Django validates choices during assignment.
Fix: retain choice restrictions for ordinary writes while permitting expressions, or deliberately revise the finite-choice read guarantee.
Expression assignments still fail inside model methods
The new branch requires instance_class, which attribute resolution does not supply for implicit self or bounded type variables:
class Counter(models.Model):
count = models.IntegerField()
def bump(self) -> None:
self.count = F("count") + 1 # Still rejectedAn explicitly annotated self: Counter works; def update[T: Counter](counter: T) still fails. This is an incomplete fix rather than a new regression.
Fix: carry the write type through attribute lookup instead of recovering it through a second Django-specific member lookup. That also avoids duplicate lookup and instantiation.
|
Diff from mypy_primer, showing the effect of this PR on open source code: ============================================================
SUMMARY
============================================================
Total: +0 new errors, -1 fixed errors
By preset: +0/-1 (default), +0/-1 (strict)
Projects with changes (1):
zulip: +0 -1
============================================================
FULL DIFF DETAILS
------------------------------------------------------------
zulip (https://github.com/zulip/zulip)
- ERROR zerver/lib/test_classes.py:2859:28-44: `Literal['6cde5f7a-1f7e-4978-9716-49f69ebfc9fe']` is not assignable to attribute `uuid` with type `UUID` [bad-assignment] |
Summary
Fixes #5322
Django fields now accept F(), Value(), and combined expressions while preserving their read types.
Test Plan
add test