GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Remove BEFORE_WITH,BINARY_SUBSCR by youknowone · Pull Request #6822 · RustPython/RustPython Conversation
|
📝 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: Click to expand dependency informationLegend:
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
| }), | ||
| ]; | ||
| // No RustPython-only opcodes anymore - all opcodes match CPython 3.14 | ||
| let custom_ops: &[u8] = &[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
😄 1 reaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.