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

Improve attrs hashability detection #16556

Merged
merged 5 commits into from Dec 12, 2023
Merged

Conversation

Tinche
Copy link
Contributor

@Tinche Tinche commented Nov 24, 2023

Fixes #16550

Improve hashability detection for attrs classes.

I added a new parameter to add_attribute_to_class, overwrite_existing, since I needed it.

Frozen classes remain hashable, non-frozen default to no. This can be overriden by passing in hash=True or unsafe_hash=True (new parameter, added to stubs) to define().

Inheritance-wise I think we're correct, if a non-hashable class inherits from a hashable one, it's still unhashable at runtime.

Accompanying tests.

This comment has been minimized.

This comment has been minimized.

@Tinche
Copy link
Contributor Author

Tinche commented Dec 8, 2023

@sobolevn Does this look good to you? 🙏

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Dec 9, 2023

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Let's wait a bit for more possible input. I will merge this in several days.

@sobolevn sobolevn merged commit 91be285 into python:master Dec 12, 2023
18 checks passed
@brianmedigate
Copy link

brianmedigate commented Mar 10, 2024

@Tinche are you sure about the inheritance behavior? I just upgraded to mypy 1.9 and encountered this. I have a frozen=True attrs class which inherits from a non-frozen one, and it does seem to be hashable at runtime:

In [1]: import attrs

In [2]: @attrs.define
   ...: class A:
   ...:     a: int
   ...:

In [3]: @attrs.define(frozen=True)
   ...: class B(A):
   ...:     b: int
   ...:

In [4]: hash(B(a=1, b=2))
Out[4]: -1340312973636267308

In [5]: hash(A(a=1))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[5], line 1
----> 1 hash(A(a=1))

TypeError: unhashable type: 'A'

However mypy doesn't seem to think it is:

import attrs
import functools


@attrs.define
class A:
    a: int


@attrs.define(frozen=True)
class B(A):
    b: int


b = B(a=1, b=2)


@functools.lru_cache()
def foo(b: B) -> int:
    return b.b


foo(b)
$ mypy test.py
test.py:23: error: Argument 1 to "__call__" of "_lru_cache_wrapper" has incompatible type "B"; expected "Hashable"  [arg-type]
test.py:23: note: Following member(s) of "B" have conflicts:
test.py:23: note:     __hash__: expected "Callable[[], int]", got "None"

This is using attrs 23.3.0, mypy 1.9.0.

@Tinche Tinche deleted the tin/fix-attrs-hash branch March 10, 2024 11:24
@Tinche
Copy link
Contributor Author

Tinche commented Mar 10, 2024

@brianmedigate I tested the other direction :/

I'll look into handling this case in a new PR.

Hnasar pushed a commit to Hnasar/mypy that referenced this pull request Mar 11, 2024
This commit fixes a couple regressions in 1.9.0 from 91be285.

Attrs' logic for hashability is slightly complex:

* https://www.attrs.org/en/stable/hashing.html
* https://github.com/python-attrs/attrs/blob/9e443b18527dc96b194e92805fa751cbf8434ba9/src/attr/_make.py#L1660-L1689

Mypy now properly emulates attrs' logic so that custom `__hash__`
implementations are preserved, `@frozen` subclasses are always hashable,
and classes are only made unhashable based on the values of eq and
`unsafe_hash`.

Fixes python#17015
Fixes python#16556 (comment)

Based on a patch in python#17012
Co-Authored-By: Tin Tvrtkovic <tinchester@gmail.com>
Hnasar added a commit to Hnasar/mypy that referenced this pull request Mar 11, 2024
This commit fixes a couple regressions in 1.9.0 from 91be285.

Attrs' logic for hashability is slightly complex:

* https://www.attrs.org/en/stable/hashing.html
* https://github.com/python-attrs/attrs/blob/9e443b18527dc96b194e92805fa751cbf8434ba9/src/attr/_make.py#L1660-L1689

Mypy now properly emulates attrs' logic so that custom `__hash__`
implementations are preserved, `@frozen` subclasses are always hashable,
and classes are only made unhashable based on the values of eq and
`unsafe_hash`.

Fixes python#17015
Fixes python#16556 (comment)

Based on a patch in python#17012
Co-Authored-By: Tin Tvrtkovic <tinchester@gmail.com>
Hnasar added a commit to Hnasar/mypy that referenced this pull request Mar 11, 2024
This commit fixes a couple regressions in 1.9.0 from 91be285.

Attrs' logic for hashability is slightly complex:

* https://www.attrs.org/en/stable/hashing.html
* https://github.com/python-attrs/attrs/blob/9e443b18527dc96b194e92805fa751cbf8434ba9/src/attr/_make.py#L1660-L1689

Mypy now properly emulates attrs' logic so that custom `__hash__`
implementations are preserved, `@frozen` subclasses are always hashable,
and classes are only made unhashable based on the values of eq and
`unsafe_hash`.

Fixes python#17015
Fixes python#16556 (comment)

Based on a patch in python#17012
Co-Authored-By: Tin Tvrtkovic <tinchester@gmail.com>
Hnasar added a commit to Hnasar/mypy that referenced this pull request Mar 11, 2024
This commit fixes a couple regressions in 1.9.0 from 91be285.

Attrs' logic for hashability is slightly complex:

* https://www.attrs.org/en/stable/hashing.html
* https://github.com/python-attrs/attrs/blob/9e443b18527dc96b194e92805fa751cbf8434ba9/src/attr/_make.py#L1660-L1689

Mypy now properly emulates attrs' logic so that custom `__hash__`
implementations are preserved, `@frozen` subclasses are always hashable,
and classes are only made unhashable based on the values of eq and
`unsafe_hash`.

Fixes python#17015
Fixes python#16556 (comment)

Based on a patch in python#17012
Co-Authored-By: Tin Tvrtkovic <tinchester@gmail.com>
Hnasar added a commit to Hnasar/mypy that referenced this pull request Mar 11, 2024
This commit fixes a couple regressions in 1.9.0 from 91be285.

Attrs' logic for hashability is slightly complex:

* https://www.attrs.org/en/stable/hashing.html
* https://github.com/python-attrs/attrs/blob/9e443b18527dc96b194e92805fa751cbf8434ba9/src/attr/_make.py#L1660-L1689

Mypy now properly emulates attrs' logic so that custom `__hash__`
implementations are preserved, `@frozen` subclasses are always hashable,
and classes are only made unhashable based on the values of `eq` and
`unsafe_hash`.

Fixes python#17015
Fixes python#16556 (comment)

Based on a patch in python#17012
Co-Authored-By: Tin Tvrtkovic <tinchester@gmail.com>
Hnasar added a commit to Hnasar/mypy that referenced this pull request Mar 12, 2024
This commit fixes a couple regressions in 1.9.0 from 91be285.

Attrs' logic for hashability is slightly complex:

* https://www.attrs.org/en/stable/hashing.html
* https://github.com/python-attrs/attrs/blob/9e443b18527dc96b194e92805fa751cbf8434ba9/src/attr/_make.py#L1660-L1689

Mypy now properly emulates attrs' logic so that custom `__hash__`
implementations are preserved, `@frozen` subclasses are always hashable,
and classes are only made unhashable based on the values of `eq` and
`unsafe_hash`.

Fixes python#17015
Fixes python#16556 (comment)

Based on a patch in python#17012
Co-Authored-By: Tin Tvrtkovic <tinchester@gmail.com>
JukkaL pushed a commit that referenced this pull request Mar 15, 2024
This commit fixes a couple regressions in 1.9.0 from 91be285.

Attrs' logic for hashability is slightly complex:

* https://www.attrs.org/en/stable/hashing.html
*
https://github.com/python-attrs/attrs/blob/9e443b18527dc96b194e92805fa751cbf8434ba9/src/attr/_make.py#L1660-L1689

Mypy now properly emulates attrs' logic so that custom `__hash__`
implementations are preserved, `@frozen` subclasses are always hashable,
and classes are only made unhashable based on the values of `eq` and
`unsafe_hash`.

Fixes #17015
Fixes #16556 (comment)

Based on a patch in #17012
Co-Authored-By: Tin Tvrtkovic <tinchester@gmail.com>
Co-authored-by: Hashem Nasarat <hashem@hudson-trading.com>
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.

False negative: attrs classes and hashability
4 participants