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

Conformance test: make dataclass_hash.py not rely on typing.Hashable? #1765

Open
grievejia opened this issue Jun 4, 2024 · 1 comment
Open
Labels
topic: conformance tests Issues with the conformance test suite

Comments

@grievejia
Copy link

grievejia commented Jun 4, 2024

I noticed that the conformance test currently enforces a hashability check on dataclasses:
https://github.com/python/typing/blame/6d5c186ea3f45dd80dcad9f172479b270e54966a/conformance/tests/dataclasses_hash.py

If we take the annotation of typing.Hashable and the annotation of object literally, then no type errors should be reported on that file, as typing.Hashable is a protocol that requires a def __hash__(self) -> int method, and object satisfies that protocol regardless of whether the dataclass transform creates its own __hash__ method or not.

I understand that the issue of deciding whether an object is hashable or not in type checkers is a tricky business and there was a lot of pre-existing discussions/proposals around it. This makes me wonder if it's worth doing a dedicated "hashability" section in the spec, and use separate conformance tests to establish how typing.Hashable assignability should be handled. My understanding is that the pre-existing dataclass hash tests is intended to just test about whether the __hash__ method is nullified or not under the dataclass transform, but by testing it via typing.Hashable it kinds of indirectly dictate hashable assignment behaviors as a side effect.

Concretely, what I had in mind was a refactor to dataclasses_hash.py, where we change the current assertions of the form

# This should generate an error because DC1 isn't hashable.
v: Hashable = DC(0)

into something like this:

assert_type(DC(0).__hash__, NoneType)  # OK

The new version does not depend on how typing.Hashable gets defined in typeshed, and it (arguably) aligns more directly with the intention of the tests. But I am unsure about how controversial this proposal would be and hence want to get some feedback on it first.

@grievejia grievejia added the topic: other Other topics not covered label Jun 4, 2024
@erictraut
Copy link
Collaborator

Yeah, as you noted, statically determining the hashability of a type is tricky because the hashing mechanism in Python unfortunately doesn't follow standard inheritance rules.

I'd welcome an extension to the typing spec that standardizes static typing behaviors related to hashability.

I agree that the current conformance test implementation makes some brittle assumptions. I'm fine with your proposed change.

@AlexWaygood AlexWaygood added topic: conformance tests Issues with the conformance test suite and removed topic: other Other topics not covered labels Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: conformance tests Issues with the conformance test suite
Projects
None yet
Development

No branches or pull requests

3 participants