Skip to content

Commit 9bb885e

Browse files
msimacekmsimacek
committed
[GR-40552][GR-40631] Fix augmented assignment evaluation
PullRequest: graalpython/2408
2 parents 3a4f83c + 0645b3b commit 9bb885e

File tree

8 files changed

+156
-12
lines changed

8 files changed

+156
-12
lines changed

‎graalpython/com.oracle.graal.python.test/src/com/oracle/graal/python/test/compiler/CompilerTests.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,16 @@ public void testAugAssignment() {
9595
doTest("a += 12.0");
9696
}
9797

98+
@Test
99+
public void testAugAssignmentAttr() {
100+
doTest("a.b += 12.0");
101+
}
102+
103+
@Test
104+
public void testAugAssignmentSubscr() {
105+
doTest("a[b] += 12.0");
106+
}
107+
98108
@Test
99109
public void testAnnAssignment() {
100110
doTest("a: int = 12");

‎graalpython/com.oracle.graal.python.test/src/tests/test_assign.py

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
3737
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
3838
# SOFTWARE.
39+
import sys
3940

4041
import unittest
4142

@@ -105,6 +106,77 @@ def test_destructuring():
105106
assert a == -1 and b == -1 and s == [0, 1, 2, 3, 4, 5, 6, 7] and c == 8 and d == 9
106107

107108

109+
def test_augassign_evaluation_subsc():
110+
if sys.implementation.name == 'graalpy' and not __graalpython__.uses_bytecode_interpreter:
111+
# FIXME AST interpreter fails this test
112+
return
113+
calls = []
114+
115+
class C(list):
116+
def __getitem__(self, item):
117+
calls.append("get")
118+
return super().__getitem__(item)
119+
120+
def __setitem__(self, key, value):
121+
calls.append("set")
122+
super().__setitem__(key, value)
123+
124+
class I:
125+
def __iadd__(self, other):
126+
calls.append("iadd")
127+
return 3
128+
129+
def index():
130+
calls.append("index")
131+
return 0
132+
133+
def value():
134+
calls.append("value")
135+
return 1
136+
137+
x = C([I()])
138+
139+
def container():
140+
calls.append("container")
141+
return x
142+
143+
container()[index()] += value()
144+
assert calls == ["container", "index", "get", "value", "iadd", "set"]
145+
146+
147+
def test_augassign_evaluation_attr():
148+
if sys.implementation.name == 'graalpy' and not __graalpython__.uses_bytecode_interpreter:
149+
# FIXME AST interpreter fails this test
150+
return
151+
calls = []
152+
153+
class C(list):
154+
def __getattr__(self, item):
155+
calls.append("get")
156+
return I()
157+
158+
def __setattr__(self, key, value):
159+
calls.append("set")
160+
161+
class I:
162+
def __iadd__(self, other):
163+
calls.append("iadd")
164+
return 3
165+
166+
def value():
167+
calls.append("value")
168+
return 1
169+
170+
x = C()
171+
172+
def container():
173+
calls.append("container")
174+
return x
175+
176+
container().attr += value()
177+
assert calls == ["container", "get", "value", "iadd", "set"]
178+
179+
108180
def test_assigning_hidden_keys():
109181
class A():
110182
def __init__(self):

‎graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testAugAssignmentAttr.co

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
Disassembly of <module>:
2+
000000 0 LOAD_NAME 0 (a)
3+
000000 2 DUP_TOP
4+
000000 3 LOAD_ATTR 1 (b)
5+
000007 5 LOAD_DOUBLE_O 0 (12.0)
6+
000000 7 BINARY_OP 1 (INPLACE_ADD)
7+
000000 9 ROT_TWO
8+
000000 10 STORE_ATTR 1 (b)
9+
000000 12 LOAD_NONE
10+
000000 13 RETURN_VALUE

‎graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testAugAssignmentSubscr.co

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
Disassembly of <module>:
2+
000000 0 LOAD_NAME 0 (a)
3+
000000 2 DUP_TOP
4+
000002 3 LOAD_NAME 1 (b)
5+
000000 5 DUP_TOP
6+
000000 6 ROT_THREE
7+
000000 7 BINARY_SUBSCR
8+
000008 8 LOAD_DOUBLE_O 0 (12.0)
9+
000000 10 BINARY_OP 1 (INPLACE_ADD)
10+
000000 12 ROT_THREE
11+
000000 13 STORE_SUBSCR
12+
000000 14 LOAD_NONE
13+
000000 15 RETURN_VALUE

‎graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testBenchmark.co

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ Disassembly of docompute:
4949
000183 71 LOAD_BYTE_I 1 can quicken
5050
000178 73 BINARY_OP 1 (INPLACE_ADD) can quicken, generalizes: 71, 69
5151
000178 75 STORE_FAST 3 (j) generalizes: 73
52-
000178 77 JUMP_BACKWARD 58 (to 19)
53-
000178 >> 79 JUMP_BACKWARD 72 (to 7)
52+
000086 77 JUMP_BACKWARD 58 (to 19)
53+
000086 >> 79 JUMP_BACKWARD 72 (to 7)
5454
000197 >> 81 LOAD_FAST 2 (sum_)
5555
000190 83 RETURN_VALUE
5656

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/compiler/Compiler.java

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,6 @@
140140
import java.util.List;
141141

142142
import com.oracle.graal.python.builtins.objects.PNone;
143-
import com.oracle.graal.python.pegparser.CopyWithContextVisitor;
144143
import com.oracle.graal.python.pegparser.ErrorCallback;
145144
import com.oracle.graal.python.pegparser.ErrorCallback.ErrorType;
146145
import com.oracle.graal.python.pegparser.FExprParser;
@@ -2020,11 +2019,30 @@ public Void visit(StmtTy.AsyncWith node) {
20202019

20212020
@Override
20222021
public Void visit(StmtTy.AugAssign node) {
2023-
SourceRange savedLocation = setLocation(node);
2024-
node.target.accept(new CopyWithContextVisitor(ExprContextTy.Load)).accept(this);
2025-
setLocation(savedLocation);
2026-
node.value.accept(this);
2022+
ExprTy target = node.target;
2023+
SourceRange savedLocation = setLocation(target);
2024+
if (target instanceof ExprTy.Attribute) {
2025+
ExprTy.Attribute attr = (ExprTy.Attribute) target;
2026+
attr.value.accept(this);
2027+
addOp(DUP_TOP);
2028+
addOpName(LOAD_ATTR, unit.names, attr.attr);
2029+
} else if (target instanceof ExprTy.Subscript) {
2030+
ExprTy.Subscript subscript = (ExprTy.Subscript) target;
2031+
subscript.value.accept(this);
2032+
addOp(DUP_TOP);
2033+
subscript.slice.accept(this);
2034+
addOp(DUP_TOP);
2035+
addOp(ROT_THREE);
2036+
addOp(BINARY_SUBSCR);
2037+
} else if (target instanceof ExprTy.Name) {
2038+
ExprTy.Name name = (ExprTy.Name) target;
2039+
addNameOp(name.id, ExprContextTy.Load);
2040+
} else {
2041+
// TODO this raises SystemError under CPython
2042+
throw new IllegalArgumentException("invalid node type for augmented assignment");
2043+
}
20272044
setLocation(node);
2045+
node.value.accept(this);
20282046
switch (node.op) {
20292047
case Add:
20302048
addOp(BINARY_OP, BinaryOps.INPLACE_ADD.ordinal());
@@ -2068,7 +2086,20 @@ public Void visit(StmtTy.AugAssign node) {
20682086
default:
20692087
throw new IllegalStateException("Unknown binary inplace operation " + node.op);
20702088
}
2071-
return node.target.accept(this);
2089+
setLocation(target);
2090+
if (target instanceof ExprTy.Attribute) {
2091+
ExprTy.Attribute attr = (ExprTy.Attribute) target;
2092+
addOp(ROT_TWO);
2093+
addOpName(STORE_ATTR, unit.names, attr.attr);
2094+
} else if (target instanceof ExprTy.Subscript) {
2095+
addOp(ROT_THREE);
2096+
addOp(STORE_SUBSCR);
2097+
} else {
2098+
ExprTy.Name name = (ExprTy.Name) target;
2099+
addNameOp(name.id, ExprContextTy.Store);
2100+
}
2101+
setLocation(savedLocation);
2102+
return null;
20722103
}
20732104

20742105
@Override

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode/ForIterINode.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@
5858
import com.oracle.truffle.api.dsl.Specialization;
5959
import com.oracle.truffle.api.frame.Frame;
6060
import com.oracle.truffle.api.frame.VirtualFrame;
61-
import com.oracle.truffle.api.profiles.LoopConditionProfile;
61+
import com.oracle.truffle.api.profiles.ConditionProfile;
6262

6363
/**
6464
* Obtains the next value of an iterator. When the iterator is exhausted it returns {@code null}. It
@@ -70,7 +70,11 @@ public abstract class ForIterINode extends PNodeWithContext {
7070

7171
@Specialization
7272
boolean doIntRange(VirtualFrame frame, PIntRangeIterator iterator, int stackTop,
73-
@Cached("createCountingProfile()") LoopConditionProfile conditionProfile) {
73+
/*
74+
* Not using LoopConditionProfile because when OSR-compiled, we might never
75+
* register the condition being false
76+
*/
77+
@Cached("createCountingProfile()") ConditionProfile conditionProfile) {
7478
if (conditionProfile.profile(iterator.hasNextInt())) {
7579
frame.setInt(stackTop, iterator.nextInt());
7680
return true;

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode/ForIterONode.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@
5656
import com.oracle.truffle.api.dsl.Specialization;
5757
import com.oracle.truffle.api.frame.Frame;
5858
import com.oracle.truffle.api.frame.VirtualFrame;
59-
import com.oracle.truffle.api.profiles.LoopConditionProfile;
59+
import com.oracle.truffle.api.profiles.ConditionProfile;
6060

6161
/**
6262
* Obtains the next value of an iterator. When the iterator is exhausted it returns {@code null}. It
@@ -68,7 +68,11 @@ public abstract class ForIterONode extends PNodeWithContext {
6868

6969
@Specialization
7070
boolean doIntRange(VirtualFrame frame, PIntRangeIterator iterator, int stackTop,
71-
@Cached("createCountingProfile()") LoopConditionProfile conditionProfile) {
71+
/*
72+
* Not using LoopConditionProfile because when OSR-compiled, we might never
73+
* register the condition being false
74+
*/
75+
@Cached("createCountingProfile()") ConditionProfile conditionProfile) {
7276
if (conditionProfile.profile(iterator.hasNextInt())) {
7377
frame.setObject(stackTop, iterator.nextInt());
7478
return true;

0 commit comments

Comments
 (0)