Opcode descs not hardcoded#7682
Conversation
📝 WalkthroughWalkthroughGenerated enum iteration helpers and descriptor methods; added explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/compiler-core/src/bytecode/oparg.rs (3)
395-414:desc()signature: takeselfand return&'static str.Two minor concerns shared by all three new
desc()impls:
- The receiver is
&self, but each of these enums isCopyand the sibling methods generated by the macro (as_u8(self),as_u32(self)) andBinaryOperator::as_inplace(self)takeself. Takeselffor consistency.- The return type
&strties the returned slice's lifetime to&selfvia elision. Since every arm returns a string literal, declaring&'static stris 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: Preferinto_iter()overiter().copied().In Rust 2021 edition, arrays implement
IntoIteratoryielding owned values, which is the idiomatic and slightly cheaper way to enumerateCopyenum variants. Also consider renaming toiter()orvariants()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_markdownand 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
📒 Files selected for processing (3)
crates/compiler-core/src/bytecode/oparg.rscrates/stdlib/src/_opcode.rscrates/vm/src/frame.rs
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/compiler-core/src/bytecode/oparg.rs (3)
394-413: Consider takingselfby value and returning&'static strfromdesc.
IntrinsicFunction1,IntrinsicFunction2, andBinaryOperatorare allCopyenums, and every arm ofdescreturns a string literal whose true lifetime is'static. The current&selfsignatures elide the return lifetime to&'a strtied to the receiver, which forces callers to keep the receiver alive even though the data is static, and is inconsistent with the existing by-valueas_inplace(self)API onBinaryOperator. Returning&'static stris also nicer for callers like_opcode.rsthat pass the result straight intovm.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_urlslint flags this, and the rest of this file consistently uses Markdown links (e.g.,[CPython RESUME_OPARG_LOCATION_MASK](...)nearResumeContext). 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
Copyderived, making.into_iter()applicable to all uses. This change is more idiomatic in Rust 2021+, avoids relying on array literal promotion semantics, and automatically preservesExactSizeIteratorandDoubleEndedIteratortraits 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
📒 Files selected for processing (2)
crates/compiler-core/src/bytecode/oparg.rscrates/stdlib/src/_opcode.rs
Summary by CodeRabbit