Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Fix
  • Loading branch information
ShaharNaveh committed Jan 9, 2026
commit 6cdf6f50e2f29fadccf487d076bb85dcdbd2e411
44 changes: 28 additions & 16 deletions crates/compiler-core/src/bytecode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -959,32 +959,42 @@ pub enum Instruction {

const _: () = assert!(mem::size_of::<Instruction>() == 1);

impl From<Instruction> for u8 {
#[inline]
fn from(ins: Instruction) -> Self {
// SAFETY: there's no padding bits
unsafe { core::mem::transmute::<Instruction, Self>(ins) }
}
}

impl TryFrom<u8> for Instruction {
type Error = MarshalError;

#[inline]
fn try_from(value: u8) -> Result<Self, MarshalError> {
let cpython_start = Instruction::Cache as u8;
let cpython_end = Instruction::YieldValue as u8;
let cpython_start = u8::from(Self::Cache);
let cpython_end = u8::from(Self::YieldValue { arg: Arg::marker() });

let resume_id = Instruction::Resume { arg: Arg::marker() } as u8;
let resume_id = u8::from(Self::Resume { arg: Arg::marker() });

let custom_start = Instruction::Break {
let custom_start = u8::from(Self::Break {
target: Arg::marker(),
} as u8;
let custom_end = Instruction::Subscript as u8;
});
let custom_end = u8::from(Self::Subscript);

let pseudo_start = Instruction::Jump {
let pseudo_start = u8::from(Self::Jump {
target: Arg::marker(),
} as u8;
let pseudo_end = Instruction::PopBlock as u8;

match value {
cpython_start..=cpython_end
| resume_id
| custom_start..=custom_end
| pseudo_start..=pseudo_end => Ok(unsafe { core::mem::transmute::<u8, Self>(value) }),
_ => Err(Self::Error::InvalidBytecode),
});
let pseudo_end = u8::from(Self::PopBlock);

if (cpython_start..=cpython_end).contains(&value)
|| value == resume_id
|| (custom_start..=custom_end).contains(&value)
|| (pseudo_start..=pseudo_end).contains(&value)
{
Ok(unsafe { core::mem::transmute::<u8, Self>(value) })
} else {
Err(Self::Error::InvalidBytecode)
}
}
}
Expand Down Expand Up @@ -1899,6 +1909,7 @@ impl Instruction {
UnaryNegative => 0,
UnaryNot => 0,
GetYieldFromIter => 0,
_ => unreachable!("Tried to get stack effect of a placeholder instruction ({self:?}"),
}
}

Expand Down Expand Up @@ -2087,6 +2098,7 @@ impl Instruction {
UnaryNot => w!(UNARY_NOT),
YieldValue { arg } => w!(YIELD_VALUE, arg),
GetYieldFromIter => w!(GET_YIELD_FROM_ITER),
_ => w!(RUSTPYTHON_PLACEHOLDER),
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/vm/src/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1779,7 +1779,7 @@ impl ExecutingFrame<'_> {
self.push_value(vm.ctx.new_bool(!value).into());
Ok(None)
}
bytecode::Instruction::Reserved => {
_ => {
unreachable!("{instruction:?} instruction should not be executed")
}
Comment on lines +1782 to 1784

@coderabbitai coderabbitai Bot Jan 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's see the context around lines 1782-1784 in frame.rs
head -1790 crates/vm/src/frame.rs | tail -20

Repository: RustPython/RustPython

Length of output: 732


🏁 Script executed:

# Find the Instruction enum definition
fd -t f "bytecode.rs" | head -5

Repository: RustPython/RustPython

Length of output: 144


🏁 Script executed:

# Let's also check if there's an instruction definition in frame.rs itself
rg "enum Instruction" crates/vm/src/ -A 20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Find and read the Instruction enum definition
rg "pub enum Instruction" crates/compiler-core/src/bytecode.rs -A 100

Repository: RustPython/RustPython

Length of output: 2516


🏁 Script executed:

# Get more context around lines 1782-1784 to see the full match statement
sed -n '1700,1790p' crates/vm/src/frame.rs | head -100

Repository: RustPython/RustPython

Length of output: 4048


🏁 Script executed:

# Find the beginning of the match statement that contains lines 1782-1784
# Start from line 1782 and go backwards to find the match statement
sed -n '1600,1782p' crates/vm/src/frame.rs | tail -200

Repository: RustPython/RustPython

Length of output: 8457


🏁 Script executed:

# Search for the function name that contains this code
sed -n '1650,1782p' crates/vm/src/frame.rs | grep -E "fn |match " | head -20

Repository: RustPython/RustPython

Length of output: 119


🏁 Script executed:

# Find the function definition containing this match statement
sed -n '1400,1700p' crates/vm/src/frame.rs | grep -B 50 "match.*instruction" | head -60

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Let's search for the execute or dispatch function that contains this match
rg "fn execute" crates/vm/src/frame.rs -A 5 | head -40

Repository: RustPython/RustPython

Length of output: 1542


🏁 Script executed:

# Find the beginning of execute_instruction function
rg "fn execute_instruction" crates/vm/src/frame.rs -A 500 | head -100

Repository: RustPython/RustPython

Length of output: 4067


🏁 Script executed:

# Count the number of instruction variants and see if the match is exhaustive
rg "bytecode::Instruction::" crates/vm/src/frame.rs | wc -l

Repository: RustPython/RustPython

Length of output: 68


🏁 Script executed:

# Check if there are placeholder instructions and how they're handled
rg "Placeholder" crates/compiler-core/src/bytecode.rs | head -20

Repository: RustPython/RustPython

Length of output: 788


🏁 Script executed:

# Look for comments around the catch-all unreachable to understand its intent
sed -n '1770,1790p' crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 786


🏁 Script executed:

# Get the full list of instruction variants from the enum
rg "^\s+([\w:]+)\s*=" crates/compiler-core/src/bytecode.rs | grep -E "pub enum Instruction" -A 200 | grep "=" | wc -l

Repository: RustPython/RustPython

Length of output: 66


🏁 Script executed:

# Check if Rust compiler would complain about non-exhaustive match
# Look for warnings about unhandled variants
rg "unreachable" crates/vm/src/frame.rs -B 5 -A 2

Repository: RustPython/RustPython

Length of output: 344


🏁 Script executed:

# Let's verify if the Instruction enum is marked as exhaustive
rg "enum Instruction" crates/compiler-core/src/bytecode.rs -B 2 -A 1

Repository: RustPython/RustPython

Length of output: 232


🏁 Script executed:

# Get the complete list of all instruction variants from the enum
rg "pub enum Instruction" crates/compiler-core/src/bytecode.rs -A 500 | grep -E "^\s+\w+.*=" | sed 's/.*Instruction:://' | sed 's/\s*=.*//' | sort > /tmp/enum_variants.txt

# Get all the instruction variants handled in execute_instruction
rg "bytecode::Instruction::" crates/vm/src/frame.rs | sed 's/.*bytecode::Instruction:://' | sed 's/[^a-zA-Z_].*//' | sort | uniq > /tmp/handled_variants.txt

# Compare the two
echo "=== Variants in enum but NOT handled (should be empty or only placeholders) ==="
comm -23 /tmp/enum_variants.txt /tmp/handled_variants.txt

echo ""
echo "=== Total enum variants ==="
wc -l /tmp/enum_variants.txt

echo ""
echo "=== Total handled variants ==="
wc -l /tmp/handled_variants.txt

Repository: RustPython/RustPython

Length of output: 2554


🏁 Script executed:

# Let me get a cleaner list of just the variant names from the enum
rg "pub enum Instruction" crates/compiler-core/src/bytecode.rs -A 500 | grep -E "^\s+[A-Z][a-zA-Z0-9]*(\s*{|$|\()" | awk '{print $1}' | sort | uniq > /tmp/enum_clean.txt

# Get handled variants more carefully
rg "Instruction::[A-Z]" crates/vm/src/frame.rs -o | sed 's/Instruction:://' | sort | uniq > /tmp/handled_clean.txt

echo "=== Unhandled instruction variants ==="
comm -23 /tmp/enum_clean.txt /tmp/handled_clean.txt

echo ""
echo "=== Count ==="
echo "Enum variants: $(wc -l < /tmp/enum_clean.txt)"
echo "Handled variants: $(wc -l < /tmp/handled_clean.txt)"

Repository: RustPython/RustPython

Length of output: 1752


🏁 Script executed:

# Let's also check if there are any placeholder instructions that don't have handlers
rg "// Placeholder" crates/compiler-core/src/bytecode.rs | grep -oE "[A-Z][a-zA-Z0-9]*.*Placeholder" | sed 's/ .*//' > /tmp/placeholders.txt

echo "=== Placeholder instructions ==="
cat /tmp/placeholders.txt

Repository: RustPython/RustPython

Length of output: 379


🏁 Script executed:

# Let me extract instruction handlers more carefully by looking at the actual match arms
rg "bytecode::Instruction::" crates/vm/src/frame.rs | head -100 | sed 's/.*Instruction:://' | sed 's/\s*=>.*//' | sed 's/\s*{.*//' | awk '{print $1}' | sort | uniq

Repository: RustPython/RustPython

Length of output: 1286


🏁 Script executed:

# Count how many match arms are in execute_instruction
rg "bytecode::Instruction::" crates/vm/src/frame.rs | wc -l

Repository: RustPython/RustPython

Length of output: 68


🏁 Script executed:

# Check if there are explicit handlers for the placeholder instructions
rg "Instruction::(Cache|BinaryOpInplaceAddUnicode|BinarySlice|EndFor|ExitInitCheck|InterpreterExit|LoadAssertionError|LoadLocals|PushNull|ReturnGenerator|StoreSlice)" crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Let's verify the total count of instruction variants more accurately
# by extracting just the variant names from the enum definition
sed -n '/pub enum Instruction/,/^}/p' crates/compiler-core/src/bytecode.rs | grep -E '^\s+[A-Z][a-zA-Z0-9]*\s*(\{|=|\()' | wc -l

Repository: RustPython/RustPython

Length of output: 68


🏁 Script executed:

# Check the structure of the enum more carefully
sed -n '/pub enum Instruction/,/^}/p' crates/compiler-core/src/bytecode.rs | tail -50

Repository: RustPython/RustPython

Length of output: 1280


🏁 Script executed:

# Let's verify if Rust's exhaustiveness checking would work here
# by checking if the enum has any special attributes
rg "#\[.*\]" crates/compiler-core/src/bytecode.rs | grep -B 5 "pub enum Instruction" | head -20

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Let's get the exact list of unhandled variants by comparing the handled list with enum
sed -n '/pub enum Instruction/,/^}/p' crates/compiler-core/src/bytecode.rs | grep -E '^\s+[A-Z]' | awk '{print $1}' | sort > /tmp/all_variants.txt

# Get all handled variants from execute_instruction
rg "bytecode::Instruction::" crates/vm/src/frame.rs | sed 's/.*Instruction:://' | sed 's/\s*=>.*//' | sed 's/\s*{.*//' | sed 's/\s*(.*//' | awk '{print $1}' | sort | uniq > /tmp/handled.txt

# Find variants that are not handled
echo "=== Variants NOT explicitly handled ==="
comm -23 /tmp/all_variants.txt /tmp/handled.txt

echo ""
echo "=== Are these all placeholders? ==="
# Check which of the unhandled are marked as placeholders
sed -n '/pub enum Instruction/,/^}/p' crates/compiler-core/src/bytecode.rs | grep -B 1 "Placeholder" | grep -E "^\s+[A-Z]" | awk '{print $1}' | sort > /tmp/placeholders_list.txt
cat /tmp/placeholders_list.txt

Repository: RustPython/RustPython

Length of output: 1504


🏁 Script executed:

# Let's verify that these "unhandled" variants are actually being rejected
# by checking if there's any handling outside of execute_instruction
rg "LoadFast|LoadDeref|StoreFast|StoreDeref|DeleteFast" crates/vm/src/frame.rs | head -20

Repository: RustPython/RustPython

Length of output: 544


🏁 Script executed:

# Check if there's pattern matching on the argument or other routing logic
# that might handle these variants separately
sed -n '1200,1400p' crates/vm/src/frame.rs | grep -E "match|fn " | head -30

Repository: RustPython/RustPython

Length of output: 2371


🏁 Script executed:

# Let's look at the actual match statement more carefully
# and search for specific instruction handlers
sed -n '1300,1500p' crates/vm/src/frame.rs | grep -A 3 "LoadFast\|StoreFast\|DeleteFast"

Repository: RustPython/RustPython

Length of output: 47


🏁 Script executed:

# Let's see if the unhandled variants are indeed in the match statement
# by checking the line count and structure more carefully
rg "bytecode::Instruction::" crates/vm/src/frame.rs | grep -c "=> {"

Repository: RustPython/RustPython

Length of output: 67


🏁 Script executed:

# Get a comprehensive list of what's actually being matched
sed -n '/fn execute_instruction/,/^    fn /p' crates/vm/src/frame.rs | grep "bytecode::Instruction::" | sed 's/.*Instruction:://' | sed 's/\s*=>.*//' | sed 's/\s*{.*//' | awk '{print $1}' | sort | uniq | wc -l

Repository: RustPython/RustPython

Length of output: 68


🏁 Script executed:

# Let's verify the exact list of handled variants with a more precise approach
sed -n '/fn execute_instruction/,/^    fn [a-z]/p' crates/vm/src/frame.rs | grep "bytecode::Instruction::" | sed 's/.*bytecode::Instruction:://' | sed 's/\s*[=({].*//' | sort | uniq > /tmp/actually_handled.txt

cat /tmp/actually_handled.txt | wc -l

echo ""
echo "=== Sample of handled variants ==="
head -20 /tmp/actually_handled.txt

Repository: RustPython/RustPython

Length of output: 366


🏁 Script executed:

# Check if these unhandled variants are truly all placeholders or only used variants
comm -23 /tmp/all_variants.txt /tmp/actually_handled.txt | while read var; do
  grep -q "$var.*Placeholder" <(sed -n '/pub enum Instruction/,/^}/p' crates/compiler-core/src/bytecode.rs) && echo "$var: PLACEHOLDER" || echo "$var: NOT PLACEHOLDER"
done

Repository: RustPython/RustPython

Length of output: 4028


🏁 Script executed:

# Let's verify if the key instructions like LoadFast are actually being handled
# by checking the actual match arms
rg "Instruction::LoadFast\(" crates/vm/src/frame.rs

Repository: RustPython/RustPython

Length of output: 118


🏁 Script executed:

# Let's count the total lines of the match statement to understand its scope
sed -n '/fn execute_instruction/,/^    fn /p' crates/vm/src/frame.rs | wc -l

Repository: RustPython/RustPython

Length of output: 69


🏁 Script executed:

# Let's check the end of execute_instruction to see the catch-all
sed -n '/fn execute_instruction/,/^    fn /p' crates/vm/src/frame.rs | tail -20

Repository: RustPython/RustPython

Length of output: 765


Add compile-time exhaustiveness checking for new instruction variants.

The catch-all pattern with unreachable!() disables Rust's exhaustiveness checking. If new Instruction variants are added to the enum without corresponding match arms here, they will panic at runtime rather than fail at compile time. Consider restructuring to leverage the compiler's exhaustiveness checking—for example, by removing the catch-all if all variants are already handled, or by using a #[non_exhaustive] attribute on the enum to make missing handlers a compile error.

🤖 Prompt for AI Agents
In @crates/vm/src/frame.rs around lines 1782 - 1784, The match currently uses a
catch-all arm with unreachable!() for the Instruction enum which defeats
compile-time exhaustiveness checking; replace the wildcard arm by explicitly
matching every Instruction variant used in this match so the compiler will error
if new variants are added (i.e., remove the `_ => unreachable!()` arm in the
match on `instruction` and add explicit arms for each `Instruction::<Variant>`
handled here), or if the enum is intended to be extensible annotate the enum
definition with `#[non_exhaustive]` and then handle the `_` case with a
documented conversion/error path; ensure you update the match in the
function/method in frame.rs where `instruction` is matched so future additions
to `Instruction` cause a compile-time failure instead of a runtime panic.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ShaharNaveh uuuuh, this is totally right. I immediately broken build by forgetting change arm during merge. listing all is better.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

}
Expand Down