Skip to content

Commit f99e710

Browse files
steve-ssteve-s
committed
[GR-76286] Fix negative sequence slot index handling also for list and mmap.
PullRequest: graalpython/4617
2 parents 1dc9395 + edd1f9b commit f99e710

8 files changed

Lines changed: 209 additions & 22 deletions

File tree

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,15 @@ def _reference_setitem(args):
211211
return seq
212212

213213

214+
def _reference_delitem(args):
215+
seq = args[0]
216+
idx = args[1]
217+
if not hasattr(seq, '__delitem__'):
218+
raise TypeError
219+
seq.__delitem__(idx)
220+
return seq
221+
222+
214223
def _reference_fast(args):
215224
obj = args[0]
216225
if isinstance(obj, tuple) or isinstance(obj, list):
@@ -1469,6 +1478,49 @@ def _reference_delslice(args):
14691478
cmpfunc=unhandled_error_compare
14701479
)
14711480

1481+
test_PySequence_SetItem_list_negative_boundary = test_PySequence_SetItem.copy()
1482+
test_PySequence_SetItem_list_negative_boundary.pfunc = _reference_setitem
1483+
test_PySequence_SetItem_list_negative_boundary.parameters = lambda: (
1484+
(['a', 'b', 'c'], -4, 'z'),
1485+
)
1486+
1487+
test_PySequence_SetItem_mmap_negative_boundary = test_PySequence_SetItem.copy()
1488+
test_PySequence_SetItem_mmap_negative_boundary.pfunc = _reference_setitem
1489+
test_PySequence_SetItem_mmap_negative_boundary.parameters = lambda: (
1490+
(create_anon_mmap(b'hello world!'), -14, b'z'),
1491+
)
1492+
1493+
test_PySequence_DelItem = CPyExtFunction(
1494+
_reference_delitem,
1495+
lambda: (
1496+
([1, 2, 3], 1),
1497+
),
1498+
code=''' PyObject* wrap_PySequence_DelItem(PyObject* sequence, Py_ssize_t idx) {
1499+
if (PySequence_DelItem(sequence, idx) < 0) {
1500+
return NULL;
1501+
}
1502+
return sequence;
1503+
}
1504+
''',
1505+
resultspec="O",
1506+
argspec='On',
1507+
arguments=["PyObject* sequence", "Py_ssize_t idx"],
1508+
callfunction="wrap_PySequence_DelItem",
1509+
cmpfunc=unhandled_error_compare
1510+
)
1511+
1512+
test_PySequence_DelItem_list_negative_boundary = test_PySequence_DelItem.copy()
1513+
test_PySequence_DelItem_list_negative_boundary.pfunc = _reference_delitem
1514+
test_PySequence_DelItem_list_negative_boundary.parameters = lambda: (
1515+
([1, 2, 3], -4),
1516+
)
1517+
1518+
test_PySequence_DelItem_mmap_negative_boundary = test_PySequence_DelItem.copy()
1519+
test_PySequence_DelItem_mmap_negative_boundary.pfunc = _reference_delitem
1520+
test_PySequence_DelItem_mmap_negative_boundary.parameters = lambda: (
1521+
(create_anon_mmap(b'hello world!'), -14),
1522+
)
1523+
14721524
test_PySequence_Tuple = CPyExtFunction(
14731525
lambda args: tuple(args[0]),
14741526
lambda: (

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,15 @@ def test_getitem(self):
263263
d = deque('superman')
264264
self.assertEqual(d[0], 's')
265265
self.assertEqual(d[-1], 'n')
266+
267+
d = deque([10, 20, 30, 40, 50])
268+
self.assertEqual(d[-len(d)], 10)
269+
self.assertEqual(d.__getitem__(-len(d)), 10)
270+
for i in range(-len(d) - 1, -2 * len(d) - 1, -1):
271+
with self.assertRaises(IndexError):
272+
d[i]
273+
self.assertRaises(IndexError, d.__getitem__, i)
274+
266275
d = deque()
267276
self.assertRaises(IndexError, d.__getitem__, 0)
268277
self.assertRaises(IndexError, d.__getitem__, -1)
@@ -380,6 +389,16 @@ def test_setitem(self):
380389
l[i] = 7*i
381390
self.assertEqual(list(d), l)
382391

392+
d = deque([10, 20, 30, 40, 50])
393+
d[-len(d)] = 11
394+
d.__setitem__(-1, 55)
395+
self.assertEqual(list(d), [11, 20, 30, 40, 55])
396+
for i in range(-len(d) - 1, -2 * len(d) - 1, -1):
397+
with self.assertRaises(IndexError):
398+
d[i] = 99
399+
self.assertRaises(IndexError, d.__setitem__, i, 99)
400+
self.assertEqual(list(d), [11, 20, 30, 40, 55])
401+
383402
def test_delitem(self):
384403
n = 500 # O(n**2) test, don't make this too big
385404
d = deque(range(n))
@@ -394,6 +413,20 @@ def test_delitem(self):
394413
self.assertTrue(val not in d)
395414
self.assertEqual(len(d), 0)
396415

416+
data = [10, 20, 30, 40, 50]
417+
d = deque(data)
418+
del d[-len(d)]
419+
self.assertEqual(list(d), [20, 30, 40, 50])
420+
d = deque(data)
421+
d.__delitem__(-1)
422+
self.assertEqual(list(d), [10, 20, 30, 40])
423+
for i in range(-len(data) - 1, -2 * len(data) - 1, -1):
424+
d = deque(data)
425+
with self.assertRaises(IndexError):
426+
del d[i]
427+
self.assertRaises(IndexError, d.__delitem__, i)
428+
self.assertEqual(list(d), data)
429+
397430
def test_negative_index_out_of_range(self):
398431
# GH-923: an index < -len must raise IndexError, not wrap around twice
399432
d = deque([10, 20, 30, 40, 50])

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,25 @@ def __index__(self):
168168
cpy = [l[idxObj], l[idxObj], l[idxObj]]
169169
self.assertEqual(cpy, l)
170170

171+
l = [10, 20, 30, 40, 50]
172+
self.assertEqual(l[-len(l)], 10)
173+
self.assertEqual(l.__getitem__(-len(l)), 10)
174+
for i in range(-len(l) - 1, -2 * len(l) - 1, -1):
175+
with self.assertRaises(IndexError):
176+
l[i]
177+
self.assertRaises(IndexError, l.__getitem__, i)
178+
179+
def test_setitem_negative_index_boundary(self):
180+
l = [10, 20, 30, 40, 50]
181+
l[-len(l)] = 11
182+
l.__setitem__(-1, 55)
183+
self.assertEqual(l, [11, 20, 30, 40, 55])
184+
for i in range(-len(l) - 1, -2 * len(l) - 1, -1):
185+
with self.assertRaises(IndexError):
186+
l[i] = 99
187+
self.assertRaises(IndexError, l.__setitem__, i, 99)
188+
self.assertEqual(l, [11, 20, 30, 40, 55])
189+
171190
def pop_all_list(self, list):
172191
size = len(list)
173192
self.assertRaises(IndexError, list.pop, size)
@@ -248,6 +267,20 @@ def test_del_item(self):
248267
del l[4]
249268
self.assertEqual(["1", "2", "3", "4", "6"], l)
250269

270+
data = [10, 20, 30, 40, 50]
271+
l = list(data)
272+
del l[-len(l)]
273+
self.assertEqual(l, [20, 30, 40, 50])
274+
l = list(data)
275+
l.__delitem__(-1)
276+
self.assertEqual(l, [10, 20, 30, 40])
277+
for i in range(-len(data) - 1, -2 * len(data) - 1, -1):
278+
l = list(data)
279+
with self.assertRaises(IndexError):
280+
del l[i]
281+
self.assertRaises(IndexError, l.__delitem__, i)
282+
self.assertEqual(l, data)
283+
251284
def test_del_border(self):
252285
l = [1, 2, 3]
253286
self.assertRaises(IndexError, l.__delitem__, 3)

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,76 @@ def test_getitem():
8787
m[i] = i
8888
assert m[slice(-10, 100)] == b'\x02\x03\x04\x05\x06\x07\x08\t\n\x0b'
8989

90+
assert m[-len(m)] == 0
91+
assert m.__getitem__(-len(m)) == 0
92+
for i in range(-len(m) - 1, -2 * len(m) - 1, -1):
93+
try:
94+
m[i]
95+
except IndexError:
96+
pass
97+
else:
98+
assert False, "expected IndexError"
99+
try:
100+
m.__getitem__(i)
101+
except IndexError:
102+
pass
103+
else:
104+
assert False, "expected IndexError"
105+
106+
107+
def test_setitem_negative_index_boundary():
108+
m = mmap.mmap(-1, 5)
109+
m[:] = b'abcde'
110+
m[-len(m)] = ord('A')
111+
m.__setitem__(-1, ord('E'))
112+
assert m[:] == b'AbcdE'
113+
for i in range(-len(m) - 1, -2 * len(m) - 1, -1):
114+
try:
115+
m[i] = ord('x')
116+
except IndexError:
117+
pass
118+
else:
119+
assert False, "expected IndexError"
120+
try:
121+
m.__setitem__(i, ord('x'))
122+
except IndexError:
123+
pass
124+
else:
125+
assert False, "expected IndexError"
126+
assert m[:] == b'AbcdE'
127+
128+
129+
def test_delitem_negative_index_boundary():
130+
m = mmap.mmap(-1, 5)
131+
m[:] = b'abcde'
132+
try:
133+
del m[-len(m)]
134+
except TypeError:
135+
pass
136+
else:
137+
assert False, "expected TypeError"
138+
try:
139+
m.__delitem__(-1)
140+
except TypeError:
141+
pass
142+
else:
143+
assert False, "expected TypeError"
144+
145+
for i in range(-len(m) - 1, -2 * len(m) - 1, -1):
146+
try:
147+
del m[i]
148+
except IndexError:
149+
pass
150+
else:
151+
assert False, "expected IndexError"
152+
try:
153+
m.__delitem__(i)
154+
except IndexError:
155+
pass
156+
else:
157+
assert False, "expected IndexError"
158+
assert m[:] == b'abcde'
159+
90160

91161
def test_readline():
92162
m = mmap.mmap(-1, 9)

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/deque/DequeBuiltins.java‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -823,10 +823,9 @@ public abstract static class DequeGetItemNode extends SqItemBuiltinNode {
823823
@Specialization
824824
@TruffleBoundary
825825
Object doGeneric(PDeque self, int idx,
826-
@Bind Node inliningTarget,
827-
@Cached PRaiseNode raiseNode) {
826+
@Bind Node inliningTarget) {
828827
if (idx < 0 || idx >= self.getSize()) {
829-
throw raiseNode.raise(inliningTarget, IndexError, ErrorMessages.DEQUE_INDEX_OUT_OF_RANGE);
828+
throw PRaiseNode.raiseStatic(inliningTarget, IndexError, ErrorMessages.DEQUE_INDEX_OUT_OF_RANGE);
830829
}
831830
return doGetItem(self, idx);
832831
}
@@ -846,12 +845,12 @@ Object doGetItem(PDeque self, int idx) {
846845
@GenerateNodeFactory
847846
public abstract static class DequeSetItemNode extends SqAssItemBuiltinNode {
848847

848+
@TruffleBoundary
849849
@Specialization
850850
static void setOrDel(PDeque self, int idx, Object value,
851-
@Bind Node inliningTarget,
852-
@Cached PRaiseNode raiseNode) {
851+
@Bind Node inliningTarget) {
853852
if (idx < 0 || idx >= self.getSize()) {
854-
throw raiseNode.raise(inliningTarget, IndexError, ErrorMessages.DEQUE_INDEX_OUT_OF_RANGE);
853+
throw PRaiseNode.raiseStatic(inliningTarget, IndexError, ErrorMessages.DEQUE_INDEX_OUT_OF_RANGE);
855854
}
856855
self.setItem(idx, value != PNone.NO_VALUE ? value : null);
857856
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/list/ListBuiltins.java‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
*/
2626
package com.oracle.graal.python.builtins.objects.list;
2727

28+
import static com.oracle.graal.python.builtins.objects.common.IndexNodes.checkBounds;
2829
import static com.oracle.graal.python.nodes.BuiltinNames.J_APPEND;
2930
import static com.oracle.graal.python.nodes.BuiltinNames.J_EXTEND;
3031
import static com.oracle.graal.python.nodes.BuiltinNames.J_LIST;
@@ -340,9 +341,11 @@ static void doInt(Object self, int index, Object value,
340341
@Bind Node inliningTarget,
341342
@Exclusive @Cached GetListStorageNode getStorageNode,
342343
@Cached ListNodes.UpdateListStorageNode updateStorageNode,
343-
@Cached("createForList()") SequenceStorageNodes.SetItemNode setItemNode) {
344+
@Cached SequenceStorageNodes.SetItemScalarGeneralizingNode setItemNode,
345+
@Exclusive @Cached PRaiseNode raiseNode) {
344346
var sequenceStorage = getStorageNode.execute(inliningTarget, self);
345-
var newStorage = setItemNode.execute(sequenceStorage, index, value);
347+
checkBounds(inliningTarget, raiseNode, ErrorMessages.LIST_ASSIGMENT_INDEX_OUT_OF_RANGE, index, sequenceStorage.length());
348+
var newStorage = setItemNode.execute(inliningTarget, sequenceStorage, index, value, SequenceStorageNodes.ListGeneralizationNode.SUPPLIER);
346349
updateStorageNode.execute(inliningTarget, self, sequenceStorage, newStorage);
347350
}
348351

@@ -351,10 +354,10 @@ static void doInt(Object self, int index, Object value,
351354
static void doGeneric(Object list, int index, @SuppressWarnings("unused") Object value,
352355
@Bind Node inliningTarget,
353356
@Exclusive @Cached GetListStorageNode getStorageNode,
354-
@Cached NormalizeIndexNode normalizeIndexNode,
355-
@Cached SequenceStorageNodes.DeleteItemNode deleteItemNode) {
357+
@Cached SequenceStorageNodes.DeleteItemNode deleteItemNode,
358+
@Exclusive @Cached PRaiseNode raiseNode) {
356359
var sequenceStorage = getStorageNode.execute(inliningTarget, list);
357-
index = normalizeIndexNode.execute(index, sequenceStorage.length());
360+
checkBounds(inliningTarget, raiseNode, ErrorMessages.LIST_ASSIGMENT_INDEX_OUT_OF_RANGE, index, sequenceStorage.length());
358361
deleteItemNode.execute(inliningTarget, sequenceStorage, index);
359362
}
360363
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/mmap/MMapBuiltins.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -398,10 +398,7 @@ static void doSingle(VirtualFrame frame, PMMap self, int index, Object val,
398398
throw raiseNode.raise(inliningTarget, PythonBuiltinClassType.ValueError, MMAP_CLOSED_OR_INVALID);
399399
}
400400
long len = self.getLength();
401-
long idx = index < 0 ? index + len : index;
402-
if (idx < 0 || idx >= len) {
403-
throw raiseNode.raise(inliningTarget, PythonBuiltinClassType.IndexError, MMAP_INDEX_OUT_OF_RANGE);
404-
}
401+
checkBounds(inliningTarget, raiseNode, MMAP_INDEX_OUT_OF_RANGE, index, len);
405402
if (val == PNone.NO_VALUE) {
406403
throw raiseNode.raise(inliningTarget, PythonBuiltinClassType.TypeError, MMAP_OBJECT_DOESNT_SUPPORT_ITEM_DELETION);
407404
}
@@ -413,7 +410,7 @@ static void doSingle(VirtualFrame frame, PMMap self, int index, Object val,
413410
}
414411
byte b = bufferLib.readByte(val, 0);
415412
try {
416-
posixSupportLib.mmapWriteByte(context.getPosixSupport(), self.getPosixSupportHandle(), idx, b);
413+
posixSupportLib.mmapWriteByte(context.getPosixSupport(), self.getPosixSupportHandle(), index, b);
417414
} catch (PosixException ex) {
418415
throw constructAndRaiseNode.get(inliningTarget).raiseOSErrorFromPosixException(frame, ex);
419416
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/slots/TpSlotSqAssItem.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,9 @@
5151
import com.oracle.graal.python.PythonLanguage;
5252
import com.oracle.graal.python.builtins.Python3Core;
5353
import com.oracle.graal.python.builtins.PythonBuiltinClassType;
54-
import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionInvoker;
5554
import com.oracle.graal.python.builtins.objects.PNone;
5655
import com.oracle.graal.python.builtins.objects.cext.capi.CExtNodes.EnsurePythonObjectNode;
57-
import com.oracle.graal.python.builtins.objects.type.slots.TpSlotInquiry.CheckInquiryResultNode;
56+
import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionInvoker;
5857
import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionNodes.PExternalFunctionWrapper;
5958
import com.oracle.graal.python.builtins.objects.cext.capi.transitions.CApiTiming;
6059
import com.oracle.graal.python.builtins.objects.cext.capi.transitions.CApiTransitions.PythonToNativeInternalNode;
@@ -67,6 +66,7 @@
6766
import com.oracle.graal.python.builtins.objects.type.slots.TpSlot.TpSlotManaged;
6867
import com.oracle.graal.python.builtins.objects.type.slots.TpSlot.TpSlotNative;
6968
import com.oracle.graal.python.builtins.objects.type.slots.TpSlot.TpSlotPython;
69+
import com.oracle.graal.python.builtins.objects.type.slots.TpSlotInquiry.CheckInquiryResultNode;
7070
import com.oracle.graal.python.builtins.objects.type.slots.TpSlotSizeArgFun.FixNegativeIndex;
7171
import com.oracle.graal.python.builtins.objects.type.slots.TpSlotSqAssItemFactory.CallSlotSqAssItemNodeGen;
7272
import com.oracle.graal.python.builtins.objects.type.slots.TpSlotSqAssItemFactory.WrapSqDelItemBuiltinNodeGen;
@@ -203,15 +203,15 @@ Object doIntIndex(VirtualFrame frame, Object self, int index) {
203203
Object doGeneric(VirtualFrame frame, Object self, Object index,
204204
@Bind Node inliningTarget,
205205
@Cached PyNumberAsSizeNode asSizeNode) {
206-
int size = asSizeNode.executeExact(frame, inliningTarget, index, PythonBuiltinClassType.OverflowError);
207-
if (size < 0) {
206+
int indexAsSize = asSizeNode.executeExact(frame, inliningTarget, index, PythonBuiltinClassType.OverflowError);
207+
if (indexAsSize < 0) {
208208
if (fixNegativeIndex == null) {
209209
CompilerDirectives.transferToInterpreterAndInvalidate();
210210
fixNegativeIndex = insert(FixNegativeIndex.create());
211211
}
212-
size = fixNegativeIndex.execute(frame, size, self);
212+
indexAsSize = fixNegativeIndex.execute(frame, indexAsSize, self);
213213
}
214-
slotNode.executeIntKey(frame, self, size, PNone.NO_VALUE);
214+
slotNode.executeIntKey(frame, self, indexAsSize, PNone.NO_VALUE);
215215
return PNone.NONE;
216216
}
217217
}

0 commit comments

Comments
 (0)