Issue32929
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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:44 | eric.smith | set | messages: + msg312905 |
2018-02-26 02:33:23 | eric.smith | set | status: open -> closed priority: release blocker -> normal versions: + Python 3.8 messages: + msg312873 resolution: fixed stage: patch review -> resolved |
2018-02-26 02:31:35 | miss-islington | set | pull_requests: + pull_request5672 |
2018-02-26 02:30:20 | eric.smith | set | messages: + msg312872 |
2018-02-26 01:29:43 | gvanrossum | set | messages: + msg312871 |
2018-02-25 18:57:52 | eric.smith | set | messages: + msg312834 |
2018-02-25 18:51:02 | eric.smith | set | keywords:
+ patch stage: patch review pull_requests: + pull_request5663 |
2018-02-25 18:01:09 | eric.smith | set | messages: + msg312829 |
2018-02-25 17:31:01 | gvanrossum | set | messages: + msg312827 |
2018-02-25 17:25:57 | eric.smith | set | messages: + msg312825 |
2018-02-24 22:26:57 | gvanrossum | set | messages: + msg312763 |
2018-02-24 17:54:31 | eric.smith | set | messages: + msg312738 |
2018-02-24 16:04:09 | gvanrossum | set | messages: + msg312729 |
2018-02-24 07:27:29 | eric.smith | set | nosy:
+ gvanrossum messages: + msg312702 |
2018-02-24 02:10:55 | steven.daprano | set | nosy:
+ steven.daprano messages: + msg312688 |
2018-02-24 01:44:53 | eric.smith | create |