| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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?) |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
I'm a bit ill right now but happy to take a look later this week (please ping me again if I forget) |
Sorry, something went wrong.
|
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:520: 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]
|
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM. I think these overloads were only added in #10427 for "consistency" with collections.OrderedDict.__or__, collections.defaultdict.__or__, and similar. But unlike those classes, dict.__or__ always returns a dict, not Self, so the same concerns don't apply here:
>>> class Foo(dict): ...
...
>>> type(Foo({}) | {})
<class 'dict'>and I agree that the divergences between type checkers here are unfortunate.
Sorry, something went wrong.
|
Happy to land this if you take it out of draft! (Which I can do myself, but I don't want to do that to your PR if you're still uncertain about it :-) |
Sorry, something went wrong.
|
@AlexWaygood Done. What about the sister PR #14282 ? I did a rebase and let's see if the primer has changed, bit I am afraid this equivalent change for list.__add__ will be more distuptive due to a long standing bug in mypy. (python/mypy#19304) |
Sorry, something went wrong.
I agree that in theory the overloads there should also be redundant, but if the mypy_primer hit is as bad as it was last time then that might be prohibitive, unfortunately :/ |
Sorry, something went wrong.
|
Yes it is. The thing is mypy has this very old bug class Vec[T]:
def get(self) -> T: ...
def set(self, index: int, value: T) -> None: ...
def __add__[S](self, other: "Vec[S]", /) -> "Vec[T | S]": return Vec()
def pprint(arg: Vec[object]) -> None: ...
def demo(ints: Vec[int], strs: Vec[str]):
pprint(ints + strs) # ❌️ mypy false positiveand I am not seeing it get solved any time soon. I tried to get my hands dirty as well, but it is messy and arduous. (randolf-scholz/mypy#4) |
Sorry, something went wrong.
|
yeah, bidirectional inference is hard... |
Sorry, something went wrong.
|
Also, sorry again for letting this slip for so long. I was pretty ill over a lot of December and then lost track of it after the Christmas period :-( |
Sorry, something went wrong.
|
No worries. Btw did you see #15297 (comment) ? |
Sorry, something went wrong.
Identifies two typeshed PRs responsible for 139 new ecosystem diagnostics: - python/typeshed#14284 (dict.__or__ overload removal): ~150 diagnostics - python/typeshed#15470 (set/frozenset canonicalization): ~3 diagnostics https://claude.ai/code/session_016z9Bez4viqJyDJAeZMQbWo
This reverts commit bfb7342.
| Back | FazBrowse Home | New Git URL |
See #14283. Sibling PR for #14282.