Skip to content
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

GH-78724: Initialize struct.Struct in __new__ #94532

Merged
merged 8 commits into from
Sep 25, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jul 3, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@kumaraditya303 kumaraditya303 added needs backport to 3.10 only security fixes extension-modules C modules in the Modules dir needs backport to 3.11 only security fixes labels Jul 3, 2022
@kumaraditya303 kumaraditya303 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 3, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit a74cdbb 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 3, 2022
@kumaraditya303 kumaraditya303 requested a review from mdickinson July 3, 2022 16:52
Copy link
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm wondering if moving initialization from __init__ to __new__ can cause some user-facing backwards compatibility, say, wrt subclassing or something. I asked for more eyes on this.

@mdickinson
Copy link
Member

Apologies for the silence; I'll get to this by the end of this weekend.

@mdickinson
Copy link
Member

LGTM too.

To be on the safe side, I'd recommend not backporting this to 3.10, though it seems fine to backport to 3.11 if @pablogsal agrees. The bugs being fixed seem to be of the self-inflicted "well don't do that then" variety, so while it's definitely nice to have them fixed, it seems unlikely that they'll be blocking any real users of Struct. And there's just enough possibility of unintended side-effects that it doesn't seem worth changing this in an already-released version of Python.

@mdickinson
Copy link
Member

mdickinson commented Jul 5, 2022

LGTM. I'm wondering if moving initialization from __init__ to __new__ can cause some user-facing backwards compatibility, say, wrt subclassing or something.

Yes, it can. This is fine, on Python 3.10:

>>> import struct
>>> class Bob(struct.Struct):
...     def __init__(self, format):
...         super().__init__(format)
... 
>>> b = Bob("!f")
>>> b.pack(2.3)
b'@\x1333'

But it breaks on this branch:

>>> import struct
>>> class Bob(struct.Struct):
...     def __init__(self, format):
...         super().__init__(format)
... 
>>> b = Bob("!f")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in __init__
TypeError: object.__init__() takes exactly one argument (the instance to initialize)

I'm finding it hard to imagine why anyone would be subclassing Struct like this, so it doesn't seem like a real issue if the behaviour changes in 3.11 or 3.12 (and in particular this certainly doesn't seem worth a deprecation period), but it does seem like sufficient reason to be careful in 3.10.

@pablogsal
Copy link
Member

To be on the safe side, I'd recommend not backporting this to 3.10, though it seems fine to backport to 3.11 if @pablogsal agrees.

I think is ok to backport to 3.11 👍 Not sure if it makes sense to mention the subclassing incompatibility in the what's new, though.

@mdickinson
Copy link
Member

@kumaraditya303 Apologies, Kumar; I'm having second thoughts here. While common sense says that no-one will be subclassing struct.Struct this way, experience says that someone somewhere will be, and it would be at least a bit irresponsible to change the behaviour.

Can you think of any ways that we could fix these bugs without the backward compatibility breakage?

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On balance, the backwards compatibility change seems like something we should avoid if possible.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Jul 6, 2022

Thanks for the reviews!

Can you think of any ways that we could fix these bugs without the backward compatibility breakage?

I have made it backwards compatible by adding __init__ and added a test for subclassing.

I have made the requested changes; please review again @mdickinson @ambv

@kumaraditya303
Copy link
Contributor Author

Thanks for the review, I'll make the changes by next weekend.

@kumaraditya303 kumaraditya303 removed needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Sep 17, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…78724

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@kumaraditya303
Copy link
Contributor Author

cc @mdickinson I have made the changes, I removed the code duplication and it is now only for 3.12+.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'll do a buildbot run, to be on the safe side.

@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 25, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit d7ce7d8 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 25, 2022
@mdickinson
Copy link
Member

The buildbots look happy. There are still three checks pending as of writing, but enough checks have passed to satisfy me. I'll merge now, but continue to watch those remaining three bots.

@dopplershift
Copy link

dopplershift commented Oct 12, 2023

So I just hit this while trying to upgrade to 3.12. While subclassing works, subclasses MUST now match the same signature for __init__:

from struct import Struct

class FloatStruct(Struct):
    def __init__(self):
        super().__init__('f')

f = FloatStruct()

gives

Traceback (most recent call last):
  File "/Users/rmay/test.py", line 7, in <module>
    f = FloatStruct()
        ^^^^^^^^^^^^^
TypeError: Struct() missing required argument 'format' (pos 1)

My actual use case was passing additional arguments, but either way fails.

I'm not here to necessarily vouch for why our implementation uses that, but it was seriously confusing not understanding why __init__ arguments were being checked before getting to my own __init__ implementation.

mdickinson added a commit to mdickinson/cpython that referenced this pull request Nov 26, 2023

Verified

This commit was signed with the committer’s verified signature.
mdickinson Mark Dickinson
…-94532)"

This reverts commit c8c0afc.
mdickinson added a commit that referenced this pull request Nov 26, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…#112424)

Revert commit c8c0afc (PR #94532),
which moved `struct.Struct` initialisation from `Struct.__init__` to `Struct.__new__`.
This caused issues with code in the wild that subclasses `struct.Struct`.
mdickinson added a commit to mdickinson/cpython that referenced this pull request Nov 26, 2023

Verified

This commit was signed with the committer’s verified signature.
mdickinson Mark Dickinson
…truct.Struct. (pythonGH-112424)

Revert commit c8c0afc (PR pythonGH-94532),
which moved `struct.Struct` initialisation from `Struct.__init__` to `Struct.__new__`.
This caused issues with code in the wild that subclasses `struct.Struct`..
(cherry picked from commit 9fe6034)

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
mdickinson added a commit that referenced this pull request Nov 27, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…Struct (GH-112424) (#112426)

* [3.12] gh-112358: Fix Python 3.12 regression with subclassing struct.Struct. (GH-112424)

Revert commit c8c0afc (PR GH-94532),
which moved `struct.Struct` initialisation from `Struct.__init__` to `Struct.__new__`.
This caused issues with code in the wild that subclasses `struct.Struct`..
(cherry picked from commit 9fe6034)

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

* Remove unrelated test
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…truct. (python#112424)

Revert commit c8c0afc (PR python#94532),
which moved `struct.Struct` initialisation from `Struct.__init__` to `Struct.__new__`.
This caused issues with code in the wild that subclasses `struct.Struct`.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…truct. (python#112424)

Revert commit c8c0afc (PR python#94532),
which moved `struct.Struct` initialisation from `Struct.__init__` to `Struct.__new__`.
This caused issues with code in the wild that subclasses `struct.Struct`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir
Projects
None yet
6 participants