bpo-32929: Dataclasses: Change the tri-state hash parameter to the boolean unsafe_hash. (#5891)

unsafe_hash=False is now the default. It is the same behavior as the old hash=None parameter. unsafe_hash=True will try to add __hash__. If it already exists, TypeError is raised.

1 files changed, 124 insertions, 116 deletions

diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py
index 9752f55..ea019ae 100755
--- a/Lib/test/test_dataclasses.py
+++ b/Lib/test/test_dataclasses.py
@@ -83,32 +83,59 @@ class TestCase(unittest.TestCase):
             class C(B):
                 x: int = 0
 
-    def test_overwriting_hash(self):
+    def test_overwrite_hash(self):
+        # Test that declaring this class isn't an error.  It should
+        #  use the user-provided __hash__.
         @dataclass(frozen=True)
         class C:
             x: int
             def __hash__(self):
-                pass
+                return 301
+        self.assertEqual(hash(C(100)), 301)
 
-        @dataclass(frozen=True,hash=False)
+        # Test that declaring this class isn't an error.  It should
+        #  use the generated __hash__.
+        @dataclass(frozen=True)
         class C:
             x: int
-            def __hash__(self):
-                return 600
-        self.assertEqual(hash(C(0)), 600)
+            def __eq__(self, other):
+                return False
+        self.assertEqual(hash(C(100)), hash((100,)))
 
-        @dataclass(frozen=True)
+        # But this one should generate an exception, because with
+        #  unsafe_hash=True, it's an error to have a __hash__ defined.
+        with self.assertRaisesRegex(TypeError,
+                                    'Cannot overwrite attribute __hash__'):
+            @dataclass(unsafe_hash=True)
+            class C:
+                def __hash__(self):
+                    pass
+
+        # Creating this class should not generate an exception,
+        #  because even though __hash__ exists before @dataclass is
+        #  called, (due to __eq__ being defined), since it's None
+        #  that's okay.
+        @dataclass(unsafe_hash=True)
         class C:
             x: int
-            def __hash__(self):
+            def __eq__(self):
                 pass
+        # The generated hash function works as we'd expect.
+        self.assertEqual(hash(C(10)), hash((10,)))
+
+        # Creating this class should generate an exception, because
+        # __hash__ exists and is not None, which it would be if it had
+        # been auto-generated do due __eq__ being defined.
+        with self.assertRaisesRegex(TypeError,
+                                    'Cannot overwrite attribute __hash__'):
+            @dataclass(unsafe_hash=True)
+            class C:
+                x: int
+                def __eq__(self):
+                    pass
+                def __hash__(self):
+                    pass
 
-        @dataclass(frozen=True, hash=False)
-        class C:
-            x: int
-            def __hash__(self):
-                return 600
-        self.assertEqual(hash(C(0)), 600)
 
     def test_overwrite_fields_in_derived_class(self):
         # Note that x from C1 replaces x in Base, but the order remains
@@ -294,19 +321,6 @@ class TestCase(unittest.TestCase):
                                             "not supported between instances of 'B' and 'C'"):
                     fn(B(0), C(0))
 
-    def test_0_field_hash(self):
-        @dataclass(hash=True)
-        class C:
-            pass
-        self.assertEqual(hash(C()), hash(()))
-
-    def test_1_field_hash(self):
-        @dataclass(hash=True)
-        class C:
-            x: int
-        self.assertEqual(hash(C(4)), hash((4,)))
-        self.assertEqual(hash(C(42)), hash((42,)))
-
     def test_eq_order(self):
         # Test combining eq and order.
         for (eq,    order, result   ) in [
@@ -407,25 +421,25 @@ class TestCase(unittest.TestCase):
         # Test all 6 cases of:
         #  hash=True/False/None
         #  compare=True/False
-        for (hash_val, compare, result  ) in [
+        for (hash_,    compare, result  ) in [
             (True,     False,   'field' ),
             (True,     True,    'field' ),
             (False,    False,   'absent'),
             (False,    True,    'absent'),
             (None,     False,   'absent'),
             (None,     True,    'field' ),
-        ]:
-            with self.subTest(hash_val=hash_val, compare=compare):
-                @dataclass(hash=True)
+            ]:
+            with self.subTest(hash=hash_, compare=compare):
+                @dataclass(unsafe_hash=True)
                 class C:
-                    x: int = field(compare=compare, hash=hash_val, default=5)
+                    x: int = field(compare=compare, hash=hash_, default=5)
 
                 if result == 'field':
                     # __hash__ contains the field.
-                    self.assertEqual(C(5).__hash__(), hash((5,)))
+                    self.assertEqual(hash(C(5)), hash((5,)))
                 elif result == 'absent':
                     # The field is not present in the hash.
-                    self.assertEqual(C(5).__hash__(), hash(()))
+                    self.assertEqual(hash(C(5)), hash(()))
                 else:
                     assert False, f'unknown result {result!r}'
 
@@ -737,7 +751,7 @@ class TestCase(unittest.TestCase):
         validate_class(C)
 
         # Now repeat with __hash__.
-        @dataclass(frozen=True, hash=True)
+        @dataclass(frozen=True, unsafe_hash=True)
         class C:
             i: int
             j: str
@@ -1107,7 +1121,7 @@ class TestCase(unittest.TestCase):
         self.assertEqual(C().x, [])
 
         # hash
-        @dataclass(hash=True)
+        @dataclass(unsafe_hash=True)
         class C:
             x: list = field(default_factory=list, hash=False)
         self.assertEqual(astuple(C()), ([],))
@@ -2242,28 +2256,13 @@ class TestOrdering(unittest.TestCase):
                     pass
 
 class TestHash(unittest.TestCase):
-    def test_hash(self):
-        @dataclass(hash=True)
+    def test_unsafe_hash(self):
+        @dataclass(unsafe_hash=True)
         class C:
             x: int
             y: str
         self.assertEqual(hash(C(1, 'foo')), hash((1, 'foo')))
 
-    def test_hash_false(self):
-        @dataclass(hash=False)
-        class C:
-            x: int
-            y: str
-        self.assertNotEqual(hash(C(1, 'foo')), hash((1, 'foo')))
-
-    def test_hash_none(self):
-        @dataclass(hash=None)
-        class C:
-            x: int
-        with self.assertRaisesRegex(TypeError,
-                                    "unhashable type: 'C'"):
-            hash(C(1))
-
     def test_hash_rules(self):
         def non_bool(value):
             # Map to something else that's True, but not a bool.
@@ -2273,89 +2272,73 @@ class TestHash(unittest.TestCase):
                 return (3,)
             return 0
 
-        def test(case, hash, eq, frozen, with_hash, result):
-            with self.subTest(case=case, hash=hash, eq=eq, frozen=frozen):
-                if with_hash:
-                    @dataclass(hash=hash, eq=eq, frozen=frozen)
-                    class C:
-                        def __hash__(self):
-                            return 0
-                else:
-                    @dataclass(hash=hash, eq=eq, frozen=frozen)
-                    class C:
-                        pass
+        def test(case, unsafe_hash, eq, frozen, with_hash, result):
+            with self.subTest(case=case, unsafe_hash=unsafe_hash, eq=eq,
+                              frozen=frozen):
+                if result != 'exception':
+                    if with_hash:
+                        @dataclass(unsafe_hash=unsafe_hash, eq=eq, frozen=frozen)
+                        class C:
+                            def __hash__(self):
+                                return 0
+                    else:
+                        @dataclass(unsafe_hash=unsafe_hash, eq=eq, frozen=frozen)
+                        class C:
+                            pass
 
                 # See if the result matches what's expected.
-                if result in ('fn', 'fn-x'):
+                if result == 'fn':
                     # __hash__ contains the function we generated.
                     self.assertIn('__hash__', C.__dict__)
                     self.assertIsNotNone(C.__dict__['__hash__'])
 
-                    if result == 'fn-x':
-                        # This is the "auto-hash test" case.  We
-                        #  should overwrite __hash__ iff there's an
-                        #  __eq__ and if __hash__=None.
-
-                        # There are two ways of getting __hash__=None:
-                        #  explicitely, and by defining __eq__.  If
-                        #  __eq__ is defined, python will add __hash__
-                        #  when the class is created.
-                        @dataclass(hash=hash, eq=eq, frozen=frozen)
-                        class C:
-                            def __eq__(self, other): pass
-                            __hash__ = None
-
-                        # Hash should be overwritten (non-None).
-                        self.assertIsNotNone(C.__dict__['__hash__'])
-
-                        # Same test as above, but we don't provide
-                        #  __hash__, it will implicitely set to None.
-                        @dataclass(hash=hash, eq=eq, frozen=frozen)
-                        class C:
-                            def __eq__(self, other): pass
-
-                        # Hash should be overwritten (non-None).
-                        self.assertIsNotNone(C.__dict__['__hash__'])
-
                 elif result == '':
                     # __hash__ is not present in our class.
                     if not with_hash:
                         self.assertNotIn('__hash__', C.__dict__)
+
                 elif result == 'none':
                     # __hash__ is set to None.
                     self.assertIn('__hash__', C.__dict__)
                     self.assertIsNone(C.__dict__['__hash__'])
+
+                elif result == 'exception':
+                    # Creating the class should cause an exception.
+                    #  This only happens with with_hash==True.
+                    assert(with_hash)
+                    with self.assertRaisesRegex(TypeError, 'Cannot overwrite attribute __hash__'):
+                        @dataclass(unsafe_hash=unsafe_hash, eq=eq, frozen=frozen)
+                        class C:
+                            def __hash__(self):
+                                return 0
+
                 else:
                     assert False, f'unknown result {result!r}'
 
-        # There are 12 cases of:
-        #  hash=True/False/None
+        # There are 8 cases of:
+        #  unsafe_hash=True/False
         #  eq=True/False
         #  frozen=True/False
         # And for each of these, a different result if
         #  __hash__ is defined or not.
-        for case, (hash,  eq,    frozen, result_no, result_yes) in enumerate([
-                  (None,  False, False,  '',        ''),
-                  (None,  False, True,   '',        ''),
-                  (None,  True,  False,  'none',    ''),
-                  (None,  True,  True,   'fn',      'fn-x'),
-                  (False, False, False,  '',        ''),
-                  (False, False, True,   '',        ''),
-                  (False, True,  False,  '',        ''),
-                  (False, True,  True,   '',        ''),
-                  (True,  False, False,  'fn',      'fn-x'),
-                  (True,  False, True,   'fn',      'fn-x'),
-                  (True,  True,  False,  'fn',      'fn-x'),
-                  (True,  True,  True,   'fn',      'fn-x'),
-        ], 1):
-            test(case, hash, eq, frozen, False, result_no)
-            test(case, hash, eq, frozen, True,  result_yes)
+        for case, (unsafe_hash,  eq,    frozen, res_no_defined_hash, res_defined_hash) in enumerate([
+                  (False,        False, False,  '',                  ''),
+                  (False,        False, True,   '',                  ''),
+                  (False,        True,  False,  'none',              ''),
+                  (False,        True,  True,   'fn',                ''),
+                  (True,         False, False,  'fn',                'exception'),
+                  (True,         False, True,   'fn',                'exception'),
+                  (True,         True,  False,  'fn',                'exception'),
+                  (True,         True,  True,   'fn',                'exception'),
+                  ], 1):
+            test(case, unsafe_hash, eq, frozen, False, res_no_defined_hash)
+            test(case, unsafe_hash, eq, frozen, True,  res_defined_hash)
 
             # Test non-bool truth values, too.  This is just to
             #  make sure the data-driven table in the decorator
             #  handles non-bool values.
-            test(case, non_bool(hash), non_bool(eq), non_bool(frozen), False, result_no)
-            test(case, non_bool(hash), non_bool(eq), non_bool(frozen), True,  result_yes)
+            test(case, non_bool(unsafe_hash), non_bool(eq), non_bool(frozen), False, res_no_defined_hash)
+            test(case, non_bool(unsafe_hash), non_bool(eq), non_bool(frozen), True,  res_defined_hash)
 
 
     def test_eq_only(self):
@@ -2373,8 +2356,8 @@ class TestHash(unittest.TestCase):
         self.assertNotEqual(C(1), C(4))
 
         # And make sure things work in this case if we specify
-        #  hash=True.
-        @dataclass(hash=True)
+        #  unsafe_hash=True.
+        @dataclass(unsafe_hash=True)
         class C:
             i: int
             def __eq__(self, other):
@@ -2384,7 +2367,7 @@ class TestHash(unittest.TestCase):
 
         # And check that the classes __eq__ is being used, despite
         #  specifying eq=True.
-        @dataclass(hash=True, eq=True)
+        @dataclass(unsafe_hash=True, eq=True)
         class C:
             i: int
             def __eq__(self, other):
@@ -2393,10 +2376,35 @@ class TestHash(unittest.TestCase):
         self.assertNotEqual(C(1), C(1))
         self.assertEqual(hash(C(1)), hash(C(1.0)))
 
+    def test_0_field_hash(self):
+        @dataclass(frozen=True)
+        class C:
+            pass
+        self.assertEqual(hash(C()), hash(()))
+
+        @dataclass(unsafe_hash=True)
+        class C:
+            pass
+        self.assertEqual(hash(C()), hash(()))
+
+    def test_1_field_hash(self):
+        @dataclass(frozen=True)
+        class C:
+            x: int
+        self.assertEqual(hash(C(4)), hash((4,)))
+        self.assertEqual(hash(C(42)), hash((42,)))
+
+        @dataclass(unsafe_hash=True)
+        class C:
+            x: int
+        self.assertEqual(hash(C(4)), hash((4,)))
+        self.assertEqual(hash(C(42)), hash((42,)))
+
     def test_hash_no_args(self):
         # Test dataclasses with no hash= argument.  This exists to
-        # make sure that when hash is changed, the default hashability
-        # keeps working.
+        # make sure that if the @dataclass parameter name is changed
+        # or the non-default hashing behavior changes, the default
+        # hashability keeps working the same way.
 
         class Base:
             def __hash__(self):