-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
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
Changes from 29 commits
fff147f
d2a0345
589fbc7
5e91e87
babb274
0e4e84b
3f36635
e576475
628aadd
979cc18
f7492ec
db8f5f2
dc8cbef
6206ae1
12143d1
d4ca6b8
6b9b4fb
79961a2
58550c0
c815882
f1388ee
685a6c4
76f8723
538d301
09d6ebd
7f0a133
26cde89
7697820
05eb292
d9c0488
3de045b
ebcf358
e003dab
d453990
66dbc04
6d60b16
d7da0f8
ca960a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,13 @@ | ||||||
| Pending removal in Python 3.20 | ||||||
| ------------------------------ | ||||||
|
|
||||||
| * Calling the ``Struct.__new__()`` without required argument now is | ||||||
| deprecated and will be removed in Python 3.20. Calling | ||||||
| :meth:`~object.__init__` method on initialized :class:`~struct.Struct` | ||||||
| objects is deprecated and will be removed in Python 3.20. | ||||||
|
|
||||||
| (Contributed by Sergey B Kirpichev in :gh:`143715`.) | ||||||
|
||||||
| (Contributed by Sergey B Kirpichev in :gh:`143715`.) | |
| (Contributed by Serhiy Storchaka in :gh:`143715`.) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1527,6 +1527,15 @@ New deprecations | |||||
|
|
||||||
| (Contributed by Bénédikt Tran in :gh:`134978`.) | ||||||
|
|
||||||
| * :mod:`struct`: | ||||||
|
|
||||||
| * Calling the ``Struct.__new__()`` without required argument now is | ||||||
| deprecated and will be removed in Python 3.20. Calling | ||||||
| :meth:`~object.__init__` method on initialized :class:`~struct.Struct` | ||||||
| objects is deprecated and will be removed in Python 3.20. | ||||||
|
|
||||||
| (Contributed by Sergey B Kirpichev in :gh:`143715`.) | ||||||
|
||||||
| (Contributed by Sergey B Kirpichev in :gh:`143715`.) | |
| (Contributed by Serhiy Storchaka in :gh:`143715`.) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -585,11 +585,16 @@ def test_Struct_reinitialization(self): | |||||
| # Struct instance. This test can be used to detect the leak | ||||||
| # when running with regrtest -L. | ||||||
| s = struct.Struct('i') | ||||||
| s.__init__('ii') | ||||||
| with self.assertWarns(DeprecationWarning): | ||||||
| s.__init__('ii') | ||||||
| self.assertEqual(s.format, 'ii') | ||||||
| packed = b'\x01\x00\x00\x00\x02\x00\x00\x00' | ||||||
| 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('2n3P1?') | ||||||
|
||||||
| totalsize = support.calcobjsize('2n3P1?') | |
| totalsize = support.calcobjsize('2n3P?') |
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.
It should be '2n3P?0P', with padding.
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
class MyStruct(struct.Struct):
def __init__(self, format):
super().__init__(format)
# some other initializationworks now and will work in future.
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.
I think this will be more clear, per Victor's suggestion:
| self.assertEqual(my_struct.pack(12345), b'\x30\x39') | |
| self.assertEqual(my_struct.format, '>h') |
(and in all cases below too)
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.
What if self->s_format and and self->s_codes are de-synchronized? The original test used Struct.pack().
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.
The original test used Struct.pack().
That's true. But I think that with format field tests will be more clear.
What if self->s_format and and self->s_codes are de-synchronized?
I see, you test some such cases with bad characters.
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.
I added assertions for format, but we need also functional tests, because internals can be initialized twice, and if something goes wrong, we will and with inconsistent format and pack(). See new cases in test_Struct_reinitialization. We should fix this in other issue.
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.
Ah, I entirely forgot that format is a positional or keyword argument in the master. I'll fix my patch.
But again, this should emit a warning.
Outdated
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.
You can factorize some tests using ony 1 positional argument:
for format in ('>h', '<h', 42, '$', '\u20ac'):
with self.subTest(format=format):
my_struct = MyStruct(format)
self.assertEqual(my_struct.format, '>h')
self.assertEqual(my_struct.pack(12345), b'\x30\x39')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.
Again, why no warnings here?
BTW, I doubt this usage pattern come from reality ;-)
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 __new__() and __init__() are called with the same argument, as expected. You are supposed to do this if you write a code that works in all versions.
As for other cases, having both custom __new__() and __init__() which call corresponding parent's methods with different arguments is unusual, but this need to be tested. I found bugs in my code when added these tests.
| 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. Patch by Sergey B Kirpichev. |
Uh oh!
There was an error while loading. Please reload this page.