requirements: Add django-stubs.

[PIG208]

#11560

• Mismatching type signature (_CodeCallable uses BaseDatabaseSchemaEditor instead of DatabaseSchemaEditor)
  • Fix RunPython type errors in migration scripts #22141
• Cursor.execute expects a str
  • retention: Guarantee type-safeness when calling move_rows. #22345
• None-check required
  • django-stubs: Improve typing with more assertions #22157
• mute_stream arg type mismatch
  • django-stubs: Fix miscellaneous typing errors #22245
• Test client incompatible types
  • tests: Refactor test client wrappers with explicitly typed fields. #22247
• Model fields type mismatch
• AbstractBaseUser missing attributes
  • django-stubs: Improve typing with more assertions #22157
• AsyncDjangoHandler type errors
  • django-stubs: Fix miscellaneous typing errors #22245
• HttpRequest missing attributes
  • django-stubs: Fix attribute errors with HttpRequest #22153
• HttpResponse missing attributes
  • typing: Access url via key "Location" instead of attribute "url". #22152
  • django-stubs: Fix HttpResponse missing attribute error. #22185
  • django-stubs: Replace HttpResponse with _MonkeyPatchedWSGIResponse. #22209
• Message missing attributes
• Realm missing attributes
• RealmEmoji missing attributes
  • django-stubs: Fix miscellaneous typing errors #22245
• RedisUtilsTest missing attributes
  • django-stubs: Fix miscellaneous typing errors #22245
• UserProfile missing attributes
• QuerySet related
  • django-stubs: Tighten function signatures with generic Queryset. #22292
• Incompatible Promise
• Message is not indexable
  • django-stubs: Fix miscellaneous typing errors #22245
• UserProfile is not indexable
  • django-stubs: Fix miscellaneous typing errors #22245
• UserProfile is not str
• Settings missing attributes
  • requirements: Add django-stubs. #18777
• RealmAuditLog incompatible type
  • Migrate extra_data field of RealmAuditLog to JSONField #18391
• QueryDict is immutable
  • requirements: Add django-stubs. #18777
• HttpResponse incompatible return type
  • django-stubs: Replace HttpResponse with _MonkeyPatchedWSGIResponse. #22209
• QuerySet incompatible return type
  • django-stubs: Tighten function signatures with generic Queryset. #22292
• QuerySet generic
  • django-stubs: Tighten function signatures with generic Queryset. #22292
• Type narrowing
  • django-stubs: Fix miscellaneous typing errors #22245
• Other
  • django-stubs: Fix miscellaneous typing errors #22245

[andersk]

update-locked-requirements needs to be run in an Ubuntu 18.04 environment (not Ubuntu 20.04).

And this is clearly going to be a backwards-incompatible provision update, so PROVISION_VERSION will need a major bump.

[PIG208]

Oops, this is accidentally closed by an empty push.

[timabbott]

There's 2K errors, but if you look at them, a huge portion are copies of a single confusion. E.g. we frequently access the .realm_id attribute of Django model objects with a UserProfile object; that attribute should be being implicitly generated by the ORM integration.

My guess is the problem is you haven't installed the plugin: https://github.com/typeddjango/django-stubs#installation

which I suspect exists to implicitly add those fields.

[timabbott]

I'm pushing back to this PR with two commits: the requirements change tweaked to also configure the mypy plugin, which has 1999 errors. And then there's a new commit that brings that down to 1946 (fixes 53 errors) by adding some missing type declarations in test_auth_backends.py, and passes mypy in master (so we happily could merge it right now). Those missing type annotations were a type-checking bug -- the base class accesses values/functions that it expects the subclass to define, but didn't tell the type system about that intent. So we shouldn't think of 2K errors as scary or intimidating; the nature of mypy is that something incorrectly typed that is used in many places results in large numbers of copies of a given error.

A good strategy for this project will be to debug with the full branch, generate commits that fix type annotations issues in a way that's correct (especially those that cause 50 errors with the Django stubs included!), and then incrementally merge those as we can. At the same time, we'll accumulate some commits that only work with the Django stubs package, e.g. because they use types that it adds or would be a regressions without the package. One needs to switch branches and provisioning to run the different check modes, but Zulip's provision caching makes that only take a few seconds.

A few large clusters I notice when skimming:

• Roughly 800 of the errors seem to be in zerver/models.py, resulting from the fact that we write # type: bool for our BooleanField attributes, which was the best approximation for typing Django model field attributes without the mypy plugin, but is wrong with it (the plugin will correct the type). So quite possibly half the errors with this branch will be fixed by just deleting those type: bool type variable annotations; that commit will need to merge late, though, since doing that deletion would be a regression without the plugin.
• In zerver/middleware.py and various other places, we monkey-patch extra fields onto HttpRequest objects. What we should be doing is declaring a ZulipHttpRequest type extending the Django HttpRequest in zerver/lib/types.py and using that type.
• In the markdown processor / message send path, we monkey-patch extra fields onto Message objects. What we should be doing is having those functions return a RenderedMessage type that explicitly declares those extra fields.

I think this is actually really promising!

[PIG208]

update 1946 -> 1892
I have refactored the render_markdown function and reduces approximately 50 errors caused by the untyped additional properties added to the message object. It has passed mypy on master. However, an additional type ignore needs to be added to rendered_message: RenderedMessage = message in do_render_markdown as we can't avoid an explicit type cast when trying to add properties that don't exist on Message before. (But this will causes error on master because the type ignore is "unused")

[PIG208]

update 1907 (rebased to master) -> 1889
I have refactored the functions related to render_incoming_message to make sure that the additional properties brought by render_markdown are now referred via rendered_message of type RenderedMessage instead of the message object. This effectively eliminated "Message" has no attribute error for those attributes related to message rendering.

[PIG208]

Because this PR is going to be huge and it is unlikely to be completed without running into merge conflicts, perhaps we can start reviewing & merging part of it on a commit-by-commit basis.

[timabbott]

Because this PR is going to be huge and it is unlikely to be completed without running into merge conflicts, perhaps we can start reviewing & merging part of it on a commit-by-commit basis.

Yeah, I completely agree. And really, we can keep this PR as mostly the commits that we'll merge at the end, and submit commits that work in master as their own PRs. I've merged 56e6bd7164131d198a528bc450364971be665c87 as a first step in that direction.

[andersk]

I’ve reordered and squashed some of the commits to preserve bisectability. I think this is now ready.

[timabbott]

This is fine for now but I'd be curious how many tests change these vs. declaring a new HostRequestMock for new values; it seems possible we could either try to purge the modification pattern or make a MutableHostRequestMock used by a small number of tests that plan to do that.

[PIG208]

The main issue here is that HostRequestMock itself relies on this mutability of POST to initialize it in the first place. POST is mutated all the time via post_data.

For GET, HostRequestMock does not modify it in __init__, but we do have one or two test cases modifying its value, which is not as often.

[timabbott]

Can you add a comment documenting what this does?

https://pypi.org/project/django-stubs-ext/ explains it allows runtime monkey-patching... but it'd be helpful to have the context on why Zulip needs that behavior.

[PIG208]

As far as I know, we only really need this for models.Lookup. Added comments on this.

[timabbott]

Does this hunk belong in the previous commit?

[PIG208]

Fixed.

[andersk]

I had actually moved that out of the previous commit intentionally, as the previous commit doesn’t type check with this hunk.

[PIG208]

I see. event_last_message_id = assert_is_not_none(log_entry.event_last_message_id) fails.

[timabbott]

I didn't full read models: Implicitly type model fields with django-stubs, which at least needs a commit message tweak; and also I guess the last commit should close the original issue? But I didn't see any significant impediments to merging this (requested a few comment tweaks above).

[PIG208]

For reviewing purposes, this is the script that generates most of dc1aa60.

#!/usr/bin/bash

# This removes all the type annnotations for model fields

set -ex

TARGETS=$(find . -name models.py)
sed -i "s|:.*\( = models.*\)|\1|g" $TARGETS

[PIG208]

I chose to make requirements: Add django-stubs and configure plugin. close the original issue, since the last two commits are only follow-ups for idiomatic improvements.

[timabbott]

This looks good; did some small edits to the comments and I've tagged this to merge once CI finishes.

Huge thanks for doing this project! It's been a long slog and I think very meaningful for the codebase.

[timabbott]

Can you post in #backend an announcement about this having been integrated? And do an edit pass on our mypy documentation to reference that we now do this, perhaps with examples/links that may be helpful.