Inline typechecking for exceptions and add typechecking for collections

[haikuginger]

This is a first step towards moving towards inline types for all of urllib3. For now, this just fixes and extends the existing types for the exceptions module, but adds typing for the _collections and utils.response modules.

Note that because not all of our codebase is covered by typechecking, we can be pretty sure that these types are internally consistent, but not that they're correct. We'll need a lot more type coverage before we can be sure of that. For now, existing tests pass with minimal modifications, which is possibly the best we can do.

[codecov]

Codecov Report

Merging #2115 (07671b7) into main (dc6284c) will decrease coverage by 0.13%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##             main    #2115      +/-   ##
==========================================
- Coverage   99.95%   99.82%   -0.14%     
==========================================
  Files          25       26       +1     
  Lines        2221     2260      +39     
==========================================
+ Hits         2220     2256      +36     
- Misses          1        4       +3     

Impacted Files	Coverage Δ
src/urllib3/_compat.py	0.00% <0.00%> (ø)
src/urllib3/_collections.py	100.00% <100.00%> (ø)
src/urllib3/exceptions.py	100.00% <100.00%> (ø)
src/urllib3/response.py	100.00% <100.00%> (ø)
src/urllib3/util/response.py	100.00% <100.00%> (ø)
src/urllib3/util/util.py	95.45% <0.00%> (+0.45%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc6284c...07671b7. Read the comment docs.

[haikuginger]

I figured since we're going to start protecting this with typechecking we could remove the runtime check on if attributes exist. Can revert though.

[sethmlarson]

This is all fine since we have the isinstance check, so we don't have to worry about users sending weird objects into this function

[sethmlarson]

Thanks for this, some early comments more coming later.

[franekmagiera]

Hi, took a look at _collections.py. Maybe it would make sense to include a comment that when Mapping or Iterable is passed to HTTPHeaderDict it is assumed that it is Mapping[str, str] and iterable of string pairs? Small thing, but the only one that caught my attention. Overall it looks OK to me, nice work :)

[franekmagiera]

Also, regarding making typing extensions a runtime dependency - if #1985 was to be implemented using Protocol and not abc, wouldn't it be necessary to have typing extensions as a runtime dependency?

[haikuginger]

Also, regarding making typing extensions a runtime dependency - if #1985 was to be implemented using Protocol and not abc, wouldn't it be necessary to have typing extensions as a runtime dependency?

Are you talking about how we describe the API? If so, then not strictly speaking. If we implemented a protocol in an if TYPE_CHECKING: block, we could refer to it via string annotations in places where it's returned or needs to be passed in. E.g.:

if TYPE_CHECKING:
    class HasSomeValuesInAList(Protocol):
        that_list_i_told_you_about: List[int]

@dataclass
class ImplementationOfAbove:
    some_other_thing: str
    that_list_i_told_you_about: List[int]

def does_stuff_with_those_things(carries_vals: "HasSomeValuesInAList") -> None:
    for item in carries_vals.that_list_i_told_you_about:
        print(item)

# Valid
does_stuff_with_those_things(ImplementationOfAbove("the string", [1, 2, 3, 4, 5]))

Using string annotations results in a ForwardRef being used rather than the actual class in question, which means that the actual class in question can not exist at runtime and it'll be fine—as long as it's there when the typechecker goes looking.

[haikuginger]

Tagging @SethMichaelLarson for re-review. I think this is ready to merge.

[sethmlarson]

Thanks for the changes, more comments for you!

[sethmlarson]

Wouldn't this override need to be Union[List[str], DefaultType where DefaultType is whatever type default is?

[haikuginger]

Yeah, if we want to support some default that isn't List[str], we'd need to do this:

@overload
def getlist(self, key: str, default: _DT) -> Union[List[str], _DT]]: ...

(in which case Union[List[str], _DT] collapses to List[str] in situations where _DT can be proven to be List[str])

The question is whether we want to support passing a default that isn't List[str]; I can take a look through the existing codebase to see what we do and where (the most likely case is that we pass in None in some places), and at typeshed to see what they do for builtins that accept defaults.

[sethmlarson]

LGTM, thanks for moving this mountain! 🎉