Opcode name - #7433
Conversation
Sorry, something went wrong.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a public name(&self) -> &str accessor to InstructionMetadata, implements it for Instruction, PseudoInstruction, and AnyInstruction, and updates disassembly/display rendering to use this accessor instead of hard-coded opcode name literals. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 1 | ❌ 2 ❌ Failed checks (1 warning, 1 inconclusive)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)crates/compiler-core/src/bytecode/instruction.rs (1)🤖 Prompt for all review comments with AI agents1713-1713: Minor style inconsistency: PopBlock has no fields.
PopBlock is defined as a unit variant (line 1608), so the { .. } pattern is unnecessary. Other unit-like matches in this block don't use this pattern.
Proposed fix🤖 Prompt for AI Agents- Self::PopBlock { .. } => "POP_BLOCK", + Self::PopBlock => "POP_BLOCK",Verify each finding against the current code and only fix it if needed. In `@crates/compiler-core/src/bytecode/instruction.rs` at line 1713, The match arm for the enum variant PopBlock unnecessarily uses a struct pattern with `{ .. }`; update the pattern in the match (the arm mapping to "POP_BLOCK") from Self::PopBlock { .. } to the unit pattern Self::PopBlock so it matches the unit variant definition of PopBlock in the Instruction enum.
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Line 1248: The match arm for Self::CallMethodDescriptorO returns the wrong
opcode name; update the returned string from "CALL_METHOD_DESCRIPTOR" to
"CALL_METHOD_DESCRIPTOR_O" so the name matches the enum variant
CallMethodDescriptorO and CPython 3.14 naming; locate the mapping in
instruction.rs where CallMethodDescriptorO is handled and change the string
literal accordingly.
- Line 1236: The match arm for the opcode name is returning the wrong string for
the enum variant Self::CallBuiltinO; update the returned name from
"CALL_BUILTIN" to "CALL_BUILTIN_O" in the function that maps instructions to
their names (the match arm handling Self::CallBuiltinO) so the opcode string
matches the enum variant and CPython 3.14 naming.
---
Nitpick comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Line 1713: The match arm for the enum variant PopBlock unnecessarily uses a
struct pattern with `{ .. }`; update the pattern in the match (the arm mapping
to "POP_BLOCK") from Self::PopBlock { .. } to the unit pattern Self::PopBlock so
it matches the unit variant definition of PopBlock in the Instruction enum.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f06d764e-9e82-49fb-9879-9fd028212ffa
📥 CommitsReviewing files that changed from the base of the PR and between 005860c and edf5793.
📒 Files selected for processing (1)- crates/compiler-core/src/bytecode/instruction.rs
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify each finding against the current code and only fix it if needed. Inline comments: In `@crates/compiler-core/src/bytecode/instruction.rs`: - Line 1248: The returned opcode name mismatches the enum variant Self::CallMethodDescriptorO by using a digit "0" instead of letter "O"; update the string literal "CALL_METHOD_DESCRIPTOR_0" to "CALL_METHOD_DESCRIPTOR_O" in the match arm for CallMethodDescriptorO so the opcode name matches the enum variant and CPython 3.14 naming convention.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e0bbe3dc-727e-40f5-aec1-8953371dc951
📥 CommitsReviewing files that changed from the base of the PR and between edf5793 and ebbcc89.
📒 Files selected for processing (1)- crates/compiler-core/src/bytecode/instruction.rs
Sorry, something went wrong.
There was a problem hiding this comment.
crates/compiler-core/src/bytecode/instruction.rs (1)🤖 Prompt for all review comments with AI agents1705-1719: Minor style inconsistency in pattern matching.
Line 1713 uses Self::PopBlock { .. } but PopBlock is a unit variant (no fields). While valid Rust, this is inconsistent with line 1707 where Self::AnnotationsPlaceholder (also a unit variant) uses no braces.
Optional: Use consistent pattern style🤖 Prompt for AI Agents- Self::PopBlock { .. } => "POP_BLOCK", + Self::PopBlock => "POP_BLOCK",Verify each finding against the current code and only fix it if needed. In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 1705 - 1719, The pattern match in the name() method mixes unit-variant style and struct-like style; change the unit variant match Self::PopBlock { .. } to the consistent unit pattern Self::PopBlock (same style as Self::AnnotationsPlaceholder) inside the name() function so all unit variants use the same pattern form.
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 1705-1719: The pattern match in the name() method mixes
unit-variant style and struct-like style; change the unit variant match
Self::PopBlock { .. } to the consistent unit pattern Self::PopBlock (same style
as Self::AnnotationsPlaceholder) inside the name() function so all unit variants
use the same pattern form.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: cbbc07cf-671c-4202-9a08-e334c4503173
📥 CommitsReviewing files that changed from the base of the PR and between ebbcc89 and ffe1708.
📒 Files selected for processing (1)- crates/compiler-core/src/bytecode/instruction.rs
Sorry, something went wrong.
There was a problem hiding this comment.
crates/compiler-core/src/bytecode/instruction.rs (1)🤖 Prompt for all review comments with AI agents1479-1508: Consider extracting common LoadGlobal* formatting logic.
The formatting logic for LoadGlobal, LoadGlobalBuiltin, and LoadGlobalModule is nearly identical. Per coding guidelines, when branches share common logic differing only in value, consider extracting to avoid duplication.
♻️ Suggested refactor- Self::LoadGlobal { namei } => { - let oparg = namei.get(arg); - let name_idx = oparg >> 1; - let opname = self.name(); - if (oparg & 1) != 0 { - write!(f, "{:pad$}({}, NULL + {})", opname, oparg, name(name_idx)) - } else { - write!(f, "{:pad$}({}, {})", opname, oparg, name(name_idx)) - } - } - Self::LoadGlobalBuiltin => { - let oparg = u32::from(arg); - let name_idx = oparg >> 1; - let opname = self.name(); - if (oparg & 1) != 0 { - write!(f, "{:pad$}({}, NULL + {})", opname, oparg, name(name_idx)) - } else { - write!(f, "{:pad$}({}, {})", opname, oparg, name(name_idx)) - } - } - Self::LoadGlobalModule => { - let oparg = u32::from(arg); - let name_idx = oparg >> 1; - let opname = self.name(); - if (oparg & 1) != 0 { - write!(f, "{:pad$}({}, NULL + {})", opname, oparg, name(name_idx)) - } else { - write!(f, "{:pad$}({}, {})", opname, oparg, name(name_idx)) - } - } + Self::LoadGlobal { namei } => { + let oparg = namei.get(arg); + fmt_load_global(f, self.name(), oparg, name(oparg >> 1), pad) + } + Self::LoadGlobalBuiltin | Self::LoadGlobalModule => { + let oparg = u32::from(arg); + fmt_load_global(f, self.name(), oparg, name(oparg >> 1), pad) + }Where fmt_load_global is a local closure or helper:
let fmt_load_global = |f: &mut fmt::Formatter<'_>, opname: &str, oparg: u32, resolved_name: &str, pad: usize| -> fmt::Result { if (oparg & 1) != 0 { write!(f, "{:pad$}({}, NULL + {})", opname, oparg, resolved_name) } else { write!(f, "{:pad$}({}, {})", opname, oparg, resolved_name) } };As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 1479 - 1508, The three match arms Self::LoadGlobal, Self::LoadGlobalBuiltin, and Self::LoadGlobalModule duplicate identical formatting logic; extract the shared formatting into a small helper or closure (e.g., fmt_load_global) that accepts opname (from self.name()), oparg (u32 from arg), resolved_name (result of name(name_idx)) and pad, then call that helper from each arm after computing name_idx and converting arg as needed; ensure you reuse the existing write!(f, ...) calls inside the helper so each arm only computes its specific inputs and invokes fmt_load_global.
Verify each finding against the current code and only fix it if needed. Nitpick comments: In `@crates/compiler-core/src/bytecode/instruction.rs`: - Around line 1479-1508: The three match arms Self::LoadGlobal, Self::LoadGlobalBuiltin, and Self::LoadGlobalModule duplicate identical formatting logic; extract the shared formatting into a small helper or closure (e.g., fmt_load_global) that accepts opname (from self.name()), oparg (u32 from arg), resolved_name (result of name(name_idx)) and pad, then call that helper from each arm after computing name_idx and converting arg as needed; ensure you reuse the existing write!(f, ...) calls inside the helper so each arm only computes its specific inputs and invokes fmt_load_global.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9e29d217-1308-40ca-acf8-648f6853f03e
📥 CommitsReviewing files that changed from the base of the PR and between ffe1708 and 1ff97e5.
📒 Files selected for processing (1)- crates/compiler-core/src/bytecode/instruction.rs
Sorry, something went wrong.
youknowone
left a comment
There was a problem hiding this comment.
Could you explain more about the motivation? The metadata seems to be described in a single function before. Now we have to edit 2 places to edit the instruction. There must be benefits to justify this inconvenience
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 1552-1556: The StoreFastLoadFast display arm currently writes an
extra leading space and prints raw indices; change it to match the other arms by
removing the extra leading space in the format string (use "{:pad$}" instead of
" {:pad$}") and resolve the indices to variable names before printing (use the
same resolution helper used by LoadFastLoadFast/StoreFastStoreFast on
var_nums.get(arg) to obtain store_name and load_name, then write them as
"opname{:pad$}({store_name}, {load_name})"). Ensure you still call
var_nums.get(arg) and extract the two positions but format the output with
names, not numeric indexes.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9da50c58-8802-4a52-9193-6060e6fa4226
📥 CommitsReviewing files that changed from the base of the PR and between 1ff97e5 and e9c0911.
📒 Files selected for processing (1)- crates/compiler-core/src/bytecode/instruction.rs
Sorry, something went wrong.
ofc, I was working on auto-generating the rust code for the opcodes (and methods). This change is extracted from those changes so the final PR wouldn't be that big. I guess it would have made more sense to implement fmt::Display instead. LMK what you think |
Sorry, something went wrong.
|
2 questions.
|
Sorry, something went wrong.
Wasn't aware of this crate. I can integrate it, it seems better tbh, although I ain't sure it's worth adding a new crate for this single use when there's a simpler (?) approach, with 0 runtime cost.
Yes, the script aims to auto generate an Opcode enum (which implements fmt::Display as the CPython name) and the Instruction enum (and the method to get the standalone opcode for said Instruction). I forgot to mention it at #7433 (comment) 🤦 which is IMO the most compelling point |
Sorry, something went wrong.
|
you are right. adding runtime library for const data is not ideal. |
Sorry, something went wrong.
I can do it, I think the reduced cognitive load comes when splitting into separate functions, and one of them called name that returns a &str, it took me some time to realize that the call for stringify! was invoked inside the macro. Anyway, feel free to close this PR if you think it's not a good idea (as always) |
Sorry, something went wrong.
|
When we have to edit something, editing single place is better than editing multiple place. We are very fragile to track multi source for single truth. So I think this change is justified only when we have more benefit than this disadvantage.
I always would like to listen the writer's opinion. The writer usually got more feedback from editing code. Let me have enough discussion about unclear situations. You actually have to prevent me not to close PRs without enough discussion if I am doing that only because I think it is not a good idea. |
Sorry, something went wrong.
In the current state you're correct. In the hypothetical scenario of having: enum Opcode {
A = 0,
B = 1,
}
enum Instruction {
A,
B { oparg: u32 },
}
impl From<Instruction> for Opcode {
...
}Would implementing fmt::Display for Opcode make sense? And moreover, do we want having two "identical" enums (one with an oparg builder and one without)? I believe this approach is suited for resolving #6746, as you could do: matches!(instruction.opcode(), Opcode::A | Opcode::B)
I like your approach |
Sorry, something went wrong.
|
I got the point. Could you include this commit when opcode is actually changed to that way? enum design must come first in that case. |
Sorry, something went wrong.
sounds better, I'll so that first |
Sorry, something went wrong.
Summary by CodeRabbit