Skip to content

Add has_* methods to opcode enums #7818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ShaharNaveh wants to merge 2 commits into from
Closed

Add has_* methods to opcode enums #7818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d946a58
Add `has_*` methods to opcode enums
ShaharNaveh May 10, 2026
17efe8e
clippy
ShaharNaveh May 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
237 changes: 214 additions & 23 deletions crates/compiler-core/src/bytecode/instruction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,11 @@ impl Instruction {
_ => 0,
}
}

#[must_use]
pub const fn is_block_push(&self) -> bool {
self.opcode().is_block_push()
}
}

impl InstructionMetadata for Instruction {
Expand Down Expand Up @@ -1024,6 +1029,103 @@ impl InstructionMetadata for Instruction {
// Only pseudo-instructions (SETUP_*) differ — see PseudoInstruction.
}

impl Opcode {
/// Does this opcode have 'HAS_CONST_FLAG' set.
#[must_use]
pub const fn has_const(self) -> bool {
matches!(
self,
Self::LoadConst | Self::LoadConstImmortal | Self::LoadConstMortal
)
}

/// Does this opcode have 'HAS_FREE_FLAG' set.
#[must_use]
pub const fn has_free(self) -> bool {
matches!(
self,
Self::DeleteDeref | Self::LoadFromDictOrDeref | Self::MakeCell | Self::StoreDeref
)
}
Comment on lines +1042 to +1049
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

CPython 3.14 HAS_FREE_FLAG opcode classification which opcodes should be included

💡 Result:

For CPython 3.14, the opcodes marked with the internal opcode-metadata flag HAS_FREE_FLAG are the following: 1) DELETE_DEREF 2) LOAD_CLOSURE 3) LOAD_DEREF 4) STORE_DEREF Evidence: CPython defines HAS_FREE_FLAG in its internal opcode metadata and associates it with the above opcode entries (e.g., DELETE_DEREF is shown with HAS_ARG_FLAG | HAS_FREE_FLAG... in the opcode metadata table) [1]. Related context: the disassembler’s public dis.hasfree set corresponds to “bytecodes that access a free (closure) variable” [2][3], i.e., the closure/free-variable access opcodes above.

Citations:

🏁 Script executed:

cd crates/compiler-core/src/bytecode && grep -n "enum Opcode\|pub const fn has_free\|LoadDeref\|LoadFromDictOrDeref\|MakeCell\|CopyFreeVars" instruction.rs | head -50

Repository: RustPython/RustPython

Length of output: 688

🏁 Script executed:

# Get the specific lines mentioned in the review
sed -n '1042,1049p' crates/compiler-core/src/bytecode/instruction.rs

Repository: RustPython/RustPython

Length of output: 320

🏁 Script executed:

# Check for LoadDeref definition
sed -n '280,300p' crates/compiler-core/src/bytecode/instruction.rs

Repository: RustPython/RustPython

Length of output: 743

🏁 Script executed:

# Check for CopyFreeVars definition
sed -n '220,230p' crates/compiler-core/src/bytecode/instruction.rs

Repository: RustPython/RustPython

Length of output: 359

🏁 Script executed:

# Search for all occurrences of these opcodes in the file
rg -n "LoadDeref|LoadFromDictOrDeref|MakeCell|CopyFreeVars|DeleteDeref|StoreDeref" crates/compiler-core/src/bytecode/instruction.rs | head -30

Repository: RustPython/RustPython

Length of output: 677

🏁 Script executed:

sed -n '1080,1090p' crates/compiler-core/src/bytecode/instruction.rs

Repository: RustPython/RustPython

Length of output: 475

🏁 Script executed:

# Check for docstrings/comments on LoadFromDictOrDeref and MakeCell
sed -n '308,315p' crates/compiler-core/src/bytecode/instruction.rs

Repository: RustPython/RustPython

Length of output: 367

🏁 Script executed:

sed -n '329,340p' crates/compiler-core/src/bytecode/instruction.rs

Repository: RustPython/RustPython

Length of output: 405

🏁 Script executed:

# Search for any usage or discussion of has_free
rg -B2 -A2 "has_free" crates/compiler-core/src/bytecode/instruction.rs | head -40

Repository: RustPython/RustPython

Length of output: 435

🏁 Script executed:

# Get full context around line 1044 to understand the structure
sed -n '1040,1055p' crates/compiler-core/src/bytecode/instruction.rs

Repository: RustPython/RustPython

Length of output: 473

🏁 Script executed:

# Let's also check if there are any comments or documentation about why these opcodes are in has_free
rg -B5 "LoadFromDictOrDeref|MakeCell" crates/compiler-core/src/bytecode/instruction.rs | head -50

Repository: RustPython/RustPython

Length of output: 1358

Add LoadDeref to has_free implementation and remove non-standard opcodes.

The has_free method is missing LoadDeref (line 289), which should be included per CPython's HAS_FREE_FLAG classification. CPython marks DELETE_DEREF, LOAD_DEREF, and STORE_DEREF as having this flag; rustpython correctly includes the first and third but omits the second.

Additionally, LoadFromDictOrDeref and MakeCell should not be in this implementation, as they do not have HAS_FREE_FLAG in CPython's opcode metadata.

Update the matches expression to:

Self::DeleteDeref | Self::LoadDeref | Self::StoreDeref
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 1042 - 1049,
The has_free() method currently matches DeleteDeref, LoadFromDictOrDeref,
MakeCell, and StoreDeref; update it to match CPython's HAS_FREE_FLAG by
including LoadDeref and removing LoadFromDictOrDeref and MakeCell. Edit the
matches expression inside the pub const fn has_free(&self) to only include
Self::DeleteDeref | Self::LoadDeref | Self::StoreDeref, ensuring the function
name has_free and the opcode variants DeleteDeref, LoadDeref, StoreDeref are the
referenced symbols to change.


/// Does this opcode have 'HAS_JUMP_FLAG' set.
#[must_use]
pub const fn has_jump(self) -> bool {
matches!(
self,
Self::EndAsyncFor
| Self::ForIter
| Self::JumpBackward
| Self::JumpBackwardNoInterrupt
| Self::JumpForward
| Self::PopJumpIfFalse
| Self::PopJumpIfNone
| Self::PopJumpIfNotNone
| Self::PopJumpIfTrue
| Self::Send
| Self::ForIterList
| Self::ForIterRange
| Self::ForIterTuple
| Self::JumpBackwardJit
| Self::JumpBackwardNoJit
| Self::InstrumentedForIter
| Self::InstrumentedEndAsyncFor
)
}
Comment on lines +1051 to +1074
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for all ForIter* and Send* opcode definitions to verify completeness
echo "=== ForIter variants ==="
rg -n "^\s+(ForIter\w*)\s*=" crates/compiler-core/src/bytecode/instruction.rs

echo -e "\n=== Send variants ==="
rg -n "^\s+(Send\w*)\s*=" crates/compiler-core/src/bytecode/instruction.rs

echo -e "\n=== Opcodes in has_jump ==="
rg -A50 "pub const fn has_jump\(self\) -> bool" crates/compiler-core/src/bytecode/instruction.rs | rg "Self::" | head -20

Repository: RustPython/RustPython

Length of output: 1150

🏁 Script executed:

# Check CPython's opcode definitions to see if ForIterGen and SendGen have HAS_JUMP_FLAG
curl -s https://raw.githubusercontent.com/python/cpython/main/Include/opcode.h | grep -A 2 -B 2 "FOR_ITER_GEN\|SEND_GEN" | head -30

Repository: RustPython/RustPython

Length of output: 47

🏁 Script executed:

# Also check if there's any documentation in RustPython about this
rg -B5 -A5 "ForIterGen|SendGen" crates/compiler-core/src/bytecode/instruction.rs | head -40

Repository: RustPython/RustPython

Length of output: 2090

🏁 Script executed:

# Check if there's a pattern or comment explaining why Gen variants are excluded
rg -B10 "pub const fn has_jump" crates/compiler-core/src/bytecode/instruction.rs | head -20

Repository: RustPython/RustPython

Length of output: 519

🏁 Script executed:

# Let's check CPython's Python/opcode_ids.h directly for the actual opcode definitions
python3 <<'EOF'
import urllib.request
import json

# Fetch CPython's opcode definitions
url = "https://raw.githubusercontent.com/python/cpython/main/Python/opcode_ids.h"
try:
    with urllib.request.urlopen(url, timeout=5) as response:
        content = response.read().decode('utf-8')
        # Search for FOR_ITER_GEN and SEND_GEN definitions
        lines = content.split('\n')
        for i, line in enumerate(lines):
            if 'FOR_ITER_GEN' in line or 'SEND_GEN' in line:
                # Print context around the definition
                start = max(0, i - 2)
                end = min(len(lines), i + 3)
                for j in range(start, end):
                    print(f"{j}: {lines[j]}")
                print("---")
except Exception as e:
    print(f"Error fetching CPython opcode definitions: {e}")

# Also check core/ceval.c for HAS_JUMP_FLAG definitions
url2 = "https://raw.githubusercontent.com/python/cpython/main/Python/ceval.c"
try:
    with urllib.request.urlopen(url2, timeout=5) as response:
        content = response.read().decode('utf-8')
        lines = content.split('\n')
        for i, line in enumerate(lines):
            if 'FOR_ITER_GEN' in line or 'SEND_GEN' in line:
                # Print context
                start = max(0, i - 2)
                end = min(len(lines), i + 3)
                for j in range(start, end):
                    print(f"{j}: {lines[j]}")
                print("---")
except Exception as e:
    print(f"Error fetching CPython ceval.c: {e}")
EOF

Repository: RustPython/RustPython

Length of output: 396

🏁 Script executed:

# Let's check if there are any comments in the RustPython code about jump flags or specialized variants
rg -B10 -A10 "has_jump" crates/compiler-core/src/bytecode/instruction.rs | grep -A30 "pub const fn has_jump"

Repository: RustPython/RustPython

Length of output: 1418

🏁 Script executed:

# Check if there's any test or documentation about which opcodes should have jump flag
fd instruction tests test | head -5

Repository: RustPython/RustPython

Length of output: 209

🏁 Script executed:

# Look for any git history or comments about ForIterGen/SendGen jump behavior
rg "ForIterGen|SendGen" crates/compiler-core/src -B3 -A3

Repository: RustPython/RustPython

Length of output: 4344

Include ForIterGen and SendGen in the has_jump method.

The implementation includes specialized variants like ForIterList, ForIterRange, ForIterTuple, JumpBackwardJit, and JumpBackwardNoJit, but omits:

  • ForIterGen (line 448) — maps to base opcode ForIter
  • SendGen (line 197) — maps to base opcode Send

Since their base opcodes (ForIter and Send) are included, and other specialized variants are also present, these should be added for consistency.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 1051 - 1074,
The has_jump() method is missing two specialized opcode variants; update the
matches! list inside pub const fn has_jump(self) -> bool (in instruction.rs) to
also include ForIterGen and SendGen so they map consistently to the base opcodes
ForIter and Send (similar to the existing ForIterList/ForIterRange/ForIterTuple
and JumpBackwardJit/JumpBackwardNoJit entries); simply add ForIterGen and
SendGen to the list of matched variants in has_jump().


/// Does this opcode have 'HAS_LOCAL_FLAG' set.
#[must_use]
pub const fn has_local(self) -> bool {
matches!(
self,
Self::BinaryOpInplaceAddUnicode
| Self::DeleteFast
| Self::LoadDeref
| Self::LoadFast
| Self::LoadFastAndClear
| Self::LoadFastBorrow
| Self::LoadFastBorrowLoadFastBorrow
| Self::LoadFastCheck
| Self::LoadFastLoadFast
| Self::StoreFast
| Self::StoreFastLoadFast
| Self::StoreFastStoreFast
)
}

/// Does this opcode have 'HAS_NAME_FLAG' set.
#[must_use]
pub const fn has_name(self) -> bool {
matches!(
self,
Self::DeleteAttr
| Self::DeleteGlobal
| Self::DeleteName
| Self::ImportFrom
| Self::ImportName
| Self::LoadAttr
| Self::LoadFromDictOrGlobals
| Self::LoadGlobal
| Self::LoadName
| Self::LoadSuperAttr
| Self::StoreAttr
| Self::StoreGlobal
| Self::StoreName
| Self::LoadAttrGetattributeOverridden
| Self::LoadAttrWithHint
| Self::LoadSuperAttrAttr
| Self::LoadSuperAttrMethod
| Self::StoreAttrWithHint
| Self::InstrumentedLoadSuperAttr
)
}

#[must_use]
pub const fn is_block_push(&self) -> bool {
false
}
}

define_opcodes!(
#[repr(u16)]
pub enum PseudoOpcode;
Expand All @@ -1044,16 +1146,10 @@ define_opcodes!(
);

impl PseudoInstruction {
/// Returns true if self is one of:
/// - [`PseudoInstruction::SetupCleanup`]
/// - [`PseudoInstruction::SetupFinally`]
/// - [`PseudoInstruction::SetupWith`]
/// See [`PseudoOpcode::is_block_push`].
#[must_use]
pub const fn is_block_push(&self) -> bool {
matches!(
self.opcode(),
PseudoOpcode::SetupCleanup | PseudoOpcode::SetupFinally | PseudoOpcode::SetupWith
)
self.opcode().is_block_push()
}
}

Expand Down Expand Up @@ -1124,6 +1220,53 @@ impl InstructionMetadata for PseudoInstruction {
}
}

impl PseudoOpcode {
/// Does this opcode have 'HAS_CONST_FLAG' set.
#[must_use]
pub const fn has_const(self) -> bool {
false
}

/// Does this opcode have 'HAS_FREE_FLAG' set.
#[must_use]
pub const fn has_free(self) -> bool {
false
}

/// Does this opcode have 'HAS_JUMP_FLAG' set.
#[must_use]
pub const fn has_jump(self) -> bool {
matches!(
self,
Self::Jump | Self::JumpIfFalse | Self::JumpIfTrue | Self::JumpNoInterrupt
)
}

/// Does this opcode have 'HAS_LOCAL_FLAG' set.
#[must_use]
pub const fn has_local(self) -> bool {
matches!(self, Self::LoadClosure | Self::StoreFastMaybeNull)
}

/// Does this opcode have 'HAS_NAME_FLAG' set.
#[must_use]
pub const fn has_name(self) -> bool {
false
}

/// Returns true if self is one of:
/// - [`Self::SetupCleanup`]
/// - [`Self::SetupFinally`]
/// - [`Self::SetupWith`]
#[must_use]
pub const fn is_block_push(&self) -> bool {
matches!(
self,
Self::SetupCleanup | Self::SetupFinally | Self::SetupWith
)
}
}

#[derive(Clone, Copy, Debug)]
pub enum AnyInstruction {
Real(Instruction),
Expand Down Expand Up @@ -1182,29 +1325,48 @@ impl From<AnyOpcode> for AnyInstruction {
}
}

macro_rules! inst_either {
(fn $name:ident ( &self $(, $arg:ident : $arg_ty:ty )* ) -> $ret:ty ) => {
fn $name(&self $(, $arg : $arg_ty )* ) -> $ret {
macro_rules! either_real_pseudo {
// Const
(
$(#[$meta:meta])*
$vis:vis const fn $name:ident(&self $(, $arg:ident : $arg_ty:ty)*) -> $ret:ty
) => {
$(#[$meta])*
$vis const fn $name(&self $(, $arg: $arg_ty)*) -> $ret {
match self {
Self::Real(v) => v.$name($($arg),*),
Self::Pseudo(v) => v.$name($($arg),*),
}
}
};

// Not const
(
$(#[$meta:meta])*
$vis:vis fn $name:ident(&self $(, $arg:ident : $arg_ty:ty)*) -> $ret:ty
) => {
$(#[$meta])*
$vis fn $name(&self $(, $arg: $arg_ty)*) -> $ret {
match self {
Self::Real(op) => op.$name($($arg),*),
Self::Pseudo(op) => op.$name($($arg),*),
Self::Real(v) => v.$name($($arg),*),
Self::Pseudo(v) => v.$name($($arg),*),
}
}
};
}

impl InstructionMetadata for AnyInstruction {
inst_either!(fn label_arg(&self) -> Option<Arg<Label>>);
either_real_pseudo!(fn label_arg(&self) -> Option<Arg<Label>>);

inst_either!(fn is_unconditional_jump(&self) -> bool);
either_real_pseudo!(fn is_unconditional_jump(&self) -> bool);

inst_either!(fn is_scope_exit(&self) -> bool);
either_real_pseudo!(fn is_scope_exit(&self) -> bool);

inst_either!(fn stack_effect(&self, oparg: u32) -> i32);
either_real_pseudo!(fn stack_effect(&self, oparg: u32) -> i32);

inst_either!(fn stack_effect_jump(&self, oparg: u32) -> i32);
either_real_pseudo!(fn stack_effect_jump(&self, oparg: u32) -> i32);

inst_either!(fn stack_effect_info(&self, oparg: u32) -> StackEffect);
either_real_pseudo!(fn stack_effect_info(&self, oparg: u32) -> StackEffect);
}

impl AnyInstruction {
Expand Down Expand Up @@ -1272,11 +1434,10 @@ impl AnyInstruction {
matches!(self, Self::Pseudo(PseudoInstruction::PopBlock))
}

/// See [`PseudoInstruction::is_block_push`].
either_real_pseudo!(
#[must_use]
pub const fn is_block_push(self) -> bool {
matches!(self, Self::Pseudo(p) if p.is_block_push())
}
pub const fn is_block_push(&self) -> bool
);
}

#[derive(Clone, Copy, Debug)]
Expand Down Expand Up @@ -1365,6 +1526,36 @@ impl AnyOpcode {
self.pseudo()
.expect("Expected AnyOpcode::Pseudo, found AnyOpcode::Real")
}

either_real_pseudo!(
#[must_use]
pub const fn has_const(&self) -> bool
);

either_real_pseudo!(
#[must_use]
pub const fn has_name(&self) -> bool
);

either_real_pseudo!(
#[must_use]
pub const fn has_jump(&self) -> bool
);

either_real_pseudo!(
#[must_use]
pub const fn has_free(&self) -> bool
);

either_real_pseudo!(
#[must_use]
pub const fn has_local(&self) -> bool
);

either_real_pseudo!(
#[must_use]
pub const fn is_block_push(&self) -> bool
);
}

/// What effect the instruction has on the stack.
Expand Down
Loading
Loading