Skip to content

Conversation

@randolf-scholz
Copy link
Contributor

See #14283. Sibling PR for #14282.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2025

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:443: error: Unused "type: ignore" comment  [unused-ignore]

artigraph (https://github.com/artigraph/artigraph)
- 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: error: Unsupported operand types for | ("dict[str, int]" and "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
Copy link
Collaborator

srittau commented Dec 9, 2025

For reference, the overloads were added in #10427 due to @AlexWaygood's comment. Are those concerns not valid anymore? (At first glace at stdlib/@tests/test_cases/builtins/check_dict.py it seems we already have test cases for this, so probably not?)

@randolf-scholz
Copy link
Contributor Author

These overloads shouldn't be necessary at all, and they lead to type checker divergence, but there is a very old bug in mypy that prevents us from fixing it (python/mypy#3933 / python/mypy#19304). Since there was a new major release of mypy I just wanted to check whether anything changed.

@srittau
Copy link
Collaborator

srittau commented Dec 9, 2025

Considering the passing tests and the positive primer output, I'd be willing to accept this change, whether the mypy bug is fixed or not. But I'd like to hear Alex's opinion, since he brought the issue up in the original PR.

@randolf-scholz
Copy link
Contributor Author

randolf-scholz commented Dec 9, 2025

Considering the passing tests and the positive primer output

That's probably more so because dict.__or__ was only added in 3.9 so many legacy code bases will not have a lot of instances of it. For the equivalent issue with list.__add__ the primer doesn't look so nice #14282 (comment).

We even get some very bad cases like Unsupported operand types for + ("list[str]" and "list[str]") which are due to the aforementioned mypy bugs.

@srittau
Copy link
Collaborator

srittau commented Dec 9, 2025

That's probably more so because dict.__or__ was only added in 3.9 so many legacy code bases will not have a lot of instances of it.

Then new code will have to use # type: ignore to work around the mypy bug. That's fine. It's more of a problem if existing code has to add it, as that's churn that we try to avoid.

@AlexWaygood
Copy link
Member

I'm a bit ill right now but happy to take a look later this week (please ping me again if I forget)

@srittau
Copy link
Collaborator

srittau commented Dec 15, 2025

@AlexWaygood Friendly ping.

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.

3 participants