Allow c functions in tp_new type slot by bschoenmaeckers · Pull Request #8346 · RustPython/RustPython · GitHub

This is my first draft of allowing C functions in type slots. I started with the NewFunc slot and would love some feedback on my approach before converting the other slots.

@youknowone Could you please share your opinion.

Summary by CodeRabbit

Bug Fixes
- Improved __new__ constructor dispatch across built-in types (bool, int, float, str, tuple, set) by invoking the resolved constructor target more reliably.
- Refined the “same/equivalent constructor” detection used for __init fast-path specialization, improving when the optimized path is selected.
- Improved handling of Rust-backed __new__ wrappers and inherited constructor behavior for classes/types.
Compatibility
- Standardized constructor wiring to support consistent Rust-backed behavior and lay groundwork for additional native __new__ variants.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info ⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 7fd75980-450b-49a4-8956-751405d32969

📥 Commits

Reviewing files that changed from the base of the PR and between 69d97b0 and f884722.

📒 Files selected for processing (11)

crates/derive-impl/src/pyclass.rs
crates/vm/src/builtins/bool.rs
crates/vm/src/builtins/float.rs
crates/vm/src/builtins/int.rs
crates/vm/src/builtins/set.rs
crates/vm/src/builtins/str.rs
crates/vm/src/builtins/tuple.rs
crates/vm/src/builtins/type.rs
crates/vm/src/class.rs
crates/vm/src/frame.rs
crates/vm/src/types/slot.rs

🚧 Files skipped from review as they are similar to previous changes (11)

crates/vm/src/builtins/int.rs
crates/vm/src/builtins/str.rs
crates/vm/src/builtins/bool.rs
crates/derive-impl/src/pyclass.rs
crates/vm/src/builtins/set.rs
crates/vm/src/builtins/tuple.rs
crates/vm/src/builtins/float.rs
crates/vm/src/frame.rs
crates/vm/src/class.rs
crates/vm/src/types/slot.rs
crates/vm/src/builtins/type.rs

📝 Walkthrough

Walkthrough

NewFunc changes from a function-pointer alias to an enum with identity and invocation methods. Generated constructors store typed Rust variants, while built-in and type dispatch paths use .invoke() and identity-based comparisons.

Changes

New slot dispatch

Layer / File(s)	Summary
NewFunc contract and producers crates/vm/src/types/slot.rs, crates/derive-impl/src/pyclass.rs	NewFunc supports Rust and C variants with identity and invocation methods; generated constructors store typed Rust variants.
Slot storage and identity checks crates/vm/src/types/slot.rs, crates/vm/src/class.rs, crates/vm/src/frame.rs, crates/vm/src/builtins/type.rs	Slot wrapper, inheritance, specialization, and safety checks compare __new__ implementations through identity().
Constructor invocation paths crates/vm/src/builtins/{bool,float,int,set,str,tuple,type}.rs	Built-in vectorcall and type dispatch paths invoke loaded __new__ slots through .invoke(...).

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant BuiltinVectorcall
  participant NewFunc
  participant TypeDispatch
  BuiltinVectorcall->>NewFunc: Load slots.new and invoke(cls, args, vm)
  TypeDispatch->>NewFunc: Invoke resolved __new__ slot
  NewFunc-->>BuiltinVectorcall: Return PyResult

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: adding C-function support to the tp_new slot.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/vm/src/types/slot.rs (1)
316-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Derive PartialEq, Eq to simplify identity checks.

Function pointers natively implement PartialEq and Eq. Deriving these traits on NewFunc allows direct equality comparisons (== and !=) on the variants, entirely eliminating the need for the manual identity() method and the verbose mapping at all call sites.

crates/vm/src/types/slot.rs#L316-L329: Add #[derive(PartialEq, Eq)] to NewFunc and remove the identity method entirely.

crates/vm/src/types/slot.rs#L909-L911: Replace the comparison with cls.slots.new.load() == Some(NewFunc::Rust(new_wrapper as _)).

crates/vm/src/builtins/type.rs#L2835-L2835: Replace the condition with slot_new != crate::types::NewFunc::Rust(crate::types::new_wrapper as _).

crates/vm/src/builtins/type.rs#L3072-L3072: Replace the check with if typ_new != staticbase_new {.

crates/vm/src/class.rs#L207-L208: Replace the inheritance check with !is_object_itself && object_new == Some(slot_new).

crates/vm/src/frame.rs#L9243-L9243: Replace the identity comparison with cls_new_fn == obj_new_fn.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/vm/src/types/slot.rs` around lines 316 - 329, Derive PartialEq and Eq
for NewFunc and remove its identity method, then replace all identity-based
comparisons with direct NewFunc equality checks: update
crates/vm/src/types/slot.rs lines 316-329 and 909-911,
crates/vm/src/builtins/type.rs lines 2835 and 3072, crates/vm/src/class.rs lines
207-208, and crates/vm/src/frame.rs line 9243 as specified, preserving the
existing comparison semantics.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/vm/src/types/slot.rs`:
- Around line 316-320: Preserve lock-free access for PyTypeSlots::new by
avoiding the two-variant NewFunc representation in AtomicCell<Option<NewFunc>>.
Update NewFunc and the associated new-slot storage/access paths to use a
pointer-sized representation compatible with the target atomic width, while
retaining support for both Rust and C constructors.

---

Nitpick comments:
In `@crates/vm/src/types/slot.rs`:
- Around line 316-329: Derive PartialEq and Eq for NewFunc and remove its
identity method, then replace all identity-based comparisons with direct NewFunc
equality checks: update crates/vm/src/types/slot.rs lines 316-329 and 909-911,
crates/vm/src/builtins/type.rs lines 2835 and 3072, crates/vm/src/class.rs lines
207-208, and crates/vm/src/frame.rs line 9243 as specified, preserving the
existing comparison semantics.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info ⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: b97f0fc2-eccf-40ed-878e-0572a321a7a8

📥 Commits

Reviewing files that changed from the base of the PR and between cc30cd5 and 00b1ca8.

📒 Files selected for processing (11)

crates/derive-impl/src/pyclass.rs
crates/vm/src/builtins/bool.rs
crates/vm/src/builtins/float.rs
crates/vm/src/builtins/int.rs
crates/vm/src/builtins/set.rs
crates/vm/src/builtins/str.rs
crates/vm/src/builtins/tuple.rs
crates/vm/src/builtins/type.rs
crates/vm/src/class.rs
crates/vm/src/frame.rs
crates/vm/src/types/slot.rs

I'd like not to introduce an enum for this.
let me try do this by adding a bridge in capi side.

We might need a Box<dyn Fn> for this. As in my testing I could not store ar c fn in a fn slot.

If you need a testbed, please try to fix this TODO here: https://github.com/bschoenmaeckers/RustPython/blob/880999db85bd14f497e48e07e74b3745dd484994/crates/capi/src/object/pytype.rs#L194-L198

this is more complicated than my first expectation. i am trying another way

could you check if this way work? #8435

bschoenmaeckers force-pushed the c-api-type-slots branch from 00b1ca8 to 69d97b0 Compare July 21, 2026 15:56

coderabbitai Bot reviewed Jul 21, 2026

View reviewed changes

Comment thread crates/vm/src/types/slot.rs

Allow c functions in tp_new type slot

f884722

bschoenmaeckers force-pushed the c-api-type-slots branch from 69d97b0 to f884722 Compare July 22, 2026 11:49

youknowone reviewed Jul 23, 2026

View reviewed changes

Allow c functions in tp_new type slot by bschoenmaeckers · Pull Request #8346 · RustPython/RustPython · GitHub

Uh oh!

Allow c functions in tp_new type slot - #8346

Allow c functions in tp_new type slot#8346
bschoenmaeckers wants to merge 1 commit into
RustPython:mainfrom
bschoenmaeckers:c-api-type-slots

bschoenmaeckers commented Jul 21, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 21, 2026 •
edited
Loading

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

youknowone Jul 23, 2026

Uh oh!

bschoenmaeckers Jul 23, 2026 •
edited
Loading

Uh oh!

Uh oh!

bschoenmaeckers Jul 23, 2026

Uh oh!

youknowone Jul 29, 2026

Uh oh!

youknowone Aug 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		Rust(fn(PyTypeRef, FuncArgs, &VirtualMachine) -> PyResult),
		C(unsafe extern "C" fn(mut PyObject, mut PyObject, mut PyObject) -> mut PyObject),

Allow c functions in tp_new type slot by bschoenmaeckers · Pull Request #8346 · RustPython/RustPython · GitHub

Uh oh!

Allow c functions in tp_new type slot - #8346

Allow c functions in tp_new type slot#8346bschoenmaeckers wants to merge 1 commit intoRustPython:mainRustPython/RustPython:mainfrom bschoenmaeckers:c-api-type-slotsbschoenmaeckers/RustPython:c-api-type-slotsCopy head branch name to clipboard

Conversation

bschoenmaeckers commented Jul 21, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

youknowone Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

bschoenmaeckers Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bschoenmaeckers Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

youknowone Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

youknowone Aug 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Allow c functions in tp_new type slot#8346
bschoenmaeckers wants to merge 1 commit into
RustPython:mainfrom
bschoenmaeckers:c-api-type-slots

bschoenmaeckers commented Jul 21, 2026 •
edited by coderabbitai Bot
Loading

coderabbitai Bot commented Jul 21, 2026 •
edited
Loading

bschoenmaeckers Jul 23, 2026 •
edited
Loading