From 9c36be6aedb0b7be1e353020bcb3979c90791399 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 2 Dec 2021 17:18:15 +0000 Subject: [PATCH 01/11] bpo-45897: Fix frozen-slotted dataclass bug --- Lib/dataclasses.py | 22 +++++++---- Lib/test/test_dataclasses.py | 39 +++++++++++++++++++ .../2021-12-01-21-45-43.bpo-45897.lAZPRy.rst | 4 ++ 3 files changed, 57 insertions(+), 8 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2021-12-01-21-45-43.bpo-45897.lAZPRy.rst diff --git a/Lib/dataclasses.py b/Lib/dataclasses.py index 8643589077a4a8..8c2cebc667efc3 100644 --- a/Lib/dataclasses.py +++ b/Lib/dataclasses.py @@ -606,14 +606,18 @@ def _frozen_get_del_attr(cls, fields, globals): fields_str = '()' return (_create_fn('__setattr__', ('self', 'name', 'value'), - (f'if type(self) is cls or name in {fields_str}:', + (f'if "__slots__" in cls.__dict__ and name not in cls.__slots__:', + ' pass', + f'elif type(self) is cls or name in {fields_str}:', ' raise FrozenInstanceError(f"cannot assign to field {name!r}")', f'super(cls, self).__setattr__(name, value)'), locals=locals, globals=globals), _create_fn('__delattr__', ('self', 'name'), - (f'if type(self) is cls or name in {fields_str}:', + (f'if "__slots__" in cls.__dict__ and name not in cls.__slots__:', + ' pass', + f'elif type(self) is cls or name in {fields_str}:', ' raise FrozenInstanceError(f"cannot delete field {name!r}")', f'super(cls, self).__delattr__(name)'), locals=locals, @@ -1072,12 +1076,6 @@ def _process_class(cls, init, repr, eq, order, unsafe_hash, frozen, f'in class {cls.__name__}. Consider using ' 'functools.total_ordering') - if frozen: - for fn in _frozen_get_del_attr(cls, field_list, globals): - if _set_new_attribute(cls, fn.__name__, fn): - raise TypeError(f'Cannot overwrite attribute {fn.__name__} ' - f'in class {cls.__name__}') - # Decide if/how we're going to create a hash function. hash_action = _hash_action[bool(unsafe_hash), bool(eq), @@ -1101,6 +1099,14 @@ def _process_class(cls, init, repr, eq, order, unsafe_hash, frozen, if slots: cls = _add_slots(cls, frozen) + # Frozen __setattr__/__delattr__ must be added after __slots__ + # bpo-45897 + if frozen: + for fn in _frozen_get_del_attr(cls, field_list, globals): + if _set_new_attribute(cls, fn.__name__, fn): + raise TypeError(f'Cannot overwrite attribute {fn.__name__} ' + f'in class {cls.__name__}') + abc.update_abstractmethods(cls) return cls diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index bcd004f4ec3aa2..228fca9f60bed6 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2902,6 +2902,45 @@ class A: self.assertEqual(obj.a, 'a') self.assertEqual(obj.b, 'b') + def test_frozen_slots_raises_correct_error(self): + # bpo-45897 + @dataclass(slots=True, frozen=True) + class Point: + x: int + y: int + + p = Point(1, 2) + self.assertEqual(p.x, 1) + self.assertEqual(p.y, 2) + with self.assertRaises(FrozenInstanceError): + p.x = 2 + try: + p.z = 5 + except FrozenInstanceError as exc: + raise TypeError( + 'FrozenInstanceError unexpectedly raised instead of AttributeError' + ) from exc + except AttributeError: + pass + else: + self.fail('AttributeError expected, but no error was raised') + + @dataclass(frozen=True) + class DataclassSubclassOfPointWithNoSlots(Point): pass + + d = DataclassSubclassOfPointWithNoSlots(1, 2) + with self.assertRaises(FrozenInstanceError): + d.x = 2 + with self.assertRaises(FrozenInstanceError): + d.z = 5 + + class NonDataclassSubclassOfPointWithNoSlots(Point): pass + + n = NonDataclassSubclassOfPointWithNoSlots(1, 2) + with self.assertRaises(FrozenInstanceError): + n.x = 2 + n.z = 5 + class TestDescriptors(unittest.TestCase): def test_set_name(self): # See bpo-33141. diff --git a/Misc/NEWS.d/next/Library/2021-12-01-21-45-43.bpo-45897.lAZPRy.rst b/Misc/NEWS.d/next/Library/2021-12-01-21-45-43.bpo-45897.lAZPRy.rst new file mode 100644 index 00000000000000..7f873b7f8899ab --- /dev/null +++ b/Misc/NEWS.d/next/Library/2021-12-01-21-45-43.bpo-45897.lAZPRy.rst @@ -0,0 +1,4 @@ +Fix bug in dataclasses where a frozen dataclass with slots would raise +``TypeError`` rather than ``AttributeError`` if a user attempted to +set/delete an attribute that was not specified in ``__slots__``. Patch by +Alex Waygood. From e968679ae4831bdf4841c8b9e701905375dde21f Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 15 Dec 2021 09:45:58 +0000 Subject: [PATCH 02/11] Tweak test --- Lib/test/test_dataclasses.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index 228fca9f60bed6..b1106eaf15c57d 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2914,14 +2914,18 @@ class Point: self.assertEqual(p.y, 2) with self.assertRaises(FrozenInstanceError): p.x = 2 + try: p.z = 5 - except FrozenInstanceError as exc: - raise TypeError( - 'FrozenInstanceError unexpectedly raised instead of AttributeError' - ) from exc - except AttributeError: - pass + except Exception as exc: + exc_type = type(exc) + if exc_type is not AttributeError: + raise TypeError( + f'AttributeError was expected; got {exc_type.__name__!r} instead' + ) from exc + else: + # Good! The test passes; AttributeError is what we want + pass else: self.fail('AttributeError expected, but no error was raised') From df2216b34a41b890f6091fdfb89f6d2c2595fe52 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 15 Dec 2021 14:40:12 +0000 Subject: [PATCH 03/11] Address review --- Lib/test/test_dataclasses.py | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index b1106eaf15c57d..0b881965813263 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2915,19 +2915,22 @@ class Point: with self.assertRaises(FrozenInstanceError): p.x = 2 - try: + with self.assertRaises(AttributeError) as ex: p.z = 5 - except Exception as exc: - exc_type = type(exc) - if exc_type is not AttributeError: - raise TypeError( - f'AttributeError was expected; got {exc_type.__name__!r} instead' - ) from exc - else: - # Good! The test passes; AttributeError is what we want - pass - else: - self.fail('AttributeError expected, but no error was raised') + type_of_caught_exception = type(ex.exception) + + # Verify that the caught exception is `AttributeError` exactly, + # and *not* a subclass of `AttributeError` such as `FrozenInstanceError`. + # + # We need to ensure that this error is being caused by the standard machinery that + # prevents new attributes being added that aren't specified in __slots__. + # + # We *do not* want the error to be caused by anything related to dataclasses. + self.assertIs( + type_of_caught_exception, + AttributeError, + msg=f"expected 'AttributeError', got {type_of_caught_exception.__name__!r}" + ) @dataclass(frozen=True) class DataclassSubclassOfPointWithNoSlots(Point): pass @@ -2943,6 +2946,9 @@ class NonDataclassSubclassOfPointWithNoSlots(Point): pass n = NonDataclassSubclassOfPointWithNoSlots(1, 2) with self.assertRaises(FrozenInstanceError): n.x = 2 + + # This should pass without any exception being raised, + # since the subclass does not define __slots__. n.z = 5 class TestDescriptors(unittest.TestCase): From a71575ffb859d9325778e0aed0bfa8c3ae9f99cf Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 15 Dec 2021 15:53:48 +0000 Subject: [PATCH 04/11] Add further clarification --- Lib/test/test_dataclasses.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index 0b881965813263..a329f8d1b5ead3 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2938,6 +2938,10 @@ class DataclassSubclassOfPointWithNoSlots(Point): pass d = DataclassSubclassOfPointWithNoSlots(1, 2) with self.assertRaises(FrozenInstanceError): d.x = 2 + + # The subclass is frozen, so any attempt to set an attribute should raise an error. + # The subclass does not define slots, however, + # so the error should be `FrozenInstanceError` here rather than `AttributeError`. with self.assertRaises(FrozenInstanceError): d.z = 5 @@ -2948,7 +2952,7 @@ class NonDataclassSubclassOfPointWithNoSlots(Point): pass n.x = 2 # This should pass without any exception being raised, - # since the subclass does not define __slots__. + # since the subclass does not define __slots__ and is not frozen. n.z = 5 class TestDescriptors(unittest.TestCase): From eda3c7f38cd94ea14812d8908902172d06a5f75e Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 16 Dec 2021 15:37:02 +0000 Subject: [PATCH 05/11] Add more comments --- Lib/dataclasses.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Lib/dataclasses.py b/Lib/dataclasses.py index 8c2cebc667efc3..b3958df3e3a02d 100644 --- a/Lib/dataclasses.py +++ b/Lib/dataclasses.py @@ -604,6 +604,11 @@ def _frozen_get_del_attr(cls, fields, globals): else: # Special case for the zero-length tuple. fields_str = '()' + + # bpo-45897: Do not raise FrozenInstanceError + # if the dataclass has slots & the attribute is not specified in __slots__. + # Instead, let the standard __slots__ machinery raise AttributeError naturally + # in the super(cls, self).__setattr__ call return (_create_fn('__setattr__', ('self', 'name', 'value'), (f'if "__slots__" in cls.__dict__ and name not in cls.__slots__:', From 9af70742953f2f8fc62124eef4931a9eda453b80 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 16 Dec 2021 22:37:56 +0000 Subject: [PATCH 06/11] Update Lib/test/test_dataclasses.py Co-authored-by: Erlend Egeberg Aasland --- Lib/test/test_dataclasses.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index a329f8d1b5ead3..93dd55e23d88bc 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2939,9 +2939,10 @@ class DataclassSubclassOfPointWithNoSlots(Point): pass with self.assertRaises(FrozenInstanceError): d.x = 2 - # The subclass is frozen, so any attempt to set an attribute should raise an error. - # The subclass does not define slots, however, - # so the error should be `FrozenInstanceError` here rather than `AttributeError`. + # The subclass is frozen, so any attempt to set an attribute should + # raise an error. The subclass does not define slots, however, so the + # error should be `FrozenInstanceError` here rather than + # `AttributeError`. with self.assertRaises(FrozenInstanceError): d.z = 5 From 9f8c91c2b6c842e56ccf1f358f44ea2741816096 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 16 Dec 2021 22:38:05 +0000 Subject: [PATCH 07/11] Update Lib/test/test_dataclasses.py Co-authored-by: Erlend Egeberg Aasland --- Lib/test/test_dataclasses.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index 93dd55e23d88bc..c57952f5606006 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2952,8 +2952,8 @@ class NonDataclassSubclassOfPointWithNoSlots(Point): pass with self.assertRaises(FrozenInstanceError): n.x = 2 - # This should pass without any exception being raised, - # since the subclass does not define __slots__ and is not frozen. + # This should pass without any exception being raised, since the + # subclass does not define __slots__ and is not frozen. n.z = 5 class TestDescriptors(unittest.TestCase): From ce675c5686f3fe993ca14d521247e81487f82a6d Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 16 Dec 2021 22:41:48 +0000 Subject: [PATCH 08/11] Add comment --- Lib/test/test_dataclasses.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index c57952f5606006..128d4b370fdf7a 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2903,7 +2903,10 @@ class A: self.assertEqual(obj.b, 'b') def test_frozen_slots_raises_correct_error(self): - # bpo-45897 + # bpo-45897: for a frozen-slotted dataclass instance, + # attempting to set an attribute not specified + # in __slots__ should raise AttributeError, + # not TypeError or FrozenInstanceError @dataclass(slots=True, frozen=True) class Point: x: int From 007a596e3c46229a0304cbc7929d817b1b88d913 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 16 Dec 2021 23:00:58 +0000 Subject: [PATCH 09/11] Use self.assertRaisesRegex, since Erlend insists ;) --- Lib/test/test_dataclasses.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index 128d4b370fdf7a..cb50ed5672c2af 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2918,7 +2918,8 @@ class Point: with self.assertRaises(FrozenInstanceError): p.x = 2 - with self.assertRaises(AttributeError) as ex: + regex = 'object has no attribute' + with self.assertRaisesRegex(AttributeError, regex) as ex: p.z = 5 type_of_caught_exception = type(ex.exception) From bea4c9d4fb40ffd252e5f46f021eec6c4e317df6 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 16 Dec 2021 23:06:40 +0000 Subject: [PATCH 10/11] 79-character wrap --- Lib/test/test_dataclasses.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index cb50ed5672c2af..def5a8ab8af281 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2903,9 +2903,8 @@ class A: self.assertEqual(obj.b, 'b') def test_frozen_slots_raises_correct_error(self): - # bpo-45897: for a frozen-slotted dataclass instance, - # attempting to set an attribute not specified - # in __slots__ should raise AttributeError, + # bpo-45897: for a frozen-slotted dataclass instance, attempting to set + # an attribute not specified in __slots__ should raise AttributeError, # not TypeError or FrozenInstanceError @dataclass(slots=True, frozen=True) class Point: From b7ef7b9bd1f596c572c4428159d15de2531b9cd3 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Sun, 24 Apr 2022 22:22:54 -0600 Subject: [PATCH 11/11] Fix trailing whitespace --- Lib/test/test_dataclasses.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index 9ea43382d1aec9..e5623cc636f193 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -3066,8 +3066,8 @@ class Point: # # We *do not* want the error to be caused by anything related to dataclasses. self.assertIs( - type_of_caught_exception, - AttributeError, + type_of_caught_exception, + AttributeError, msg=f"expected 'AttributeError', got {type_of_caught_exception.__name__!r}" )