Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

Walkthrough

The change adds native layout identifiers and payload support checks. Generated and built-in types use them to describe compatible layouts. Object downcasts, upcasts, and type assignment compatibility now validate native layout information.

Changes

Native Payload Layout Validation

Priority: ⬆️ High

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant PyObject
  participant PyObjVTable
  participant PyPayload
  PyObject->>PyObjVTable: Query native layout support
  PyObjVTable->>PyPayload: Call supports_native_layout(layout)
  PyPayload-->>PyObjVTable: Return layout support
  PyObjVTable-->>PyObject: Return layout support
  PyObject->>PyObject: Check Python class and payload compatibility

Suggested reviewers: youknowone, moreal

Merge Risk: 🟡 Moderate · up to dc78d

Dropping a sufficiently deep frame chain can overflow the native stack. Restore recursion protection for untracked objects before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 43e00

The change strengthens native casts by validating the allocated payload rather than trusting mutable Python class metadata. No newly enabled attack path was established. Residual risk remains because the contract governs interpreter memory safety, and downstream native implementations and concurrent type mutations are not fully verified.

Retained concerns
No architecture-level concerns identified.

Security Blast Radius

• inferred — This is an interpreter-wide memory-safety contract shared by native payload conversions and generated classes. A false physical-layout guarantee could affect the embedding process's address space, rather than only the semantics of one Python object; such a violation was not established in the inspected implementations.

Trust Boundaries and Controls

• observed — Python class and base assignment remain compatibility-controlled operations. The new check requires equal native layout identities in addition to structural compatibility. Native cast authorization separately queries allocation metadata, preventing mutable Python ancestry alone from authorizing an incompatible cast.
• observed — Native Rust implementations supply the layout declarations consumed by runtime checks. PySubclass explicitly places physical-prefix obligations on unsafe implementors; generated transparent subclasses and struct sequences acknowledge that obligation in their unsafe implementations.

Resilience and Maintainability Implications

• observed — Owned general upcasts validate before transferring the pointer. Direct base conversion relies on the unsafe prefix guarantee and forgets the original owner after transfer, preserving one ownership obligation for the same allocation.

Hardening Proposals

• proposed — Document the offset, alignment, validity, and lifetime obligations of manual layout-support declarations alongside the unsafe subclass contract, so future native implementations preserve the guarantees relied upon by safe casts.

❌ Failed checks (1 warning)

• Fix all pre-merge checks with AI

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

• X
• Mastodon
• Reddit
• LinkedIn

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

Merging this PR will improve performance by 15.18%

⚡ 1 improved benchmark
✅ 61 untouched benchmarks
⏩ 4 skipped benchmarks1

Performance Changes

Comparing 1ndahous3:native_payload_layout (dc78dae) with main (54e47cd)

Footnotes

1. 4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@coderabbitai Both findings in your review are addressed in 43e006a9c:

• PySubclass is now an unsafe trait. Its contract requires a valid base prefix, matching payload offsets, compatible alignment, and validity independent of Python class or MRO. Both generators and all manual implementations now use unsafe impl; these guarantees belong to the implementor, while the conversion methods remain safe.
• upcast and upcast_ref now validate the allocation's payload root and native layout instead of requiring Python MRO membership. The owning conversion retains the original allocation pointer, and incompatible layouts are still rejected. Ordinary downcasts retain their Python-class checks.

🏁 Script executed:

#!/bin/bash
set -euo pipefail
gh api repos/RustPython/RustPython/pulls/8963/reviews/5407198764
gh api repos/RustPython/RustPython/pulls/8963/comments --paginate --jq '.[] | select(.pull_request_review_id == 5407198764) | {path, line, body}'
git show --stat 43e006a9c9c9c4aa86b17c3c1fb3cfaec4cc2e9d
ast-grep outline crates/vm/src/class.rs --match 'PySubclass' --view expanded
ast-grep outline crates/vm/src/object/core.rs --match 'upcast|into_base' --view expanded
git show 43e006a9c9c9c4aa86b17c3c1fb3cfaec4cc2e9d -- crates/vm/src/class.rs crates/vm/src/object/core.rs

Length of output: 15882

🏁 Script executed:

#!/bin/bash
set -euo pipefail
git show 43e006a9c9c9c4aa86b17c3c1fb3cfaec4cc2e9d -- crates/derive-impl/src/pyclass.rs crates/derive-impl/src/pystructseq.rs crates/vm/src/exceptions.rs crates/vm/src/exception_group.rs
rg -n -C2 'impl.*PySubclass' --glob '*.rs' .
sed -n '2855,2925p' crates/vm/src/object/core.rs

Length of output: 14018

@1ndahous3 I checked 43e006a9c. Both findings are addressed.

• PySubclass now places the physical-layout guarantees on unsafe implementors. Both generators and all manual implementations use unsafe impl. The conversion methods remain safe.
• upcast and upcast_ref now validate PAYLOAD_TYPE_ID and NATIVE_LAYOUT_ID without requiring Python MRO membership. The owning conversion preserves the allocation pointer.

The new regression test covers custom MROs and rejection of incompatible layouts. I inspected the test but did not run it.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

Thank you for contributing. about this patch, I investigated the topic and then I found #9004 approach looks better at the moment.

@youknowone in this PR, I was trying to address two things:

1. Make the PySubclass safety contract more explicit. My intention was to require authors of manual implementations to guarantee the base layout and alignment that safe native conversions rely on. I agree that this is not necessary for the specific fix in Give builtin_method its own type #9004, but it seemed to me like a more honest contract at the Rust/Python boundary. I hoped that drawing attention to these assumptions could help prevent future bugs or UB. The downside, of course, is the additional - and perhaps unfamiliar -placement of unsafe impl and // SAFETY: comments.
2. What seemed more important to me was checking casts against the actual allocation, rather than relying on the object's current Python class and its declared size. I also tried to restrict incompatible class/base assignments and reject invalid native upcast targets, since I saw these as related ways to arrive at an incorrect native representation.

For example, consider two macro-defined native classes, Base and Derived, where Derived embeds Base and adds another Rust field. Omitting their definitions:

let _ = Base::make_static_type();
let derived = Derived::make_static_type();

// Allocate only Base, but associate it with Derived's Python class.
let obj = PyRef::new_ref(Base { value: 5 }, derived, None);

assert!(!obj.as_object().downcastable::<Derived>());

With the current validation, this assertion fails: both payloads share the base's PAYLOAD_TYPE_ID, and the remaining checks consult the Python class, which claims to have Derived's layout. A subsequent safe downcast_ref::<Derived>() can therefore produce a reference to a representation that was never allocated.

The proposed allocation check rejects this case. My understanding is that #9004 resolves the specific function/method layout ambiguity, while this more general case remains. That is why I thought this part might still be useful independently.

Would you be open to a smaller follow-up focused on this allocation check? I would be happy to separate the unsafe PySubclass contract or adjust the approach if you think there is a simpler way to cover this.

you are right. I agree about unsafe PySubclass changes. if that part is splitted, it will be merged easy.

Could you please rebase this branch onto a fresh copy of main?

@youknowone @fanninpm I have updated this PR and its code, and moved the unsafe PySubclass changes into #9012.

for other parts, let me take another look later