GeezerVison

🤖 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.

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

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 ":hammer: 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.

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.