Skip to content

Raise IndexError for deque indices below -len. Fixes oracle/graalpython#923#927

Open
ztawfick-ux wants to merge 1 commit into
oracle:masterfrom
ztawfick-ux:GH923-deque-negative-index
Open

Raise IndexError for deque indices below -len. Fixes oracle/graalpython#923#927
ztawfick-ux wants to merge 1 commit into
oracle:masterfrom
ztawfick-ux:GH923-deque-negative-index

Conversation

@ztawfick-ux
Copy link
Copy Markdown

Summary

Subscripting a deque with an index less than -len(d) wrapped around a second time instead of raising IndexError. On a length-5 deque, d[-6] returned d[4] rather than raising, and IndexError only appeared once the index went below -2*len. This affected reads, assignments, and deletions via the subscript operator. Fixes #923.

from collections import deque
d = deque([10, 20, 30, 40, 50])
d[-6]   # CPython: IndexError;  GraalPy (before): 50

Root cause

The negative index was normalized twice:

  1. The subscript machinery (PyObjectGetItem / PyObjectSetItem / PyObjectDelItemIndexForSqSlot) already adds len(self) to a negative index, mirroring CPython's PySequence_GetItem.
  2. The deque slots DequeGetItemNode (sq_item) and DequeSetItemNode (sq_ass_item) then added len(self) again via NormalizeIndexCustomMessageNode.

So d[-6] on a length-5 deque became -6 + 5 = -1 (step 1) and then -1 + 5 = 4 (step 2), reading a valid slot instead of raising.

This is deque-specific: every other builtin sequence (list, tuple, str, bytes, array, …) also defines mp_subscript, so its subscripting goes through the mapping slot (normalized once). deque is the only builtin sequence that defines sq_item/sq_ass_item without the mp_* counterparts, so its subscripting goes through the sequence-slot path that double-normalized.

Fix

Aligns deque with CPython's division of labor (the caller normalizes; the slot only bounds-checks):

  • DequeGetItemNode / DequeSetItemNode now only perform a bounds check and raise IndexError, matching _collectionsmodule.c:deque_item / deque_ass_item (and how list's sq_item slot, SequenceStorageSqItemNode, already behaves).
  • The shared sq_* wrappers WrapSqItemBuiltinNode, WrapSqSetItemBuiltinNode, and WrapSqDelItemBuiltinNode now pass self (not the index object) to FixNegativeIndex, so the adjustment uses len(self), matching CPython's typeobject.c:getindex. Previously they passed the index object, making FixNegativeIndex compute len(index) — a no-op for integer indices. That latent bug was masked while the slot still normalized; once the slot stops normalizing, the wrapper must do the single normalization so the __getitem__/__setitem__/__delitem__ method path stays correct.

Testing

  • New regression test test_negative_index_out_of_range in test_deque.py (covers d[i], d[i] = x, del d[i] for i < -len).

Signed-off-by: Zac Tawfick <ztawfick@netflix.com>
@oracle-contributor-agreement
Copy link
Copy Markdown

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement Bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Jun 5, 2026
@ztawfick-ux
Copy link
Copy Markdown
Author

My Company has already signed the OCA. My membership request is currently under review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OCA Required At least one contributor does not have an approved Oracle Contributor Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Negative Indices normalized twice for deque subscript

1 participant