Repository navigation
fix Django: Instance vs Class access of attributes should return different types #4080 - #4336
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 D114011430. (Because this pull request was imported automatically, there will not be any future comments.) |
Merging this PR will not alter performance
Comparing Footnotes
|
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.
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.
stroxler
left a comment
There was a problem hiding this comment.
This has some behaviors we need to fix up before we can consider merging:
-
Models stop satisfying field protocols, and valid field overrides fail.
Descriptor preservation inclass_field.rschanges the representation used by compatibility checks, but those checks still reject descriptor-versus-ordinary-attribute comparisons and compare descriptor classes nominally.- A model with
name = models.CharField(...)no longer satisfies aProtocolwithname: str. - Replacing an abstract base’s
TextFieldwithCharFieldnow reportsbad-override, although Django permits it.
Compare the effective instance getter/setter contracts rather than the field representation.
- A model with
-
Relation assignments lose validation.
Setter specialization indjango.rscopiesForeignKey._pyi_private_set_type, which containsAny, into_ST. Assignments of an unrelated model, an integer, orNoneto a non-nullable foreign key become accepted; the base rejects them. Direct list assignment toManyToManyFieldalso becomes accepted.
Preserve resolved relation setter constraints instead of using placeholder stub types. Django runtime confirms assigning the wrong model raisesValueError. -
Explicit field type arguments are overwritten.
[This call]() passes onlypyrefly/pyrefly/lib/alt/class/class_field.rs
Lines 2187 to 2192 in 847c989
cls.class_object(), dropping existing arguments. A field annotatedCharField[str, str]now accepts123, and class access exposesCharField[str | int | Combinable, str]. Preserve explicit arguments; infer defaults only where necessary. Mypy rejects the integer assignment.
This comment has been minimized.
This comment has been minimized.
|
Diff from mypy_primer, showing the effect of this PR on open source code: ============================================================
SUMMARY
============================================================
Total: +21 new errors, -11 fixed errors
By preset: +6/-1 (default), +6/-2 (strict)
Projects with changes (2):
zulip: +20 -7
django-modern-rest: +1 -4
============================================================
FULL DIFF DETAILS
------------------------------------------------------------
zulip (https://github.com/zulip/zulip)
+ ERROR corporate/lib/stripe.py:1447:22-66: `-` is not supported between `int` and `None` [unsupported-operation]
- ERROR corporate/models/stripe_state.py:36:12-30: Implicit conversion of `Any | None` to `bool` is not allowed [implicit-bool]
+ ERROR corporate/models/stripe_state.py:36:17-30: The type of attribute `handler_error` is an explicit `Any` [explicit-any-use]
+ ERROR corporate/tests/test_stripe.py:4312:13-32: The type of this argument is an explicit `Any` [explicit-any-use]
+ ERROR corporate/tests/test_stripe.py:4312:19-32: The type of attribute `handler_error` is an explicit `Any` [explicit-any-use]
- ERROR zerver/actions/data_import.py:169:17-85: The type of this argument is an explicit `Any` [explicit-any-use]
- ERROR zerver/actions/data_import.py:169:61-85: The type of attribute `email_address_visibility` is an explicit `Any` [explicit-any-use]
+ ERROR zerver/actions/message_send.py:2032:20-32: `int` is not assignable to attribute `type` with type `AbstractMessage.MessageType` [bad-assignment]
- ERROR zerver/lib/import_realm.py:2242:18-29: The type of attribute `deactivated` is an explicit `Any` [explicit-any-use]
+ ERROR zerver/lib/realm_description.py:39:12-38: Returned type `str | None` is not assignable to declared return type `str` [bad-return]
- ERROR zerver/lib/send_email.py:546:5-548:15: Result of call expression is of type `tuple[int, dict[str, int]]` and is not used; assign to `_` if this is intentional [unused-call-result]
+ ERROR zerver/lib/send_email.py:546:19-26: The type of attribute `objects` is an explicit `Any` [explicit-any-use]
+ ERROR zerver/lib/send_email.py:546:27-33: The type of attribute `filter` is an explicit `Any` [explicit-any-use]
+ ERROR zerver/lib/send_email.py:548:7-13: The type of attribute `delete` is an explicit `Any` [explicit-any-use]
+ ERROR zerver/lib/send_email.py:553:16-78: The type of `subquery` is an explicit `Any` [explicit-any-use]
+ ERROR zerver/lib/send_email.py:553:30-37: The type of attribute `objects` is an explicit `Any` [explicit-any-use]
+ ERROR zerver/lib/send_email.py:553:38-44: The type of attribute `filter` is an explicit `Any` [explicit-any-use]
+ ERROR zerver/lib/send_email.py:554:67-75: The type of this argument is an explicit `Any` [explicit-any-use]
- 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]
+ ERROR zerver/tests/test_realm.py:1004:30-57: The type of this argument is an explicit `Any` [explicit-any-use]
+ ERROR zerver/tests/test_realm.py:1078:30-63: The type of this argument is an explicit `Any` [explicit-any-use]
+ ERROR zerver/tests/test_realm.py:1145:30-56: The type of this argument is an explicit `Any` [explicit-any-use]
+ ERROR zerver/tests/test_zulip_update_announcements.py:242:36-78: Object of class `NoneType` has no attribute `id` [missing-attribute]
+ ERROR zerver/tests/test_zulip_update_announcements.py:261:26-68: Object of class `NoneType` has no attribute `id` [missing-attribute]
+ ERROR zerver/tests/test_zulip_update_announcements.py:262:34-73: Argument `Any | None` is not assignable to parameter `stream` with type `Stream` in function `zerver.actions.streams.do_deactivate_stream` [bad-argument-type]
+ ERROR zerver/tests/test_zulip_update_announcements.py:270:17-46: The type of this argument is unknown because it comes from an error [unknown-from-error]
- ERROR zerver/views/registration.py:406:49-51: Cannot infer type of empty container; it will be treated as containing `Any` [implicit-any-empty-container]
django-modern-rest (https://github.com/wemake-services/django-modern-rest)
+ ERROR dmr/security/token/app/models.py:84:33-61: Unused `# type: ignore` comment [unused-type-ignore]
- ERROR dmr/security/token/app/models.py:104:16-25: Returning Any from function declared to return "AbstractBaseUser" [no-any-return-explicit]
- ERROR dmr/security/token/app/models.py:104:21-25: The type of attribute `user` is an explicit `Any` [explicit-any-use]
- ERROR dmr/security/token/app/models.py:109:16-25: Returning Any from function declared to return "AbstractBaseUser" [no-any-return-explicit]
- ERROR dmr/security/token/app/models.py:109:21-25: The type of attribute `user` is an explicit `Any` [explicit-any-use] |
stroxler
left a comment
There was a problem hiding this comment.
After looking at this more, I'm concerned that this is not a clean solution to the problem.
I'm a little fuzzy on Django internals, but if we need attribute access to vary by instance versus class, a normal descriptor can already do this via overloads. If we need a similar thing again, we maybe should just synthesize the appropriate overloads in a helper in django.rs?
Putting django-specific hooks directly in ClassField doesn't seem like the right approach.
Summary
Fixes #4080
Django fields now preserve descriptor semantics for class access while retaining inferred instance types,
_ST/_GTparameters are specialized from Django stubs.Test Plan
add test