Skip to content

add Sealed to pyclass traits#5845

Open
luxedo wants to merge 1 commit intoPyO3:mainfrom
luxedo:3908/seal-pyclass
Open

add Sealed to pyclass traits#5845
luxedo wants to merge 1 commit intoPyO3:mainfrom
luxedo:3908/seal-pyclass

Conversation

@luxedo
Copy link
Copy Markdown
Contributor

@luxedo luxedo commented Feb 28, 2026

Part of #3908

Adds Sealed to traits in impl_/pyclass.rs

@luxedo luxedo mentioned this pull request Feb 28, 2026
Open
@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Mar 25, 2026
davidhewitt
davidhewitt reviewed Mar 25, 2026
View reviewed changes
Copy link
Copy Markdown
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.

Thanks, a couple of these seals are probably not needed, the rest look good to me!

src/impl_/pyclass.rs Outdated
Comment on lines +1138 to +1144
mod pyclass_base_type {
use crate::impl_::pyclass::PyClassBaseType;

pub trait Sealed {}

impl<T: PyClassBaseType> Sealed for T {}
}
Copy link
Copy Markdown
Member

@davidhewitt davidhewitt Mar 25, 2026

Choose a reason for hiding this comment

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

This seal doesn't achieve anything it seals PyClassBaseType so creates circular logic.

I think it's ok to leave this trait un-sealed, any user-defined type could in principle implement it (maybe let's document it as such).

src/impl_/pyclass.rs Outdated
/// Users are discouraged from implementing this trait manually; it is a PyO3 implementation detail
/// and may be changed at any time.
pub trait PyClassImpl: Sized + 'static {
pub trait PyClassImpl: Sized + 'static + generic_pyclass::Sealed {
Copy link
Copy Markdown
Member

@davidhewitt davidhewitt Mar 25, 2026

Choose a reason for hiding this comment

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

It is expected for user-defined types to implement this trait (via the macros), so there's no point sealing it (it's not a closed set of implementations).

@luxedo luxedo force-pushed the 3908/seal-pyclass branch from 40de57d to a2d5930 Compare April 1, 2026 01:48
@davidhewitt
Copy link
Copy Markdown
Member

davidhewitt commented Apr 14, 2026

Thanks, I think if you merge main CI will be green.

@luxedo luxedo force-pushed the 3908/seal-pyclass branch from a2d5930 to 4cba28d Compare April 14, 2026 15:48
@luxedo luxedo force-pushed the 3908/seal-pyclass branch from 4cba28d to a99c305 Compare April 15, 2026 00:10
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 15, 2026

Merging this PR will degrade performance by 10.73%

❌ 1 regressed benchmark
✅ 104 untouched benchmarks
⏩ 1 skipped benchmark1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ bench_pyclass_create 3.9 µs 4.4 µs -10.73%

Comparing luxedo:3908/seal-pyclass (a99c305) with main (962a535)

Open in CodSpeed

Footnotes

  1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩

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

Reviewers

@davidhewitt davidhewitt davidhewitt left review comments

Assignees

No one assigned

Labels

CI-skip-changelog Skip checking changelog entry

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@luxedo @davidhewitt