8000 Improve protocol return types by srittau · Pull Request #7093 · python/typeshed · GitHub
[go: up one dir, main page]
Skip to content

Improve protocol return types #7093

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 1, 2022
Merged

Improve protocol return types #7093

merged 4 commits into from
Feb 1, 2022

Conversation

srittau
Copy link
Collaborator
@srittau srittau commented Jan 31, 2022
  • Dunder comparisons must return bool.
  • write() return type should be ignored.

@srittau
Improve protocol return types
4feb667 
* Dunder comparisons must return bool.
* write() return type should be ignored.
@AlexWaygood
Copy link
Member

FWIW, the reason I annotated the dunder comparison functions as returning Any rather than bool was @JelleZijlstra's comment here. But I don't have strong opinions about this :)

@srittau
Copy link
Collaborator Author
srittau commented Jan 31, 2022

Returning a non-bool from these methods isn't all that uncommon; numpy arrays do it.

I believe that's the case when the comparison operator are used for non-comparison purposes? For example, like pathlib uses the / operator for path concatenation? In that case I would suggest to use a custom protocol, instead of our typeshed protocol.

@github-actions

This comment has been minimized.

Sign in to view
@AlexWaygood
Copy link
Member

Returning a non-bool from these methods isn't all that uncommon; numpy arrays do it.

I believe that's the case when the comparison operator are used for non-comparison purposes?

I don't think so; I think the issue is that the runtime doesn't care what these methods return, as long as it's something that bool() can be called on (which is nearly everything). The most common approach is obviously to return bool; but that's not always the case, as the pandas example shows.

@srittau
Copy link
Collaborator Author
srittau commented Jan 31, 2022

I feel that this exactly one of the cases that type checkers should catch. While of course everything is "boolean-like", returning arbitrary non-bools from those dunders seems error-prone. I am not making sense of the typing of numpy, but the documentation for ndarray.__bool__ says: self != 0, so it seems it should return a bool.

@srittau
Copy link
Collaborator Author
srittau commented Jan 31, 2022

Also, it's just one primer hit, despite the length of the output indicating something different.

@AlexWaygood
Copy link
Member

I feel that this exactly one of the cases that type checkers should catch. While of course everything is "boolean-like", returning arbitrary non-bools from those dunders seems error-prone. I am not making sense of the typing of numpy, but the documentation for ndarray.__bool__ says: self != 0, so it seems it should return a bool.

Makes sense!

@Akuli
Copy link
Collaborator
Akuli commented Jan 31, 2022

self != 0 doesn't return a bool in numpy:

>>> import numpy as np
>>> a = np.array([0,1,2,3])
>>> a != 0
array([False,  True,  True,  True])
>>> if a != 0: print("lol")
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

@JelleZijlstra
Copy link
Member
8000 JelleZijlstra commented Jan 31, 2022

Yes, in numpy all array operations do elementwise work where possible, so x OP y is broadly equivalent to [a OP b for a, b in zip(x, y)]. And numpy is very widely used, so I'm hesitant to make a change that drops some usage of numpy arrays.

However, we use these protocols both for functions like min() that really do need a bool (or something bool-like), and for the operator.* family of functions where the return value doesn't matter. We could perhaps split those up.

@srittau
Copy link
Collaborator Author
srittau commented Jan 31, 2022

Splitting them sounds like a good idea. It would make most sense to me to keep the protocols in the PR as they are in the PR, and add custom protocols to operator.pyi directly.

@AlexWaygood
Copy link
Member

It would make most sense to me to keep the protocols in the PR as they are in the PR, and add custom protocols to operator.pyi directly.

Let's add a short comment in operator, too, pointing people to this discussion — I'll admit I didn't understand that the use of these methods in operator required a different return type to elsewhere when I wrote #6583 :)

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@srittau
Copy link
Collaborator Author
srittau commented Jan 31, 2022

Primer looks good now.

AlexWaygood
AlexWaygood reviewed Jan 31, 2022
View reviewed changes
@github-actions
Copy link
Contributor
github-actions bot commented Feb 1, 2022

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

pandas (https://github.com/pandas-dev/pandas)
- pandas/core/indexes/base.py:4053: error: Argument 1 has incompatible type "Union[ExtensionArray, ndarray[Any, Any]]"; expected "Union[SupportsDunderLE, SupportsDunderGE, SupportsDunderGT, SupportsDunderLT]"  [arg-type]
+ pandas/core/indexes/base.py:4053: error: Argument 1 has incompatible type "Union[ExtensionArray, ndarray[Any, Any]]"; expected "Union[_SupportsDunderLE, _SupportsDunderGE, _SupportsDunderGT, _SupportsDunderLT]"  [arg-type]
- pandas/core/indexes/base.py:4053: error: Argument 2 has incompatible type "Union[ExtensionArray, ndarray[Any, Any]]"; expected "Union[SupportsDunderLE, SupportsDunderGE, SupportsDunderGT, SupportsDunderLT]"  [arg-type]
+ pandas/core/indexes/base.py:4053: error: Argument 2 has incompatible type "Union[ExtensionArray, ndarray[Any, Any]]"; expected "Union[_SupportsDunderLE, _SupportsDunderGE, _SupportsDunderGT, _SupportsDunderLT]"  [arg-type]
- pandas/io/formats/style.py:3709: error: Argument 2 to "ge" has incompatible type "Union[str, Any, float, Period, Timestamp, Timedelta, Sequence[Any], ndarray[Any, Any], NDFrame]"; expected "Union[SupportsDunderLE, SupportsDunderGE, SupportsDunderGT, SupportsDunderLT]"  [arg-type]
+ pandas/io/formats/style.py:3709: error: Argument 2 to "ge" has incompatible type "Union[str, Any, float, Period, Timestamp, Timedelta, Sequence[Any], ndarray[Any, Any], NDFrame]"; expected "Union[_SupportsDunderLE, _SupportsDunderGE, _SupportsDunderGT, _SupportsDunderLT]"  [arg-type]
- pandas/io/formats/style.py:3714: error: Argument 2 to "le" has incompatible type "Union[str, Any, float, Period, Timestamp, Timedelta, Sequence[Any], ndarray[Any, Any], NDFrame]"; expected "Union[SupportsDunderLE, SupportsDunderGE, SupportsDunderGT, SupportsDunderLT]"  [arg-type]
+ pandas/io/formats/style.py:3714: error: Argument 2 to "le" has incompatible type "Union[str, Any, float, Period, Timestamp, Timedelta, Sequence[Any], ndarray[Any, Any], NDFrame]"; expected "Union[_SupportsDunderLE, _SupportsDunderGE, _SupportsDunderGT, _SupportsDunderLT]"  [arg-type]

@srittau srittau merged commit 54fde0c into python:master Feb 1, 2022
@srittau srittau deleted the proto-return-object branch February 1, 2022 07:59
@AlexWaygood AlexWaygood mentioned this pull request Feb 1, 2022
Merged
@lkct lkct mentioned this pull request Jun 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@AlexWaygood AlexWaygood AlexWaygood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@srittau @AlexWaygood @Akuli @JelleZijlstra
0