Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 35.2k
gh-143715: Deprecate incomplete initialization of struct.Struct()#145580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
fff147fd2a0345589fbc75e91e87babb2740e4e84b3f36635e576475628aadd979cc18f7492ecdb8f5f2dc8cbef6206ae112143d1d4ca6b86b9b4fb79961a258550c0c815882f1388ee685a6c476f8723538d30109d6ebd7f0a13326cde89769782005eb292d9c04883de045bebcf358e003dabd45399066dbc046d60b16d7da0f8ca960a8File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -591,27 +591,36 @@ def test_Struct_reinitialization(self): | ||||||
| # Struct instance. This test can be used to detect the leak | ||||||
| # when running with regrtest -L. | ||||||
| s = struct.Struct('>h') | ||||||
| s.__init__('>hh') | ||||||
| msg = 'Re-initialization .* will not work' | ||||||
| with self.assertWarnsRegex(FutureWarning, msg): | ||||||
| s.__init__('>hh') | ||||||
| self.assertEqual(s.format, '>hh') | ||||||
| packed = b'\x00\x01\x00\x02' | ||||||
| self.assertEqual(s.pack(1, 2), packed) | ||||||
| self.assertEqual(s.unpack(packed), (1, 2)) | ||||||
| with self.assertRaises(UnicodeEncodeError): | ||||||
| s.__init__('\udc00') | ||||||
| s.__init__('>hh') # same format | ||||||
| self.assertEqual(s.format, '>hh') | ||||||
| self.assertEqual(s.pack(1, 2), packed) | ||||||
| self.assertEqual(s.unpack(packed), (1, 2)) | ||||||
| with self.assertRaises(struct.error): | ||||||
| s.__init__('$') | ||||||
| with self.assertWarnsRegex(FutureWarning, msg): | ||||||
| with self.assertRaises(UnicodeEncodeError): | ||||||
| s.__init__('\udc00') | ||||||
| self.assertEqual(s.format, '>hh') | ||||||
| self.assertEqual(s.pack(1, 2), packed) | ||||||
| self.assertEqual(s.unpack(packed), (1, 2)) | ||||||
| with self.assertWarnsRegex(FutureWarning, msg): | ||||||
| with self.assertRaises(struct.error): | ||||||
| s.__init__('$') | ||||||
| self.assertEqual(s.format, '>hh') | ||||||
| self.assertEqual(s.pack(1, 2), packed) | ||||||
| self.assertEqual(s.unpack(packed), (1, 2)) | ||||||
| def check_sizeof(self, format_str, number_of_codes): | ||||||
| # The size of 'PyStructObject' | ||||||
| totalsize = support.calcobjsize('2n3P') | ||||||
| totalsize = support.calcobjsize('2n3P?0P') | ||||||
| # The size taken up by the 'formatcode' dynamic array | ||||||
| totalsize += struct.calcsize('P3n0P') * (number_of_codes + 1) | ||||||
| support.check_sizeof(self, struct.Struct(format_str), totalsize) | ||||||
| @@ -809,14 +818,152 @@ def test_error_propagation(fmt_str): | ||||||
| test_error_propagation('N') | ||||||
| test_error_propagation('n') | ||||||
| def test_struct_subclass_instantiation(self): | ||||||
| def test_custom_struct_init(self): | ||||||
| # Regression test for https://github.com/python/cpython/issues/112358 | ||||||
| class MyStruct(struct.Struct): | ||||||
| def __init__(self): | ||||||
| def __init__(self, *args, **kwargs): | ||||||
| super().__init__('>h') | ||||||
| my_struct = MyStruct('>h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this will be more clear, per Victor's suggestion:
Suggested change
(and in all cases below too) MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's true. But I think that with format field tests will be more clear.
I see, you test some such cases with bad characters. MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added assertions for | ||||||
| my_struct = MyStruct(format='>h') | ||||||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I entirely forgot that But again, this should emit a warning. | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| warnmsg = r"Different format arguments for __new__\(\) and __init__\(\) methods of Struct" | ||||||
| with self.assertWarnsRegex(FutureWarning, warnmsg): | ||||||
| my_struct = MyStruct('<h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertWarnsRegex(FutureWarning, warnmsg): | ||||||
| my_struct = MyStruct(format='<h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| warnmsg = r"Struct\(\) missing required argument 'format' \(pos 1\)" | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg): | ||||||
| my_struct = MyStruct() | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg): | ||||||
| my_struct = MyStruct(arg='>h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| warnmsg = r"Struct\(\) takes at most 1 argument \(2 given\)" | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg): | ||||||
| my_struct = MyStruct('>h', 42) | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg): | ||||||
| my_struct = MyStruct('>h', arg=42) | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg): | ||||||
| my_struct = MyStruct('>h', format=42) | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg): | ||||||
| my_struct = MyStruct(format='>h', arg=42) | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| warnmsg = r"Invalid 'format' argument for Struct\.__new__\(\): " | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg + '.*must be'): | ||||||
| my_struct = MyStruct(42) | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg + '.*must be'): | ||||||
| my_struct = MyStruct(format=42) | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg + 'bad char'): | ||||||
| my_struct = MyStruct('$') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg + 'bad char'): | ||||||
| my_struct = MyStruct(format='$') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg + ".*can't encode"): | ||||||
| my_struct = MyStruct('\udc00') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertWarnsRegex(DeprecationWarning, warnmsg + ".*can't encode"): | ||||||
| my_struct = MyStruct(format='\udc00') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| def test_custom_struct_new(self): | ||||||
| # New way, no warnings: | ||||||
| class MyStruct(struct.Struct): | ||||||
| def __new__(cls, *args, **kwargs): | ||||||
| return super().__new__(cls, '>h') | ||||||
| for format in '>h', '<h', 42, '$', '\u20ac', '\udc00', b'\xa4': | ||||||
| with self.subTest(format=format): | ||||||
| my_struct = MyStruct(format) | ||||||
| self.assertEqual(my_struct.format, '>h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| my_struct = MyStruct(format='<h') | ||||||
| self.assertEqual(my_struct.format, '>h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| my_struct = MyStruct() | ||||||
| self.assertEqual(my_struct.format, '>h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| my_struct = MyStruct('<h', 42) | ||||||
| self.assertEqual(my_struct.format, '>h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| def test_custom_struct_new_and_init(self): | ||||||
| # New way, no warnings: | ||||||
| class MyStruct(struct.Struct): | ||||||
| def __new__(cls, newargs, initargs): | ||||||
| return super().__new__(cls, *newargs) | ||||||
| def __init__(self, newargs, initargs): | ||||||
| if initargs is not None: | ||||||
| super().__init__(*initargs) | ||||||
| my_struct = MyStruct(('>h',), ('>h',)) | ||||||
Comment on lines
+904
to
+912
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, why no warnings here? BTW, I doubt this usage pattern come from reality ;-) MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because As for other cases, having both custom | ||||||
| self.assertEqual(my_struct.format, '>h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct((), ()) | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct(('>h',), ()) | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct((), ('>h',)) | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct((42,), ('>h',)) | ||||||
| with self.assertWarns(FutureWarning): | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct(('>h',), (42,)) | ||||||
| with self.assertRaises(struct.error): | ||||||
| MyStruct(('$',), ('>h',)) | ||||||
| with self.assertWarns(FutureWarning): | ||||||
| with self.assertRaises(struct.error): | ||||||
| MyStruct(('>h',), ('$',)) | ||||||
| with self.assertRaises(UnicodeEncodeError): | ||||||
| MyStruct(('\udc00',), ('>h',)) | ||||||
| with self.assertWarns(FutureWarning): | ||||||
| with self.assertRaises(UnicodeEncodeError): | ||||||
| MyStruct(('>h',), ('\udc00',)) | ||||||
| with self.assertWarns(FutureWarning): | ||||||
| my_struct = MyStruct(('>h',), ('<h',)) | ||||||
| self.assertEqual(my_struct.format, '<h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x39\x30') | ||||||
| def test_no_custom_struct_new_or_init(self): | ||||||
| class MyStruct(struct.Struct): | ||||||
| pass | ||||||
| my_struct = MyStruct('>h') | ||||||
| self.assertEqual(my_struct.format, '>h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| my_struct = MyStruct(format='>h') | ||||||
| self.assertEqual(my_struct.format, '>h') | ||||||
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct() | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct(42) | ||||||
| with self.assertRaises(struct.error): | ||||||
| MyStruct('$') | ||||||
| with self.assertRaises(UnicodeEncodeError): | ||||||
| MyStruct('\udc00') | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct('>h', 42) | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct('>h', arg=42) | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct(arg=42) | ||||||
| with self.assertRaises(TypeError): | ||||||
| MyStruct('>h', format='>h') | ||||||
| def test_repr(self): | ||||||
| s = struct.Struct('=i2H') | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Calling the ``Struct.__new__()`` without required argument now is deprecated. | ||
| Calling :meth:`~object.__init__` method on initialized :class:`~struct.Struct` | ||
| objects is deprecated. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this not raises a warning? I think we should warn in all cases, where
Struct.__init__()was explicitly called. // #143659 (comment)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this works with old and with new code. Both
__new__and__init__take the same argument.The code
works now and will work in future.