Created a builder for creating types, allowing for more flexibility

[vultix]

This is the first in a series of pull requests as I implement issues #417 and #906. This seemed like a relatively easy piece to review separately.

Creating metaclasses and custom types for different enum variants required more flexibility than our current create_type_object_impl allowed for. I originally had lots of near-duplicates of the function, but found a builder like what I have here covers my use cases better.

Question regarding unsafe

I'm not entirely sure which methods should / shouldn't be considered unsafe to use. The approach I've taken here is to assume pushing a c pointer to a slot is always unsafe, but that functions doing so are considered safe if we've verified the slot type is correct. For example, is PyTypeBuilder::build always safe to call if the earlier safety invariants are met? I believe so, but am not positive.

[mejrs]

The approach I've taken here is to assume pushing a c pointer to a slot is always unsafe

In general, a function taking a raw pointer as an argument has to be unsafe, because it has no way to check that the pointer is correct.

[mejrs]

I'm really happy to have a builder for this. It has gotten quite messy so thanks for the cleanup :)

[davidhewitt]

Thanks very much for working on the enums and for kicking off with a much-needed refactor!

Using a builder is a great idea. For a bit of context why we had create_type_object and create_type_object_impl, the motivation was to keep the codgen cost of this code as low as possible by having minimal generic wrapper create_type_object around concrete create_type_object_impl. We had some severe compile-time regressions reported in the past by users in this code.

I think with a builder we could actually cut the codegen cost further. I wonder if we could have a trait like the following, shifting the builder creation to const eval and meaning we only need concrete runtime code to be generated:

trait HasPyClassBuilder {
    const BUILDER: PyClassBuilder;
}

I think it should be possible to give HasPyClassBuilder a blanket impl for T: PyClassImpl?

[vultix]

Alright, cleaned up that last clippy warning, and all other comments don't seem to be actionable. Let me know the next steps from here!

[vultix]

I think it should be possible to give HasPyClassBuilder a blanket impl for T: PyClassImpl?

The builder currently allocates a Vec to store all of the slots, so can't be const.

[davidhewitt]

The builder currently allocates a Vec to store all of the slots, so can't be const.

Ok fair, instead I pushed vultix#1 which captures all of the arguments we feed to the builder as a constant in the same fashion.

I checked cargo llvm-lines and create_type_object goes from 44 to 175 lines per pyclass with this PR as currently implemented. If you accept my patch on top then the cost per pyclass is zero and my concern is resolved :)

[davidhewitt]

As discussed in vultix#1 we've shown that there is a compile performance regression, but it doesn't appear to be so significant that it should be a blocker for merging.

So I think #2553 (comment) is the last tweak proposed here, and then let's merge this.

[vultix]

I'm not sure why CI is failing now - is this on my end?

[mejrs]

No, it isnt anything you did - Bumpalo bumped its msrv, so we need to use an older version.

[davidhewitt]

I'll rebase this on main; that should then go through. Thanks!

[davidhewitt]

EDIT: looks like you didn't check the box to give me permission to push to your branch - you'll need to do the rebase :)

[davidhewitt]

Actually I just pushed the rebase to my own fork, opened as #2565; will close this one. Thanks!