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

Type __call__ on builtins._NotImplementedType as None. #12984

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

rchen152
Copy link
Collaborator

@rchen152 rchen152 commented Nov 8, 2024

Currently, this is intentionally incorrectly typed in order to produce a better mypy error message. But pyright (and presumably other type checkers) end up just treating instances of _NotImplementedType as callable.

I'm happy to drop this if it's particularly important that mypy generate a good error message here, but IMO it would be better to change this so that other type checkers can understand that instances of _NotImplementedType aren't callable, at the cost of making the error message a little worse.

Currently, this is intentionally incorrectly typed in order to produce a
better mypy error message. But pyright (and presumably other type checkers)
end up just treating instances of _NotImplementedType as callable.

With this change, the mypy error message gets a little worse, but other type
checkers can understand that instances of _NotImplementedType aren't
callable, which I think is an improvement.
Copy link
Contributor

github-actions bot commented Nov 8, 2024

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, this was one of my weirder ideas. This changes mypy's message for raise NotImplemented() from NotImplemented? not callable to "None" not callable

With the PR as is, pyright says Object of type "None" cannot be called but if you replace it with Never, pyright gives a better error: Invalid exception class or object (mypy says "Never" not callable)

@rchen152
Copy link
Collaborator Author

rchen152 commented Nov 8, 2024

How are you testing pyright? I tried Never, but I didn't get any error at all in the pyright playground, which is why I went with None: https://pyright-play.net/?code=GYJw9gtgBALgngBwJYDsDmUkQWEMoCCKcANFAHICmAbpSAFD0DGANgIYDOHUA%2BuWDACS2FpQiUUMSgBMAKokoAKInACUALnpRtvHkzYsWPHuoo06jfkJFiJU6ab4DhCUeMkz5CSlAC8vKxc3O08FRVV6QJt3e3CgA

@hauntsaninja
Copy link
Collaborator

I was testing locally and my version of pyright was a little stale. With latest pyright, there is no error.
So let's merge, I think it's more common for people to override with None to make special methods go away!

@hauntsaninja hauntsaninja merged commit ea368c7 into main Nov 8, 2024
103 checks passed
@hauntsaninja hauntsaninja deleted the not_implemented branch November 8, 2024 21:05
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.

2 participants