Use strict optional checking in req_install.py

[hauntsaninja]

Suggested by pradyunsg in #11374

Since half of the API of this class depends on self.req not being None, it seems like we should just prevent users from passing None here. However, I wasn't able to make that change. Maybe someone more familiar with the codebase would be able to assess whether that's feasible.

Rather than sprinkle asserts everywhere, I added "checked" properties. I find this less ad hoc and easier to adapt if e.g. we're able to make self.req never None in the future.

There are now some code paths where we have asserts that we didn't before. I relied on other type hints in pip's code base to be accurate. If that is not the case and we actually relied on some function being able to accept None when not typed as such, we may hit these asserts. But hopefully tests would catch such a thing.

[pradyunsg]

Thanks for picking this up @hauntsaninja!

Rather than sprinkle asserts everywhere, I added "checked" properties.

Personally, I don't think checked properties are a good way to do this. I'd much prefer to have asserts sprinkled in various places for this. That'll do a better job of making it explicit what the contract is, and can help guide refactors as well. FWIW, I'll say that you picked the most difficult one first. ;)

[hauntsaninja]

Sure thing! Sounds good and can do. I'll wait to hear back from Paul to make sure he's onboard with adding asserts, just so I don't end up wasting my time with this!

[pradyunsg]

First of all, I'm honestly rather appalled by mypy's current behaviour here. We tell the type checker that req is an Optional[Requirement], and yet it still lets us try to access properties of req without ensuring it's not None? That's basically just not doing its job.

That's because we'd have disabled the check that it does via the strict-optional=False comment that this PR removes! :)

[hauntsaninja]

Okay, I've gotten rid of the checked properties. The one exception is that the pre-existing self.specifier is functionally doing the same things the checked properties were (it's used in a couple other files, and that use depends on it not returning None, so I didn't get rid of it).

[pfmoore]

That's because we'd have disabled the check that it does via the strict-optional=False comment that this PR removes! :)

Hmm, I’d got the impression this was the default for mypy, and we had enabled the check explicitly but then re-disabled it on a file by file basis. Regardless, I think it’s bizarre to be able to disable this check. Why just this one? Maybe people want to disable checks that things are str, so they can support str-like classes too…

[hauntsaninja]

Since you seem curious for the history here / why the appalling behaviour existed. It did use to be the default for mypy a long time ago (like >5 years).

• A lot of mypy decisions revolve around the idea of allowing users to gradually introduce types to a codebase and notch up strictness over time, rather than having to make huge sweeping changes all at once. mypy's goal was to be usable in large existing codebases by users who were unused to and resistant to type checking. And a lot of its success there is attributable to reducing the upfront pain of adoption and the pragmatic spirit of "starting with some checking is better than no checking".

• mypy trailblazed a lot of the way the type system should work. Things that are clear today probably weren't so clear when static typing was niche. The appalling behaviour is basically how null works in many typed languages today (and I'm sure some of the initial audience for mypy was users from such languages). I think there was concern that Optional[int] was too verbose and users would revolt. Before PEP 526, the idiomatic way of typing instance variables invited confusion with None. And in the earlier days of mypy, I think there was also some concern that mypy didn't yet understand enough idioms that users use to ensure things are not None.

• But yes, it is appalling, not the default behaviour in recent history, and generally unsupported by other type checkers

Here's the pip commit where the relevant mypy config was changed: https://github.com/pypa/pip/pull/6704