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

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

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

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.

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.

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.

@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?

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.

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

Thanks for making the requested changes!

@mdickinson, @ambv: please review the changes made to this pull request.

ping @mdickinson @ambv

@kumaraditya303 Sorry for taking so long to get back to this.

So the current code in this PR ends up duplicating some of the work: we end up calling prepare_s twice every time a struct.Struct instance is created, for example.

Suggestion:

1. Drop all content from the __init__ method: the body of the method can simply be return 0.
2. Apply this change only for 3.12 and later.

The reason for (2) here is that (1) is technically a breaking change, in that doing something like s = struct.Struct('<f') followed by s.__init__('>f') would not have the same effect as before. But I think that change would still be okay for 3.12: unlike the subclassing that we were worried about above, deliberately calling s.__init__ with a new format just doesn't seem like a plausible use-case.

Alternatively, if we really want to try to stay perfectly backwards compatible (modulo the bugs being fixed, of course), we'd need to find a way re-organise the logic so that we don't end up calling prepare_s twice in normal use.

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

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

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

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

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.