Revert "remove dict.__or__ overloads"

[ilevkivskyi]

Reverts #14284

This causes a regression in mypy, see python/mypy#21141, cc @hauntsaninja

[github-actions]

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

rotki (https://github.com/rotki/rotki)
- rotkehlchen/chain/evm/decoding/aave/v3/decoder.py:583: error: Unused "type: ignore" comment  [unused-ignore]

artigraph (https://github.com/artigraph/artigraph)
- tests/arti/internal/test_mappings.py:40: error: Unsupported operand types for | ("dict[str, int]" and "frozendict[Never, int]")  [operator]
+ tests/arti/internal/test_mappings.py:40: error: No overload variant of "__or__" of "dict" matches argument type "frozendict[Never, int]"  [operator]
+ tests/arti/internal/test_mappings.py:40: note: Possible overload variants:
+ tests/arti/internal/test_mappings.py:40: note:     def __or__(self, dict[str, int], /) -> dict[str, int]
+ tests/arti/internal/test_mappings.py:40: note:     def [_T1, _T2] __or__(self, dict[_T1, _T2], /) -> dict[str | _T1, int | _T2]

[srittau]

Primer output seems to indicate that reverting this has beneficial and detrimental effects, but overall the impact seems low. I prefer users to work around problems with particular type checkers using # type: ignore rather than working around it in typeshed. The time when mypy was the only type checker we had to cater to are long gone.

[carljm]

These overloads are clearly redundant; if a type checker depends on them existing, that seems like a bug in the type checker. Including these redundant overloads means any type checker following the typing spec's algorithm for overload resolution will often infer Unknown due to multiple ambiguous matching overloads.

[ilevkivskyi]

@srittau

The time when mypy was the only type checker we had to cater to are long gone.

Could you please clarify this comment? I sense some kind of hostility.

@carljm This should have been discussed while merging the original PR.

[srittau]

This is not meant as hostility, just as a factual statement. I was just trying to point out that we used to work around problems with mypy when it was the only type checker around. And while mypy is deeply important for typeshed (it's the basis for most of our tests after all), I don't think we should work around issues with it in typeshed, unless it's a major annoyance for our users.

Edit: And to clarify: The same is true for all type checkers, I'd even say even more so for non-mypy type checkers, considering how important mypy is for typeshed.

[ilevkivskyi]

unless it's a major annoyance for our users.

And who decides what is major? In general, I would prefer that PRs that are known to affect mypy specifically are not merged without at least pinging us first. So that we can fix issues affecting typeshed in a more planned way, rather than rush because there is a regression.

[hauntsaninja]

To be fair, I don't see this as a major regression. It's +1 and -1 on mypy_primer.

We also get the chance to review changes when we merge typeshed syncs into mypy, like in python/mypy#21021 . Common for us to double check typeshed changes or make mypy specific changes or typeshed patches at that point

Anyway, I opened a PR against mypy that I think is a bandaid for the regression: python/mypy#21148

[randolf-scholz]

As the author of the original PR I'd like to add some context: These overloads (and also the similar ones on list.__add__) seem to purely exist¹ to band-aid some ancient unresolved bugs in mypy (python/mypy#3933 from 2017 / python/mypy#5874 from 2018). Specifically mypy incorrectly errors on the example below: playground

def concat[T, S](x: list[T], y: list[S]) -> list[T | S]: ...

a: list[int] = [1,2,3]
b: list[str] = ["foo"]
r: list[int | str] = concat(a, b)

Removing the overloads reduces type-checker divergence, and leads to more accurate inference when one dict has value type Any, which also improves inference with mypy in the context free case: playground

from typing import Any, reveal_type

x: dict[str, str]
y: dict[str, Any]

reveal_type(x | y)  # 1.19: dict[Any, Any], 1.20: dict[str, str | Any]

Imo, instead of reverting this mypy should try to fix python/mypy#3933 / python/mypy#5874 and then we can also merge the list.__add__ PR (#14282) which was intentionally held back because of the larger effect on mypy-primer.

Footnotes

1. https://github.com/python/mypy/issues/5874#issuecomment-442942426 ↩

[ilevkivskyi]

@hauntsaninja Unfortunately your PR doesn't fix the second reported regression caused by this (also it is arguably another hack, not a real fix either).

@randolf-scholz If the only motivation is consistency between type inference in different type-checkers, it is not enough justification for causing actual problems for users of one of type-checkers (especially since IIUC the change mainly affects mypy behavior, pyright behavior is almost the same, and you don't mention ty/pyrefly).

If the previous status quo didn't cause actual (i.e. reported by users) issues for any other type-checkers, I am going to merge this later today or tomorrow. If I hear from maintainers of any other type-checker, I am instead going to revert this locally in the copy of typeshed we ship with mypy.

cc @JukkaL

[AlexWaygood]

If the previous status quo didn't cause actual (i.e. reported by users) issues for any other type-checkers, I am going to merge this later today or tomorrow. If I hear from maintainers of any other type-checker, I am instead going to revert this locally in the copy of typeshed we ship with mypy.

The previous status quo caused real issues for users of ty. Because of the rules specified in the typing spec for how overload resolution should work in ambiguous cases, before #14284 ty inferred {"foo": "bar"} | {} as Unknown (equivalent to Any). The latest version of ty, as a result of #14284, infers {"foo": "bar"} | {} as dict[str | Unknown, str | Unknown], which is obviously a far superior type for users of ty.

[ilevkivskyi]

@AlexWaygood to double-check: was this issue actually reported as a problem by users, or you found this effect post hoc?

[AlexWaygood]

@AlexWaygood to double-check: was this issue actually reported as a problem by users

yes: see astral-sh/ty#2979. I wrote that issue up after diagnosing the issue, but it was reported by real ty users on Discord to us.

[ilevkivskyi]

OK, I am going to revert locally then.