Update `opcode` from 3.13.7 by ShaharNaveh · Pull Request #6156 · RustPython/RustPython · GitHub

No description provided.

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches 🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

where the magic numbers came from? will this be changed if we change instruction enum?

Oh, I just ran:

import opcode

for name in (
    "hasarg",
    "hasconst",
    "hasname",
    "hasjump",
    "hasjrel",
    "hasjabs",
    "hasfree",
    "haslocal",
    "hasexc",
):
    lst = getattr(opcode, name)
    print(f"{name}\n{lst}")

And copied the results xD

But it's calculated based on https://github.com/python/cpython/blob/bcee1c322115c581da27600f2ae55e5439c027eb/Include/internal/pycore_opcode_metadata.h#L966-L1190

I've put a comment at https://github.com/ShaharNaveh/RustPython/blob/a7b9356da2a85cbc02df1f0fac9eda6a26869b6d/stdlib/src/opcode.rs#L46-L47 stating this:)

We should have our own metadata table, but I think that's a bit out of scope for this PR

At

RustPython/compiler/core/src/bytecode.rs

Lines 534 to 541 in aa0eb4b

    
           #[derive(Debug, Copy, Clone, PartialEq, Eq)] 
        
           #[repr(u8)] 
        
           pub enum Instruction { 
        
               Nop, 
        
               /// Importing by name 
        
               ImportName { 
        
                   idx: Arg<NameIdx>, 
        
               },

I tried doing something like:

 #[repr(u8)] 
 pub enum Instruction { 
     Nop = 30, 
     ImportName { 
         idx: Arg<NameIdx>, 
     } = 75, 
     ...
}

Based on:

But this change alone was enough to break RustPython entirely.

@youknowone can you please point me to where any of:

RustPython/compiler/core/src/bytecode.rs

Lines 796 to 815 in aa0eb4b

    
           impl From<Instruction> for u8 { 
        
               #[inline] 
        
               fn from(ins: Instruction) -> Self { 
        
                   // SAFETY: there's no padding bits 
        
                   unsafe { std::mem::transmute::<Instruction, Self>(ins) } 
        
               } 
        
           } 
        
           impl TryFrom<u8> for Instruction { 
        
               type Error = crate::marshal::MarshalError; 
        
               #[inline] 
        
               fn try_from(value: u8) -> Result<Self, crate::marshal::MarshalError> { 
        
                   if value <= u8::from(LAST_INSTRUCTION) { 
        
                       Ok(unsafe { std::mem::transmute::<u8, Self>(value) }) 
        
                   } else { 
        
                       Err(crate::marshal::MarshalError::InvalidBytecode) 
        
                   } 
        
               } 
        
           }

are being called? I've tried looking into it and got a bit lost

Thank you for explanation. Since we occasionally break bytecode compatibility, the numbers need to be regenerated.
Could you also attach the updating code around enum Instruction not to let it be desynced?

The From<Instruction> part is used form marshal. cc @coolreader18

Since we occasionally break bytecode compatibility

Is there a reason for that? or it's something that's desired but not implemented yet?

the numbers need to be regenerated. Could you also attach the updating code around enum Instruction not to let it be desynced?

I haven't written any code to do that (yet, but was planning to). As I just changed those two enum members and I couldn't even do cargo run :/

The From<Instruction> part is used form marshal. cc @coolreader18

ty:) will look into it.

@youknowone I have opened #6174 so we can discuss the next steps there and to not block this PR

Is there a reason for that? or it's something that's desired but not implemented yet?

No specific reason. We just add or remove instructions by following CPython update. The order changing is unintended side effect. Python doesn't guarantee bytecode compatibiltity anyway.

Does giving specific numbers to each instruction make it easier to be maintained? I guess so.

After #6174, then can we rewrite 63 | 66 to Instruction::Abc as u8 | Instruction::Def as u8 and so on?

finally we get _opcode, great!

I didn't know it can be over 256, huh

Those are pseudo opcodes:)
Real opcodes can't be more than 256

Is there a reason for that? or it's something that's desired but not implemented yet?

No specific reason. We just add or remove instructions by following CPython update. The order changing is unintended side effect. Python doesn't guarantee bytecode compatibiltity anyway.

Does giving specific numbers to each instruction make it easier to be maintained? I guess so.

After #6174, then can we rewrite 63 | 66 to Instruction::Abc as u8 | Instruction::Def as u8 and so on?

For some reason I can't comment on the thread of #6156 (comment) so answering it here:

Does giving specific numbers to each instruction make it easier to be maintained? I guess so.

Yes! it also makes it easier for writing the helpers of:

is_valid
has_arg
has_const
has_name
etc...

but that wouldn't be needed as I intend #6174 to generate those automatically (just like CPython does it)

After #6174, then can we rewrite 63 | 66 to Instruction::Abc as u8 | Instruction::Def as u8 and so on?

Well, yes you could. but I thought that we could have an API that looks similar to this:

// This is inside the `_opcode` module
fn has_arg(opcode: i32) -> bool {
  Instruction::try_from(opcode).map_or(false, |v| !v.is_pseudo() && v.has_arg())
}

where:

Instruction::try_from
instruction.is_pseudo()
instruction.has_arg()

are all auto generated.

Looks good in general.

I am worrying about the newly introduced magic numbers. I wish it could be quickly resolved in next patch

I am worrying about the newly introduced magic numbers. I wish it could be quickly resolved in next patch

I'll send a quick patch to resolve it in the next couple of days

ShaharNaveh added 10 commits September 17, 2025 10:53

Update opcode from 3.13.7

fb44bc9

Base _opcode

f14e623

Add test__opcode.py from 3.13.7

12aea60

Impl has_* methods

16b4eda

Add more methods

7fe1032

Update dis.py from 3.13.7

b5b7534

Update support/bytecode_helper.py from 3.13.7

45ab229

correct is_valid

4224563

Patch failing tests

af92d55

Unpatch support/__init__.py

aa02f02

ShaharNaveh added 6 commits September 18, 2025 01:55

clippy

f629c7f

Make comments to doc

56a0edb

impl _varname_from_oparg for code

6b3c578

Unmark passing tests

2634c5f

Revert changes to dis

98abe59

Mark failing tests

a7b9356

ShaharNaveh changed the title Impl _opcode. Update dis from 3.13.7 Update opcode from 3.13.7 Sep 18, 2025

ShaharNaveh marked this pull request as ready for review September 18, 2025 14:53

ShaharNaveh requested a review from youknowone September 18, 2025 14:53

youknowone reviewed Sep 20, 2025

View reviewed changes

ShaharNaveh mentioned this pull request Sep 23, 2025

Opcode metadata auto generation #6174

Closed

youknowone reviewed Sep 25, 2025

View reviewed changes

youknowone approved these changes Oct 5, 2025

View reviewed changes

youknowone merged commit 3a6fda4 into RustPython:main Oct 5, 2025
11 checks passed

youknowone mentioned this pull request Dec 4, 2025

Update Python libraries and tests from Python 3.13 #5529

Closed

+                  'INSTRUMENTED_POP_JUMP_IF_FALSE': 251,
+                  'INSTRUMENTED_POP_JUMP_IF_NONE': 252,
+                  'INSTRUMENTED_POP_JUMP_IF_NOT_NONE': 253,
+                  'JUMP': 256,

+                          Self::is_valid(opcode)
+                              && matches!(
+                                  opcode,
+| 66

	#[derive(Debug, Copy, Clone, PartialEq, Eq)]
	#[repr(u8)]
	pub enum Instruction {
	Nop,
	/// Importing by name
	ImportName {
	idx: Arg<NameIdx>,
	},

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

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

	#[inline]
	fn try_from(value: u8) -> Result<Self, crate::marshal::MarshalError> {
	if value <= u8::from(LAST_INSTRUCTION) {
	Ok(unsafe { std::mem::transmute::<u8, Self>(value) })
	} else {
	Err(crate::marshal::MarshalError::InvalidBytecode)
	}
	}
	}

Update `opcode` from 3.13.7 by ShaharNaveh · Pull Request #6156 · RustPython/RustPython · GitHub

Uh oh!

Update opcode from 3.13.7 - #6156

Update opcode from 3.13.7#6156youknowone merged 16 commits intoRustPython:mainRustPython/RustPython:mainfrom ShaharNaveh:update-opcodeShaharNaveh/RustPython:update-opcodeCopy head branch name to clipboard

Conversation

ShaharNaveh commented Sep 17, 2025

Uh oh!

coderabbitai Bot commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone commented Sep 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ShaharNaveh commented Oct 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Update opcode from 3.13.7#6156
youknowone merged 16 commits into
RustPython:mainfrom
ShaharNaveh:update-opcode

coderabbitai Bot commented Sep 17, 2025 •
edited
Loading

ShaharNaveh Sep 22, 2025 •
edited
Loading

ShaharNaveh commented Sep 25, 2025 •
edited
Loading