Skip to content

Conversation

@jab
Copy link
Contributor

@jabjab commented Feb 7, 2022

collections.abc.Set provides a _hash() method (see https://github.com/python/cpython/blob/f20ca766/Lib/_collections_abc.py#L677) that includes the following in its docstring:

Note that we don't define __hash__: not all sets are hashable. But if you define a hashable set type, its __hash__ should call this function.

Following this instruction currently causes mypy to give an attr-defined error because AbstractSet is missing a type hint for this _hash() method.

I've been hitting this for a while in my bidict library, whose frozenbidict.__hash__() returns ItemsView(self)._hash(), and working around it with a type: ignore comment, but I believe the fix is as simple as this PR.

Thanks for your consideration.

@github-actions
Copy link
Contributor

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

bidict (https://github.com/jab/bidict) + bidict/_frozenbidict.py:36: error: Unused "type: ignore" comment edgedb (https://github.com/edgedb/edgedb) + edb/common/checked.py:335:9: error: Cannot assign to a method+ edb/common/checked.py:335:22: error: Incompatible types in assignment (expression has type "int", variable has type "Callable[[], int]")+ edb/common/checked.py:338:12: error: Non-overlapping equality check (left operand type: "Callable[[], int]", right operand type: "Literal[-1]")+ edb/common/checked.py:339:13: error: Cannot assign to a method+ edb/common/checked.py:339:26: error: Incompatible types in assignment (expression has type "int", variable has type "Callable[[], int]")+ edb/common/checked.py:340:16: error: Incompatible return value type (got "Callable[[], int]", expected "int")

@jab
Copy link
ContributorAuthor

jab commented Feb 7, 2022

@elprans@1st1 in case of interest – looks like AbstractCheckedSet in EdgeDB is (unintentionally?) overriding the _hash method it inherits from collections.abc.Set, setting this attribute to an int (changing the type from the inherited attribute), and would therefore need to be updated to type check successfully if this PR is accepted.

(All the best with the upcoming 1.0 launch btw, can't wait to try out EdgeDB sometime!)

Copy link
Collaborator

@AkuliAkuli left a comment

Choose a reason for hiding this comment

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

This isn't ideal, because type checkers think that the built-in set inherits from AbstractSet and also has a _hash() method. At runtime, set doesn't actually inherit from AbstractSet though. So code like set()._hash() will type check but fail at runtime.

According to our CONTRIBUTING.md, we usually prefer false negatives (no error for incorrect code, this PR) over false positives (errors for correct code, current situation). But before merging this, I'll leave this open for a while, so other maintainers can decide what to do with this.

Copy link
Collaborator

@srittausrittau left a comment

Choose a reason for hiding this comment

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

Agree with @Akuli

@JelleZijlstraJelleZijlstra merged commit 9a257c1 into python:masterFeb 8, 2022
@elprans
Copy link

@elprans@1st1 in case of interest – looks like AbstractCheckedSet in EdgeDB is (unintentionally?) overriding the _hash method it inherits from collections.abc.Set, setting this attribute to an int (changing the type from the inherited attribute), and would therefore need to be updated to type check successfully if this PR is accepted.

Thanks for the heads up! Yes, the shadowing isn't intentional, will fix.

(All the best with the upcoming 1.0 launch btw, can't wait to try out EdgeDB sometime!)

Thank you!

Sign up for freeto 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.

5 participants

@jab@elprans@srittau@Akuli@JelleZijlstra