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

Remove un-needed __hash__ methods from stdlib #8465

Merged
merged 1 commit into from Aug 6, 2022

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Aug 2, 2022

While working on stubs for django I've noticed that sometimes people redefine __hash__.

But, this is not what we do for other methods. For example, if base class A defines foo, its child class B does not need to also define foo. Parent's definition will be used by default. We only annotate redefinitions.

So, __hash__ is defined in object. Child classes may not redefine it, unless:

  • __hash__ returns something different, not int (like enum.Enum does right now)
  • __hash__ is set to be ClassVar[None]
  • __hash__ is reset after parent's ClassVar[None] to indicate that subclass is immutable again
  • __hash__ is required to be set explicitly as a part of @abstractmethod parent's def, like numbers.Number does

Mypy sample: https://mypy-play.net/?mypy=latest&python=3.10&gist=cef2a15be8adfa8d646d260232e6b38a

from typing import Hashable

class MyHash:
    def __hash__(self) -> int:
        ...

class MyClass: ...

def requires_hash(o: Hashable): ...

requires_hash(MyHash())   # ok
requires_hash(MyClass())  # ok

What do others think?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

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

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

LGTM, although I'd like another maintainer to confirm.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM too, but I'd like to get @JelleZijlstra's feedback. I think he had some Thoughts when I tried to do something along these lines a while back with some other methods.

@sobolevn
Copy link
Member Author

sobolevn commented Aug 6, 2022

@JelleZijlstra frienly ping 🙂

@AlexWaygood AlexWaygood merged commit 64bc059 into python:master Aug 6, 2022
@AlexWaygood AlexWaygood mentioned this pull request Jan 6, 2023
AlexWaygood added a commit to AlexWaygood/typeshed that referenced this pull request Jul 9, 2023
JelleZijlstra pushed a commit that referenced this pull request Jul 9, 2023
…10426)

Reverts #8465
Fixes #10424
Closes #10425

#8465 caused regressions: see #10424 and python/mypy#13800. Since it didn't fix any known problems (just some stylistic nits that we had), let's just revert the PR.
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.

None yet

4 participants