Opcode descs not hardcoded by ShaharNaveh · Pull Request #7682 · RustPython/RustPython

ShaharNaveh

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