NotificationsYou must be signed in to change notification settings
Fork1.4k
Star20.8k

Update`opcode` from 3.13.7#6156

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

youknowone merged 16 commits intoRustPython:mainfromShaharNaveh:update-opcode

Oct 5, 2025

Merged

Update`opcode` from 3.13.7#6156

youknowone merged 16 commits intoRustPython:mainfromShaharNaveh:update-opcode

Oct 5, 2025

Conversation

Copy link

Collaborator

ShaharNaveh commentedSep 17, 2025

No description provided.

ShaharNaveh added10 commits

September 17, 2025 10:53

Updateopcode from 3.13.7

fb44bc9

Base_opcode

f14e623

Addtest__opcode.py from 3.13.7

12aea60

Implhas_* methods

16b4eda

Add more methods

7fe1032

Updatedis.py from 3.13.7

b5b7534

Updatesupport/bytecode_helper.py from 3.13.7

45ab229

correct is_valid

4224563

Patch failing tests

af92d55

Unpatchsupport/__init__.py

aa02f02

Copy link

Contributor

coderabbitaibot commentedSep 17, 2025•
edited
Loading

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 thereviews.review_status tofalse 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.}

ShaharNaveh added6 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 todis

98abe59

Mark failing tests

a7b9356

ShaharNaveh changed the title~~Impl_opcode. Updatedis from 3.13.7~~Updateopcode 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 fromyouknowone

September 18, 2025 14:53

youknowone reviewed

Sep 20, 2025

View reviewed changes

stdlib/src/opcode.rs

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

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

CollaboratorAuthor

Choose a reason for hiding this comment

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

Oh, I just ran:

importopcodefornamein ("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 onhttps://github.com/python/cpython/blob/bcee1c322115c581da27600f2ae55e5439c027eb/Include/internal/pycore_opcode_metadata.h#L966-L1190

I've put a comment athttps://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

Copy link

CollaboratorAuthor

Choose a reason for hiding this comment

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

RustPython/compiler/core/src/bytecode.rs

Lines 534 to 541 inaa0eb4b

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

I tried doing something like:

#[repr(u8)]pubenumInstruction{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 inaa0eb4b

	implFrom<Instruction>foru8{
	#[inline]
	fnfrom(ins:Instruction) ->Self{
	// SAFETY: there's no padding bits
	unsafe{ std::mem::transmute::<Instruction,Self>(ins)}
	}
	}

	implTryFrom<u8>forInstruction{
	typeError =crate::marshal::MarshalError;

	#[inline]
	fntry_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

Copy link

Member

Choose a reason for hiding this comment

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

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

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

Copy link

CollaboratorAuthor

ShaharNavehSep 22, 2025•
edited
Loading

Choose a reason for hiding this comment

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

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 aroundenum 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 docargo run :/

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

ty:) will look into it.

Copy link

CollaboratorAuthor

ShaharNavehSep 23, 2025

Choose a reason for hiding this comment

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

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

Copy link

Member

youknowoneSep 24, 2025

Choose a reason for hiding this comment

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

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 rewrite63 | 66 toInstruction::Abc as u8 | Instruction::Def as u8 and so on?

Copy link

Member

youknowone commentedSep 20, 2025

finally we get_opcode, great!

ShaharNaveh mentioned this pull request

Sep 23, 2025

Opcode metadata auto generation#6174

Draft

youknowone reviewed

Sep 25, 2025

View reviewed changes

Lib/_opcode_metadata.py

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

Copy link

Member

youknowoneSep 24, 2025

Choose a reason for hiding this comment

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

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

Copy link

CollaboratorAuthor

ShaharNavehSep 25, 2025

Choose a reason for hiding this comment

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

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

stdlib/src/opcode.rs

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

Copy link

Member

youknowoneSep 24, 2025

Choose a reason for hiding this comment

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

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 rewrite63 | 66 toInstruction::Abc as u8 | Instruction::Def as u8 and so on?

Copy link

CollaboratorAuthor

ShaharNaveh commentedSep 25, 2025•
edited
Loading

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` modulefnhas_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.

youknowone approved these changes

Oct 5, 2025

View reviewed changes

Copy link

Member

youknowone left a comment

Choose a reason for hiding this comment

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

Looks good in general.

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

youknowone merged commit3a6fda4 intoRustPython:main

Oct 5, 2025

11 checks passed

Copy link

CollaboratorAuthor

ShaharNaveh commentedOct 5, 2025

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

Labels

None yet

Movatterモバイル変換

Updateopcode from 3.13.7#6156

Updateopcode from 3.13.7#6156

Uh oh!

Conversation

ShaharNaveh commentedSep 17, 2025

Uh oh!

coderabbitaibot commentedSep 17, 2025• editedLoading 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!

ShaharNavehSep 22, 2025• editedLoading 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 commentedSep 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 commentedSep 25, 2025• editedLoading 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 commentedOct 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Update`opcode` from 3.13.7#6156

Update`opcode` from 3.13.7#6156

coderabbitaibot commentedSep 17, 2025•
edited
Loading

ShaharNavehSep 22, 2025•
edited
Loading

ShaharNaveh commentedSep 25, 2025•
edited
Loading