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

struct.Struct inheritance with Python 3.12.0 #112358

Closed
livrrr opened this issue Nov 24, 2023 · 7 comments
Closed

struct.Struct inheritance with Python 3.12.0 #112358

livrrr opened this issue Nov 24, 2023 · 7 comments
Assignees
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@livrrr
Copy link

livrrr commented Nov 24, 2023

Bug report

Bug description:

import struct

class MyStruct(struct.Struct):
    def __init__(self):
        super().__init__('>h')

obj = MyStruct()

When I run this code I receive an error:

Traceback (most recent call last):
  File "/home/user/bug.py", line 7, in <module>
    obj = MyStruct()
          ^^^^^^^^^^
TypeError: Struct() missing required argument 'format' (pos 1)

It is rather strange error, I have passed format parameter to base class constructor, I receive this error with any value of format parameter.
There are no any problems with this code in Python 3.11 and older.

CPython versions tested on:

3.12

Operating systems tested on:

Linux

Linked PRs

@livrrr livrrr added the type-bug An unexpected behavior, bug, or error label Nov 24, 2023
@Eclips4
Copy link
Member

Eclips4 commented Nov 24, 2023

bisected to c8c0afc
cc @kumaraditya303

@Eclips4 Eclips4 added the extension-modules C modules in the Modules dir label Nov 24, 2023
@AlexWaygood AlexWaygood added 3.12 bugs and security fixes 3.13 bugs and security fixes labels Nov 24, 2023
@AlexWaygood
Copy link
Member

Looks like another user also hit this problem, but reported it on the PR rather than opening an issue: #94532 (comment)

@mdickinson
Copy link
Member

Hmm, this isn't nice. I'm not seeing any good options here other than simply reverting the offending commit (and re-opening the issues that PR #94532 was supposed to close). @kumaraditya303?

@mdickinson
Copy link
Member

Hmm, this isn't nice. [...]

To elaborate: the existence of struct.Struct subclasses that extend the __init__ method means that Struct.__new__ can make essentially no assumptions about the arguments it gets. It might get no arguments; it might get just the format, it might get the format plus additional things, or it might get a collection of arguments that doesn't include the format at all. Previously all those things worked because Struct.__new__ was simply object.__new__, which accepted any combination of args and kwargs.

This makes the solution in #94532 of initialising the struct in Struct.__new__ non-viable - at least, not without backwards compatibility breakage, which would have to be managed in the usual way (deprecation period, etc.).

I've opened #112424 for reversion. Once merged, it'll need to be backported to 3.12, and issues #75960 and #78724 will need to be re-opened.

mdickinson added a commit that referenced this issue Nov 26, 2023
…#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 issue Nov 26, 2023
…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 issue Nov 27, 2023
…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
@mdickinson
Copy link
Member

Fixed in #112424 (main), #112426 (3.12).

@serhiy-storchaka
Copy link
Member

It is more convenient to initialize the Struct instance in __new__ than in __init__, and it makes sense, since Struct instances are cached and therefore can be considered immutable like ints or tuples. But the possibility of creating subclasses and the existence of subclasses in the wild makes this a breaking change.

Can this breakage be considered a lesser evil?

@mdickinson
Copy link
Member

@serhiy-storchaka: Maybe. I'd failed to recognise the scope of the breakage introduced by #94532, and I think reverting in the short term is the right solution. Longer term, if we can find a properly deprecated path to moving the initialisation to __new__, I agree that that would be worth considering.

For the particular issues that #94532 was addressing, I think there are likely less disruptive fixes available.

aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
…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
3.12 bugs and security fixes 3.13 bugs and security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

6 participants