compile_subscript by youknowone · Pull Request #6008

compile_subscript by youknowone · Pull Request #6008 · RustPython/RustPython

youknowone commented
Jul 20, 2025
•

edited by coderabbitai bot

Loading

Summary by CodeRabbit

New Features
- Improved handling of subscript expressions, enabling more consistent and context-aware behavior for indexing and slicing.
- Enhanced support for starred unpacking in collection literals (tuples, lists, sets).
- Introduced support for converting lists to tuples via a new intrinsic function.
Bug Fixes
- Improved error reporting when attempting to convert non-list objects to tuples.

Walkthrough

This update centralizes subscript compilation logic in the compiler by introducing a compile_subscript method and refactors starred unpacking in collection literals into a helper function using a new CollectionType enum. It also adds a new intrinsic function, ListToTuple, to the bytecode and virtual machine, while removing or commenting out several older intrinsic variants.

Changes

File(s)	Change Summary
compiler/codegen/src/compile.rs	Added `compile_subscript`, `compile_slice`, and `starunpack_helper` methods; introduced `CollectionType` enum; refactored subscript and collection literal compilation logic.
compiler/core/src/bytecode.rs	Commented out several `IntrinsicFunction1` enum variants; added new `ListToTuple` variant.
vm/src/frame.rs	Implemented handling for the new `ListToTuple` intrinsic in `call_intrinsic_1`.

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

In the warren of code, subscripts now shine,
With helpers and enums, their logic aligns.
Lists turn to tuples, a hop and a leap,
Intrinsics updated, old ones asleep.
A rabbit’s delight—refactored and neat,
This patch makes the bytecode all the more sweet!
🐇✨

📜 Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9f54540 and 5efbe66.

📒 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)

🚧 Files skipped from review as they are similar to previous changes (3)

vm/src/frame.rs
compiler/core/src/bytecode.rs
compiler/codegen/src/compile.rs

⏰ 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 (macos-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Check Rust code with rustfmt and clippy
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Ensure compilation on various targets

✨ Finishing Touches

📝 Generate Docstrings

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

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)

compiler/codegen/src/compile.rs (1)

420-422: Consider providing more context in error message

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between c497061 and 7a03b28.

📒 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 Implementation

The new ListToTuple = 6 variant is correctly defined and implemented:

✅ Clear documentation comment: “Convert list to tuple”

✅ Numeric value 6 fits 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 unpacking

The starunpack_helper method 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 compilation

All subscript compilation calls have been successfully migrated to use the new compile_subscript method 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 compilation

The migration to use starunpack_helper for 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 slices

The optimization for two-element slices (slices without a step) is correctly implemented. The exclusion of the Del context 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 omitted

The commented-out check_subscripter/check_index calls refer to CPython’s runtime type‐ and bounds checking, but our compiler emits BINARY_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 raise TypeError or IndexError as needed. No changes required here.

Choose a reason for hiding this comment

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

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_subscripter and check_index in 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7a03b28 and 9f54540.

📒 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

⏰ 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 CollectionType enum 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_subscript method 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_subscript cannot be used due to the need for DUP_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_helper with the appropriate CollectionType eliminates code duplication while maintaining type-specific behavior.

This was referenced

Nov 29, 2025

youknowone commented Jul 20, 2025 • edited by coderabbitai bot Loading

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

Poem

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone commented
Jul 20, 2025
•

edited by coderabbitai bot

Loading