Recover after #909

[sterliakov]

Fix revealed in #909 problems

Note on QueryDict support

Now http.request.QueryDict mutability is checked with plugin. When it is instantiated with mutable=True or obtained from query_dict.copy(), it is mutable, otherwise — immutable. mypy will report error on the line where immutable version is modified and report any following code as unreachable.

"Convenient" signatures for __getitem__: (str) -> str and dict: () -> dict[str, str] are guaranteed only for immutable instances.

HttpRequest.GET and HttpRequest.POST are immutable in most cases. The only exception: when request is directly instantiated, it has mutable parts.

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'  # E: This QueryDict is immutable.
x = 1  # E: Statement is unreachable

Related issues

Refs #924
Fixes #916
Fixes #893

[sobolevn]

Sorry, I fail to understand how this works.

None of these calls use .copy or mutable=True. Why is there any difference?

[sterliakov]

My bad, probably I haven't explained that good enough.

We assume that anything annotated as HttpRequest is referring to {W,A}SGIRequest and thus has immutable GET and POST. However, this assumption doesn't hold for HttpRequest itself that has mutable dicts. So when we instantiate HttpRequest directly, overridden __new__ returns fake _MutableHttpRequest class, so the first part of case is actually operating on it (see reveal_type there). The second part works with HttpRequest and assumes it's immutable.

[sobolevn]

Do you think that having _MutableHttpRequest is worth it?
In my experience, we almost never do such things.

I fear that it complicates our code quite a bit.

[intgr]

@sobolevn For background, the reason of this complexity is not distinguishing between mutable/immutable QueryDict, but about what types QueryDict methods can return.

1. As streliakov points out in this comment, the dict() and __getitem__() methods of QueryDict and MultiValueDict may return str as well as empty list (but never a list with length>0).

   IMO this is crazy behavior on Django's part -- it should throw KeyError -- but nothing we can do about that. This isn't mentioned in official Django documentation, but is mentioned in MultiValueDict docstring: https://github.com/django/django/blob/main/django/utils/datastructures.py#L78-L82

2. However, the vast majority of code that uses QueryDict is within the request lifecycle of Django. In these cases, the empty lists cannot appear. So dict() and __getitem__() can safely be assumed to only have str values.

3. Large amounts of code rely on the # 2 assumption, even example code from Django's official tutorial (explained here).

4. The QueryDict attributes of Django request are created with mutable=False

So there are three options:

• We could have one QueryDict class and these methods so they always return Union[str, List]. This is the situation after Large update #909. But this causes false positive errors in a significant amount of idiomatic Django code, since most code assumes list return can not appear in request.GET["key"]
• Do the opposite, have one QueryDict class, and hint these methods as returning str. This is not strictly correct typing, although the list return is a rare fringe case.
• A mix of the above, which is what this PR does. In Django request lifecycle, use the magic _ImmutableQueryDict type, which only holds str values. In other cases, assume mutable QueryDict may also return empty list for values.

I would prefer the second or third option out of these three.

[intgr]

Thanks! I can confirm that this PR fixes all the regressions I saw in #909, and introduces no new regressions.

(I would have tested sooner, but I was not made aware of this PR -- feel free to ping me directly next time).

[sobolevn]

Ok then, thanks a lot to both of you! 👍

[intgr]

django-stubs still supports Python 3.7, but the / positional-only parameter syntax is supported only in Python 3.8+.

Opened PR #945 to fix this.