Skip to content

[mypyc] Match evaluation order of multiple assignment from iterable (#793) #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[mypyc] Match evaluation order of multiple assignment from iterable (#793) #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff24ef3
[mypyc] Match evaluation order of multiple assignment from iterable w…
ChetanKhanna May 7, 2021
32d2aad
changed int64 -> native_int
ChetanKhanna May 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions mypyc/irbuild/builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,8 @@ def process_iterator_tuple_assignment(self,
# This may be the whole lvalue list if there is no starred value
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
Copy link

Choose a reason for hiding this comment

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

medium

Consider clarifying in the comment that the purpose of reading values first is to ensure correct evaluation order, matching Python semantics.

values = []
for litem in target.items[:split_idx]:
ritem = self.call_c(next_op, [iterator], line)
error_block, ok_block = BasicBlock(), BasicBlock()
Expand All @@ -592,9 +593,13 @@ def process_iterator_tuple_assignment(self,

self.activate_block(ok_block)

values.append(ritem)

# Assign read values to target lvalues
for litem, ritem in zip(target.items[:split_idx], values):
self.assign(litem, ritem, line)
Comment on lines +598 to 600
Copy link

Choose a reason for hiding this comment

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

medium

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?

Comment on lines +598 to 600
Copy link

Choose a reason for hiding this comment

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

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


# Assign the starred value and all values after it
# Read the starred value and all values after it
if target.star_idx is not None:
Comment on lines +602 to 603
Copy link

Choose a reason for hiding this comment

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

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

post_star_vals = target.items[split_idx + 1:]
iter_list = self.call_c(to_list, [iterator], line)
Expand All @@ -612,8 +617,13 @@ def process_iterator_tuple_assignment(self,

self.activate_block(ok_block)

values = []
for litem in reversed(post_star_vals):
ritem = self.call_c(list_pop_last, [iter_list], line)
Comment on lines +620 to 622
Copy link

Choose a reason for hiding this comment

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

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

Comment on lines +620 to 622
Copy link

Choose a reason for hiding this comment

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

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

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)
Comment on lines +625 to 627
Copy link

Choose a reason for hiding this comment

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

medium

Similar to the previous zip usage, a comment explaining the purpose of using zip with reversed here would improve readability.

Comment on lines +625 to 627
Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
# 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

Comment on lines +620 to 627
Copy link

Choose a reason for hiding this comment

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

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


# Assign the starred value
Expand Down
109 changes: 97 additions & 12 deletions mypyc/test-data/irbuild-statements.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,9 +521,9 @@ L0:
def from_any(a):
a, r0, r1 :: object
r2 :: bool
x, r3 :: object
r3 :: object
r4 :: bool
y, r5 :: object
x, y, r5 :: object
r6 :: bool
L0:
r0 = PyObject_GetIter(a)
Expand All @@ -533,13 +533,13 @@ L1:
r2 = raise ValueError('not enough values to unpack')
unreachable
L2:
x = r1
r3 = PyIter_Next(r0)
if is_error(r3) goto L3 else goto L4
L3:
r4 = raise ValueError('not enough values to unpack')
unreachable
L4:
x = r1
y = r3
r5 = PyIter_Next(r0)
if is_error(r5) goto L6 else goto L5
Expand Down Expand Up @@ -577,9 +577,9 @@ L0:
def from_any(a):
a, r0, r1 :: object
r2 :: bool
r3, x :: int
r4 :: object
r5 :: bool
r3 :: object
r4 :: bool
r5, x :: int
y, r6 :: object
r7 :: bool
L0:
Expand All @@ -590,15 +590,15 @@ L1:
r2 = raise ValueError('not enough values to unpack')
unreachable
L2:
r3 = unbox(int, r1)
x = r3
r4 = PyIter_Next(r0)
if is_error(r4) goto L3 else goto L4
r3 = PyIter_Next(r0)
if is_error(r3) goto L3 else goto L4
L3:
r5 = raise ValueError('not enough values to unpack')
r4 = raise ValueError('not enough values to unpack')
unreachable
L4:
y = r4
r5 = unbox(int, r1)
x = r5
y = r3
r6 = PyIter_Next(r0)
if is_error(r6) goto L6 else goto L5
L5:
Expand All @@ -607,6 +607,91 @@ L5:
L6:
return 1

[case testStarUnpack]
from typing import Any, List, Iterator

it: Iterator = iter(['x', 'y', 'z1', 'z2', 'z3', 'u', 'w'])

def f(a: Any) -> None:
a.x, a.y, *a.z, a.u, a.w = it
[out]
def f(a):
a :: object
r0 :: dict
r1 :: str
r2, r3, r4 :: object
r5 :: bool
r6 :: object
r7 :: bool
r8 :: str
r9 :: int32
r10 :: bit
r11 :: str
r12 :: int32
r13 :: bit
r14 :: list
r15 :: ptr
r16 :: native_int
r17 :: short_int
r18 :: bit
r19 :: bool
r20, r21 :: object
r22 :: str
r23 :: int32
r24 :: bit
r25 :: str
r26 :: int32
r27 :: bit
r28 :: str
r29 :: int32
r30 :: bit
L0:
r0 = __main__.globals :: static
r1 = 'it'
r2 = CPyDict_GetItem(r0, r1)
r3 = PyObject_GetIter(r2)
r4 = PyIter_Next(r3)
if is_error(r4) goto L1 else goto L2
L1:
r5 = raise ValueError('not enough values to unpack')
unreachable
L2:
r6 = PyIter_Next(r3)
if is_error(r6) goto L3 else goto L4
L3:
r7 = raise ValueError('not enough values to unpack')
unreachable
L4:
r8 = 'x'
r9 = PyObject_SetAttr(a, r8, r4)
r10 = r9 >= 0 :: signed
r11 = 'y'
r12 = PyObject_SetAttr(a, r11, r6)
r13 = r12 >= 0 :: signed
r14 = PySequence_List(r3)
r15 = get_element_ptr r14 ob_size :: PyVarObject
r16 = load_mem r15 :: native_int*
keep_alive r14
r17 = r16 << 1
r18 = 4 <= r17 :: signed
if r18 goto L6 else goto L5 :: bool
L5:
r19 = raise ValueError('not enough values to unpack')
unreachable
L6:
r20 = CPyList_PopLast(r14)
r21 = CPyList_PopLast(r14)
r22 = 'w'
r23 = PyObject_SetAttr(a, r22, r20)
r24 = r23 >= 0 :: signed
r25 = 'u'
r26 = PyObject_SetAttr(a, r25, r21)
r27 = r26 >= 0 :: signed
r28 = 'z'
r29 = PyObject_SetAttr(a, r28, r14)
r30 = r29 >= 0 :: signed
return 1

[case testMultiAssignmentNested]
from typing import Tuple, Any, List

Expand Down