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
Merged
youknowone merged 2 commits into from
Jan 7, 2026
Merged

Move typeid into vtable #6231

youknowone merged 2 commits into from
Jan 7, 2026

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

  • Chores

    • Updated minimum Rust toolchain requirement to 1.91.0. Users compiling from source must upgrade their Rust toolchain.

  • Refactor

    • Internal type-identification implementation switched from callable lookups to associated constants. No user-visible behavior changes, only internal API shape.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025 •
edited
Loading

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Workspace Rust toolchain minimum bumped from "1.89.0" to "1.91.0" in Cargo.toml. Several implementations of ::rustpython_vm::PyPayload replace the payload_type_id() function with an associated constant PAYLOAD_TYPE_ID (using core::any::TypeId) in generated and builtin types.

Changes

Cohort / File(s) Summary
Toolchain Version Bump
Cargo.toml		 Updated workspace.package.rust-version from "1.89.0" to "1.91.0".
Derived PyPayload impls
crates/derive-impl/src/pyclass.rs, crates/derive-impl/src/pystructseq.rs		 Replaced fn payload_type_id() -> ::std::any::TypeId with an associated const PAYLOAD_TYPE_ID: ::core::any::TypeId; constants initialized from base type PAYLOAD_TYPE_ID.
Builtin payload const
crates/vm/src/builtins/str.rs		 Removed payload_type_id() and added public PAYLOAD_TYPE_ID: core::any::TypeId = core::any::TypeId::of::<PyStr>();.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Wtf8-compatible fixes #5985: Changes the PyPayload API by replacing payload_type_id() with an associated constant PAYLOAD_TYPE_ID, which directly relates to the trait-implementation updates here.

Poem

🐰
I nibbled code beneath a moonlit patch,
Replaced a function with a constant hatch,
From std to core the TypeId springs,
Toolchain hops up—new tiny things!

Pre-merge checks

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title 'Move typeid into vtable' does not match the changeset, which only updates the Rust toolchain version from 1.89.0 to 1.91.0 in Cargo.toml. Update the PR title to accurately reflect the actual change, such as 'Bump Rust toolchain requirement to 1.91.0' or 'Update workspace rust-version to 1.91.0'.
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 (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
📜 Recent 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 cdb78f6 and 1756a5c.

📒 Files selected for processing (4)
  • Cargo.toml
  • crates/derive-impl/src/pyclass.rs
  • crates/derive-impl/src/pystructseq.rs
  • crates/vm/src/builtins/str.rs

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.

@youknowone
Copy link
Member

youknowone commented Dec 30, 2025

const PAYLOAD_TYPE_ID evaluates to different values across compilation units

@youknowone youknowone marked this pull request as draft January 4, 2026 15:43
@youknowone youknowone marked this pull request as ready for review January 7, 2026 03:32
youknowone
youknowone approved these changes Jan 7, 2026
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.

awesome!

@youknowone youknowone merged commit 09bde28 into RustPython:main Jan 7, 2026
32 of 35 checks passed
@coolreader18 coolreader18 deleted the vtable-typeid branch January 7, 2026 23:03
terryluan12 pushed a commit to terryluan12/RustPython that referenced this pull request Jan 15, 2026
@coolreader18 @terryluan12
Move typeid into vtable (RustPython#6231)
da30dba 
* Move typeid into vtable

* Bump rust-version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@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

@coolreader18 @youknowone