Skip to content

Conversation

@Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Oct 4, 2025

This proposes a first step in removing the FromPyObject blanket implementation for cloneable pyclasses.

This adds a marker trait ExtractPyClassWithClone which is automatically implemented by the #[pyclass] macro for every pyclass and modifies the blanket to require the marker. I think this should be functionally equivalent. It also adds a new skip_from_py_object option to opt out of emitting the marker trait and thus enable a custom FromPyObject implementation.

The following steps could then be:

  • add an option to explicitly opt-in (this can then just emit the FromPyObject impl directly)
  • emit a warning for pyclasses that do not declare out-out nor opt in
  • switch the default from opt-out to opt-in
  • remove the blanket and the marker and deprecate the out-out option

Ref #5419

@Icxolu Icxolu force-pushed the move-away-from-py-object-blanket branch 4 times, most recently from 8cc541d to 074b324 Compare October 5, 2025 16:42
@Icxolu Icxolu marked this pull request as ready for review October 5, 2025 17:07
@davidhewitt
Copy link
Member

This approach looks good to me, thanks. Documentation with the rest of the #[pyclass] options needs updating, and somewhere in the guide probably too, maybe in the "Conversion Traits" section?

@Icxolu Icxolu force-pushed the move-away-from-py-object-blanket branch 2 times, most recently from 0476533 to f5c3be0 Compare October 5, 2025 20:59
@Icxolu
Copy link
Contributor Author

Icxolu commented Oct 5, 2025

Thanks for the feedback! I've added the newsfragment, extended the pyclass option table and added a paragraph in the guide. Should we also mention this in the migration guide already, are do we wait until we emit a warning?

@Icxolu Icxolu force-pushed the move-away-from-py-object-blanket branch from 73830a8 to 6b9ad95 Compare October 6, 2025 16:05
@Icxolu Icxolu force-pushed the move-away-from-py-object-blanket branch from 6b9ad95 to 568f4ee Compare October 6, 2025 16:13
Copy link
Member

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

Perfect, thanks!

I think no need for this to go in the migration guide until we begin emitting the warning. Users don't need to do anything with this to upgrade to 0.27.

Co-authored-by: David Hewitt <mail@davidhewitt.dev>
@Icxolu Icxolu enabled auto-merge October 7, 2025 21:26
@Icxolu Icxolu added this pull request to the merge queue Oct 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 7, 2025
@davidhewitt davidhewitt added this pull request to the merge queue Oct 8, 2025
@Icxolu Icxolu mentioned this pull request Oct 8, 2025
Merged via the queue into PyO3:main with commit cfe0a25 Oct 9, 2025
42 of 44 checks passed
@Icxolu Icxolu deleted the move-away-from-py-object-blanket branch October 9, 2025 15:53
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