Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upgrade mypy to v1.10.1 #21158

Merged
merged 5 commits into from
Sep 19, 2024
Merged

upgrade mypy to v1.10.1 #21158

merged 5 commits into from
Sep 19, 2024

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Jul 11, 2024

Upgrade MyPy to v1.10.1 for Pants' own source code.

Overview of issues found:

  • Many # type: ignore comments were removed due to improvements in MyPy.
  • MyPy wonderfully found uses of dataclasses.replace where the code was using the wrong type for a replaced field (especially egregious since some cases passed in a mutable list where an immutable value was expected to be stored in the (frozen) dataclass).
  • MyPy could not infer that FrozenDict implements the SupportsKeysAndGetItem protocol (for whatever reasons) and so I added ignores of dict-item in certain places.
  • Upgraded strawberry-graphql which is used by repo-internal plugin pants-plugins/pants_explorer/server to work around an ICE in mypy. I also had to add some # type: ignore comments to that module. I have not tested pants_explorer since I did not find a README about how to use it for Pants development.

@tdyas tdyas added category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required PR doesn't require mention in release notes labels Jul 11, 2024
@cburroughs
Copy link
Contributor

I know reports indicate this doesn't fully fix #18519, but I don't think there is any reason not to do this upgrade. Anything todo before un-drafting?

@tdyas
Copy link
Contributor Author

tdyas commented Aug 21, 2024

I know reports indicate this doesn't fully fix #18519, but I don't think there is any reason not to do this upgrade. Anything todo before un-drafting?

Still need to cleanup unused type comments as per this build failure: https://github.com/pantsbuild/pants/actions/runs/9894141143/job/27331097301?pr=21158

@tdyas
Copy link
Contributor Author

tdyas commented Aug 22, 2024

And still not understanding why inclusion of types-setuptools does not fix this error:

src/python/pants/util/pip_requirement.py:10:1: error: Library stubs not installed for "pkg_resources.extern.packaging.requirements"  [import-untyped]
    from pkg_resources.extern.packaging.requirements import (
    ^
src/python/pants/util/pip_requirement.py:10:1: note: Hint: "python3 -m pip install types-setuptools"
src/python/pants/util/pip_requirement.py:10:1: note: (or run "mypy --install-types" to install all missing stub packages)
src/python/pants/util/pip_requirement.py:10:1: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 1 source file)

But I haven't tracked whether types-setuptools actually has the requisite type stubs.

@tdyas tdyas marked this pull request as ready for review September 13, 2024 21:18
@tdyas tdyas requested review from cburroughs and benjyw September 13, 2024 21:18
@tdyas
Copy link
Contributor Author

tdyas commented Sep 13, 2024

Gave up on trying to solve the setuptools type stub issue, just ignored the applicable import from pkg_resources. This is ready for review.

@tdyas tdyas requested a review from huonw September 19, 2024 17:26
@tdyas
Copy link
Contributor Author

tdyas commented Sep 19, 2024

Merged in main. This is still ready for review. (This upgrades mypy for Pants itself.)

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

@@ -454,7 +454,7 @@ async def create_tool_runner(

append_only_caches = {
**merged_extras.append_only_caches,
**(request.named_caches or {}),
**(request.named_caches or {}), # type: ignore[dict-item]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's this or construct that's causing the issues, likely because the {} becomes dict[Never, Never] and that apparently doesn't satisfy SupportsKeysAndGetItem[K, V] with the specific K and V types.

def direct_splat_frozen_only(fd: FrozenDict[str, int]) -> dict[str, int]:
    return {**fd} # okay

def direct_splat_dict_too(fd: FrozenDict[str, int] | dict[str, int]) -> dict[str, int]:
    return {**fd} # okay
    
def or_splat_explicit(fd: FrozenDict[str, int]) -> dict[str, int]:
    empty: dict[str, int] = {}
    return {**(fd or empty)} # okay

def or_splat_implicit(fd: None | FrozenDict[str, int]) -> dict[str, int]:
    return {**(fd or {})} # error: Unpacked dict entry 0 has incompatible type "FrozenDict[str, int] | dict[Never, Never]"; expected "SupportsKeysAndGetItem[str, int]"  [dict-item]

https://mypy-play.net/?mypy=latest&python=3.12&gist=661f39bc47fc9b1b4f8a80fe0814d1c1

I filed python/mypy#17790

Happy to go with what you've got for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Yeah, let's ignore it for now. If and when it gets fixed upstream, we should get a "unused type ignore" error once the ignore is no longer necessary.

@tdyas tdyas merged commit 0b7ba6e into pantsbuild:main Sep 19, 2024
25 checks passed
@tdyas tdyas deleted the upgrade_mypy branch September 19, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required PR doesn't require mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants