Mypy and the `--no-implicit-optional` option

[DiddiLeija]

Overview

This is the second part of the discussion from #10065 (review).

Description

I have been working on converting the type hint commentaries into proper annotations (#10004, #10018, #10065, #10074). But I had a simple mistake on #10065 by annotating this:

        download_dir: str = None

when it must be annotated like this:

        download_dir: Optional[str] = None

We expected that Mypy was going to fail, but it just passed by. Then, we saw that Mypy accepts implicit optional annotations, until we use --no-implicit-optional.

What we want

We want Mypy to use --no-implicit-optional on its checks, to make a better diagnosis. View the PR review where the original discussion is for more information.

Code of Conduct

• [x] I agree to follow the PSF Code of Conduct.

[DiddiLeija]

I opened the issue here because it is not exactly a Mypy problem. It is related to the way we use it on pypa/pip checks.

[pradyunsg]

Yea, we have to enable a bunch of mypy's strictness flags (this is probably the highest impact among those).

The ideal goal would be to get to a point where we can run mypy --strict . and have everything pass.

[DiddiLeija]

The ideal goal would be to get to a point where we can run mypy --strict . and have everything pass.

How can we make that for the CI checks?

[DiddiLeija]

How can we make that for the CI checks?

Sorry, I found it. We are already using options for Mypy checks (--pretty). I will submit a PR to add a --strict option to Mypy.

[DiddiLeija]

Can I self-assign this, as I created the related pull request?

[uranusjr]

Assigned

[DiddiLeija]

Hi, do we still wanna get this done? You know, we couldn't apply #10091, but maybe we can try again :)

Thoughts @pypa/pip-committers?

[pradyunsg]

I think so -- ideally, we'd be passing --strict to mypy.

[pradyunsg]

As of c260ecc3e3e836b4d0dcfd56e1c365e40421ac6e, running pre-commit run --all-files mypy (with --strict added to mypy's arguments in pre-commit-config.yaml results in:

Found 185 errors in 73 files (checked 288 source files)

I'd like to fix all of those, TBH, so that we're getting the benefits of the complete checks/protections that mypy can provide.

[DiddiLeija]

Sounds good to me. We once tried to add --strict, but it crashed with vendored code. Anyway, if we can either fix or ignore that, then I'm a +1 to adding the --strict flag.

[93578237]

There are lots of errors cs mypy cant use annotations of 3rd party libraries (like requests) as they are imported from _vendor and its annoying

[potiuk]

FYI. You can configure mypy (in pyproject.toml/setup.cfg etc. ) to ignore certain errors in certain packages. https://mypy.readthedocs.io/en/stable/config_file.html?highlight=pyproject%20toml#examples

This what we do in airflow for one (and I believe vendored-in code should be minimally modified if ever so probably it's better to ignore those in vendored packages, rather than fix them).

# Let's assume all google.cloud packages have no implicit optional
# Most of them don't but even if they do, it does not matter
[mypy-google.cloud.*]
no_implicit_optional = False

My 3 cents.

[93578237]

I try to adapt strict=True. I also need types for requests, so additional dependency types-requests. Maybe vendor types-requests or rewrite imports in the pip codebase.

[93578237]

It does not recognize requests.Response as it was imported from _vendor.requests. So the solution would be adding #type: ignore comments to the function signatures and so on. Somewhere you can use TYPE_CHECKING to import from requests.

[pradyunsg]

I'm not sure I follow what you're seeing. Could you point to a commit that shows the behaviour that you're seeing?

[pradyunsg]

it's better to ignore those in vendored packages

That defeats the whole point of having stubs in place for vendored modules -- we want to have the type from these modules and want the type checking benefits that come from that, while ignoring errors in only the vendored source files (ie. in pip._vendor). We've got the appropriate mypy configuration to achieve that.

We also have redirects for the vendored modules, via stub files generated for them. If that isn't working for some reason, I'd like to get clearer details on what that looks like that's more detailed than "there are lots of errors". :)

[potiuk]

it's better to ignore those in vendored packages

That defeats the whole point of having stubs in place for vendored modules -- we want to have the type from these modules and want the type checking benefits that come from that, while ignoring errors in only the vendored source files (ie. in pip._vendor). We've got the appropriate mypy configuration to achieve that.

You probably misunderstood what I was explaining. I was only referring to "no_implicit_option" case. (and this is precisely what we are ignoring in the example).

This particular setting is quite problematic and disabling it has benefits in this case. Because when the 3rd-party library has it set to False and your code has it set to true (like it is in this case and is in cae of google packages I mentioned) it's the easiest way to reap all the benefits of the vendored-in code without changing it. In the case I described, Google libraries were built with "no_implicit_optional=False" and our code with = True, setting this (and only this) setting for Google packages allowed us to still get all the Static Typing benefitsm while ignoring problem that Google code had parameter: String = None rather than parameter: Optional[String]=None in hundreds of methods.

MyPy in this case is still able to correclty infer the parameter type, but simply does not complain.

[pradyunsg]

That option is a subset of strict (which is what the rest of the discussion is about) and, as I said, we have mypy configured to not report errors about vendored code but to pick up annotations appropriately.

[93578237]

@pradyunsg for example reveal_type for Response in network/utils.py. it would be pip._vendor.requests.models.Response so on line https://github.com/pypa/pip/blob/90665842980ba25ecbec68f48037e1dd03661378/src/pip/_internal/network/utils.py#L43 it would raise Unsupported operand types for <= ("int" and "None") because mypy does not know about types-requests stubs, but this works

if TYPE_CHECKING:
    from requests.models import CONTENT_CHUNK_SIZE, Response

[pradyunsg]

How are you running mypy? I reckon this is because we don’t have any types-requests in https://github.com/pypa/pip/blob/main/.pre-commit-config.yaml#L47

[93578237]

Via pre-commit. I've added types-requests=2.28.1 in additional-dependencies

[pradyunsg]

src/pip/_internal/network/utils.py:45: note: Revealed type is "pip._vendor.requests.models.Response"

Hmm... I think there's something more happening here.

[93578237]

It happens because of imports. Vendor pyi or rewrite imports

[DiddiLeija]

Renamed this thread, since we're now discussing to add --strict :upside_down_face:

[hauntsaninja]

Here's a checklist ordered by benefit:

• [ ] Enable strict optional checking everywhere because we're in the 2020s (grep for # mypy: strict-optional)
  • https://github.com/pypa/pip/pull/11379
  • https://github.com/pypa/pip/pull/11382
  • https://github.com/pypa/pip/pull/12091
  • https://github.com/pypa/pip/pull/12295
  • src/pip/_internal/locations/_distutils.py
  • src/pip/_internal/operations/prepare.py
  • src/pip/_internal/cli/cmdoptions.py
• [x] Opt in to most strictness checks https://github.com/pypa/pip/pull/12209
• [ ] Upgrade mypy https://github.com/pypa/pip/pull/12293
• [x] Avoid follow_imports=skip for vendored code https://github.com/pypa/pip/pull/12294
• [ ] Get pre-commit to be strict about missing imports
• [x] Use --no-implicit-optional https://github.com/pypa/pip/pull/11374

I think once these are done, pip will be in a great place, with much lower marginal benefit to any additional type checker configuration changes