From 776843d99df427d65374591035c79414f925d2b7 Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Fri, 10 Mar 2023 09:49:51 +0000 Subject: [PATCH 01/13] Use `set`-based name lookup rather than `tuple`s for frozen dataclasses --- Lib/dataclasses.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/dataclasses.py b/Lib/dataclasses.py index 5c0257eba186d1..452967dadeb11b 100644 --- a/Lib/dataclasses.py +++ b/Lib/dataclasses.py @@ -618,10 +618,10 @@ def _frozen_get_del_attr(cls, fields, globals): locals = {'cls': cls, 'FrozenInstanceError': FrozenInstanceError} if fields: - fields_str = '(' + ','.join(repr(f.name) for f in fields) + ',)' + fields_str = '{' + ','.join(repr(f.name) for f in fields) + '}' else: - # Special case for the zero-length tuple. - fields_str = '()' + # Special case for the zero-length set. + fields_str = 'set()' return (_create_fn('__setattr__', ('self', 'name', 'value'), (f'if type(self) is cls or name in {fields_str}:', From 54dd9c3f357517547e2ec46c58bd3e6428b6daba Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Fri, 10 Mar 2023 10:10:17 +0000 Subject: [PATCH 02/13] Add news entry --- .../next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst diff --git a/Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst b/Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst new file mode 100644 index 00000000000000..77c44d0c2bbbeb --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst @@ -0,0 +1 @@ +Use ``set``-based name lookup rather than ``tuple``s for frozen dataclasses From 845c8ffb36ea8291788e5e70ec6b6655e73ce43d Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Fri, 10 Mar 2023 10:17:30 +0000 Subject: [PATCH 03/13] Update new entry --- .../next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst b/Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst index 77c44d0c2bbbeb..7f1830ec247cea 100644 --- a/Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst +++ b/Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst @@ -1 +1 @@ -Use ``set``-based name lookup rather than ``tuple``s for frozen dataclasses +Use ``set``-based name lookup rather than ``tuple``\s for frozen dataclasses From 0f0d97c58d1c966a536a31206dd018faed879865 Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Fri, 10 Mar 2023 12:13:50 +0000 Subject: [PATCH 04/13] Use empty tuple singleton in simple case --- Lib/dataclasses.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/dataclasses.py b/Lib/dataclasses.py index c307d8c5554e71..81150622e5f0e8 100644 --- a/Lib/dataclasses.py +++ b/Lib/dataclasses.py @@ -620,7 +620,8 @@ def _frozen_get_del_attr(cls, fields, globals): fields_str = '{' + ','.join(repr(f.name) for f in fields) + '}' else: # Special case for the zero-length set. - fields_str = 'set()' + # Use the empty tuple singleton to avoid unnecessary `set` construction + fields_str = '()' return (_create_fn('__setattr__', ('self', 'name', 'value'), (f'if type(self) is cls or name in {fields_str}:', From 0524ee4eed81dc6d426ab168668d68b95c57535b Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Fri, 10 Mar 2023 13:21:30 +0000 Subject: [PATCH 05/13] Update new entry --- .../next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst | 1 - .../next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) delete mode 100644 Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst create mode 100644 Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst diff --git a/Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst b/Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst deleted file mode 100644 index 7f1830ec247cea..00000000000000 --- a/Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst +++ /dev/null @@ -1 +0,0 @@ -Use ``set``-based name lookup rather than ``tuple``\s for frozen dataclasses diff --git a/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst b/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst new file mode 100644 index 00000000000000..66fbabdaa303e6 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst @@ -0,0 +1,2 @@ +Speed up sanity check for ``__setattr__`` and ``__delattr__`` for frozen +``dataclass``\es by using ``set``-based name lookup rather than ``tuple``\s. From 4055fa36ebcdd9a6a768bf074fef5e9d852ded7c Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Fri, 10 Mar 2023 13:57:51 +0000 Subject: [PATCH 06/13] Add more details about the change in news entry --- ...-03-10-13-21-16.gh-issue-102578.-gujoI.rst | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst b/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst index 66fbabdaa303e6..e8613d9a8ea118 100644 --- a/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst +++ b/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst @@ -1,2 +1,24 @@ Speed up sanity check for ``__setattr__`` and ``__delattr__`` for frozen ``dataclass``\es by using ``set``-based name lookup rather than ``tuple``\s. + +The sanity check is always performed, even if the ``dataclass`` is inherited. +For example: + +.. code-block:: python + + @dataclass(frozen=True) + class FrozenBase: + x: int + y: int + ... # N_FIELDS + + class Foo(FrozenBase): + def __init__(self, x, y, somevalue, someothervalue): + super().__init__(x, y) + self.somevalue = somevalue # takes O(N_FIELDS) + self.someothervalue = someothervalue # takes O(N_FIELDS) time again + + foo = Foo(1, 2, 3, 4) + foo.extravalue = extravalue # takes O(N_FIELDS) time again + +This change makes the sanity check :math:`O(1)` in the number of fields. From 0bbf3a8344a8ed0bbad230bca7f41bca45da23fc Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Fri, 10 Mar 2023 22:08:14 +0800 Subject: [PATCH 07/13] Update news entry Co-authored-by: Alex Waygood --- ...-03-10-13-21-16.gh-issue-102578.-gujoI.rst | 28 +++---------------- 1 file changed, 4 insertions(+), 24 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst b/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst index e8613d9a8ea118..7307148d9a81ef 100644 --- a/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst +++ b/Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst @@ -1,24 +1,4 @@ -Speed up sanity check for ``__setattr__`` and ``__delattr__`` for frozen -``dataclass``\es by using ``set``-based name lookup rather than ``tuple``\s. - -The sanity check is always performed, even if the ``dataclass`` is inherited. -For example: - -.. code-block:: python - - @dataclass(frozen=True) - class FrozenBase: - x: int - y: int - ... # N_FIELDS - - class Foo(FrozenBase): - def __init__(self, x, y, somevalue, someothervalue): - super().__init__(x, y) - self.somevalue = somevalue # takes O(N_FIELDS) - self.someothervalue = someothervalue # takes O(N_FIELDS) time again - - foo = Foo(1, 2, 3, 4) - foo.extravalue = extravalue # takes O(N_FIELDS) time again - -This change makes the sanity check :math:`O(1)` in the number of fields. +Speed up setting or deleting mutable attributes on non-dataclass subclasses of +frozen dataclasses. Due to the implementation of ``__setattr__`` and +``__delattr__`` for frozen dataclasses, this previously had a time complexity +of ``O(n)``. It now has a time complexity of ``O(1)``. From b90d2514b1a8cba831a2946fd982a54dcd1fe269 Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Sat, 11 Mar 2023 00:06:18 +0800 Subject: [PATCH 08/13] Eliminate unnecessary check for empty fields --- Lib/dataclasses.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/Lib/dataclasses.py b/Lib/dataclasses.py index 81150622e5f0e8..e0a22357bbb82c 100644 --- a/Lib/dataclasses.py +++ b/Lib/dataclasses.py @@ -1,3 +1,4 @@ +from asyncio import Condition import re import sys import copy @@ -616,22 +617,19 @@ def _repr_fn(fields, globals): def _frozen_get_del_attr(cls, fields, globals): locals = {'cls': cls, 'FrozenInstanceError': FrozenInstanceError} + condition = 'type(self) is cls' if fields: - fields_str = '{' + ','.join(repr(f.name) for f in fields) + '}' - else: - # Special case for the zero-length set. - # Use the empty tuple singleton to avoid unnecessary `set` construction - fields_str = '()' + condition += ' or name in {' + ', '.join(repr(f.name) for f in fields) + '}' return (_create_fn('__setattr__', ('self', 'name', 'value'), - (f'if type(self) is cls or name in {fields_str}:', + (f'if {condition}:', ' 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 {condition}:', ' raise FrozenInstanceError(f"cannot delete field {name!r}")', f'super(cls, self).__delattr__(name)'), locals=locals, From d3c84037d25e94e6d26a5b4882d6a912143e2b60 Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Sat, 11 Mar 2023 00:11:03 +0800 Subject: [PATCH 09/13] Remove mistake from auto import --- Lib/dataclasses.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/dataclasses.py b/Lib/dataclasses.py index e0a22357bbb82c..78a126f051e2e7 100644 --- a/Lib/dataclasses.py +++ b/Lib/dataclasses.py @@ -1,4 +1,3 @@ -from asyncio import Condition import re import sys import copy From 84b5bdd3a84be5b392fc7519233e03c3be5bf4e0 Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Sat, 11 Mar 2023 00:13:25 +0800 Subject: [PATCH 10/13] Add test for empty frozen dataclass --- Lib/test/test_dataclasses.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index 589f229f462359..2c7e5073ed86d2 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2767,6 +2767,19 @@ class C: c.i = 5 self.assertEqual(c.i, 10) + def test_frozen_empty(self): + @dataclass(frozen=True) + class C: + pass + + c = C() + with self.assertRaises(FrozenInstanceError): + c.i = 5 + with self.assertRaises(FrozenInstanceError): + del c.i + with self.assertRaises(AttributeError): + del c.i + def test_inherit(self): @dataclass(frozen=True) class C: From 2d38e9c4968cefd21d355962994ac72390f45ced Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Sat, 11 Mar 2023 00:19:30 +0800 Subject: [PATCH 11/13] Add test for empty frozen dataclass --- Lib/test/test_dataclasses.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index 2c7e5073ed86d2..14229befc005db 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2773,8 +2773,10 @@ class C: pass c = C() + self.assertFalse(hasattr(c, 'i')) with self.assertRaises(FrozenInstanceError): c.i = 5 + self.assertFalse(hasattr(c, 'i')) with self.assertRaises(FrozenInstanceError): del c.i with self.assertRaises(AttributeError): From ef8e83fc86de0e2357b114116bcf6bbece30b8cc Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Sat, 11 Mar 2023 00:51:02 +0800 Subject: [PATCH 12/13] Add test for empty frozen dataclass --- Lib/test/test_dataclasses.py | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index 14229befc005db..de5a910b679fbe 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2779,8 +2779,6 @@ class C: self.assertFalse(hasattr(c, 'i')) with self.assertRaises(FrozenInstanceError): del c.i - with self.assertRaises(AttributeError): - del c.i def test_inherit(self): @dataclass(frozen=True) @@ -2905,6 +2903,35 @@ class S(D): self.assertEqual(s.y, 10) self.assertEqual(s.cached, True) + with self.assertRaises(FrozenInstanceError): + del s.x + self.assertEqual(s.x, 3) + with self.assertRaises(FrozenInstanceError): + del s.y + self.assertEqual(s.y, 10) + del s.cached + self.assertFalse(hasattr(s, 'cached')) + with self.assertRaises(AttributeError): + del s.cached + + def test_non_frozen_normal_derived_from_empty_frozen(self): + @dataclass(frozen=True) + class D: + pass + + class S(D): + pass + + s = S() + self.assertFalse(hasattr(s, 'x')) + s.x = 5 + self.assertEqual(s.x, 5) + + del s.x + self.assertFalse(hasattr(s, 'x')) + with self.assertRaises(AttributeError): + del s.x + def test_overwriting_frozen(self): # frozen uses __setattr__ and __delattr__. with self.assertRaisesRegex(TypeError, From 5da787ecae6523a4b784873ec8f12ade0e732ebd Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Sat, 11 Mar 2023 01:01:17 +0800 Subject: [PATCH 13/13] Add extra check for the exception type --- Lib/test/test_dataclasses.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py index de5a910b679fbe..5486b2ef3f47e5 100644 --- a/Lib/test/test_dataclasses.py +++ b/Lib/test/test_dataclasses.py @@ -2911,8 +2911,9 @@ class S(D): self.assertEqual(s.y, 10) del s.cached self.assertFalse(hasattr(s, 'cached')) - with self.assertRaises(AttributeError): + with self.assertRaises(AttributeError) as cm: del s.cached + self.assertNotIsInstance(cm.exception, FrozenInstanceError) def test_non_frozen_normal_derived_from_empty_frozen(self): @dataclass(frozen=True) @@ -2929,8 +2930,9 @@ class S(D): del s.x self.assertFalse(hasattr(s, 'x')) - with self.assertRaises(AttributeError): + with self.assertRaises(AttributeError) as cm: del s.x + self.assertNotIsInstance(cm.exception, FrozenInstanceError) def test_overwriting_frozen(self): # frozen uses __setattr__ and __delattr__.