Remove BEFORE_WITH,BINARY_SUBSCR #6822
Merged
+264
−212
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThis PR aligns RustPython's bytecode with CPython 3.14 by removing custom opcodes (BeforeWith, BeforeAsyncWith, BinarySubscr, BuildConstKeyMap), introducing a SpecialMethod enum for explicit context-manager method resolution, and unifying subscripting into BinaryOperator::Subscr across compilation and runtime execution. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes The changes span multiple compilation pipeline layers with significant structural modifications: removal of public opcodes, introduction of new enum types, updates to codegen emission patterns, and corresponding runtime execution changes. Verification requires checking consistency across codegen, bytecode representation, and VM execution paths. Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: Click to expand dependency informationLegend:
|
ShaharNaveh
reviewed
Jan 20, 2026
| }), | ||
| ]; | ||
| // No RustPython-only opcodes anymore - all opcodes match CPython 3.14 | ||
| let custom_ops: &[u8] = &[]; |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/frame.rs`:
- Around line 1234-1267: Update the user-facing TypeError message in the
Instruction::LoadSpecial handling so it uses "missing" instead of "missed":
locate the vm.new_type_error call inside the LoadSpecial match arm (in frame
handling where get_special_method returns None) and change the message string to
"'{}' object does not support the context manager protocol (missing {} method)"
to correct the grammar.
Comment on lines
+1234
to
+1267
| Instruction::LoadSpecial { method } => { | ||
| // Stack effect: 0 (replaces TOS with bound method) | ||
| // Input: [..., obj] | ||
| // Output: [..., bound_method] | ||
| use crate::vm::PyMethod; | ||
| use bytecode::SpecialMethod; | ||
|
|
||
| let obj = self.pop_value(); | ||
| let method_name = match method.get(arg) { | ||
| SpecialMethod::Enter => identifier!(vm, __enter__), | ||
| SpecialMethod::Exit => identifier!(vm, __exit__), | ||
| SpecialMethod::AEnter => identifier!(vm, __aenter__), | ||
| SpecialMethod::AExit => identifier!(vm, __aexit__), | ||
| }; | ||
|
|
||
| let bound = match vm.get_special_method(&obj, method_name)? { | ||
| Some(PyMethod::Function { target, func }) => { | ||
| // Create bound method: PyBoundMethod(object=target, function=func) | ||
| crate::builtins::PyBoundMethod::new(target, func) | ||
| .into_ref(&vm.ctx) | ||
| .into() | ||
| } | ||
| Some(PyMethod::Attribute(bound)) => bound, | ||
| None => { | ||
| return Err(vm.new_type_error(format!( | ||
| "'{}' object does not support the context manager protocol (missed {} method)", | ||
| obj.class().name(), | ||
| method_name | ||
| ))); | ||
| } | ||
| }; | ||
| self.push_value(bound); | ||
| Ok(None) | ||
| } |
Contributor
There was a problem hiding this comment.
Minor: Fix grammatical error in error message.
The error message uses "missed" (past tense verb) instead of "missing" (adjective). This is a minor user-facing text quality issue.
📝 Suggested fix
None => {
return Err(vm.new_type_error(format!(
- "'{}' object does not support the context manager protocol (missed {} method)",
+ "'{}' object does not support the context manager protocol (missing {} method)",
obj.class().name(),
method_name
)));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Instruction::LoadSpecial { method } => { | |
| // Stack effect: 0 (replaces TOS with bound method) | |
| // Input: [..., obj] | |
| // Output: [..., bound_method] | |
| use crate::vm::PyMethod; | |
| use bytecode::SpecialMethod; | |
| let obj = self.pop_value(); | |
| let method_name = match method.get(arg) { | |
| SpecialMethod::Enter => identifier!(vm, __enter__), | |
| SpecialMethod::Exit => identifier!(vm, __exit__), | |
| SpecialMethod::AEnter => identifier!(vm, __aenter__), | |
| SpecialMethod::AExit => identifier!(vm, __aexit__), | |
| }; | |
| let bound = match vm.get_special_method(&obj, method_name)? { | |
| Some(PyMethod::Function { target, func }) => { | |
| // Create bound method: PyBoundMethod(object=target, function=func) | |
| crate::builtins::PyBoundMethod::new(target, func) | |
| .into_ref(&vm.ctx) | |
| .into() | |
| } | |
| Some(PyMethod::Attribute(bound)) => bound, | |
| None => { | |
| return Err(vm.new_type_error(format!( | |
| "'{}' object does not support the context manager protocol (missed {} method)", | |
| obj.class().name(), | |
| method_name | |
| ))); | |
| } | |
| }; | |
| self.push_value(bound); | |
| Ok(None) | |
| } | |
| Instruction::LoadSpecial { method } => { | |
| // Stack effect: 0 (replaces TOS with bound method) | |
| // Input: [..., obj] | |
| // Output: [..., bound_method] | |
| use crate::vm::PyMethod; | |
| use bytecode::SpecialMethod; | |
| let obj = self.pop_value(); | |
| let method_name = match method.get(arg) { | |
| SpecialMethod::Enter => identifier!(vm, __enter__), | |
| SpecialMethod::Exit => identifier!(vm, __exit__), | |
| SpecialMethod::AEnter => identifier!(vm, __aenter__), | |
| SpecialMethod::AExit => identifier!(vm, __aexit__), | |
| }; | |
| let bound = match vm.get_special_method(&obj, method_name)? { | |
| Some(PyMethod::Function { target, func }) => { | |
| // Create bound method: PyBoundMethod(object=target, function=func) | |
| crate::builtins::PyBoundMethod::new(target, func) | |
| .into_ref(&vm.ctx) | |
| .into() | |
| } | |
| Some(PyMethod::Attribute(bound)) => bound, | |
| None => { | |
| return Err(vm.new_type_error(format!( | |
| "'{}' object does not support the context manager protocol (missing {} method)", | |
| obj.class().name(), | |
| method_name | |
| ))); | |
| } | |
| }; | |
| self.push_value(bound); | |
| Ok(None) | |
| } |
🤖 Prompt for AI Agents
In `@crates/vm/src/frame.rs` around lines 1234 - 1267, Update the user-facing
TypeError message in the Instruction::LoadSpecial handling so it uses "missing"
instead of "missed": locate the vm.new_type_error call inside the LoadSpecial
match arm (in frame handling where get_special_method returns None) and change
the message string to "'{}' object does not support the context manager protocol
(missing {} method)" to correct the grammar.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.