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

Should Mypy warn about potential invalid arguments to max? #4051

Closed
willmcgugan opened this issue May 20, 2020 · 7 comments · Fixed by #4227
Closed

Should Mypy warn about potential invalid arguments to max? #4051

willmcgugan opened this issue May 20, 2020 · 7 comments · Fixed by #4227
Labels
stubs: false negative Type checkers do not report an error, but should

Comments

@willmcgugan
Copy link

I was surprised that Mypy didn't flag an issue where I was calling max with an int and a None, which throws a TypeError at runtime.

For example, in the following code, should I not expect Mypy to warn that top is potentially None, which wouldn't work with max ?

from typing import Optional
top: Optional[int] = None
print(max(5, top))
@JelleZijlstra
Copy link
Member

JelleZijlstra commented May 21, 2020

This is more of a typeshed issue. Currently max() is stubbed as accepting basically anything (https://github.com/python/typeshed/blob/master/stdlib/2and3/builtins.pyi#L1318). Perhaps it could be changed to accept only types that define <.

@JelleZijlstra JelleZijlstra transferred this issue from python/mypy May 21, 2020
@srittau srittau added size-small stubs: false negative Type checkers do not report an error, but should and removed size-small labels May 21, 2020
@ramalho
Copy link
Contributor

ramalho commented May 26, 2020

Hello, today I sketched a solution for this. I wrote my own clone of max() in Python to be able to test it and run Mypy experiments. This is what I got: do you think I am on the right track? What am I missing? (I'll rename the protocol and type vars with leading _ before I merge into builtins.pyi for a PR)

https://gist.github.com/ramalho/1ed164b65ec8c36716ca9c90780890b8

@JelleZijlstra
Copy link
Member

@ramalho That looks about right! A couple of the overloads (1 and 2 for example) can be merged by putting key: None = ....

@ramalho
Copy link
Contributor

ramalho commented May 26, 2020

Thanks, @JelleZijlstra!

You are right, I was able to merge 3 overloads, now down to 6: https://gist.github.com/ramalho/1ed164b65ec8c36716ca9c90780890b8

It occurred to me that the Comparable protocol can also be used for sorted(), and that would be a smaller PR.

So my plan is to send a PR for the sorted type hints, which will include defining Comparable and a TypeVar bounded by it. After that PR is merged, I'll do a second one for max and min.

Would you prefer that I file a bug about sorted first, or may I just send the PR?

@JelleZijlstra
Copy link
Member

Thanks! No need to file an issue.

I feel like there's some chance narrowing the types of sorted() and max() will lead to false positives, just because of how common these are (maybe some types that should define __lt__ are missing it in stubs?). Once you submit a PR, I'll test it on our codebase to see if anything like that comes up.

@ramalho
Copy link
Contributor

ramalho commented May 31, 2020

I am working on this. Instead of Comparable I think SupportsLessThan is a better name: precise, explicit, and follows the naming convention of several SupportsXyz protocols already defined in typing.

class _SupportsLestThan(Protocol):
    def __lt__(self, other: Any) -> bool: ...

First PR will touch str.list() and listed().

@willmcgugan
Copy link
Author

Fantastic, thanks. I'll never make that silly error again. 👍

vishalkuo pushed a commit to vishalkuo/typeshed that referenced this issue Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stubs: false negative Type checkers do not report an error, but should
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants