`oparg_enum!` to support custom display value by ShaharNaveh · Pull Request #7654 · RustPython/RustPython

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

287-295: Macro arms look correct; minor robustness nit on numeric Display.

In @display $f:expr, $num:literal, the literal $num has no type annotation in this context, so &$num defaults to &i32. It happens to format identically for these small values, but if a future variant uses a non-i32-representable literal it will fail to compile here while @discriminant (used as u8) would succeed. Consider pinning the type so both arms agree:

Proposed tweak

-    (`@display` $f:expr, $num:literal) => {
-        ::core::fmt::Display::fmt(&$num, $f)
-    };
+    (`@display` $f:expr, $num:literal) => {
+        ::core::fmt::Display::fmt(&($num as u8), $f)
+    };

🤖 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 287 - 295, The
`@display` macro arm uses &$num which defaults to &i32 and can mismatch
`@discriminant` (which is effectively u8); change the `@display` arm to pin the
literal to the same type as `@discriminant` (e.g., cast the literal to u8 before
formatting) so both arms agree on type; update the (`@display` $f:expr,
$num:literal) arm to format the literal as the same integer type used by
(`@discriminant`) to avoid future compile failures for non-i32-representable
literals.

---

312-314: Stale-ish NOTE: None still renders via the numeric branch as "0".

The comment says we should never reach Display for ConvertValueOparg::None, which is fine, but since the generated Display now silently prints "0" for it, any future misuse would produce a confusing disassembly rather than a visible error. Consider making the invariant explicit by labeling it (e.g. None = (0, "none")) or unreachable!()-ing in a dedicated Display override, so the assumption doesn't rot silently. Not blocking.

🤖 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 312 - 314, The enum
variant ConvertValueOparg::None currently falls through to the numeric Display
branch and prints "0"; update the Display implementation for ConvertValueOparg
(the impl that formats opargs) to explicitly handle the None variant instead of
letting it hit the numeric fallback — either format it with an explicit label
like "none" or call unreachable!() (prefer unreachable!() if you want the
invariant to panic when violated); ensure you update the match arm for
ConvertValueOparg::None in the Display impl (and keep the note about
Instruction::FormatSimple handling FVC_NONE) so misuse surfaces immediately
rather than silently printing "0".

---

275-285: Consider future-proofing the nested macro call if oparg_enum! is ever exported.

The concern is valid: impl_oparg_enum! is a helper macro not meant for direct user calls, and it references $crate::marshal::MarshalError. If oparg_enum! were to become a #[macro_export]'d public macro, downstream crates would fail to resolve the nested impl_oparg_enum!(@discriminant ...) and similar calls (macro-by-example doesn't re-export helper macros automatically).

However, since both macros are currently internal to this crate and impl_oparg_enum! is not exposed as part of the public API, no immediate action is required. If these macros are planned for public export in the future, apply the suggested fix: mark impl_oparg_enum! with #[doc(hidden)] #[macro_export] and prefix the nested calls with $crate:: (e.g., $crate::impl_oparg_enum!(@discriminant $value)).

🤖 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 275 - 285, The
nested helper macro impl_oparg_enum! used inside the impl for Display (and other
nested calls within oparg_enum!) should be made robust if oparg_enum! is ever
exported: mark impl_oparg_enum! with #[doc(hidden)] #[macro_export] and update
all internal invocations to use the $crate:: prefix (e.g., replace bare
impl_oparg_enum!(`@discriminant` ...) / impl_oparg_enum!(`@display` ...) calls with
$crate::impl_oparg_enum!(`@discriminant` ...) and
$crate::impl_oparg_enum!(`@display` ...)), so downstream crates can resolve the
helper when oparg_enum! is exported.