compile_subscript #6008
compile_subscript #6008
Conversation
WalkthroughThis update centralizes subscript compilation logic in the compiler by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant Bytecode
participant VM
participant PyList
participant PyTuple
Compiler->>Bytecode: Emit CALL_INTRINSIC_1(ListToTuple)
VM->>VM: call_intrinsic_1(ListToTuple, arg)
VM->>PyList: Downcast arg to PyList
alt arg is PyList
VM->>PyTuple: Create tuple from PyList elements
VM-->>Compiler: Return PyTuple
else arg is not PyList
VM-->>Compiler: Raise TypeError("LIST_TO_TUPLE expects a list")
end
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
compiler/codegen/src/compile.rs (1)
420-422: Consider providing more context in error messageThe error message "compile_slice expects a slice expression" could be more helpful by including what was actually received.
- _ => Err(self.error(CodegenErrorType::SyntaxError( - "compile_slice expects a slice expression".to_owned(), - ))), + _ => Err(self.error(CodegenErrorType::SyntaxError( + format!("compile_slice expects a slice expression, got {:?}", slice.python_name()), + ))),
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
compiler/codegen/src/compile.rs(8 hunks)compiler/core/src/bytecode.rs(1 hunks)vm/src/frame.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (1)
compiler/codegen/src/compile.rs (1)
undefined
<retrieved_learning>
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
</retrieved_learning>
🔇 Additional comments (6)
compiler/core/src/bytecode.rs (1)
402-403: Approve ListToTuple Intrinsic ImplementationThe new
ListToTuple = 6variant is correctly defined and implemented:
- ✅ Clear documentation comment: “Convert list to tuple”
- ✅ Numeric value
6fits the existing sequence without conflicts- ✅ Naming follows Rust conventions
- ✅ VM match arm in
vm/src/frame.rs
- Properly downcasts non-list arguments and returns a type error
- Clones the underlying list’s vector to produce a new tuple (empty lists yield empty tuples)
No further changes required—everything looks solid!
compiler/codegen/src/compile.rs (5)
491-532: Well-structured helper for starred unpackingThe
starunpack_helpermethod effectively centralizes the logic for building collections with potential starred elements. The special case handling for single tuples (line 507) is correct - when there's only one starred element and nothing else pushed, the tuple is already on the stack.
1738-1742: Consistent migration to centralized subscript compilationAll subscript compilation calls have been successfully migrated to use the new
compile_subscriptmethod with appropriate contexts. This centralization improves maintainability and ensures consistent behavior across different expression contexts.Also applies to: 4106-4110, 4431-4435
4464-4472: Clean refactoring of collection compilationThe migration to use
starunpack_helperfor List, Tuple, and Set expressions significantly simplifies the code and unifies the handling of starred unpacking across collection types. This is a good example of DRY principle in action.
448-463: Appropriate special handling for two-element slicesThe optimization for two-element slices (slices without a step) is correctly implemented. The exclusion of the
Delcontext from this optimization is appropriate, as delete operations may have different semantics. The logic correctly builds slice objects and emits context-appropriate instructions.
438-443: Compile-time subscript checks can be omittedThe commented-out
check_subscripter/check_indexcalls refer to CPython’s runtime type‐ and bounds checking, but our compiler emitsBINARY_SUBSCR(and related opcodes) which defer those errors to runtime. There are no implementations of those helper functions in the repo, and the VM will correctly raiseTypeErrororIndexErroras needed. No changes required here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
compiler/codegen/src/compile.rs (1)
438-443: Consider implementing the commented CPython compatibility checks.The commented-out checks for
check_subscripterandcheck_indexin Load context might be important for CPython compatibility. Consider implementing these checks or documenting why they're not needed.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
compiler/codegen/src/compile.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (1)
compiler/codegen/src/compile.rs (1)
undefined
<retrieved_learning>
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (9)
compiler/codegen/src/compile.rs (9)
378-383: LGTM! Well-structured enum for collection types.The
CollectionTypeenum clearly represents the three collection types that support starred unpacking.
387-391: LGTM! Clear helper for detecting two-element slices.The method correctly identifies slices without a step component using pattern matching.
393-424: LGTM! Comprehensive slice compilation logic.The method correctly handles all slice components with proper None defaults for missing bounds and returns the stack count.
484-532: LGTM! Clean abstraction for starred unpacking.The helper effectively unifies the compilation of collection literals with potential starred expressions, correctly handling the special case for single-element starred tuples.
1738-1742: LGTM! Proper delegation to centralized subscript compilation.The change correctly uses the new
compile_subscriptmethod for delete operations.
4106-4110: LGTM! Consistent use of centralized subscript compilation.The store operation correctly delegates to
compile_subscript.
4190-4203: LGTM! Correct handling of augmented assignment special case.The code appropriately retains the original logic with a clear comment explaining why
compile_subscriptcannot be used due to the need forDUP_TOP2.
4431-4435: LGTM! Clean integration of centralized subscript compilation.The expression compilation correctly delegates to
compile_subscript.
4465-4472: LGTM! Excellent unification of collection literal compilation.The use of
starunpack_helperwith the appropriateCollectionTypeeliminates code duplication while maintaining type-specific behavior.
Summary by CodeRabbit
New Features
Bug Fixes