sum: Protocol defining __add__
#7578
Conversation
stdlib/builtins.pyi
Outdated
| @overload | ||
| def sum(__iterable: Iterable[_T]) -> _T | Literal[0]: ... | ||
| def sum(__iterable: Iterable[_SumT]) -> Any | Literal[0]: ... |
There was a problem hiding this comment.
In 90% of cases, I think people will probably be using sum to add up variables that are all of the same type, e.g. sum((1.4, 2.3, 3.7, 4.2)). We can safely say for this that the result will be of type float.
So, although strictly speaking you're entirely correct — the return type isn't guaranteed to always be _SumT, far from it! — I nonetheless think I prefer using _SumT over Any for the return type, so that we can preserve precise type inference for the common case.
But I'm curious about what other people think :)
There was a problem hiding this comment.
Also, note that the TypeVar is currently unbound in this overload, so either the return type needs to use _SumT instead of Any, or the parameter type needs to change to use _SupportsSum instead of _SumT :)
There was a problem hiding this comment.
What if we make the protocol generic over the return type of __add__, and make it sum(Iterable[_SupportsSum[_T]]) -> _T?
There was a problem hiding this comment.
Ahh shoot this was actually unintentional, was playing round with the typings and forgot to change it back 😅
I agree that _SumT makes sense, so will change it back for now 👍
There was a problem hiding this comment.
What if we make the protocol generic over the return type of
__add__, and make itsum(Iterable[_SupportsSum[_T]]) -> _T?
Good shout, will give it a go
There was a problem hiding this comment.
@JelleZijlstra, strictly speaking I'm not sure it's that simple, because there's __radd__ to consider, which can change the return type. If Y is a subclass of X, and Y defines __radd__, then Y.__radd__ takes precedence over X.__add__ when evaluating the operation x + y (where x is an instance of X, and y is an instance of Y)
There was a problem hiding this comment.
Idk how much that actually matters though
There was a problem hiding this comment.
Yes, getting a completely precise return type in the face of mixed types and __radd__ is very complicated. The current version is technically incorrect for the case where two arguments are passed, the iterable is empty, and the default returns an unrelated type from its __add__.
mypy-primer found a lot of new false positives from my suggestion, so we should probably undo that and just go back to assuming that __add__ returns the same type.
There was a problem hiding this comment.
Yeah I've reverted back to using TypeVars in the latest commit 😄
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
What happened to the freqtrade one? |
Yes, that one looked like a true positive. I think |
Was just wondering the same thing! All that's changed vs the first commit is altering the return type diff --git a/stdlib/builtins.pyi b/stdlib/builtins.pyi
index 9372b2a1..ce2042bc 100644
--- a/stdlib/builtins.pyi
+++ b/stdlib/builtins.pyi
@@ -1485,7 +1485,7 @@ _SumT = TypeVar("_SumT", bound=_SupportsSum)
_SumS = TypeVar("_SumS", bound=_SupportsSum)
@overload
-def sum(__iterable: Iterable[_SumT]) -> Any | Literal[0]: ...
+def sum(__iterable: Iterable[_SumT]) -> _SumT | Literal[0]: ...
if sys.version_info >= (3, 8):
@overload |
|
This is very strange, I can't reproduce it on MyPy playground: from typing import *
class _SupportsSum(Protocol):
def __add__(self, __x: Any) -> Any: ...
_SumT = TypeVar("_SumT", bound=_SupportsSum)
_SumS = TypeVar("_SumS", bound=_SupportsSum)
@overload # type: ignore[no-overload-impl]
def sum(__iterable: Iterable[_SumT]) -> _SumT | Literal[0]: ...
@overload
def sum(__iterable: Iterable[_SumT], start: _SumS) -> _SumT | _SumS: ...
it: Iterable[float | None]
sum(it) # error: Value of type variable "_SumT" of "sum" cannot be "Optional[float]"Can't reproduce it with the conditional overloads either. |
|
I've triggered a new run of mypy_primer |
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
So testing locally these examples work as expected for me from typing import Iterator, Union
def foo() -> Iterator[int]: ...
def bar() -> Iterator[Union[int, None]]: ...
sum([1, 2])
sum([1, None])
sum(foo())
sum(bar())But if I run |
|
I thought type context might help, but this variation that's closer to the freqtrade code doesn't repro it either: https://mypy-play.net/?mypy=latest&python=3.10&flags=strict&gist=3ba27f6c0834c95cfdb5b2d752387275 |
|
Okay here's something odd. I've put a total_profit = sum(
reveal_type(t.close_profit_abs for t in LocalTrade.get_trades_proxy(is_open=False)))If I run All works as expected However, with No error raised and 2 revealed types?? Seems like that second one could be causing the error to get ignored? And when I use the error raised and only 1 revealed type So is this a mypy specific issue? |
I think so, almost certainly :) We should probably merge this as it is, if @JelleZijlstra is in agreement with me. It's just very strange that we can't reproduce it in miniature. The "double output for |
|
Something else that's interesting: this PR has also exposed a (completely unrelated) pyright bug, I think. The first commit, where the TypeVar was unbound in the first overload, should have triggered pyright's |
|
Yes, I think we should merge. I tried some other variations in mypy playground but couldn't get the double reveal type. Normally that happens with TypeVars with constraints, but there aren't any here (or in the issue Alex linked above). Not sure what's happening there. |
|
I think I know what's going on with freqtrade. The change in this PR that silenced diff --git a/stdlib/builtins.pyi b/stdlib/builtins.pyi
index 9372b2a1..ce2042bc 100644
--- a/stdlib/builtins.pyi
+++ b/stdlib/builtins.pyi
@@ -1485,7 +1485,7 @@ _SumT = TypeVar("_SumT", bound=_SupportsSum)
_SumS = TypeVar("_SumS", bound=_SupportsSum)
@overload
-def sum(__iterable: Iterable[_SumT]) -> Any | Literal[0]: ...
+def sum(__iterable: Iterable[_SumT]) -> _SumT | Literal[0]: ...
if sys.version_info >= (3, 8):
@overloadIn freqtrade, the result of |
|
Thanks @Akuli, I missed that the variable was of type |
Partially addresses #7574.
As suggested by @JelleZijlstra in this comment we should define an
__add__protocol for use insumto prevent things like