Skip to content

Opcode descs not hardcoded#7682

Merged
youknowone merged 7 commits into
RustPython:mainfrom
ShaharNaveh:opcode-descs-no-hardcoded
Apr 25, 2026
Merged

Opcode descs not hardcoded#7682
youknowone merged 7 commits into
RustPython:mainfrom
ShaharNaveh:opcode-descs-no-hardcoded

Conversation

@ShaharNaveh
Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh commented Apr 25, 2026

Summary by CodeRabbit

  • Refactor
    • Enhanced internal opcode/enumeration utilities with iteration and descriptive helpers; several helpers now derive intrinsic/operator metadata from those enums.
  • Bug Fixes
    • Added explicit handling for invalid intrinsic variants in VM dispatch to prevent unexpected or undefined behavior.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 25, 2026

📝 Walkthrough

Walkthrough

Generated enum iteration helpers and descriptor methods; added explicit Invalid = 0 variants for intrinsic enums; updated stdlib opcode helpers to build lists from oparg iterators; and made VM intrinsic dispatch explicitly match Invalid variants.

Changes

Cohort / File(s) Summary
Core oparg enhancements
crates/compiler-core/src/bytecode/oparg.rs
Macro impl_oparg_enum! now generates iter() (iterator over variants). Added Invalid = 0 to IntrinsicFunction1 and IntrinsicFunction2. Added const fn desc(&self) -> &str to IntrinsicFunction1, IntrinsicFunction2, and BinaryOperator. New methods use #[must_use].
Standard library opcode generation
crates/stdlib/src/_opcode.rs
Replaced hardcoded descriptor lists with dynamic collection by iterating oparg enums (IntrinsicFunction1, IntrinsicFunction2, BinaryOperator, SpecialMethod) and using desc()/to_string() to build results.
VM intrinsic dispatch
crates/vm/src/frame.rs
Added explicit match arms for Invalid variants in call_intrinsic_1 and call_intrinsic_2, routing them to unreachable!() to ensure exhaustive matching.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • youknowone
  • coolreader18

Poem

🐇 I hopped through enums, variant by variant,
Added iterators, descs, and one "Invalid" hint.
Stdlib lists now bloom from each enum's song,
VM matches tidy — nothing falls along.
Hooray for bytecode, carrots, and runtime sprint! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Opcode descs not hardcoded' directly references the main change—replacing hardcoded opcode descriptor lists with dynamic iteration over enum variants.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

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

Copy link
Copy Markdown
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.

🧹 Nitpick comments (3)
crates/compiler-core/src/bytecode/oparg.rs (3)

395-414: desc() signature: take self and return &'static str.

Two minor concerns shared by all three new desc() impls:

  • The receiver is &self, but each of these enums is Copy and the sibling methods generated by the macro (as_u8(self), as_u32(self)) and BinaryOperator::as_inplace(self) take self. Take self for consistency.
  • The return type &str ties the returned slice's lifetime to &self via elision. Since every arm returns a string literal, declaring &'static str is more accurate and frees callers from an unnecessary borrow on the receiver.
♻️ Suggested signature change (apply to all three impls)
-    pub const fn desc(&self) -> &str {
+    pub const fn desc(self) -> &'static str {
         match self {

Also applies to: 430-443, 673-706

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 395 - 414, The
desc() impls currently use &self -> &str which unnecessarily ties the returned
string's lifetime to the receiver and is inconsistent with other methods; change
the signature of IntrinsicFunction1::desc (and the two other similar impls) to
take self (by value) and return &'static str, i.e. pub const fn desc(self) ->
&'static str, and keep the match arms returning the same string literals so
callers don't need a borrow and lifetimes are correct.

246-250: Prefer into_iter() over iter().copied().

In Rust 2021 edition, arrays implement IntoIterator yielding owned values, which is the idiomatic and slightly cheaper way to enumerate Copy enum variants. Also consider renaming to iter() or variants() to match Rust naming conventions — iterator() is uncommon.

♻️ Suggested change
             /// Iterate over the variants.
             #[must_use]
-            $vis fn iterator() -> impl Iterator<Item = Self> {
-                [$(Self::$variant),*].iter().copied()
+            $vis fn iter() -> impl Iterator<Item = Self> {
+                [$(Self::$variant),*].into_iter()
             }

Note: callers in crates/stdlib/src/_opcode.rs (IntrinsicFunction1::iterator(), etc.) would need to be updated accordingly.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 246 - 250, The
current iterator() returns owned enum variants via [.iter().copied()], which is
non-idiomatic; change the method (e.g., rename iterator() to iter() or
variants()) to return the iterator using the array's IntoIterator (use [. .
.].into_iter()) and update all callers (e.g., IntrinsicFunction1::iterator()
usages in crates/stdlib/src/_opcode.rs) to the new name; ensure the signature
and visibility ($vis fn ...) remain the same except for the name and that the
body uses into_iter() instead of iter().copied().

396-396: Use a markdown link for the CPython source reference.

Bare URLs in doc comments are flagged by clippy::doc_markdown and are inconsistent with the existing convention in this file (e.g. line 908 uses [CPython ...](https://...)). Wrap them in a markdown link or angle brackets.

♻️ Example for line 396 (apply analogously at lines 431 and 673)
-    /// https://github.com/python/cpython/blob/v3.14.4/Include/internal/pycore_intrinsics.h#L9-L20
+    /// See [CPython `pycore_intrinsics.h`](https://github.com/python/cpython/blob/v3.14.4/Include/internal/pycore_intrinsics.h#L9-L20).

As per coding guidelines: "Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints that are introduced by your changes".

Also applies to: 431-431, 673-673

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` at line 396, Doc comments in
crates/compiler-core/src/bytecode/oparg.rs contain bare URLs (e.g., the CPython
reference at the comment currently shown) which triggers clippy::doc_markdown;
update those doc comments to use markdown links or angle-bracketed links instead
of plain URLs. Locate the three offending doc comments in oparg.rs (the CPython
reference occurrences around the noted spots) and replace each bare URL with a
Markdown link like [CPython
pycore_intrinsics.h](https://github.com/python/cpython/blob/v3.14.4/Include/internal/pycore_intrinsics.h#L9-L20)
or use <https://...> so clippy no longer flags them, then run cargo clippy and
fix any resultant doc_markdown warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 395-414: The desc() impls currently use &self -> &str which
unnecessarily ties the returned string's lifetime to the receiver and is
inconsistent with other methods; change the signature of
IntrinsicFunction1::desc (and the two other similar impls) to take self (by
value) and return &'static str, i.e. pub const fn desc(self) -> &'static str,
and keep the match arms returning the same string literals so callers don't need
a borrow and lifetimes are correct.
- Around line 246-250: The current iterator() returns owned enum variants via
[.iter().copied()], which is non-idiomatic; change the method (e.g., rename
iterator() to iter() or variants()) to return the iterator using the array's
IntoIterator (use [. . .].into_iter()) and update all callers (e.g.,
IntrinsicFunction1::iterator() usages in crates/stdlib/src/_opcode.rs) to the
new name; ensure the signature and visibility ($vis fn ...) remain the same
except for the name and that the body uses into_iter() instead of
iter().copied().
- Line 396: Doc comments in crates/compiler-core/src/bytecode/oparg.rs contain
bare URLs (e.g., the CPython reference at the comment currently shown) which
triggers clippy::doc_markdown; update those doc comments to use markdown links
or angle-bracketed links instead of plain URLs. Locate the three offending doc
comments in oparg.rs (the CPython reference occurrences around the noted spots)
and replace each bare URL with a Markdown link like [CPython
pycore_intrinsics.h](https://github.com/python/cpython/blob/v3.14.4/Include/internal/pycore_intrinsics.h#L9-L20)
or use <https://...> so clippy no longer flags them, then run cargo clippy and
fix any resultant doc_markdown warnings.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 10c5c000-ded6-437c-8165-1849c5fe1c28

📥 Commits

Reviewing files that changed from the base of the PR and between adafaf2 and e0eac8e.

📒 Files selected for processing (3)
  • crates/compiler-core/src/bytecode/oparg.rs
  • crates/stdlib/src/_opcode.rs
  • crates/vm/src/frame.rs

Copy link
Copy Markdown
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.

🧹 Nitpick comments (3)
crates/compiler-core/src/bytecode/oparg.rs (3)

394-413: Consider taking self by value and returning &'static str from desc.

IntrinsicFunction1, IntrinsicFunction2, and BinaryOperator are all Copy enums, and every arm of desc returns a string literal whose true lifetime is 'static. The current &self signatures elide the return lifetime to &'a str tied to the receiver, which forces callers to keep the receiver alive even though the data is static, and is inconsistent with the existing by-value as_inplace(self) API on BinaryOperator. Returning &'static str is also nicer for callers like _opcode.rs that pass the result straight into vm.ctx.new_str(...).

♻️ Suggested signature (apply analogously to all three impls)
-    /// https://github.com/python/cpython/blob/v3.14.4/Include/internal/pycore_intrinsics.h#L9-L20
-    #[must_use]
-    pub const fn desc(&self) -> &str {
+    /// See [CPython `pycore_intrinsics.h`](https://github.com/python/cpython/blob/v3.14.4/Include/internal/pycore_intrinsics.h#L9-L20).
+    #[must_use]
+    pub const fn desc(self) -> &'static str {
         match self {
             Self::Invalid => "INTRINSIC_1_INVALID",
             // ...
         }
     }

Also applies to: 429-442, 671-705

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 394 - 413, The desc
methods currently take &self and return an elided &str tied to the receiver;
change them to take self by value and return &'static str (e.g. pub const fn
desc(self) -> &'static str) because IntrinsicFunction1, IntrinsicFunction2 and
BinaryOperator are Copy enums and all arms return string literals; update the
three impls (IntrinsicFunction1::desc, IntrinsicFunction2::desc,
BinaryOperator::desc) accordingly and adjust any callers that relied on a
borrowed receiver to pass the enum by value instead (callers like _opcode.rs
that pass straight into vm.ctx.new_str(...) will continue to work and benefit
from a 'static return).

395-395: Wrap bare URLs in doc comments.

These three doc comments use plain URLs as their entire content. Rustdoc's bare_urls lint flags this, and the rest of this file consistently uses Markdown links (e.g., [CPython RESUME_OPARG_LOCATION_MASK](...) near ResumeContext). For consistency and to avoid potential lint failures under -D warnings, prefer <https://...> or a Markdown link.

♻️ Example fix
-    /// https://github.com/python/cpython/blob/v3.14.4/Include/internal/pycore_intrinsics.h#L9-L20
+    /// See [CPython `pycore_intrinsics.h` (intrinsic 1 table)](https://github.com/python/cpython/blob/v3.14.4/Include/internal/pycore_intrinsics.h#L9-L20).

As per coding guidelines: "Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints that are introduced by your changes".

Also applies to: 430-430, 672-672

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` at line 395, Several doc comments
in crates/compiler-core/src/bytecode/oparg.rs contain bare URLs (e.g., the
CPython pycore_intrinsics link and the comment near ResumeContext) which
triggers the Rustdoc `bare_urls` lint; update each such doc comment to wrap the
URL in angle brackets like <https://...> or convert it to a Markdown link
`[...](https://...)` so the lint is satisfied and formatting matches the rest of
the file. Ensure you modify all three occurrences referenced in the review (the
pycore_intrinsics link and the two other bare-URL comments) so clippy/cargo
clippy no longer reports warnings.

246-249: Use .into_iter() instead of .iter().copied() for better idiom and clarity.

All enums generated by this macro have Copy derived, making .into_iter() applicable to all uses. This change is more idiomatic in Rust 2021+, avoids relying on array literal promotion semantics, and automatically preserves ExactSizeIterator and DoubleEndedIterator traits without additional reasoning.

♻️ Proposed change
             /// Iterate over the variants.
             $vis fn iter() -> impl Iterator<Item = Self> {
-                [$(Self::$variant),*].iter().copied()
+                [$(Self::$variant),*].into_iter()
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 246 - 249, The
generated enum iterator function ($vis fn iter() -> impl Iterator<Item = Self>)
should use array into_iter() instead of calling .iter().copied(); locate the
macro output that builds the array of variants ([ $(Self::$variant),* ]) and
replace the trailing .iter().copied() call with .into_iter() so the iterator is
produced idiomatically (preserves ExactSizeIterator/DoubleEndedIterator for the
Copy enum variants).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 394-413: The desc methods currently take &self and return an
elided &str tied to the receiver; change them to take self by value and return
&'static str (e.g. pub const fn desc(self) -> &'static str) because
IntrinsicFunction1, IntrinsicFunction2 and BinaryOperator are Copy enums and all
arms return string literals; update the three impls (IntrinsicFunction1::desc,
IntrinsicFunction2::desc, BinaryOperator::desc) accordingly and adjust any
callers that relied on a borrowed receiver to pass the enum by value instead
(callers like _opcode.rs that pass straight into vm.ctx.new_str(...) will
continue to work and benefit from a 'static return).
- Line 395: Several doc comments in crates/compiler-core/src/bytecode/oparg.rs
contain bare URLs (e.g., the CPython pycore_intrinsics link and the comment near
ResumeContext) which triggers the Rustdoc `bare_urls` lint; update each such doc
comment to wrap the URL in angle brackets like <https://...> or convert it to a
Markdown link `[...](https://...)` so the lint is satisfied and formatting
matches the rest of the file. Ensure you modify all three occurrences referenced
in the review (the pycore_intrinsics link and the two other bare-URL comments)
so clippy/cargo clippy no longer reports warnings.
- Around line 246-249: The generated enum iterator function ($vis fn iter() ->
impl Iterator<Item = Self>) should use array into_iter() instead of calling
.iter().copied(); locate the macro output that builds the array of variants ([
$(Self::$variant),* ]) and replace the trailing .iter().copied() call with
.into_iter() so the iterator is produced idiomatically (preserves
ExactSizeIterator/DoubleEndedIterator for the Copy enum variants).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: a95a63e2-2bea-4dbf-8452-88d3e2213cf1

📥 Commits

Reviewing files that changed from the base of the PR and between 8326872 and 73983d1.

📒 Files selected for processing (2)
  • crates/compiler-core/src/bytecode/oparg.rs
  • crates/stdlib/src/_opcode.rs

@youknowone youknowone merged commit f0e23aa into RustPython:main Apr 25, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants