Fix defstruct with bases using a StructMeta subclass by abhijeet117 · Pull Request #1164 · msgspec/msgspec · GitHub
Skip to content

Fix defstruct with bases using a StructMeta subclass - #1164

Open
abhijeet117 wants to merge 3 commits into
msgspec:mainfrom
abhijeet117:fix/defstruct-custom-metaclass
Open

Fix defstruct with bases using a StructMeta subclass#1164
abhijeet117 wants to merge 3 commits into
msgspec:mainfrom
abhijeet117:fix/defstruct-custom-metaclass

Conversation

@abhijeet117

Copy link
Copy Markdown

Fixes #974

Summary

defstruct(bases=...) raised TypeError: Struct types cannot define __slots__ when a base used a custom StructMeta subclass, while equivalent class syntax worked. defstruct always built the class through StructMeta, so type creation re-dispatched through the base's more derived metaclass and re-entered construction with __slots__ already injected into the namespace. We now resolve the most derived metaclass among the bases (mirroring CPython's calculation) and construct through it, matching class definition behavior.

Testing

Added test_defstruct_custom_metaclass_base (constructs, instantiates, JSON round-trips) plus test_defstruct_default_metaclass. Full unit suite: 6380 passed, 145 skipped (CPython 3.13, Windows). ruff check, ruff format --diff, and codespell clean on changed files.

Checklist

  • Bug reproduced before fix
  • Root cause identified
  • Bug fixed
  • Tests passed

defstruct always created the new class through msgspec.StructMeta, ignoring the metaclasses of the provided bases. When a base used a custom StructMeta subclass, type creation re-dispatched through that more derived metaclass, running class construction a second time on a namespace msgspec had already injected __slots__ into, and failing with TypeError: Struct types cannot define __slots__.

Resolve the most derived metaclass among the bases before creating the class, mirroring CPython's metaclass calculation, so defstruct constructs classes under the same metaclass as equivalent class definitions.

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The selected metaclass is assigned to the generated type, but its construction hooks are never invoked. StructMeta_new_inner calls PyType_Type.tp_new(metatype, ...) directly, bypassing the selected metaclass's __new__ and __init__. That breaks the documented StructMeta subclass pattern for supplying default struct configuration.

At exact head b11a57d52c3329fa460a1a1d60e7059106796119, using the documented pattern with a metaclass that does config.setdefault("kw_only", True) produces these divergent results:

defstruct: calls=[]; signature=(child: int, *, base: int); Dynamic(2, base=1) succeeds
class syntax: calls=[("new", "Static"), ("init", "Static")]; signature=(*, base: int, child: int); Static(2, base=1) raises TypeError

The added custom-metaclass test only delegates from __new__ and checks the resulting metaclass, so it cannot detect that the hook was skipped. Please preserve the selected metaclass's hooks exactly once (including their configuration/validation behavior) and add a regression covering a hook with an observable effect.

Verification: exact base f51f378335b01dc0026dc6553a0b9e1915a8edae reproduces the reported Struct types cannot define __slots__ failure; the two added tests pass on the head; the complete unit suite passes (6382 passed, 143 skipped). The hook/configuration probe above remains red on the head.

Reviewed with OpenAI Codex assistance

PyType_Type.tp_new only runs the metaclass __new__ hook, skipping __init__.
This meant metaclass config changes from __init__ were never applied for
defstruct-created classes. Call type->tp_init after tp_new so both hooks
run exactly once, matching class syntax behavior.

Add regression test verifying __new__ and __init__ are each called once
and that a kw_only=True config change from the metaclass hook is honored.
@abhijeet117

Copy link
Copy Markdown
Author

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The follow-up still does not preserve the selected metaclass's construction semantics, and the direct tp_init call introduces a regression for ordinary class syntax. At exact head 995ca0b6ffc0ae4cdfeff1de0babce5b3347bc72, PyType_Type.tp_new(type, ...) still bypasses CustomMeta.__new__; type->tp_init is then invoked inside StructMeta_new_inner, before the struct fields/configuration are installed.

For defstruct I observe only [("init", "Dynamic")], with __struct_fields__ still missing during that hook, signature (child: int, *, base: int), and a positional child accepted. Equivalent class syntax observes [("new", "Static"), ("init", "Static"), ("init", "Static")], signature (*, base: int, child: int), and rejects the positional child. A metaclass whose __new__ raises for the generated name is likewise bypassed by defstruct but honored by class syntax.

The added regression test is also red as written because it retains the hooks recorded while creating Base; the complete local unit run is 1 failed, 6382 passed, 143 skipped on CPython 3.13.5. Clearing those setup calls would still expose the missing __new__ and unapplied kw_only policy. The public Build and Test, docs-pr, and CodSpeed workflows for this head are currently action_required with zero jobs.

There is also a deterministic failure-path reference leak in the new code. Py_CLEAR(args) was moved below both the tp_new and tp_init failure branches, while cleanup: never decrefs args. Repeating 25 custom-__init__ validation failures increases the refcount of a namespace sentinel by 25 on this head; exact base increases it by 0.

Please dispatch construction through the selected metaclass on the original namespace/configuration (or otherwise preserve the same lifecycle), so __new__ and __init__ each run exactly once at the class-syntax-equivalent point, validation/config hooks are honored, and all failure-owned references are released. The compatible-MRO and metaclass-conflict behavior did match class syntax in my probes.

Reviewed with OpenAI Codex assistance.

Dispatching through the metaclass runs its __new__ and __init__ hooks once
each, at the same point they run for an equivalent class definition, so a
metaclass that supplies or validates struct configuration now behaves the
same for defstruct. This replaces the direct tp_init call, which ran
__init__ before the struct fields were installed and skipped __new__
entirely.
@abhijeet117

Copy link
Copy Markdown
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

msgspec.defstruct is not compatible with msgspec.StructMeta

2 participants