classification
Title: Change dataclasses hashing to use unsafe_hash boolean (default to False)
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.smith Nosy List: eric.smith, gvanrossum, ned.deily, steven.daprano
Priority: normal Keywords: patch

Created on 2018-02-24 01:44 by eric.smith, last changed 2018-02-26 09:43 by eric.smith. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 5891 merged eric.smith, 2018-02-25 18:51
PR 5902 merged miss-islington, 2018-02-26 02:31
Messages (14)
msg312686 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-02-24 01:44
See https://mail.python.org/pipermail/python-dev/2018-February/152150.html for a discussion.

This will remove the @dataclass hash= tri-state parameter, and replace it with an unsafe_hash= boolean-only parameter.

From that thread:

"""
I propose that with @dataclass(unsafe_hash=False) (the default), a __hash__
method is added when the following conditions are true:

- frozen=True (not the default)
- compare=True (the default)
- no __hash__ method is defined in the class

If we instead use @dataclass(unsafe_hash=True), a __hash__ will be added
regardless of the other flags, but if a __hash__ method is present, an
exception is raised.

Other values (e.g. unsafe_hash=None) are illegal for this flag.
"""
msg312688 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2018-02-24 02:10
This is a truly awful name, there's nothing unsafe about the hash 
parameter. We rightly hesitate to label *actual* unsafe functions, like 
eval and exec, with that name. Are we committed to this name?
msg312702 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-02-24 07:27
That's a quote from Guido. There weren't any better ideas on the python-dev thread.
msg312729 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-02-24 16:04
It is important to convey the idea that if you are thinking of using this you are probably doing something fishy.

An alternative could be `_hash`, which at least indicates it is not for general use, like `sys._getframe()`.
msg312738 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-02-24 17:54
Note that this class (from the test suite) will now raise an exception:

        @dataclass(unsafe_hash=True)
        class C:
            i: int
            def __eq__(self, other):
                return self.i == other.i

That's because it has a __hash__, added when __eq__ is defined. I think we're better off with the rule being:

If unsafe_hash=True, raise an exception if __hash__ exists and is not None. Otherwise add __hash__.

That's how it used to be with hash=True (in 3.70b1).

Or is that what you meant by "but if a __hash__ method is present, an exception is raised"? Notice the word "method", instead of attribute.
msg312763 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-02-24 22:26
Sorry, I used imprecise language. What you propose is fine.

On Feb 24, 2018 09:54, "Eric V. Smith" <report@bugs.python.org> wrote:

>
> Eric V. Smith <eric@trueblade.com> added the comment:
>
> Note that this class (from the test suite) will now raise an exception:
>
>         @dataclass(unsafe_hash=True)
>         class C:
>             i: int
>             def __eq__(self, other):
>                 return self.i == other.i
>
> That's because it has a __hash__, added when __eq__ is defined. I think
> we're better off with the rule being:
>
> If unsafe_hash=True, raise an exception if __hash__ exists and is not
> None. Otherwise add __hash__.
>
> That's how it used to be with hash=True (in 3.70b1).
>
> Or is that what you meant by "but if a __hash__ method is present, an
> exception is raised"? Notice the word "method", instead of attribute.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue32929>
> _______________________________________
>
msg312825 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-02-25 17:25
Here's the simplest way I can describe this, and it's also the table used inside the code.  The column "has-explicit-hash?" is trying to answer the question "is there a __hash__ function defined in this class?". It is set to False if either __hash__ is missing, or if __hash__ is None and there's an __eq__ function. I'm assuming that the __hash__ is implicit in the latter case.

# Decide if/how we're going to create a hash function.  Key is
#  (unsafe_hash, eq, frozen, does-hash-exist).  Value is the action to
#  take.
# Actions:
#  '':          Do nothing.
#  'none':      Set __hash__ to None.
#  'add':       Always add a generated __hash__function.
#  'exception': Raise an exception.
#
#                +-------------------------------------- unsafe_hash?
#                |      +------------------------------- eq?
#                |      |      +------------------------ frozen?
#                |      |      |      +----------------  has-explicit-hash?
#                |      |      |      |
#                |      |      |      |        +-------  action
#                |      |      |      |        |
#                v      v      v      v        v
_hash_action = {(False, False, False, False): (''),
                (False, False, False, True ): (''),
                (False, False, True,  False): (''),
                (False, False, True,  True ): (''),
                (False, True,  False, False): ('none'),
                (False, True,  False, True ): (''),
                (False, True,  True,  False): ('add'),
                (False, True,  True,  True ): (''),
                (True,  False, False, False): ('add'),
                (True,  False, False, True ): ('exception'),
                (True,  False, True,  False): ('add'),
                (True,  False, True,  True ): ('exception'),
                (True,  True,  False, False): ('add'),
                (True,  True,  False, True ): ('exception'),
                (True,  True,  True,  False): ('add'),
                (True,  True,  True,  True ): ('exception'),
                }

PR will be ready as soon as I clean a few things up.
msg312827 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-02-25 17:31
I don't know if I'm unique, but I find such a table with 4 Boolean keys
hard to understand. I'd rather see it written up as a series of nested 'if'
statements.
msg312829 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-02-25 18:01
if unsafe_hash:
        # If there's already a __hash__, raise TypeError, otherwise add __hash__.
        if has_explicit_hash:
            hash_action = 'exception'
        else:
            hash_action = 'add'
    else:
        # unsafe_hash is False (the default).
        if has_explicit_hash:
            # There's already a __hash__, don't overwrite it.
            hash_action = ''
        else:
            if eq and frozen:
                # It's frozen and we added __eq__, generate __hash__.
                hash_action = 'add'
            elif eq and not frozen:
                # It's not frozen but has __eq__, make it unhashable.
                #  This is the default if no params to @dataclass.
                hash_action = 'none'
            else:
                # There's no __eq__, use the base class __hash__.
                hash_action = ''
msg312834 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-02-25 18:57
I've been focused on getting the code fix in for the next beta release on 2018-02-26, and leaving the possible parameter name change for later. I don't feel strongly about the parameter name. Right now it's unsafe_hash, per Guido's original proposal.
msg312871 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-02-26 01:29
@steven.daprano the name was +1'ed by many people in the python-dev discussion as sending the right message. So I'm reluctant to change it. If you can cause a landslide of support for `_hash` there we can change it in b3.
msg312872 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-02-26 02:30
New changeset dbf9cff48a4ad0fd58e1c623ce1f36c3dd3d5f38 by Eric V. Smith in branch 'master':
bpo-32929: Dataclasses: Change the tri-state hash parameter to the boolean unsafe_hash. (#5891)
https://github.com/python/cpython/commit/dbf9cff48a4ad0fd58e1c623ce1f36c3dd3d5f38
msg312873 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-02-26 02:33
I'm going to close this. Steven: if you gain support for a parameter name change, please open another issue.
msg312905 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-02-26 09:43
New changeset 4cffe2f66b581fa7538f6de884d54a5c7364d8e0 by Eric V. Smith (Miss Islington (bot)) in branch '3.7':
bpo-32929: Dataclasses: Change the tri-state hash parameter to the boolean unsafe_hash. (GH-5891) (GH-5902)
https://github.com/python/cpython/commit/4cffe2f66b581fa7538f6de884d54a5c7364d8e0
History
Date User Action Args
2018-02-26 09:43:44eric.smithsetmessages: + msg312905
2018-02-26 02:33:23eric.smithsetstatus: open -> closed
priority: release blocker -> normal
versions: + Python 3.8
messages: + msg312873

resolution: fixed
stage: patch review -> resolved
2018-02-26 02:31:35miss-islingtonsetpull_requests: + pull_request5672
2018-02-26 02:30:20eric.smithsetmessages: + msg312872
2018-02-26 01:29:43gvanrossumsetmessages: + msg312871
2018-02-25 18:57:52eric.smithsetmessages: + msg312834
2018-02-25 18:51:02eric.smithsetkeywords: + patch
stage: patch review
pull_requests: + pull_request5663
2018-02-25 18:01:09eric.smithsetmessages: + msg312829
2018-02-25 17:31:01gvanrossumsetmessages: + msg312827
2018-02-25 17:25:57eric.smithsetmessages: + msg312825
2018-02-24 22:26:57gvanrossumsetmessages: + msg312763
2018-02-24 17:54:31eric.smithsetmessages: + msg312738
2018-02-24 16:04:09gvanrossumsetmessages: + msg312729
2018-02-24 07:27:29eric.smithsetnosy: + gvanrossum
messages: + msg312702
2018-02-24 02:10:55steven.dapranosetnosy: + steven.daprano
messages: + msg312688
2018-02-24 01:44:53eric.smithcreate