Skip to content
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.

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

Experiment: Let SupportsDunderLT return object. #12573

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

randolf-scholz
Copy link
Contributor

Fixes #12562

This comment has been minimized.

This comment has been minimized.

@randolf-scholz randolf-scholz changed the title Experiment: Introduce SupportsBool Protocol. Experiment: Let SupportsDunderLT return object. Aug 21, 2024

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Aug 22, 2024

I'm not a fan of this change. This would decrease the type safety for the common case. For example, TestCase.assertGreater really wants an object that returns a bool and not just any overloaded object.

@JelleZijlstra
Copy link
Member

The change here would affect objects that do have an __lt__ or similar method, but where that method doesn't return a bool. I don't think that's likely to cause a lot of real unsafety.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems right to me but I retriggered mypy-primer to get another look.

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Oct 3, 2024

I'm still opposed to this. Overloading the comparison operators to return something other than bool is unexpected and using # type: ignore to make this explicit is acceptable.

@jorenham
Copy link
Contributor

jorenham commented Oct 3, 2024

I'm still opposed to this. Overloading the comparison operators to return something other than bool is unexpected and using # type: ignore to make this explicit is acceptable.

I'd agree with you if it indeed would be fixeable with one or two # type: ignore comments.
But as I've shown in #12562, this unfortunately isn't the case for user-defined types that return something other than a bool from e.g. __lt__, which currently is rejected when used with e.g. builtins.min.

@randolf-scholz
Copy link
Contributor Author

I can't help but be reminded of this comment python/mypy#1020 (comment).

As someone who uses numpy daily, I don't find it surprising that < or > might return something other than a bool. If one finds that surprising or a code smell, using a linter might be more appropriate. Imo, the stubs should reflect runtime behavior as best as possible.

@randolf-scholz
Copy link
Contributor Author

@srittau @JelleZijlstra With this PR seemingly being stuck, can we at least consider revisiting the SupportsBool approach #12939 ? If this weaker modification is agreeable to @srittau, maybe we can merge it and reconsider this PR at a later point.

This comment has been minimized.

Copy link
Contributor

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

ibis (https://github.com/ibis-project/ibis)
- ibis/expr/types/relations.py:2924: error: Value of type variable "SupportsRichComparisonT" of "sorted" cannot be "NumericValue | float"  [type-var]

jax (https://github.com/google/jax)
+ jax/_src/pallas/mosaic/pipeline.py:981: error: Unused "type: ignore" comment  [unused-ignore]

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.

False positive with builtins.min(T, T) if T.__lt__ doesn't return a builtins.bool
4 participants