Skip to content

More precise overloads for get/pop methods #10501

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

Merged
merged 6 commits into from
Jul 26, 2023

Conversation

eltoder
Copy link
Contributor

@eltoder eltoder commented Jul 22, 2023

Fixes #10293

@github-actions

This comment has been minimized.

@eltoder
Copy link
Contributor Author

eltoder commented Jul 22, 2023

I think both subtest errors are false-positives:

error: contextvars.Context.get is inconsistent, runtime argument "default" has a default value of type None, which is incompatible with stub argument type Union[_T`-1, _D`-2]. This is often caused by overloads failing to account for explicitly passing in the default value.
Stub: in file stdlib/contextvars.pyi:59
Overload(def [_T] (self: contextvars.Context, contextvars.ContextVar[_T`-1]) -> Union[_T`-1, None], def [_T] (self: contextvars.Context, contextvars.ContextVar[_T`-1], _T`-1) -> _T`-1, def [_T, _D] (self: contextvars.Context, contextvars.ContextVar[_T`-1], _D`-2) -> Union[_T`-1, _D`-2])
Inferred signature: def (self: contextvars.Context, __key: contextvars.ContextVar[_T`-1], __default: Union[_T`-1, _D`-2] = ...)
Runtime:
def (self, key, default=None, /)

error: shelve.Shelf.get is inconsistent, runtime argument "default" has a default value of type None, which is incompatible with stub argument type Union[_VT`1, _T`-1]. This is often caused by overloads failing to account for explicitly passing in the default value.
Stub: in file stdlib/shelve.pyi:18
Overload(def (self: shelve.Shelf[_VT`1], key: builtins.str) -> Union[_VT`1, None], def (self: shelve.Shelf[_VT`1], key: builtins.str, default: _VT`1) -> _VT`1, def [_T] (self: shelve.Shelf[_VT`1], key: builtins.str, default: _T`-1) -> Union[_VT`1, _T`-1])
Inferred signature: def (self: shelve.Shelf[_VT`1], key: builtins.str, default: Union[_VT`1, _T`-1] = ...)
Runtime: in file /opt/hostedtoolcache/Python/3.7.[17](https://github.com/python/typeshed/actions/runs/5632876407/job/15261237271?pr=10501#step:6:18)/x64/lib/python3.7/shelve.py:104
def (self, key, default=None)

The Union type is compatible with None, since one of the alternatives is an unconstrained type variable. I'll revert these changes for now.

@github-actions

This comment has been minimized.

@eltoder
Copy link
Contributor Author

eltoder commented Jul 22, 2023

As far as I can tell, almost all diffs in mypy_primer are caused by the TypedDict plug-in not recognizing the new dict.get() method.

@AlexWaygood
Copy link
Member

As far as I can tell, almost all diffs in mypy_primer are caused by the TypedDict plug-in not recognizing the new dict.get() method.

That's still disruptive enough to be blocking, however. Are you able to work on a PR to fix mypy's TypedDict plugin to work with your proposed changes?

@eltoder
Copy link
Contributor Author

eltoder commented Jul 22, 2023

Yes, we cannot merge this PR with a big breakage like this. I'm looking into what can be done about it.

@github-actions

This comment has been minimized.

@eltoder
Copy link
Contributor Author

eltoder commented Jul 22, 2023

@AlexWaygood I added a work-around for TypedDict. Now all diffs are one of 1) no actual change 2) an improvement 3) a true positive. The jax error is a bit unfortunate, but they simply need to add type annotations for arguments of _pp_eqn: https://github.com/google/jax/blob/32cbc3678d07ab6c805358f17571fd5895d09c55/jax/_src/core.py#L2988
What do you think?

If I make a PR for mypy so I don't need a work-around here, how do we coordinate releasing it? I assume I need to make the plug-in work with both versions and then we have to wait for a mypy release?

@AlexWaygood
Copy link
Member

AlexWaygood commented Jul 23, 2023

Haven't looked in-depth yet, but at first glance, this looks pretty promising now — thank you! I'll take a closer look tomorrow.

@eltoder
Copy link
Contributor Author

eltoder commented Jul 23, 2023

@AlexWaygood The mypy change is very straightforward, but can you suggest the preferred way of making it?

I can make a minimal change that works with both versions of typeshed. This will have a small cosmetic downside (reporting an unnecessary Union type in a error message). Otherwise it will be no worse than the current situation. Then I probably have to wait for a mypy release before we can merge this PR?

Alternatively, I can make a change in the typeshed version bundled with mypy. I can make a slightly better change this way. For example, I can override get in TypedDict to prevent these issues in the future. Type inference can also be a bit better, similar to the dict.get() change I'm making here. If this approach is possible, how do we coordinate the changes between mypy and typeshed?

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pylox (https://github.com/sco1/pylox)
+ pylox/containers/array.py:84: note:     def pop(self, Any, Any, /) -> Any
- pylox/containers/array.py:84: note:     def [_T] pop(self, Any, Any | _T, /) -> Any | _T
+ pylox/containers/array.py:84: note:     def [_T] pop(self, Any, _T, /) -> Any | _T

anyio (https://github.com/agronholm/anyio)
+ src/anyio/streams/memory.py:226: error: Unused "type: ignore" comment  [unused-ignore]
+ src/anyio/streams/memory.py:231: error: Unused "type: ignore" comment  [unused-ignore]

operator (https://github.com/canonical/operator)
- ops/testing.py:929: error: Argument 2 to "get" of "dict" has incompatible type "None"; expected "dict[str, str] | Mapping[str, str]"  [arg-type]
+ ops/testing.py:929: error: Argument 2 to "get" of "dict" has incompatible type "None"; expected "Mapping[str, str]"  [arg-type]
- ops/testing.py:2014: note:     def [_T] pop(self, int, dict[str, Any] | _T, /) -> dict[str, Any] | _T
+ ops/testing.py:2014: note:     def pop(self, int, dict[str, Any], /) -> dict[str, Any]
+ ops/testing.py:2014: note:     def [_T] pop(self, int, _T, /) -> dict[str, Any] | _T

ibis (https://github.com/ibis-project/ibis)
- ibis/expr/analysis.py:282: error: Unsupported operand types for + ("list[Expr | None]" and "list[Expr]")  [operator]

jax (https://github.com/google/jax)
+ jax/_src/core.py:2986: error: Cannot call function of unknown type  [operator]

pyjwt (https://github.com/jpadilla/pyjwt)
+ jwt/api_jwk.py:73: error: Returning Any from function declared to return "str | None"  [no-any-return]
+ jwt/api_jwk.py:77: error: Returning Any from function declared to return "str | None"  [no-any-return]
+ jwt/api_jwk.py:81: error: Returning Any from function declared to return "str | None"  [no-any-return]

speedrun.com_global_scoreboard_webapp (https://github.com/Avasam/speedrun.com_global_scoreboard_webapp)
- backend/services/user_updater_helpers.py:190: error: Incompatible types in assignment (expression has type "list[list[Run]] | list[Run]", variable has type "list[Run]")  [assignment]
+ backend/services/user_updater_helpers.py:190: error: Incompatible types in assignment (expression has type "list[list[Run]]", variable has type "list[Run]")  [assignment]
- backend/services/user_updater_helpers.py:190: error: Argument 2 to "get" of "dict" has incompatible type "list[<nothing>]"; expected "list[list[Run]] | list[Run]"  [arg-type]
- backend/services/user_updater_helpers.py:190: note: "List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
- backend/services/user_updater_helpers.py:190: note: Consider using "Sequence" instead, which is covariant
- backend/services/user_updater_helpers.py:196: error: Incompatible types in assignment (expression has type "list[list[Run]] | list[Run]", variable has type "list[Run]")  [assignment]
+ backend/services/user_updater_helpers.py:196: error: Incompatible types in assignment (expression has type "list[list[Run]]", variable has type "list[Run]")  [assignment]
- backend/services/user_updater_helpers.py:196: error: Argument 2 to "get" of "dict" has incompatible type "list[<nothing>]"; expected "list[list[Run]] | list[Run]"  [arg-type]
- backend/services/user_updater_helpers.py:196: note: "List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
- backend/services/user_updater_helpers.py:196: note: Consider using "Sequence" instead, which is covariant

steam.py (https://github.com/Gobot1234/steam.py)
- steam/state.py:1433: error: Argument 2 to "pop" of "dict" has incompatible type "None"; expected "Friend | CMsgClientFriendsListFriend"  [arg-type]
+ steam/state.py:1433: error: Argument 2 to "pop" of "dict" has incompatible type "None"; expected "CMsgClientFriendsListFriend"  [arg-type]
+ steam/ext/commands/utils.py:38: note:          def get(self, str, _VT, /) -> _VT
+ steam/ext/commands/utils.py:38: note:          @overload
- steam/ext/commands/utils.py:38: note:          def [_T] get(self, str, _VT | _T, /) -> _VT | _T
+ steam/ext/commands/utils.py:38: note:          def [_T] get(self, str, _T, /) -> _VT | _T
+ steam/ext/commands/utils.py:49: note:          def pop(self, str, _VT, /) -> _VT
+ steam/ext/commands/utils.py:49: note:          @overload
- steam/ext/commands/utils.py:49: note:          def [_T] pop(self, str, _VT | _T, /) -> _VT | _T
+ steam/ext/commands/utils.py:49: note:          def [_T] pop(self, str, _T, /) -> _VT | _T
+ steam/ext/commands/utils.py:49: note:          def pop(self, str, /, default: _VT) -> _VT
+ steam/ext/commands/utils.py:49: note:          @overload
- steam/ext/commands/utils.py:49: note:          def [_T] pop(self, str, /, default: _VT | _T) -> _VT | _T
+ steam/ext/commands/utils.py:49: note:          def [_T] pop(self, str, /, default: _T) -> _VT | _T

cwltool (https://github.com/common-workflow-language/cwltool)
+ cwltool/job.py: note: In function "_setup":
+ cwltool/job.py:193:21: error: Returning Any from function declared to return "bool"  [no-any-return]

@AlexWaygood
Copy link
Member

Mypy_primer analysis

anyio and ibis: these are situations where mypy currently emits false-positive errors, and this PR makes them go away:

pyjwt: This is definitely a true positive. They have a _jwt_data dictionary that's typed as being dict[str, Any]. It stands to reason that the inferred type of _jwt_data.get("kty", None) should be Any | None or Any, and it's pretty bad that mypy isn't able to make that inference with the current stubs. https://github.com/jpadilla/pyjwt/blob/72ad55f6d7041ae698dc0790a690804118be50fc/jwt/api_jwk.py#L71-L81

jax: This error will likely be somewhat annoying for the project, but it isn't, strictly speaking, a false-positive error; it's in line with how mypy behaves in similar situations. They've "got away" with what they're doing until now due to inferred Any types. I think it should be fairly easily fixable if they add an explicit annotation for the rule variable. https://github.com/google/jax/blob/1054fe5a3bdccc5019ad1ba43580fb07131ec5f3/jax/_src/core.py#L2982-L2986

cwltool: This one is a bit unfortunate, but again, isn't strictly speaking a false positive. An isinstance(inp, dict) call in the wrong place is causing mypy to infer that inp has type dict[Any, Any], which then causes mypy to infer that inp.get("streamable", False) has type bool | Any. https://github.com/common-workflow-language/cwltool/blob/1573509eea2faa3cd1dc959224e52ff1d796d3eb/cwltool/job.py#L191-L193

All other things in the mypy_primer output are just error messages changing slightly.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thank you!

@AlexWaygood AlexWaygood merged commit e86c61d into python:main Jul 26, 2023
@AlexWaygood
Copy link
Member

AlexWaygood commented Jul 26, 2023

@AlexWaygood The mypy change is very straightforward, but can you suggest the preferred way of making it?

@eltoder I'm not a mypy maintainer (only a triager), but here's a possible plan of action that I think would work well:

  1. File a typeshed issue stating the change you'd like to make to typing.Mapping, explain the impact this has on the mypy TypedDict plugin. Ping maintainers of the big 4 major type checkers to make sure they're all on board, since this could impact other type checkers as well as mypy.
  2. If we all agree that the change is good, make a PR to mypy that fixes the TypeDict plugin at the same time as changing mypy's vendored typeshed stubs.
  3. Immediately after the PR from (2) is merged, we can change typeshed upstream to match the changes that have been made to mypy's vendored stubs in stage (2).

@eltoder
Copy link
Contributor Author

eltoder commented Jul 27, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The type of __default= in dict.pop and dict.get doesn't look right
2 participants