Skip to content

Commit 7fa1cfb

Browse files
tomasstupkatomasstupka
committed
[GR-23219] Make test_dict pass - update sideeffects.
PullRequest: graalpython/1032
2 parents fa42168 + 812acce commit 7fa1cfb

File tree

12 files changed

+156
-46
lines changed

12 files changed

+156
-46
lines changed

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -828,3 +828,29 @@ def __hash__(self):
828828
assert d2[MyObject("1")] == 112
829829
del d2[MyObject("1")]
830830
assert "1" not in d2
831+
832+
def test_update_side_effect_on_other():
833+
class X:
834+
def __hash__(self):
835+
return 0
836+
def __eq__(self, o):
837+
other.clear()
838+
return False
839+
840+
other = {'a':1, 'b': 2, X(): 3, 'c':4}
841+
d = {X(): 0, 1: 1}
842+
assert_raises(RuntimeError, d.update, other)
843+
assert 'c' not in d
844+
845+
other = {'a':1, 'b': 2, X(): 3, 'c':4}
846+
d = {X(): 0, 1: 0}
847+
kw = {'kw': 1}
848+
849+
raised = False
850+
try:
851+
d.update(other, **kw)
852+
except RuntimeError:
853+
raised = True
854+
assert raised
855+
856+
assert 'kw' not in d

‎graalpython/com.oracle.graal.python.test/src/tests/unittest_tags/test_dict.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@
3131
*graalpython.lib-python.3.test.test_dict.DictTest.test_keys_contained
3232
*graalpython.lib-python.3.test.test_dict.DictTest.test_len
3333
*graalpython.lib-python.3.test.test_dict.DictTest.test_literal_constructor
34+
*graalpython.lib-python.3.test.test_dict.DictTest.test_merge_and_mutate
35+
*graalpython.lib-python.3.test.test_dict.DictTest.test_missing
3436
*graalpython.lib-python.3.test.test_dict.DictTest.test_mutating_iteration
3537
*graalpython.lib-python.3.test.test_dict.DictTest.test_mutating_lookup
3638
*graalpython.lib-python.3.test.test_dict.DictTest.test_object_set_item_single_instance_non_str_key
@@ -54,6 +56,7 @@
5456
*graalpython.lib-python.3.test.test_dict.DictTest.test_track_dynamic
5557
*graalpython.lib-python.3.test.test_dict.DictTest.test_track_literals
5658
*graalpython.lib-python.3.test.test_dict.DictTest.test_track_subtypes
59+
*graalpython.lib-python.3.test.test_dict.DictTest.test_tuple_keyerror
5760
*graalpython.lib-python.3.test.test_dict.DictTest.test_update
5861
*graalpython.lib-python.3.test.test_dict.DictTest.test_values
5962
*graalpython.lib-python.3.test.test_dict.GeneralMappingTests.test_bool

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/DynamicObjectStorage.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -341,9 +341,7 @@ private static Object runNode(DynamicObjectStorage self, Object key, Object acc,
341341
@ExportMessage
342342
@TruffleBoundary
343343
public HashingStorage clear() {
344-
store.setShapeAndResize(store.getShape(), EMPTY_SHAPE);
345-
store.updateShape();
346-
return this;
344+
return new DynamicObjectStorage();
347345
}
348346

349347
@Override

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/EconomicMapStorage.java

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -372,17 +372,16 @@ static HashingStorage delItemWithStateWithSideEffect(EconomicMapStorage self, Ob
372372
static class Clear {
373373

374374
@Specialization(guards = "!hasSideEffect(self)")
375-
static HashingStorage clear(EconomicMapStorage self) {
376-
self.map.clear();
377-
return self;
375+
static HashingStorage clear(@SuppressWarnings("unused") EconomicMapStorage self) {
376+
return create();
378377
}
379378

380379
@Specialization
381380
static HashingStorage clearWithSideEffect(EconomicMapStorage self,
382381
@Exclusive @Cached LookupInheritedAttributeNode.Dynamic lookup,
383382
@Exclusive @Cached CallUnaryMethodNode callNode) {
384383
if (self.map.size() == 0) {
385-
return self;
384+
return create();
386385
}
387386
Object[] entries = new Object[self.map.size() * 2];
388387
MapCursor<DictKey, Object> cursor = self.map.getEntries();
@@ -399,7 +398,7 @@ static HashingStorage clearWithSideEffect(EconomicMapStorage self,
399398
callNode.executeObject(lookup.execute(o, __DEL__), o);
400399
}
401400
}
402-
return self;
401+
return create();
403402
}
404403

405404
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/dict/DictBuiltins.java

Lines changed: 91 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,10 @@
3838
import static com.oracle.graal.python.nodes.SpecialMethodNames.__ITER__;
3939
import static com.oracle.graal.python.nodes.SpecialMethodNames.__REVERSED__;
4040
import static com.oracle.graal.python.nodes.SpecialMethodNames.__LEN__;
41-
import static com.oracle.graal.python.nodes.SpecialMethodNames.__MISSING__;
4241
import static com.oracle.graal.python.nodes.SpecialMethodNames.__SETITEM__;
4342
import static com.oracle.graal.python.runtime.exception.PythonErrorType.KeyError;
4443
import static com.oracle.graal.python.runtime.exception.PythonErrorType.TypeError;
44+
import static com.oracle.graal.python.runtime.exception.PythonErrorType.RuntimeError;
4545

4646
import java.util.List;
4747

@@ -51,26 +51,30 @@
5151
import com.oracle.graal.python.builtins.PythonBuiltins;
5252
import com.oracle.graal.python.builtins.objects.PNone;
5353
import com.oracle.graal.python.builtins.objects.PNotImplemented;
54+
import com.oracle.graal.python.builtins.objects.common.EconomicMapStorage;
5455
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodes;
5556
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodes.GetDictStorageNode;
5657
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodes.SetDictStorageNode;
5758
import com.oracle.graal.python.builtins.objects.common.HashingStorage;
5859
import com.oracle.graal.python.builtins.objects.common.HashingStorage.DictEntry;
5960
import com.oracle.graal.python.builtins.objects.common.HashingStorage.StorageSupplier;
6061
import com.oracle.graal.python.builtins.objects.common.HashingStorageLibrary;
62+
import com.oracle.graal.python.builtins.objects.common.HashingStorageLibrary.HashingStorageIterator;
6163
import com.oracle.graal.python.builtins.objects.common.SequenceNodes;
6264
import com.oracle.graal.python.builtins.objects.function.PArguments;
6365
import com.oracle.graal.python.builtins.objects.function.PBuiltinFunction;
6466
import com.oracle.graal.python.builtins.objects.function.PKeyword;
6567
import com.oracle.graal.python.builtins.objects.mappingproxy.PMappingproxy;
6668
import com.oracle.graal.python.builtins.objects.method.PBuiltinMethod;
69+
import com.oracle.graal.python.builtins.objects.method.PMethod;
6770
import com.oracle.graal.python.builtins.objects.object.PythonObjectLibrary;
6871
import com.oracle.graal.python.builtins.objects.str.PString;
6972
import com.oracle.graal.python.builtins.objects.type.LazyPythonClass;
7073
import com.oracle.graal.python.builtins.objects.type.PythonBuiltinClass;
7174
import com.oracle.graal.python.nodes.ErrorMessages;
72-
import com.oracle.graal.python.nodes.PGuards;
7375
import com.oracle.graal.python.nodes.PRaiseNode;
76+
import com.oracle.graal.python.nodes.SpecialMethodNames;
77+
import static com.oracle.graal.python.nodes.SpecialMethodNames.__MISSING__;
7478
import com.oracle.graal.python.nodes.builtins.ListNodes;
7579
import com.oracle.graal.python.nodes.call.CallNode;
7680
import com.oracle.graal.python.nodes.call.special.LookupAndCallBinaryNode;
@@ -93,9 +97,11 @@
9397
import com.oracle.truffle.api.dsl.Cached.Exclusive;
9498
import com.oracle.truffle.api.dsl.Fallback;
9599
import com.oracle.truffle.api.dsl.GenerateNodeFactory;
100+
import com.oracle.truffle.api.dsl.ImportStatic;
96101
import com.oracle.truffle.api.dsl.Specialization;
97102
import com.oracle.truffle.api.frame.VirtualFrame;
98103
import com.oracle.truffle.api.library.CachedLibrary;
104+
import com.oracle.truffle.api.nodes.Node;
99105
import com.oracle.truffle.api.profiles.BranchProfile;
100106
import com.oracle.truffle.api.profiles.ConditionProfile;
101107

@@ -271,18 +277,41 @@ public abstract static class GetItemNode extends PythonBinaryBuiltinNode {
271277
Object getItem(VirtualFrame frame, PDict self, Object key,
272278
@CachedLibrary(value = "self.getDictStorage()") HashingStorageLibrary hlib,
273279
@Exclusive @Cached("createBinaryProfile()") ConditionProfile profile,
274-
@Cached("create(__MISSING__)") LookupAndCallBinaryNode specialNode) {
280+
@Cached DispatchMissingNode missing) {
275281
final Object result = hlib.getItemWithFrame(self.getDictStorage(), key, profile, frame);
276282
if (result == null) {
277-
return specialNode.executeObject(frame, self, key);
283+
return missing.execute(frame, self, key);
278284
}
279285
return result;
280286
}
281287
}
282288

283-
@Builtin(name = __MISSING__, minNumOfPositionalArgs = 2)
284-
@GenerateNodeFactory
285-
public abstract static class MissingNode extends PythonBinaryBuiltinNode {
289+
@ImportStatic(SpecialMethodNames.class)
290+
protected abstract static class DispatchMissingNode extends Node {
291+
292+
protected abstract Object execute(VirtualFrame frame, Object self, Object key);
293+
294+
@Specialization(guards = "hasMissing(self, lib)", limit = "1")
295+
protected Object misssing(Object self, Object key,
296+
@CachedLibrary("self") PythonObjectLibrary lib,
297+
@Exclusive @Cached CallNode callNode) {
298+
return callNode.execute(lib.lookupAttribute(self, __MISSING__), key);
299+
}
300+
301+
@Specialization(guards = "!hasMissing(self, lib)", limit = "1")
302+
protected Object misssing(VirtualFrame frame, Object self, Object key,
303+
@SuppressWarnings("unused") @CachedLibrary("self") PythonObjectLibrary lib,
304+
@Exclusive @Cached DefaultMissingNode missing) {
305+
return missing.execute(frame, self, key);
306+
}
307+
308+
protected boolean hasMissing(Object self, PythonObjectLibrary lib) {
309+
Object missing = lib.lookupAttribute(self, __MISSING__);
310+
return missing != PNone.NO_VALUE && missing instanceof PMethod;
311+
}
312+
}
313+
314+
protected abstract static class DefaultMissingNode extends PythonBinaryBuiltinNode {
286315
@SuppressWarnings("unused")
287316
@Specialization
288317
Object run(Object self, PString key,
@@ -298,13 +327,8 @@ Object run(Object self, String key) {
298327

299328
@SuppressWarnings("unused")
300329
@Specialization(guards = "!isString(key)")
301-
Object run(VirtualFrame frame, Object self, Object key,
302-
@Cached("create(__REPR__)") LookupAndCallUnaryNode specialNode) {
303-
Object name = specialNode.executeObject(frame, key);
304-
if (!PGuards.isString(name)) {
305-
throw raise(TypeError, ErrorMessages.RETURNED_NON_STRING, "__repr__", name);
306-
}
307-
throw raise(KeyError, "%s", name);
330+
Object run(VirtualFrame frame, Object self, Object key) {
331+
throw raise(KeyError, new Object[]{key});
308332
}
309333
}
310334

@@ -452,8 +476,10 @@ public abstract static class ClearNode extends PythonUnaryBuiltinNode {
452476

453477
@Specialization(limit = "3")
454478
public PDict clear(PDict dict,
455-
@CachedLibrary("dict.getDictStorage()") HashingStorageLibrary lib) {
456-
lib.clear(dict.getDictStorage());
479+
@CachedLibrary("dict.getDictStorage()") HashingStorageLibrary lib,
480+
@Cached SetDictStorageNode setStorage) {
481+
HashingStorage newStorage = lib.clear(dict.getDictStorage());
482+
setStorage.execute(dict, newStorage);
457483
return dict;
458484
}
459485
}
@@ -498,7 +524,7 @@ public Object update(VirtualFrame frame, PDict self, @SuppressWarnings("unused")
498524
return PNone.NONE;
499525
}
500526

501-
@Specialization(guards = {"isDict(args)", "kwargs.length == 0"})
527+
@Specialization(guards = {"isDictButNotEconomicMap(args, getStorage)", "kwargs.length == 0"})
502528
public Object updateDict(PDict self, Object[] args, @SuppressWarnings("unused") PKeyword[] kwargs,
503529
@CachedLibrary(limit = "1") HashingStorageLibrary lib,
504530
@Cached GetDictStorageNode getStorage,
@@ -508,7 +534,7 @@ public Object updateDict(PDict self, Object[] args, @SuppressWarnings("unused")
508534
return PNone.NONE;
509535
}
510536

511-
@Specialization(guards = {"isDict(args)", "kwargs.length > 0"})
537+
@Specialization(guards = {"isDictButNotEconomicMap(args, getStorage)", "kwargs.length > 0"})
512538
public Object updateDict(VirtualFrame frame, PDict self, Object[] args, PKeyword[] kwargs,
513539
@CachedLibrary(limit = "1") HashingStorageLibrary lib,
514540
@Cached HashingStorage.InitNode initNode,
@@ -520,6 +546,45 @@ public Object updateDict(VirtualFrame frame, PDict self, Object[] args, PKeyword
520546
return PNone.NONE;
521547
}
522548

549+
@Specialization(guards = {"isDictEconomicMap(args, getStorage)", "kwargs.length == 0"}, limit = "1")
550+
public Object updateDict(PDict self, Object[] args, @SuppressWarnings("unused") PKeyword[] kwargs,
551+
@Cached GetDictStorageNode getStorage,
552+
@Cached SetDictStorageNode setStorage,
553+
@CachedLibrary("getStorage.execute(self)") HashingStorageLibrary libSelf,
554+
@CachedLibrary(limit = "1") HashingStorageLibrary libOther) {
555+
HashingStorage newStorage = addAll(self, (PDict) args[0], getStorage, libSelf, libOther);
556+
setStorage.execute(self, newStorage);
557+
return PNone.NONE;
558+
}
559+
560+
@Specialization(guards = {"isDictEconomicMap(args, getStorage)", "kwargs.length > 0"}, limit = "1")
561+
public Object updateDict(VirtualFrame frame, PDict self, Object[] args, PKeyword[] kwargs,
562+
@Cached GetDictStorageNode getStorage,
563+
@Cached SetDictStorageNode setStorage,
564+
@CachedLibrary("getStorage.execute(self)") HashingStorageLibrary libSelf,
565+
@CachedLibrary(limit = "1") HashingStorageLibrary libOther,
566+
@Cached HashingStorage.InitNode initNode) {
567+
HashingStorage newStorage = addAll(self, (PDict) args[0], getStorage, libSelf, libOther);
568+
newStorage = libOther.addAllToOther(initNode.execute(frame, PNone.NO_VALUE, kwargs), newStorage);
569+
setStorage.execute(self, newStorage);
570+
return PNone.NONE;
571+
}
572+
573+
private HashingStorage addAll(PDict self, PDict other, GetDictStorageNode getStorage, HashingStorageLibrary libSelf, HashingStorageLibrary libOther) throws PException {
574+
HashingStorage selfStorage = getStorage.execute(self);
575+
HashingStorage otherStorage = getStorage.execute(other);
576+
HashingStorageIterator<DictEntry> itOther = libOther.entries(otherStorage).iterator();
577+
HashingStorage newStorage = selfStorage;
578+
while (itOther.hasNext()) {
579+
DictEntry next = itOther.next();
580+
newStorage = libSelf.setItem(selfStorage, next.key, next.value);
581+
if (otherStorage != getStorage.execute(other)) {
582+
throw raise(RuntimeError, ErrorMessages.MUTATED_DURING_UPDATE, "dict");
583+
}
584+
}
585+
return newStorage;
586+
}
587+
523588
@Specialization(guards = {"args.length == 1", "!isDict(args)", "hasKeysAttr(args, libArg)"})
524589
public Object updateMapping(VirtualFrame frame, PDict self, Object[] args, PKeyword[] kwargs,
525590
@SuppressWarnings("unused") @CachedLibrary(limit = "1") PythonObjectLibrary libArg,
@@ -570,6 +635,14 @@ protected boolean isDict(Object[] args) {
570635
return args.length == 1 && args[0] instanceof PDict;
571636
}
572637

638+
protected boolean isDictEconomicMap(Object[] args, GetDictStorageNode getStorage) {
639+
return args.length == 1 && args[0] instanceof PDict && getStorage.execute((PDict) args[0]) instanceof EconomicMapStorage;
640+
}
641+
642+
protected boolean isDictButNotEconomicMap(Object[] args, GetDictStorageNode getStorage) {
643+
return args.length == 1 && args[0] instanceof PDict && !(getStorage.execute((PDict) args[0]) instanceof EconomicMapStorage);
644+
}
645+
573646
protected boolean isSeq(Object[] args) {
574647
return args.length == 1 && args[0] instanceof PSequence;
575648
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/set/PBaseSet.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333

3434
public abstract class PBaseSet extends PHashingCollection {
3535

36-
protected final HashingStorage set;
36+
private HashingStorage set;
3737

3838
public PBaseSet(Object clazz, DynamicObject storage) {
3939
super(clazz, storage);
@@ -58,4 +58,9 @@ public int size() {
5858
public HashingStorage getDictStorage() {
5959
return set;
6060
}
61+
62+
@Override
63+
public void setDictStorage(HashingStorage storage) {
64+
set = storage;
65+
}
6166
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/set/PSet.java

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
package com.oracle.graal.python.builtins.objects.set;
2727

2828
import com.oracle.graal.python.builtins.objects.common.HashingStorage;
29-
import com.oracle.truffle.api.CompilerDirectives;
3029
import com.oracle.truffle.api.object.DynamicObject;
3130

3231
public final class PSet extends PBaseSet {
@@ -38,13 +37,4 @@ public PSet(Object clazz, DynamicObject storage) {
3837
public PSet(Object clazz, DynamicObject storage, HashingStorage store) {
3938
super(clazz, storage, store);
4039
}
41-
42-
@Override
43-
public void setDictStorage(HashingStorage newStorage) {
44-
// ignore if storage stays unchanged
45-
if (newStorage != getDictStorage()) {
46-
CompilerDirectives.transferToInterpreterAndInvalidate();
47-
throw new RuntimeException("set has fixed storage");
48-
}
49-
}
5040
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/set/SetBuiltins.java

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@
3838
import com.oracle.graal.python.builtins.PythonBuiltins;
3939
import com.oracle.graal.python.builtins.objects.PNone;
4040
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodes;
41+
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodes.GetDictStorageNode;
42+
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodes.SetDictStorageNode;
4143
import com.oracle.graal.python.builtins.objects.common.HashingStorage;
4244
import com.oracle.graal.python.builtins.objects.common.HashingStorageLibrary;
4345
import com.oracle.graal.python.nodes.ErrorMessages;
@@ -74,9 +76,13 @@ protected List<? extends NodeFactory<? extends PythonBuiltinBaseNode>> getNodeFa
7476
@GenerateNodeFactory
7577
public abstract static class ClearNode extends PythonUnaryBuiltinNode {
7678

77-
@Specialization
78-
public Object clear(PSet self, @CachedLibrary(limit = "1") HashingStorageLibrary lib) {
79-
lib.clear(self.getDictStorage());
79+
@Specialization(limit = "1")
80+
public Object clear(PSet self,
81+
@Cached GetDictStorageNode getStorage,
82+
@Cached SetDictStorageNode setStorage,
83+
@CachedLibrary("getStorage.execute(self)") HashingStorageLibrary lib) {
84+
HashingStorage newStorage = lib.clear(getStorage.execute(self));
85+
setStorage.execute(self, newStorage);
8086
return PNone.NONE;
8187
}
8288
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/ErrorMessages.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,7 @@ public abstract class ErrorMessages {
323323
public static final String MUST_BE_STRINGS = "%s must be strings";
324324
public static final String MUST_BE_STRINGS_NOT_P = "%s must be strings, not %p";
325325
public static final String MUST_SPECIFY_FILTERS = "Must specify filters for FORMAT_RAW";
326+
public static final String MUTATED_DURING_UPDATE = "%s mutated during update";
326327
public static final String NAME_IS_ASSIGNED_BEFORE_NONLOCAL = "name '%s' is assigned to before nonlocal declaration";
327328
public static final String NAME_NOT_DEFINED = "name '%s' is not defined";
328329
public static final String NEED_BYTELIKE_OBJ = "decoding to str: need a bytes-like object, %p found";

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/PRaiseNode.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,10 @@ public final PException raise(PythonBuiltinClassType type, String format, Object
7070
throw execute(type, PNone.NO_VALUE, format, arguments);
7171
}
7272

73+
public final PException raise(PythonBuiltinClassType type, Object... arguments) {
74+
throw execute(type, PNone.NO_VALUE, PNone.NO_VALUE, arguments);
75+
}
76+
7377
public final PException raise(PythonBuiltinClassType type, Exception e) {
7478
throw execute(type, PNone.NO_VALUE, getMessage(e), new Object[0]);
7579
}
@@ -158,6 +162,12 @@ PException doPythonManagedClass(PythonManagedClass exceptionType, @SuppressWarni
158162
throw raise(factory.createBaseException(exceptionType));
159163
}
160164

165+
@Specialization(guards = {"isNoValue(cause)", "isNoValue(format)", "arguments.length > 0"})
166+
PException doBuiltinType(PythonBuiltinClassType type, @SuppressWarnings("unused") PNone cause, @SuppressWarnings("unused") PNone format, Object[] arguments,
167+
@Shared("factory") @Cached PythonObjectFactory factory) {
168+
throw raise(factory.createBaseException(type, factory.createTuple(arguments)));
169+
}
170+
161171
@Specialization(guards = {"isNoValue(cause)"})
162172
PException doBuiltinType(PythonBuiltinClassType type, @SuppressWarnings("unused") PNone cause, String format, Object[] arguments,
163173
@Shared("factory") @Cached PythonObjectFactory factory) {

0 commit comments

Comments
 (0)