PyList indexing: negative indices?

[birkenfeld]

PyList::get_item and PySequence::get_item accept isize indices, indicating that negative indices can be used as in Python.

PyList::get_item, however, does not support index<0 and segfaults ☹️ (Also did so before the change to use the macro, since there was no check for error returns of PyList_GetItem.)

The assert in there is written as if negative indices were supported.

PyTuple::get_item accepts usize.

Also, PySequence::get_item returns a Result while PyList and PyTuple panic on invalid indices.

I see several things to think about here:

• Should we use Index for the panicing impls? get_item should then always return a Result.
• Should PyList support negative indices (we'd have to implement the correction ourselves)? If yes, the same should apply to PyTuple.
• If we take only "real" indices, should the type switch to usize?
• What about other APIs that take indices? E.g. set_item.

I also noticed some inconsistencies of the tuple and list APIs. E.g., tuple has slice (and I don't mean as_slice, it's clear that list can't have that) but list doesn't.

[birkenfeld]

OK, I checked all current APIs of list, tuple and sequence that take indices:

Doesn't work:

• PyTuple::slice - always returns ()
• PyTuple::split_from - always returns full slice
• PyList::get_item - crashes
• PyList::set_item - raises IndexError

Works:

• PyList::insert
• all PySequence APIs

Takes usize, not isize:

• PyTuple::get_item

[davidhewitt]

Mmm, thanks for taking the time to look at this; I'd noticed the inconsistencies but not had time to reason about them.

Should we use Index for the panicing impls? get_item should then always return a Result.

I would 100% support implementing Index. Rather than get_item returning Result, it's perhaps possible also to mirror Vec::get() which returns Option and also does slicing? We might want to play around with that to figure out what feels nice.

Should PyList support negative indices (we'd have to implement the correction ourselves)? If yes, the same should apply to PyTuple.

I think Python users would appreciate this. Note that if we implemented Index<usize> above then we could implement Index<isize> later...

If we take only "real" indices, should the type switch to usize?

I think the function signature should mirror possible usage, so yes, usize if positive indices only, isize` if we allow negative-from-the-end lookup.

What about other APIs that take indices? E.g. set_item.

I wonder if we can draw similar inspiration from std::Vec and friends.

[birkenfeld]

OK, I'll see if I can come up with a consistent plan later.

For now I'll just submit a PR that fixes the crash in PyList::get_item.

[birkenfeld]

I would rather not implement Index for usize and isize - this would require specifying a type for every tuple[0] expression. If we agree to support negative indices everywhere, I would just implement it only for isize.

So the initial plan is:

• implement Index<isize> for PyTuple, PyList and PySequence
• make PyTuple::get_item and PyList::get_item fallible
• support negative indices in PyTuple::slice, PyTuple::split_from and PyList::set_item
• add PyList::slice

[davidhewitt]

👍 sounds reasonable! Shall we target it for the 0.15 release so we can have some time to play around with it?

[birkenfeld]

Yes, I think that's a good plan.

[davidhewitt]

For future reference of anyone who wants to help with this proposal, see discussion in #1733. TLDR; we're going to go with using usize rather than isize everywhere because it's simpler and consistent with std.

[davidhewitt]

Note to self: I think this is a big enough change that it should go in the 0.15 migration guide.