Use PEP 696 for model fields#3317
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
329fab2 to
5d6b276
Compare
This comment has been minimized.
This comment has been minimized.
|
Great work on this! It’s really impressive.! |
This comment was marked as outdated.
This comment was marked as outdated.
5d6b276 to
b8c5edb
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
@sobolevn A bunch of issues in I'm gonna be expanding the scope of this pr to do the |
This comment was marked as outdated.
This comment was marked as outdated.
We will be able to test out on a 16M LoC codebase at work when you're ready. (FYI @delfick) |
|
well, we only run mypy on 5.9 million lines of that (excluding blank lines and comments), but yes, our codebase is often a rich source of edge cases! |
|
Shh! You make it sound less impressive 😂 |
This comment was marked as resolved.
This comment was marked as resolved.
dd1711d to
8f638a5
Compare
This comment has been minimized.
This comment has been minimized.
8f638a5 to
ebae746
Compare
This comment has been minimized.
This comment has been minimized.
435477f to
91b7025
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
91b7025 to
c0676f8
Compare
This comment has been minimized.
This comment has been minimized.
|
I got around to giving this a shot on our codebase. I'm clearly gonna need to make it work against 6.0.3 first before I can be specific about this branch (might take me up to a few weeks), but that aside there's 84 unused type ignores with this branch (🥳 ) and seems to throw a bunch of new errors that indicate it actually understands what types things should be so that's nice. I think this will be great :) |
|
Awesome, I'll try to finish this later this week, would be awesome if you manage to check if some new issues are false positives or not 🙏 |
|
yeah, I imagine you'll have merged something by the time I get a chance to run our code on a version of django-stubs that doesn't give me several hundred errors before adding this change, but don't let that stop you. Once I catch up I'm sure we'll be able to point out oversights our code reveals :) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Just fixed 2 edge cases I found running this on my work code base in the last two commits. Other than that it is looking good mostly with ~100 unused type ignore and new correct errors surfacing in places that where previously not checked. However, it's a bit anoying for custom field to have to redeclare the full typevars with defaults like so: from typing import Literal, reveal_type
from typing_extensions import TypeVar # for `default=` (PEP 696)
from django.db import models
from django.db.models.expressions import Combinable
_ST = TypeVar("_ST", contravariant=True, default=float | int | str | Combinable)
_GT = TypeVar("_GT", covariant=True, default=int)
_NT = TypeVar("_NT", Literal[True], Literal[False], default=Literal[False])
class MyIntegerField(models.IntegerField[_ST, _GT, _NT]):
...I'm wondering if we should expose our typevars so that people can do something like that instead (if we go down that path, we probably need better names too) from django.db import models
from django_stubs_ext import ST_Int, GT_Int, NT
class MyIntegerField(models.IntegerField[ST_Int, GT_Int, NT]):
... |
sobolevn
left a comment
There was a problem hiding this comment.
This is an impressive work!
My main concern right now is that existing Field[A, B] annotations can change. For example, we use default=Literal[False] in the third type var, which can be rather limiting? Would it make sence to use Any as the default?
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.
|
Diff from mypy_primer, showing the effect of this PR on type check results on a corpus of open source code: zulip (https://github.com/zulip/zulip)
- zerver/models/users.py:62: error: Need type annotation for "enter_sends" [var-annotated]
- zerver/models/users.py:68: error: Need type annotation for "left_side_userlist" [var-annotated]
- zerver/models/users.py:69: error: Need type annotation for "default_language" [var-annotated]
- zerver/models/users.py:72: error: Need type annotation for "web_home_view" [var-annotated]
- zerver/models/users.py:73: error: Need type annotation for "web_escape_navigates_to_home_view" [var-annotated]
- zerver/models/users.py:74: error: Need type annotation for "fluid_layout_width" [var-annotated]
- zerver/models/users.py:75: error: Need type annotation for "high_contrast_mode" [var-annotated]
- zerver/models/users.py:76: error: Need type annotation for "translate_emoticons" [var-annotated]
- zerver/models/users.py:77: error: Need type annotation for "display_emoji_reaction_users" [var-annotated]
- zerver/models/users.py:78: error: Need type annotation for "twenty_four_hour_time" [var-annotated]
- zerver/models/users.py:79: error: Need type annotation for "starred_message_counts" [var-annotated]
- zerver/models/users.py:80: error: Need type annotation for "web_suggest_update_timezone" [var-annotated]
- zerver/models/users.py:85: error: Need type annotation for "color_scheme" [var-annotated]
- zerver/models/users.py:93: error: Need type annotation for "web_font_size_px" [var-annotated]
- zerver/models/users.py:94: error: Need type annotation for "web_line_height_percent" [var-annotated]
- zerver/models/users.py:99: error: Need type annotation for "web_animate_image_previews" [var-annotated]
- zerver/models/users.py:113: error: Need type annotation for "demote_inactive_streams" [var-annotated]
- zerver/models/users.py:117: error: Need type annotation for "web_left_sidebar_show_channel_folders" [var-annotated]
- zerver/models/users.py:132: error: Need type annotation for "web_mark_read_on_scroll_policy" [var-annotated]
- zerver/models/users.py:150: error: Need type annotation for "web_channel_default_view" [var-annotated]
- zerver/models/users.py:164: error: Need type annotation for "emojiset" [var-annotated]
- zerver/models/users.py:175: error: Need type annotation for "user_list_style" [var-annotated]
- zerver/models/users.py:186: error: Need type annotation for "web_stream_unreads_count_display_policy" [var-annotated]
- zerver/models/users.py:190: error: Need type annotation for "web_left_sidebar_unreads_count_summary" [var-annotated]
- zerver/models/users.py:194: error: Need type annotation for "web_navigate_to_sent_message" [var-annotated]
- zerver/models/users.py:198: error: Need type annotation for "email_notifications_batching_period_seconds" [var-annotated]
- zerver/models/users.py:201: error: Need type annotation for "enable_stream_desktop_notifications" [var-annotated]
- zerver/models/users.py:202: error: Need type annotation for "enable_stream_email_notifications" [var-annotated]
- zerver/models/users.py:203: error: Need type annotation for "enable_stream_push_notifications" [var-annotated]
- zerver/models/users.py:204: error: Need type annotation for "enable_stream_audible_notifications" [var-annotated]
- zerver/models/users.py:205: error: Need type annotation for "notification_sound" [var-annotated]
- zerver/models/users.py:206: error: Need type annotation for "wildcard_mentions_notify" [var-annotated]
- zerver/models/users.py:209: error: Need type annotation for "enable_followed_topic_desktop_notifications" [var-annotated]
- zerver/models/users.py:210: error: Need type annotation for "enable_followed_topic_email_notifications" [var-annotated]
- zerver/models/users.py:211: error: Need type annotation for "enable_followed_topic_push_notifications" [var-annotated]
- zerver/models/users.py:212: error: Need type annotation for "enable_followed_topic_audible_notifications" [var-annotated]
- zerver/models/users.py:213: error: Need type annotation for "enable_followed_topic_wildcard_mentions_notify" [var-annotated]
- zerver/models/users.py:216: error: Need type annotation for "enable_desktop_notifications" [var-annotated]
- zerver/models/users.py:217: error: Need type annotation for "pm_content_in_desktop_notifications" [var-annotated]
- zerver/models/users.py:218: error: Need type annotation for "enable_sounds" [var-annotated]
- zerver/models/users.py:219: error: Need type annotation for "enable_offline_email_notifications" [var-annotated]
- zerver/models/users.py:220: error: Need type annotation for "message_content_in_email_notifications" [var-annotated]
- zerver/models/users.py:221: error: Need type annotation for "enable_offline_push_notifications" [var-annotated]
- zerver/models/users.py:222: error: Need type annotation for "enable_online_push_notifications" [var-annotated]
- zerver/models/users.py:234: error: Need type annotation for "desktop_icon_count_display" [var-annotated]
- zerver/models/users.py:238: error: Need type annotation for "enable_digest_emails" [var-annotated]
- zerver/models/users.py:239: error: Need type annotation for "enable_login_emails" [var-annotated]
- zerver/models/users.py:240: error: Need type annotation for "enable_marketing_emails" [var-annotated]
- zerver/models/users.py:241: error: Need type annotation for "presence_enabled" [var-annotated]
- zerver/models/users.py:251: error: Need type annotation for "realm_name_in_email_notifications_policy" [var-annotated]
- zerver/models/users.py:272: error: Need type annotation for "automatically_follow_topics_policy" [var-annotated]
- zerver/models/users.py:275: error: Need type annotation for "automatically_unmute_topics_in_muted_streams_policy" [var-annotated]
- zerver/models/users.py:278: error: Need type annotation for "automatically_follow_topics_where_mentioned" [var-annotated]
- zerver/models/users.py:280: error: Need type annotation for "resolved_topic_notice_auto_read_policy" [var-annotated]
- zerver/models/users.py:287: error: Need type annotation for "enable_drafts_synchronization" [var-annotated]
- zerver/models/users.py:290: error: Need type annotation for "send_stream_typing_notifications" [var-annotated]
- zerver/models/users.py:291: error: Need type annotation for "send_private_typing_notifications" [var-annotated]
- zerver/models/users.py:292: error: Need type annotation for "send_read_receipts" [var-annotated]
- zerver/models/users.py:293: error: Need type annotation for "allow_private_data_export" [var-annotated]
- zerver/models/users.py:296: error: Need type annotation for "receives_typing_notifications" [var-annotated]
- zerver/models/users.py:300: error: Need type annotation for "web_inbox_show_channel_folders" [var-annotated]
- zerver/models/users.py:311: error: Need type annotation for "email_address_visibility" [var-annotated]
- zerver/models/users.py:326: error: Need type annotation for "hide_ai_features" [var-annotated]
- zerver/models/users.py:443: error: Need type annotation for "realm" [var-annotated]
- zerver/models/users.py:491: error: Need type annotation for "id" [var-annotated]
- zerver/models/users.py:509: error: Need type annotation for "delivery_email" [var-annotated]
- zerver/models/users.py:510: error: Need type annotation for "email" [var-annotated]
- zerver/models/users.py:512: error: Need type annotation for "realm" [var-annotated]
- zerver/models/users.py:521: error: Need type annotation for "full_name" [var-annotated]
- zerver/models/users.py:523: error: Need type annotation for "date_joined" [var-annotated]
- zerver/models/users.py:531: error: Need type annotation for "tos_version" [var-annotated]
- zerver/models/users.py:532: error: Need type annotation for "api_key" [var-annotated]
- zerver/models/users.py:538: error: Need type annotation for "uuid" [var-annotated]
- zerver/models/users.py:541: error: Need type annotation for "is_staff" [var-annotated]
- zerver/models/users.py:548: error: Need type annotation for "is_active" [var-annotated]
- zerver/models/users.py:553: error: Need type annotation for "is_deleted" [var-annotated]
- zerver/models/users.py:555: error: Need type annotation for "is_bot" [var-annotated]
- zerver/models/users.py:556: error: Need type annotation for "bot_type" [var-annotated]
- zerver/models/users.py:557: error: Need type annotation for "bot_owner" [var-annotated]
- zerver/models/users.py:569: error: Need type annotation for "role" [var-annotated]
- zerver/models/users.py:611: error: Need type annotation for "long_term_idle" [var-annotated]
- zerver/models/users.py:614: error: Need type annotation for "last_active_message_id" [var-annotated]
- zerver/models/users.py:621: error: Need type annotation for "is_mirror_dummy" [var-annotated]
- zerver/models/users.py:624: error: Need type annotation for "is_imported_stub" [var-annotated]
- zerver/models/users.py:628: error: Need type annotation for "can_forge_sender" [var-annotated]
- zerver/models/users.py:630: error: Need type annotation for "can_create_users" [var-annotated]
- zerver/models/users.py:632: error: Need type annotation for "can_change_user_emails" [var-annotated]
- zerver/models/users.py:635: error: Need type annotation for "last_reminder" [var-annotated]
- zerver/models/users.py:642: error: Need type annotation for "rate_limits" [var-annotated]
- zerver/models/users.py:645: error: Need type annotation for "default_sending_stream" [var-annotated]
- zerver/models/users.py:651: error: Need type annotation for "default_events_register_stream" [var-annotated]
- zerver/models/users.py:657: error: Need type annotation for "default_all_public_streams" [var-annotated]
- zerver/models/users.py:667: error: Need type annotation for "timezone" [var-annotated]
- zerver/models/users.py:678: error: Need type annotation for "avatar_source" [var-annotated]
- zerver/models/users.py:681: error: Need type annotation for "avatar_version" [var-annotated]
- zerver/models/users.py:685: error: Need type annotation for "avatar_hash" [var-annotated]
- zerver/models/users.py:1262: error: Need type annotation for "user" [var-annotated]
- zerver/models/users.py:1263: error: Need type annotation for "realm" [var-annotated]
- zerver/models/users.py:1264: error: Need type annotation for "date_created" [var-annotated]
- zerver/models/users.py:1267: error: Need type annotation for "external_auth_method_name" [var-annotated]
- zerver/models/users.py:1268: error: Need type annotation for "external_auth_id" [var-annotated]
- zerver/models/clients.py:13: error: Need type annotation for "id" [var-annotated]
- zerver/models/clients.py:14: error: Need type annotation for "name" [var-annotated]
- zerver/models/user_activity.py:22: error: Need type annotation for "user_profile" [var-annotated]
- zerver/models/user_activity.py:23: error: Need type annotation for "client" [var-annotated]
- zerver/models/user_activity.py:24: error: Need type annotation for "query" [var-annotated]
- zerver/models/user_activity.py:26: error: Need type annotation for "count" [var-annotated]
- zerver/models/user_activity.py:27: error: Need type annotation for "last_visit" [var-annotated]
- zerver/models/user_activity.py:36: error: Need type annotation for "user_profile" [var-annotated]
- zerver/models/user_activity.py:37: error: Need type annotation for "start" [var-annotated]
- zerver/models/user_activity.py:38: error: Need type annotation for "end" [var-annotated]
- zerver/models/recipients.py:38: error: Need type annotation for "id" [var-annotated]
- zerver/models/recipients.py:39: error: Need type annotation for "type_id" [var-annotated]
- zerver/models/recipients.py:40: error: Need type annotation for "type" [var-annotated]
- zerver/models/recipients.py:121: error: Need type annotation for "huddle_hash" [var-annotated]
- zerver/models/recipients.py:123: error: Need type annotation for "recipient" [var-annotated]
- zerver/models/recipients.py:125: error: Need type annotation for "group_size" [var-annotated]
- zerver/models/push_notifications.py:19: error: Need type annotation for "kind" [var-annotated]
- zerver/models/push_notifications.py:25: error: Need type annotation for "token" [var-annotated]
- zerver/models/push_notifications.py:29: error: Need type annotation for "last_updated" [var-annotated]
- zerver/models/push_notifications.py:32: error: Need type annotation for "ios_app_id" [var-annotated]
- zerver/models/push_notifications.py:40: error: Need type annotation for "user" [var-annotated]
- zerver/models/onboarding_steps.py:9: error: Need type annotation for "user" [var-annotated]
- zerver/models/onboarding_steps.py:10: error: Need type annotation for "onboarding_step" [var-annotated]
- zerver/models/onboarding_steps.py:11: error: Need type annotation for "timestamp" [var-annotated]
- zerver/models/navigation_views.py:14: error: Need type annotation for "user" [var-annotated]
- zerver/models/navigation_views.py:18: error: Need type annotation for "fragment" [var-annotated]
- zerver/models/navigation_views.py:21: error: Need type annotation for "is_pinned" [var-annotated]
- zerver/models/navigation_views.py:24: error: Need type annotation for "name" [var-annotated]
- zerver/models/muted_users.py:12: error: Need type annotation for "user_profile" [var-annotated]
- zerver/models/muted_users.py:13: error: Need type annotation for "muted_user" [var-annotated]
- zerver/models/muted_users.py:14: error: Need type annotation for "date_muted" [var-annotated]
- zerver/models/groups.py:44: error: Need type annotation for "realm" [var-annotated]
- zerver/models/groups.py:53: error: Need type annotation for "usergroup_ptr" [var-annotated]
- zerver/models/groups.py:65: error: Need type annotation for "name" [var-annotated]
- zerver/models/groups.py:66: error: Need type annotation for "description" [var-annotated]
- zerver/models/groups.py:67: error: Need type annotation for "date_created" [var-annotated]
- zerver/models/groups.py:68: error: Need type annotation for "creator" [var-annotated]
- zerver/models/groups.py:71: error: Need type annotation for "is_system_group" [var-annotated]
- zerver/models/groups.py:73: error: Need type annotation for "can_add_members_group" [var-annotated]
- zerver/models/groups.py:76: error: Need type annotation for "can_join_group" [var-annotated]
- zerver/models/groups.py:77: error: Need type annotation for "can_leave_group" [var-annotated]
- zerver/models/groups.py:78: error: Need type annotation for "can_manage_group" [var-annotated]
- zerver/models/groups.py:79: error: Need type annotation for "can_mention_group" [var-annotated]
- zerver/models/groups.py:82: error: Need type annotation for "can_remove_members_group" [var-annotated]
- zerver/models/groups.py:86: error: Need type annotation for "realm_for_sharding" [var-annotated]
- zerver/models/groups.py:87: error: Need type annotation for "deactivated" [var-annotated]
- zerver/models/groups.py:167: error: Need type annotation for "user_group" [var-annotated]
- zerver/models/groups.py:168: error: Need type annotation for "user_profile" [var-annotated]
- zerver/models/groups.py:175: error: Need type annotation for "supergroup" [var-annotated]
- zerver/models/groups.py:176: error: Need type annotation for "subgroup" [var-annotated]
- zerver/models/devices.py:19: error: Need type annotation for "user" [var-annotated]
- zerver/models/devices.py:26: error: Need type annotation for "push_key" [var-annotated]
- zerver/models/devices.py:28: error: Need type annotation for "push_key_id" [var-annotated]
- zerver/models/devices.py:31: error: Need type annotation for "push_token_id" [var-annotated]
- zerver/models/devices.py:33: error: Need type annotation for "pending_push_token_id" [var-annotated]
- zerver/models/devices.py:36: error: Need type annotation for "push_token_last_updated_timestamp" [var-annotated]
- zerver/models/devices.py:42: error: Need type annotation for "push_token_kind" [var-annotated]
- zerver/models/devices.py:50: error: Need type annotation for "push_registration_error_code" [var-annotated]
- zerver/models/bots.py:28: error: Need type annotation for "name" [var-annotated]
- zerver/models/bots.py:32: error: Need type annotation for "user_profile" [var-annotated]
- zerver/models/bots.py:33: error: Need type annotation for "base_url" [var-annotated]
- zerver/models/bots.py:34: error: Need type annotation for "token" [var-annotated]
- zerver/models/bots.py:36: error: Need type annotation for "interface" [var-annotated]
- zerver/models/bots.py:66: error: Need type annotation for "bot_profile" [var-annotated]
... (truncated 554 lines) ...
|
ngnpope
left a comment
There was a problem hiding this comment.
A couple of comments about test placement below.
A quick note that we've tried this on our large repo and things mostly seem good, but there are some things not quite right. We currently have 235 new errors with the following breakdown:
10 [arg-type]
12 [assignment]
110 [attr-defined]
5 [call-overload]
44 [misc]
14 [operator]
1 [return-value]
3 [type-var]
12 [union-attr]
2 [unreachable]
10 [unused-ignore]
12 [var-annotated]
The first thing I want to look into are the attr-defined. The majority of these look to be an issue with GenericRelation where I think that it's not seeing this as a QuerySet. We're typically seeing something like:
error: "MyModel" has no attribute "aggregate" [attr-defined]
error: "MyModel" has no attribute "exists" [attr-defined]
error: "MyModel" has no attribute "filter" [attr-defined]
error: "MyModel" has no attribute "get" [attr-defined]
...
I'm not currently sure whether this is something that would have been Any before and is now resolving to something else, and have yet to come up with a reproducer. But I wanted to highlight it now in case anyone else is able to get around to working something out before I can.
Once that's been resolved we can sift the rest. I suspect some of them will be merely due to needing to specify defaults for type variables of field subclasses. But starting with the biggest issue makes sense.
There was a problem hiding this comment.
Should this come under tests/assert_type/contrib/postgres/fields/test_array.py?
There was a problem hiding this comment.
Should this come under tests/assert_type/contrib/postgres/fields/test_ranges.py?
I have made things!
Replace
_pyi_private_set_type/_pyi_private_get_typeattributes on field stubs with PEP 696TypeVardefaults. This makes field set/get types visible to all type checkers, not just mypy with the plugin.This is a big PR, sry for that but the plugin code is so entangled I did not manage to make a smaller chunk.
I've rebased to make it a bit more digestible, hopefully that helps.
I'm pretty convinced we can improve the stubs of
ForeignKey/ManyToManyand friends using trick similar to what is done toArrayFieldin this PR and remove even more plugin code.Most of the code in
transform_into_proper_return_typeshould go I think.Related issues
Fixes #766
Fixes #1264
Fixes #1900
Fixes #2043
Fixes #2590
Fixes #2724
Improves #579
Handling
null=TrueThe important part is to make
nullworking (and laterprimary_key). Two approches have emerged in the other issue discussion, I've investigated further with various type checker and was able to make the_NTtypevar approach work with every typechecker I know off!Using
__new__overloads (django-typesis doing that)Code snippet
Works with:
__new__overrides on every custom field (and need to be quoted)__new__overrides on every custom fieldNot with:
__new__overrides on every custom field__new__overrides on every custom field, (but a false positive error on the__new__definition)Using third type variable
_NTtoFieldencoding nullability asLiteral[True]orLiteral[False]Code snippet
Works with all type-checkers:
---> todo
Custom field typing
Currently, bare custom field (
class HTMLField(models.TextField): ...) are an issue because the TypeVar default machinery makesbody_nullable = HTMLField(null=True)resolve toTextField[str, str, Literal[False]], causing this kind of errorsThe mypy plugin currently makes this work by reparametrizing HTMLField to remain generic in
_ST_Text,_GT_Text,_NT, so mypy's natural inference picks up_NT = Literal[True]from the call. But other typecheckers cannot do that so it means that for them, they have to explicitely fix their custom fieldsfollowing the recommendation in the README ie
That is probably something we can live with because the workaround is documented (and I say workaround, it's more of a regular typing pattern, omitting generics is often an issue and flagged in strict modes)
Alternative I considered
Drop
default=Literal[False]from_NThis Violates PEP 696 ordering (no-default TypeVar can't follow defaulted ones) so we have to either:
- ❌ reorder typevars to
Generic[_NT, _ST, _GT]which would completely change the meaning of existingField[X, Y]annotations, it feels bad- drop the
_ST/_GTdefaults we just added and inline the defaults. This would make custom field subclassing anything other thanFieldvery difficult as the_GT/_STare not reachable anymore. People would have to redeclare the whole__get__/__set__machinery which seems bad. It also requires that we donull: _NT = Falseto not regress Bare instantiation (models.IntegerField()->_NT=Literal[False]) but this only works with pyright and pyrefly.Passing a default value to a TypeVar is not supported by mypy:
Unkown_NT = TypeVar("_NT", bound=bool, default=Any)
But this breaks narrowing to
_GTor_GT | Nonebased onnull=...becauseLiteral[False]/Literal[True]become bool which is a blockerFollowups
# TODO: ty should reject that too