More precise overloads for get/pop methods by eltoder · Pull Request #10501 · python/typeshed

eltoder · 2023-07-22T21:44:01Z

These trigger false positives in stubtest.

eltoder · 2023-07-22T22:10:26Z

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.

eltoder · 2023-07-22T22:26:54Z

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 · 2023-07-22T22:33:26Z

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 · 2023-07-22T22:35:51Z

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

eltoder · 2023-07-22T23:39:59Z

@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 · 2023-07-23T00:25:52Z

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 · 2023-07-23T18:01:46Z

@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 · 2023-07-26T11:41:04Z

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 · 2023-07-26T15:10:28Z

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.

AlexWaygood

Thank you!

AlexWaygood · 2023-07-26T15:40:38Z

@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:

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.
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.
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 · 2023-07-27T03:33:22Z

Thanks!

eltoder and others added 4 commits July 21, 2023 23:30

More precise overloads for get/pop methods

8be6bd5

Fixes python#10293

Revert change to Mapping to fix TypedDict plugin

00b3b4a

[pre-commit.ci] auto fixes from pre-commit.com hooks

ac69e64

Revert changes to Shelf and Context

ba31e47

These trigger false positives in stubtest.

This comment has been minimized.

Sign in to view

AlexWaygood and others added 2 commits July 26, 2023 12:31

Fix Shelf and Context in a way stubtest is happy with

3cd6ab8

Update shelve.pyi

c54f331

AlexWaygood approved these changes Jul 26, 2023

View reviewed changes

AlexWaygood merged commit e86c61d into python:main Jul 26, 2023

AlexWaygood mentioned this pull request Jul 26, 2023

Remove unnecessary union in the default type in .get() and .pop() methods #10294

Closed

eltoder deleted the feature/dict-get-pop branch July 27, 2023 03:33

AlexWaygood mentioned this pull request Aug 1, 2023

Sync typeshed python/mypy#15792

Merged

tungol mentioned this pull request Dec 8, 2024

Alternate method of handling dict.get default value #13224

Closed

ndmitchell mentioned this pull request Jul 2, 2025

Make Mapping.get(default) more constrained #14360

Merged

Conversation

eltoder commented Jul 22, 2023

Uh oh!

This comment has been minimized.

eltoder commented Jul 22, 2023

Uh oh!

This comment has been minimized.

eltoder commented Jul 22, 2023

Uh oh!

AlexWaygood commented Jul 22, 2023

Uh oh!

eltoder commented Jul 22, 2023

Uh oh!

This comment has been minimized.

eltoder commented Jul 22, 2023

Uh oh!

AlexWaygood commented Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eltoder commented Jul 23, 2023

Uh oh!

github-actions Bot commented Jul 26, 2023

Uh oh!

AlexWaygood commented Jul 26, 2023

Mypy_primer analysis

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eltoder commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AlexWaygood commented Jul 23, 2023 •

edited

Loading

AlexWaygood commented Jul 26, 2023 •

edited

Loading