Skip to content

fix type confusion when different #[pyclass] types returned from #[new]#6062

Open
davidhewitt wants to merge 4 commits into
PyO3:mainfrom
davidhewitt:pyclass-init-safety
Open

fix type confusion when different #[pyclass] types returned from #[new]#6062
davidhewitt wants to merge 4 commits into
PyO3:mainfrom
davidhewitt:pyclass-init-safety

Conversation

@davidhewitt
Copy link
Copy Markdown
Member

This fixes a bug where a #[pyclass] struct B could be returned from the #[new] implementation for a separate #[pyclass] struct A (essentially instantly crashes).

The newly added test in test_class_init.rs is an example of what currently fails on main.

I moved the implementation of tp_new_impl to src/internal as it doesn't need to be publicly reachable, and used deref specialization to build PyClassInitializer<T> if possible inside #[new] implementation of T.

While I was at it, I also needed some new ok-wrapping machinery (to avoid needing my trait shenanigans to handle results) and also improved the diagnostics for incorrect return types (see new UI test).

cc @Icxolu

(Credit to Codex security scanning for the bug discovery; solution is my own design.)

@Icxolu
Copy link
Copy Markdown
Member

Icxolu commented May 20, 2026

Thanks, will take a look in the coming days.

Copy link
Copy Markdown
Member Author

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

No worries, I am not sure how much I will get done before Sunday at the earliest myself.

Comment on lines +74 to +77
/// This resolution step is necessary in order to encode the preference for `PyClassInitializer<T>`
/// to use proper machinery instead of `IntoPyObject`; if we removed the implementation of
/// `IntoPyObject` for `PyClassInitializer<T>` then this machinery could probably collapse away to
/// type inference.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is not true, I was confused when writing this due to conflicting implementations of an earlier draft of my changes PyClassInit. I think really what's going on is that it's legal for users to add implementations of IntoPyObject for PyClassInitializer<UserType>? Will need to think about this again.

Comment on lines -1492 to -1493
{ #pyo3_path::impl_::pyclass::IsPyClass::<#output>::VALUE },
{ #pyo3_path::impl_::pyclass::IsInitializerTuple::<#output>::VALUE }
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For the record, the original bug was that these specializations apply for all pyclasses, not just the #[pyclass] whose #[new] we are currently implementing, so we were running the specialized conversion for the wrong type.

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.

2 participants