Skip to content

Extend special-case for context-based typevar inference in return position to typevar unions #18976

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sterliakov
Copy link
Collaborator

@sterliakov sterliakov commented Apr 26, 2025

When using context, we can perform some overly optimistic inference when return type is T1 | T2. This breaks in important case of builtins.min when default and key are passed, essentially making them always incompatible. This is not the most principled approach, but let's see the primer results.

This resolves quite a few issues (some of them duplicates, but some - substantially different), min problem was a very popular one... Diff run: sterliakov/mypy-issues#30

This PR introduces one major regression. Namely, the following code becomes rejected:

from collections.abc import Mapping
from typing import TypeVar, Union

_T1 = TypeVar("_T1")

def check(mapping: Mapping[str, _T1]) -> None:
    fail1 = mapping.get("", "")
    fail2: Union[_T1, str] = mapping.get("", "")

Prior to this PR, fail1 was rejected but fail2 was accepted. Now both trigger a diagnostic. This problem stems from Mapping.get definition in typeshed:

class Mapping(Iterable[T], Generic[T, T_co], metaclass=ABCMeta):
    @overload
    def get(self, k: T) -> Optional[T_co]: pass
    @overload
    def get(self, k: T, default: Union[T_co, V]) -> Union[T_co, V]: pass
    ...

I don't know why union is used for default. This only applies to Mapping with this weird definition, dict overrides get to a longer version and works as intended.

class dict(MutableMapping[_KT, _VT]):
    # Positional-only in dict, but not in MutableMapping
    @overload  # type: ignore[override]
    def get(self, key: _KT, default: None = None, /) -> _VT | None: ...
    @overload
    def get(self, key: _KT, default: _VT, /) -> _VT: ...
    @overload
    def get(self, key: _KT, default: _T, /) -> _VT | _T: ...

I haven't yet figured out how to handle this sensibly assuming we don't want to change typeshed definition.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

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

antidote (https://github.com/Finistere/antidote)
- src/antidote/lib/interface_ext/_provider.py:313: error: Generator has incompatible item type "NewWeight"; expected "Weight"  [misc]
- src/antidote/lib/interface_ext/_provider.py:314: error: Argument 2 to "sum" has incompatible type "NewWeight"; expected "Weight"  [arg-type]
+ src/antidote/lib/interface_ext/_provider.py:312: error: Argument "weight" to "replace" of "CandidateImplementation[Weight]" has incompatible type "NewWeight"; expected "Weight"  [arg-type]

xarray (https://github.com/pydata/xarray)
+ xarray/core/utils.py: note: In function "get":
+ xarray/core/utils.py:503: error: Argument 2 to "get" of "Mapping" has incompatible type "T | None"; expected "V"  [arg-type]
+ xarray/conventions.py: note: In function "decode_cf_variables":
+ xarray/conventions.py:410: error: Argument "decode_times" to "decode_cf_variable" has incompatible type "object"; expected "bool | CFDatetimeCoder"  [arg-type]
+ xarray/conventions.py:413: error: Argument "decode_timedelta" to "decode_cf_variable" has incompatible type "object"; expected "bool | CFTimedeltaCoder | None"  [arg-type]

bidict (https://github.com/jab/bidict)
+ bidict/_base.py: note: In member "_dedup" of class "BidictBase":
+ bidict/_base.py:330:41: error: Argument 2 to "get" of "Mapping" has incompatible type "Literal[MissingT.MISSING]"; expected "VT"  [arg-type]
+ bidict/_base.py:331:41: error: Argument 2 to "get" of "Mapping" has incompatible type "Literal[MissingT.MISSING]"; expected "KT"  [arg-type]

Tanjun (https://github.com/FasterSpeeding/Tanjun)
- tanjun/clients.py:2428: error: Argument 1 to "get_type_dependency" of "Client" has incompatible type "type[SingleStoreCache[AuthorizationApplication]]"; expected "type[SingleStoreCache[Application]] | type[None]"  [arg-type]

operator (https://github.com/canonical/operator)
- ops/_main.py:95: error: Argument 2 to "next" has incompatible type "None"; expected "Storage"  [arg-type]
+ ops/_main.py:95: error: Incompatible types in assignment (expression has type "Storage | None", variable has type "Storage")  [assignment]

setuptools (https://github.com/pypa/setuptools)
+ setuptools/msvc.py:1442: error: Unused "type: ignore" comment  [unused-ignore]

werkzeug (https://github.com/pallets/werkzeug)
+ src/werkzeug/datastructures/mixins.py:280: error: Unused "type: ignore" comment  [unused-ignore]
+ src/werkzeug/datastructures/mixins.py:280: error: Incompatible types in assignment (expression has type "Union[V, T]", variable has type "V")  [assignment]
+ src/werkzeug/datastructures/mixins.py:280: note: Error code "assignment" not covered by "type: ignore" comment

pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/monkeypatch.py:293: error: Argument 2 to "get" of "Mapping" has incompatible type "Notset"; expected "V"  [arg-type]
+ src/_pytest/monkeypatch.py:307: error: Argument 2 to "get" of "Mapping" has incompatible type "Notset"; expected "V"  [arg-type]

ilevkivskyi pushed a commit that referenced this pull request May 6, 2025
Sometimes our constraints builder needs to express "at least one of
these constraints must be true". Sometimes it's trivial, but sometimes
they have nothing in common - in that case we fall back to forgetting
all of them and (usually) inferring `Never`.

This PR extends the fallback handling logic by one more rule: before
giving up, we try restricting the constraints to meta variables only.
This means that when we try to express `T :> str || U :> str` in the
following setup:

```
from typing import TypeVar

T = TypeVar("T")
U = TypeVar("U")

class Foo(Generic[T]):
    def func(self, arg: T | U) -> None: ...
```

we won't ignore both and fall back to `Never` but will pick `U :> str`
instead. The reason for this heuristic is that function-scoped typevars
are definitely something we're trying to infer, while everything else is
usually out of our control, any other type variable should stay intact.

There are other places where constraint builder may emit restrictions on
type variables it does not control, handling them consistently
everywhere is left as an exercise to the reader.

This isn't safe in general case - it might be that another typevar
satisfies the constraints but the chosen one doesn't. However, this
shouldn't make any existing inference worse: if we used to infer `Never`
and it worked, then anything else should almost definitely work as well.

See the added testcase for motivation: currently `mypy` fails to handle
`Mapping.get` with default without return type context when `Mapping`
has a type variable as the second argument.
https://mypy-play.net/?mypy=1.15.0&python=3.12&flags=strict&gist=2f9493548082e66b77750655d3a90218

This is a prerequisite of #18976 - that inference change makes the
problem solved here occur more often.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment