[mypyc] Match evaluation order of multiple assignment from iterable (#793) #8
Conversation
…ith Python (python#793) Refactored to iterate rvalues first before performing any assignments. Closes mypyc/mypyc#793.
WalkthroughThe changes refactor the logic for multiple assignment from iterators in the IR builder. The updated implementation first collects all values from the iterator before performing assignments to the target variables, rather than interleaving iteration and assignment. This adjustment ensures that the evaluation order matches Python semantics, especially in cases involving starred expressions. The test suite is updated to reflect the new evaluation order, and a new test case is added to verify unpacking with a starred target. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant IRBuilder
participant Iterator
participant TargetVars
Caller->>IRBuilder: process_iterator_tuple_assignment(it, targets)
IRBuilder->>Iterator: Collect all values (prefix, starred, suffix)
Iterator-->>IRBuilder: Return values as list
IRBuilder->>TargetVars: Assign prefix values
IRBuilder->>TargetVars: Assign starred value
IRBuilder->>TargetVars: Assign suffix values
Assessment against linked issues
Poem
✨ 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:
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
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses issue python#793 in the mypyc project, focusing on the evaluation order of multiple assignments from iterables. The primary goal is to refactor the code to ensure that rvalues (right-hand side values) are iterated before any assignments are made to lvalues (left-hand side values), aligning with standard Python semantics. This involves changes to mypyc/irbuild/builder.py to adjust the assignment logic and the addition of a new test case in mypyc/test-data/irbuild-statements.test to verify the correct behavior with starred expressions.
Highlights
- Evaluation Order Fix: The core change involves modifying the
process_iterator_tuple_assignmentfunction inmypyc/irbuild/builder.pyto first read all rvalues from the iterator before assigning them to the corresponding lvalues. This ensures correct evaluation order, especially when dealing with iterators that might have side effects. - Test Case Addition: A new test case,
testStarUnpack, has been added tomypyc/test-data/irbuild-statements.testto specifically test the unpacking of iterables with starred lvalues. This test verifies that the assignment order is correct when using the*xsyntax. - Code Refactoring: The code has been refactored to use temporary lists (
values) to store the rvalues before assigning them to the lvalues. This change improves the clarity and correctness of the assignment process.
Changelog
- mypyc/irbuild/builder.py
- Modified
process_iterator_tuple_assignmentto iterate through rvalues before assigning to lvalues. - Introduced temporary
valueslist to store rvalues during iteration. - Adjusted assignment logic to use the stored
valueslist for assigning to lvalues.
- Modified
- mypyc/test-data/irbuild-statements.test
- Added a new test case
testStarUnpackto verify correct unpacking with starred lvalues. - Modified existing test cases
from_anyto adjust variable declarations and assignment order to align with the intended behavior.
- Added a new test case
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
From iterable's flow,
Values unpack, order known,
Python's way is shown.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request refactors the code to ensure the correct evaluation order for multiple assignments from iterables in mypyc. The changes involve reading all rvalues before assigning them to lvalues, which aligns with Python semantics. A new test case has been added to verify the unpacking with *x type lvalues. Overall, the changes seem well-structured and address the identified issue.
Summary of Findings
- Potential performance impact: The changes introduce temporary lists (
values) to store rvalues before assignment. While this ensures correct evaluation order, it might introduce a slight performance overhead due to the list operations. It would be beneficial to analyze the performance impact of these changes, especially for large iterables. - Error handling in starred assignments: The code includes error handling for cases where there are not enough values to unpack. However, it might be beneficial to add more specific error messages or handling for cases where the iterable contains non-compatible types, especially when dealing with starred assignments.
Merge Readiness
The pull request addresses an important correctness issue and includes a new test case. However, it's recommended to analyze the potential performance impact of the introduced temporary lists and consider adding more specific error handling for non-compatible types in starred assignments. I am unable to approve this pull request, and users should have others review and approve this code before merging. Given the potential performance impact, it would be best to address this before merging.
| split_idx = target.star_idx if target.star_idx is not None else len(target.items) | ||
|
|
||
| # Assign values before the first starred value | ||
| # Read values before the first starred value |
There was a problem hiding this comment.
Consider clarifying in the comment that the purpose of reading values first is to ensure correct evaluation order, matching Python semantics.
| # Assign read values to target lvalues | ||
| for litem, ritem in zip(target.items[:split_idx], values): | ||
| self.assign(litem, ritem, line) |
There was a problem hiding this comment.
It might be beneficial to add a comment explaining why the zip function is used here. Is it to handle cases where the lengths of target.items[:split_idx] and values might be different due to some edge case?
| # Assign the read values to target lvalues | ||
| for litem, ritem in zip(reversed(post_star_vals), values): | ||
| self.assign(litem, ritem, line) |
There was a problem hiding this comment.
Similar to the previous zip usage, a comment explaining the purpose of using zip with reversed here would improve readability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mypyc/irbuild/builder.py (1)
621-623: Unused loop variable in value collectionThe loop control variable
litemis not used within the loop body. Consider using_to indicate an unused variable.-for litem in reversed(post_star_vals): +for _ in reversed(post_star_vals): ritem = self.call_c(list_pop_last, [iter_list], line) values.append(ritem)🧰 Tools
🪛 Ruff (0.8.2)
621-621: Loop control variable
litemnot used within loop body(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mypyc/irbuild/builder.py(3 hunks)mypyc/test-data/irbuild-statements.test(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
mypyc/irbuild/builder.py (2)
mypyc/test-data/fixtures/ir.py (6)
values(175-175)append(144-144)zip(259-259)zip(261-261)items(176-176)reversed(242-242)mypyc/irbuild/ll_builder.py (2)
self(129-134)call_c(1012-1069)
🪛 Ruff (0.8.2)
mypyc/irbuild/builder.py
621-621: Loop control variable litem not used within loop body
(B007)
🔇 Additional comments (5)
mypyc/irbuild/builder.py (2)
583-584: Improvement: Properly separating value collection from assignment in tuple unpackingThe refactoring correctly separates the reading of values from the iterator from their assignment to target lvalues. This ensures that all right-hand side values are fully evaluated before any assignments occur, matching Python's semantics for unpacking operations.
Also applies to: 596-597, 599-600
🧰 Tools
🪛 Ruff (0.8.2)
584-584: Loop control variable
litemnot used within loop body(B007)
625-627: Correctly implemented assignment phase for tuple suffixThe code now properly separates reading suffix values (in reverse order) from assigning them to the targets, maintaining consistent semantics with the rest of the implementation.
mypyc/test-data/irbuild-statements.test (3)
522-544: Well-aligned IR code for basic unpackingThe test case IR ordering has been updated to match the new implementation where values are read from the iterator first before any assignments occur. The modified IR correctly reflects the evaluation order: first fetching both values (
r1andr3), then assigning them to variablesxandy.
578-602: Correctly updated IR for unpacking with type coercionThe IR code for type coercion during unpacking has been properly updated to match the new evaluation order. This test case correctly illustrates that all values are read first before any assignments or type coercions happen, which is essential for matching Python's semantics.
610-693: Excellent new test case for starred expressionsThis new test case effectively verifies the updated tuple unpacking implementation for the more complex scenario with a starred expression. The test covers:
- Obtaining the iterator from a global variable
- Reading prefix elements (
a.x,a.y)- Converting the remainder to a list for starred assignment
- Checking if there are enough elements for unpacking
- Popping suffix elements in reverse order (
a.w,a.u)- Error handling for insufficient values
The test case aligns well with the PR objective to match Python's evaluation order in unpacking operations, especially for scenarios with starred expressions.
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Iterator Processing Evaluation Order Fix👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| values = [] | ||
| for litem in reversed(post_star_vals): | ||
| ritem = self.call_c(list_pop_last, [iter_list], line) |
There was a problem hiding this comment.
Post-Star Assignment Order
Similar to the first issue, values after the starred expression are assigned immediately after reading, violating Python's evaluation-then-assignment order. The fix correctly reads all values first, then performs assignments separately.
Standards
- Algorithm-Correctness-Execution-Order
- Language-Compliance-Python-Semantics
- Logic-Verification-Side-Effects
| # Read the starred value and all values after it | ||
| if target.star_idx is not None: |
There was a problem hiding this comment.
Star Unpack Testing
Test case for star unpacking exists but comment indicates implementation change. Ensure test coverage validates both reading and assignment phases for starred values.
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Iterator Assignment Execution Order👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| # Assign the read values to target lvalues | ||
| for litem, ritem in zip(reversed(post_star_vals), values): | ||
| self.assign(litem, ritem, line) |
There was a problem hiding this comment.
Reversed Post-Star Assignment
The post-star values are collected in reverse order (using list_pop_last), but the assignment loop also uses reversed(post_star_vals), causing double reversal. This incorrectly assigns values to the wrong variables compared to Python's expected behavior.
| # Assign the read values to target lvalues | |
| for litem, ritem in zip(reversed(post_star_vals), values): | |
| self.assign(litem, ritem, line) | |
| # Assign the read values to target lvalues | |
| for litem, ritem in zip(post_star_vals, reversed(values)): | |
| self.assign(litem, ritem, line) |
Standards
- Algorithm-Correctness-Execution-Order
- Logic-Verification-Data-Flow
- Business-Rule-State-Consistency
| values = [] | ||
| for litem in reversed(post_star_vals): | ||
| ritem = self.call_c(list_pop_last, [iter_list], line) | ||
| values.append(ritem) | ||
|
|
||
| # Assign the read values to target lvalues | ||
| for litem, ritem in zip(reversed(post_star_vals), values): | ||
| self.assign(litem, ritem, line) |
There was a problem hiding this comment.
Inconsistent Pattern Implementation
The code correctly implements the read-then-assign pattern for post-star values, but uses a different approach than for pre-star values. This inconsistency could lead to maintenance issues and potential bugs when modifying either section. The pattern should be consistently applied across both code paths.
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Consistency
- DbC-Implementation-Consistency
| values = [] | ||
| for litem in reversed(post_star_vals): | ||
| ritem = self.call_c(list_pop_last, [iter_list], line) |
There was a problem hiding this comment.
Redundant Value Processing
Values are immediately assigned after being popped from the list, creating the same evaluation order issue as above. This pattern leads to inconsistent state if an exception occurs during iteration, causing performance overhead from unnecessary partial assignments that may need to be rolled back.
Standards
- ISO-IEC-25010-Performance-Efficiency-Resource-Utilization
- Optimization-Pattern-Batch-Processing
- Algorithmic-Complexity-Exception-Handling
| # Assign read values to target lvalues | ||
| for litem, ritem in zip(target.items[:split_idx], values): | ||
| self.assign(litem, ritem, line) |
There was a problem hiding this comment.
Two-Phase Assignment Pattern
The code implements a clear two-phase pattern: first reading all values, then assigning them. This separation improves maintainability by making the execution order explicit and matching Python's evaluation semantics for unpacking assignments.
Standards
- Clean-Code-Clarity
- Design-Pattern-Separation
- Maintainability-Quality-Consistency
Description
Refactored to iterate rvalues first before performing any assignments to match with Python semantics.
Closes mypyc/mypyc#793.
Test Plan
Added a new test case to test the unpacking with
*xtype lvaluesSummary by CodeRabbit
Refactor
Tests