add generics type parameters to ImmutableMultiDict

[adriangb]

Slice of #1403

The goal here is to make these data structures more reusable and fix some currently under specified type annotations that make type checkers unhappy downstream

[tomchristie]

In the trade-off between readability vs. type-strictness I'm personally happy enough to stick with the under-defined "Any" cases, because I just find it so awkwardly complicated to read through the annotations. (And because Any in these cases isn't incorrect, it's just under-specifying.)

I don't have strong objections to it, but just gonna pitch that one out there.

[adriangb]

Yeah, I see that side of this as well.

What for me makes this worth it is that:

1. These data structures have a lot of downstream consumers (both inside of Starlette and in anything that depends on Starlette
2. We're unlikely to change these classes much, they're pretty stable.
3. We also gain some readability, for example looking at class FormData(ImmutableMultiDict[str, Union[str, UploadFile]) you'll be able to tell that it's a mapping of string field names to strings or UploadFiles without looking at the docs (and IDEs will also give you this info)

So we do give up some readability in Starlette's codebase, but we will be improving legibility for downstream classes and consumers of the codebase.

But that's just my thought process / personal bias, I'll understand if others are not of the same opinion.

[adriangb]

As a motivating example of what this can do for users of Starlette and downstream packages:

from starlette.requests import Request

async def func(req: Request) -> str:
    form = await req.form()
    file_or_string = form["field"]
    if isinstance(file_or_string, str):
        return file_or_string
    return (await file_or_string.read()).decode()

Running mypy on this:

test.py:9: error: Statement is unreachable  [unreachable]
Found 1 error in 1 file (checked 1 source file)

Because mypy thinks that file_or_string is always a string.

And from an IDE's perspective:

I get no auto-complete for .read(), thus no auto-complete for .decode() as well as several false positives like a redundant isinstance check and returning an unknown type.

On the other hand, this is what it looks like on #1403 (which this current PR is working towards):

IDE examples are on VSCode + Pylance (which is what a large percentage of devs use nowadays)

[tomchristie]

Always appreciate seeing a good user-centred "this is why we might choose to make this change". 😎

[Kludex]

Just for reference about the "why": https://mypy.readthedocs.io/en/stable/common_issues.html#invariance-vs-covariance

[Kludex]

We don't know the returned value of the operator + when there's a sum of two ImmutableMultiDict. That's why we ignore this error here.

[Kludex]

This PR is clear to me, and after I've start using a more strict type checking environment, I do feel @adriangb 's pain.

I'm not sure if _T, _KT and _VT_co are the best names for those, even if this convention is spreadly used... Maybe DefaultType, KeyType and ValueType can improve the readability, and @tomchristie will see it with better eyes (?).

[adriangb]

Maybe DefaultType, KeyType and ValueType can improve the readability

I'm 👍 on this, happy to make the change or accept suggestions from either of you.

[adriangb]

Rename done

[abersheeran]

https://docs.python.org/3/library/collections.abc.html#collections-abstract-base-classes

I'm not quite sure if there are other benefits to implementing mixin methods like .get. MutableMapping already implements them. And they are already of the correct type.

[adriangb]

So we can just delete get here! Great point. This makes this a PR that (maybe) changes behavior instead of just typing, but I think that's worth it 😄

[adriangb]

Thank again for this, great catch!

[adriangb]

This is just to make mypy happy

[adriangb]

Just to make MyPy happy, previously it was picking up str from ImmutableMultiDict but now that ImmutableMultiDict is generic it was complaining about an unknown type.

[florimondmanca]

Also means we now get QueryParams(...).get(...) to be a str, rather than Any. Which is neat and correct, right?

[adriangb]

Yep! There's a lot of downstream work from this (like actually fixing Form), but I wanted to keep this PR as narrowly scoped as possible. Hopefully we can approve and merge those downstream PRs quicker once the basic structure for stronger typing is added via this PR.

[florimondmanca]

I do like this as well. I had already found myself in cases when the default typing on Form required me to do unecessary assert or isinstance checks to benefit from typing.