Large update

[sterliakov]

Major fix in different areas

I found out that django.forms were typed very poorly and forked to improve it. Then I wrote a small script to check this stubs with real code (targeting 3.2 because stubs are created for this version): it a) reports all incompatible stubs and b) allows me to run mypy on Django itself. Django code style is far from type safety, of course, but "Incompatible return value", for instance, is often a bad sign.

Major steps:

• Sync outdated and deprecated code (SafeText, OrderedDict)
• Fix wrong signatures (where some arguments were renamed/added/removed)
• Fix wrong decorators (missing @property)
• Replace None as return type or variable type where needed (often within generator functions)
• Define common type aliases and reuse them (more DRY)
• Where possible, replace Any with more specific type.
• Replace WSGIRequest with HttpRequest in most files (not related to handlers).
• When callable is assigned as instance attribute, use protocol instead of Callable to avoid mypy confusion (false-positives)

I also added a few new test cases (and will add more for forms and views, they are not really covered now).

Related issues

• Closes PasswordValidator Protocol methods do not match #848
• Closes Return type for force_bytes() on protected types is incorrect #834
• Closes utils.timesince.timesince should accept an optional integer depth #895
• Closes RemoteUserBackend.configure_user uses User instead of AbstractUser #744
• Refs MultipleObjectMixin and SingleObjectMixin has incompatible types for queryset and context_object_name #873

[intgr]

This is also related to the change of MultiValueDict.dict(): technically correct, but for most use cases, a step backward.

The Django tutorial contains this example:

def vote(request, question_id):
    question = get_object_or_404(Question, pk=question_id)
    try:
        selected_choice = question.choice_set.get(pk=request.POST['choice'])
        #                                         ^^^^^^^^^^^^^^^^^^^^^^^^^
    except (KeyError, Choice.DoesNotExist):
        ...

But now accessing .get(pk=request.POST['choice']) will cause mypy errors like:

Incompatible type for lookup 'pk': (got "Union[str, List[object]]", expected "Union[str, int]") [misc]

What's your proposed solution for this common usage pattern?

One could add a guard like if isinstance(request.POST['choice'], str): or assert. But that gets very unwieldy and as explained in #899 (comment), the list return cannot really occur with request.GET and request.POST.

[intgr]

One option would be to keep MultiValueDict as you have changed, but override QueryDict back to the old behavior of assuming no empty lists, for the request.POST/GET use case. Is that a compromise you could live with?

Although that way this issue would still affect request.FILES.

[sterliakov]

Overriding QueryDict to old behavior could be a better choice, also I don't insist on preserving this change if you think it's troublesome and doesn't really help.

However, I should also note that there is QueryDict.get interface, that works exactly like dict.get, returning None or default if key is missing or corresponds to empty list.

Also I have one more solution, which may be a massive overhead but should provide very descriptive interface: define in stubs fake _ImmutableQueryDict with get methods that behave as expected (getitem: (_K) -> _V; it is perfectly valid assuming that this querydict was created by django and is coming from request.GET(POST)), modifying methods disallowed (__setitem__: (_K, _V) -> NoReturn etc.) and copy: () -> QueryDict. This way we can additionally detect almost all disallowed operations (with --warn-unreachable) while keeping edge-cases checked for mutable copy. It does not eliminate issue with request.FILES, but for this using request.FILES.get(key) is almost always convenient.

[intgr]

Hmm, yes, the _ImmutableQueryDict option may work. Although it looks like there are some situations where mutation is considered legal in Django:

class HttpRequest:
    def __init__(self):
        self.GET = QueryDict(mutable=True)
        self.POST = QueryDict(mutable=True)

Apparently this was introduced in django/django#2778 and the reason given is "GET and POST on a vanilla HttpRequest object to be mutable QueryDicts (mutable because the Django tests, and probably many third party tests were expecting it)."

Not sure if this is relevant any more in 2022.

[sterliakov]

Interesting, I haven't noticed that before. HttpRequest.{GET, POST} are really mutable, while {W, A}SGIRequest.{GET, POST} are not. As far as I understand (quick grep 'HttpRequest('), it's used directly only in tests. We don't use {W, A}SGIRequest in stubs because it depends on deploy method, not on code itself, so we have to make HttpRequest.GET immutable too in stubs. I see two options:

• add _CommonHttpRequest(HttpRequest) with immutable GET/POST and use it in stubs instead of HttpRequest. Will require some work and is hard to maintain.
• make HttpRequest.{GET, POST} immutable. It will break code that instantiates HttpRequest manually.

I think that the latter is a reasonable choice, but I'm not sure about it. Is there any scenario when somebody can intentionally create HttpRequest manually and modify its GET/POST data?

[sterliakov]

I found one more solution which looks very promising. mypy does not 100% support this currently, but the same can be achieved with a tweak (requiring type: ignore on __init__ in stub, because mypy doesn't consider it valid, but working outside like a charm):

class _ImmutableQueryDict(QueryDict): ...  # declaring immutability here
class HttpRequest:
    GET: _ImmutableQueryDict
    POST: _ImmutableQueryDict
    # Magic happens here:
    def __init__(self: _MutableHttpRequest) -> None: ...  # type: ignore
    # If mypy issue #1020 is resolved, the following (better) will work:
    # def __new__(cls, *args, **kwargs) -> _MutableHttpRequest: ...
class _MutableHttpRequest(HttpRequest):
    GET: QueryDict  # type: ignore
    POST: QueryDict  # type: ignore

And now

from django.http.request import HttpRequest

request = HttpRequest()
reveal_type(request)  # N: Revealed type is "django.http.request._MutableHttpRequest"
reveal_type(request.GET)  # N: Revealed type is "django.http.request.QueryDict"
request.GET['foo'] = 'bar'

def mk_request() -> HttpRequest:
    return HttpRequest()

req = mk_request()
reveal_type(req)  # N: Revealed type is "django.http.request.HttpRequest"
reveal_type(req.GET)  # N: Revealed type is "django.http.request._ImmutableQueryDict"
req.GET['foo'] = 'bar'

This way any code that does for some reason request = HttpRequest() will get mutable version while all request: HttpRequest will have immutable dict.

[intgr]

If mypy issue #1020 is resolved, the following (better) will work

Hmm, this issue python/mypy#1020 is closed. Wasn't it solved by python/mypy#7188 already?

[intgr]

Anyway this solution is a bit hacky, but seems fine to me. Of course in the end it's up to django-stubs reviewers.

If your changes are in a good enough shape, I would gladly test out your branch (or maybe open a draft PR already).

[intgr]

Should this derive from HttpResponse instead?

I have a test that accesses resp.content and it works as expected. The content attribute is defined in HttpResponse not HttpResponseBase.

[sterliakov]

No: it can be FileResponse that inherits from HttpResponseBase.

[intgr]

Looks like the __iter__ and getvalue() APIs are "the right way" to access the response content in a universal manner -- both StreamingHttpResponse and HttpResponse define these methods.

But since these methods aren't defined on the level of HttpResponseBase either, this is still an issue.

Should we add __iter__ and getvalue() to HttpResponseBase since presumably all subclasses are expected to implement these methods?

[sterliakov]

Yeah, it should be great solution, I don't see any drawbacks. I'll add it to the patch.

[intgr]

This also allows RegexPattern, RoutePattern. I have opened a PR for this: #941

[intgr]

This is not correct. ValidationError accepts arbitrarily nested data structures as the message argument.

For example this correct code from django-rest-framework produces a false positive error: https://github.com/encode/django-rest-framework/blob/3.13.1/tests/test_fields.py#L2428-L2433

Mypy does not support recursive type aliases, so it's impossible to describe this type accurately AFAIK.

For now I think it's preferable to revert than to have false positives. I have opened PR #943 for that. If you have any thoughts, please comment there.