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

bschoenmaeckers · 2026-07-21T15:52:30Z

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.

coderabbitai · 2026-07-21T15:52:51Z

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

coderabbitai

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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!

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

bschoenmaeckers commented Jul 21, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading