Skip to content

Move typeid into vtable #6231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Move typeid into vtable #6231

wants to merge 2 commits into from

Conversation

@coolreader18
Copy link
Member

@coolreader18 coolreader18 commented Oct 31, 2025 •
edited by coderabbitai bot
Loading

Now that TypeId::of() is const, we can move the typeid into PyObjVTable

Summary by CodeRabbit

  • Refactor
    • Switched from per-object stored type identifiers to vtable-based type identifiers, reducing memory per object and centralizing type identity checks.
    • Payload implementations now expose a compile-time type identifier used for downcasting.
    • VTable now publishes its type identifier for runtime checks.
  • Chores
    • Updated Rust toolchain requirement in project metadata.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025 •
edited
Loading

Walkthrough

Refactors type identity from a per-instance typeid field to a vtable-stored typeid. Replaces PyPayload::payload_type_id() with an associated constant PAYLOAD_TYPE_ID, removes PyInner::typeid, and adds typeid to PyObjVTable; callers now read type IDs from the vtable.

Changes

Cohort / File(s) Summary
Trait API refactoring
vm/src/object/payload.rs		 PyPayload::payload_type_id() method replaced by const PAYLOAD_TYPE_ID: TypeId; downcastable_from() updated to compare obj.typeid() against Self::PAYLOAD_TYPE_ID.
Builtin payload impl
vm/src/builtins/str.rs		 PyUtf8Str's PyPayload impl changed to define PAYLOAD_TYPE_ID constant and use it for downcast checks instead of calling the old method.
Instance storage removal & API usage
vm/src/object/core.rs		 Removed typeid: TypeId from PyInner<T> and its initializations; PyObject::payload_is() and PyObject::typeid() now obtain the type ID from self.0.vtable.typeid.
Vtable augmentation
vm/src/object/traverse_object.rs		 Added pub(in crate::object) typeid: TypeId to PyObjVTable; PyObjVTable::of<T>() sets typeid: T::PAYLOAD_TYPE_ID.
Toolchain metadata
Cargo.toml		 Bumped workspace rust-version from 1.89.0 to 1.91.0.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Caller (e.g., downcast)
    participant PyObj as PyObject
    participant VTable as PyObjVTable

    Note over Caller,PyObj: Old flow (prior to this change)
    Caller->>PyObj: obj.typeid() reads instance field
    PyObj-->>Caller: returns per-instance TypeId

    Note over Caller,PyObj: New flow (after this change)
    Caller->>PyObj: obj.typeid() reads from vtable
    PyObj->>VTable: access vtable.typeid
    VTable-->>PyObj: TypeId (T::PAYLOAD_TYPE_ID)
    PyObj-->>Caller: returns vtable TypeId
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay attention to vm/src/object/core.rs changes where all type-id accessors were redirected to the vtable.
  • Verify PyObjVTable::of<T>() is invoked for every payload type so typeid is correctly populated.
  • Inspect any other payload implementations for correct PAYLOAD_TYPE_ID usage.

Possibly related PRs

Suggested reviewers

  • youknowone
  • arihant2math

Poem

🐰 I hopped from field to vtable bright,
Constants now hold the type-id light,
Methods trimmed, the hops are neat,
Payloads match where vtables meet,
A little rabbit cheers this tidy sight! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Move typeid into vtable" directly and accurately describes the main architectural change across the entire changeset. The modifications in vm/src/object/traverse_object.rs introduce a new typeid field to PyObjVTable, while vm/src/object/core.rs removes the per-instance typeid field from PyInner<T> and updates methods to access type IDs through the vtable instead. Supporting changes in payload.rs and str.rs refactor the PyPayload trait and implementations to use a const PAYLOAD_TYPE_ID rather than a method, enabling this structural shift. The title is concise, specific, and clearly conveys the primary refactoring intent to someone reviewing the PR history.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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 and usage tips.

coderabbitai[bot]
coderabbitai bot reviewed Oct 31, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7249af9 and a8b2f9e.

📒 Files selected for processing (1)
  • Cargo.toml (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-11T05:06:59.971Z 
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files; only make minimal changes to work around RustPython limitations

Applied to files:

  • Cargo.toml
📚 Learning: 2025-09-11T05:06:59.971Z 
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to {vm,stdlib}/**/*.rs : Use RustPython macros (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Applied to files:

  • Cargo.toml

youknowone
youknowone reviewed Nov 1, 2025
View reviewed changes
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Looks great, the failing CI must not be related, right?

@youknowone
Copy link
Member

youknowone commented Nov 1, 2025

No idea how they are related, but re-running CI doesn't fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone left review comments

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

At least 1 approving review is required to merge this pull request.

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

@coolreader18 @youknowone