Skip to content

Validate native subclass object layouts - #8905

Merged
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:pyclass_payload_layout
Sep 29, 2026
Merged

Validate native subclass object layouts#8905
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:pyclass_payload_layout

Conversation

@1ndahous3

@1ndahous3 1ndahous3 commented Sep 29, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

AI assistance

Written with Codex (GPT-6), reviewed by a human before submission.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected native subclass layout and alignment checks, improving compatibility with base classes and preventing invalid subclass declarations.
    • Updated object size calculations to account for payload layout and alignment, including on 32-bit targets.
  • Documentation
    • Clarified layout requirements for native subclasses and how they are checked.
  • Tests
    • Added coverage for aligned payload sizes and inherited and subclass-specific getters.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026

Copy link
Copy Markdown

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.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml

Review profile: CHILL

Plan: Advanced

Run ID: 7e07d003-c65f-4c4e-ba93-26cb9b4ad5b3

📥 Commits

Reviewing files that changed from the base of the PR and between a7d75d2 and 09fbaad.

📒 Files selected for processing (6)
  • crates/derive-impl/src/pyclass.rs
  • crates/derive/src/lib.rs
  • crates/vm/src/builtins/dict.rs
  • crates/vm/src/object/core.rs
  • crates/vm/src/object/payload.rs
  • crates/vm/src/stdlib/_io.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Walkthrough

The pyclass derive macro now checks native subclass field offsets, payload offsets, and object alignment. BASICSIZE uses payload layout, PyDict and _IOBase have 8-byte alignment, and tests cover aligned payload size and inherited and derived getters.

Changes

Native subclass layout

Layer / File(s) Summary
Base layout validation and constraints
crates/derive-impl/src/pyclass.rs, crates/derive/src/lib.rs, crates/vm/src/object/payload.rs		 The derive macro rejects non-struct base items and checks base-field offset, matching payload offsets, and derived object alignment. The documentation describes the layout requirements and includes compile-fail examples.
Payload size, alignment, and behavior tests
crates/derive-impl/src/pyclass.rs, crates/vm/src/builtins/dict.rs, crates/vm/src/stdlib/_io.rs, crates/vm/src/object/core.rs		 Generated BASICSIZE uses the payload offset and size. PyDict and _IOBase gain 8-byte alignment. Tests check the size of an over-aligned payload and getters on a native subclass.

Priority: ➖ Normal

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

Change: Bug fix

Suggested reviewers: youknowone

Merge Risk: ⚪ Minimal · up to 09fba

No identified layout or allocation issue blocks merging. The compile-fail examples and wasm32 behavior remain unexecuted in the supplied evidence.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 09fba

The new checks appear to prevent incompatible native subclass layouts rather than expose a new runtime entry point. The change still warrants review because it can affect which native subclasses build and how their size is reported, particularly across target architectures.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected exposure is native Rust subclass layout and VM class metadata, including the aligned dictionary and I/O bases; no tenant, credential, or network boundary change is established.

Trust Boundaries and Controls

  • observed — For a declared native base, generated constants check the base-field address, payload address, and object alignment at compile time.

Resilience and Maintainability Implications

  • inferred — The full typed Py layout remains the allocation and deallocation basis, countering an inference that the changed BASICSIZE alone reduces allocated object memory.

Hardening Proposals

  • proposed — Exercise native-subclass compile-fail and size behavior on both 32-bit and 64-bit targets to detect target-specific layout drift before release.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 6 files. 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 and concisely describes the main change: adding validation for native subclass object layouts.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

@codspeed

codspeed Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 62 untouched benchmarks
⏩ 4 skipped benchmarks1

Comparing 1ndahous3:pyclass_payload_layout (09fbaad) with main (a7d75d2)

Open in CodSpeed

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

youknowone
youknowone approved these changes Sep 29, 2026
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@youknowone
youknowone merged commit 55a5f1c into RustPython:main Sep 29, 2026
31 checks passed
@1ndahous3
1ndahous3 deleted the pyclass_payload_layout branch September 29, 2026 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@1ndahous3 @youknowone