Tuple/List get_item to fallible by aviramha · Pull Request #1733 · PyO3/pyo3

aviramha · 2021-07-19T20:02:27Z

Hey, trying to help with:
#1667
specifically:
make PyTuple::get_item and PyList::get_item fallible
I'm not quite sure about the extract thingy, any guidance or feedback will be welcome.

davidhewitt

Thanks for kicking this off!

Another way to design this which is potentially better for migration is to deprecate .get_item() and to introduce two new methods:

.get() -> PyResult<&PyAny>
.get_unchecked() -> &PyAny, doesn't do checking and only available on #[cfg(not(Py_LIMITED_API))]

Or .get() -> Option<&PyAny>, if handling the error internally feels appropriate. I'd be open to opinions on this.

Regarding the .extract() error, you should check for uses of .get_item() inside pyo3-macros-backend. TBH this is an example why deprecating get_item might be the better idea - the compilation will still go through and you'll just get warnings of where it's used, which can be fixed instead.

src/types/tuple.rs

aviramha · 2021-07-22T17:25:12Z

@davidhewitt
Ping!

davidhewitt

Thanks, this is taking shape!

@birkenfeld as you came up with the proposals in #1667 I'd be really interested for your opinion on this.

src/types/list.rs

src/types/tuple.rs

mejrs

Thanks :)

I do have some suggestions..

mejrs · 2021-07-22T20:39:52Z

src/types/list.rs

    }

+    /// Gets the list item at the specified index.
+    pub fn get(&self, index: usize) -> PyResult<&PyAny> {


We should consider using isize rather than usize for this, if we still want negative indexing per #1667 (?).

We want negative indexing, but when using index (slicing). There's no real sense in implementing it at this function level.

Personally I'm leaning towards using usize everywhere, anyway 👍

I don't see why this should only be relevant for slicing. Python allows negative indices for single item access as well as for slices. IMO either we allow negative indices for both, or none.

src/types/list.rs

src/types/tuple.rs

birkenfeld

I'm happy with returning PyResult. The new name makes the API asymmetric (get vs set_item, and btw why is there is no del_item or remove...) Are we still allowed to break APIs if the result is a clear compile error?

usize vs isize needs to be discussed still.

A follow-up to this is the implementation of Index traits.

birkenfeld · 2021-07-25T07:02:17Z

src/types/list.rs

    }

+    /// Gets the list item at the specified index.
+    pub fn get(&self, index: usize) -> PyResult<&PyAny> {


I don't see why this should only be relevant for slicing. Python allows negative indices for single item access as well as for slices. IMO either we allow negative indices for both, or none.

birkenfeld · 2021-07-25T07:02:52Z

src/types/list.rs

-            let item = self.list.get_item(self.index);
+        if self.index < self.list.len() {
+            #[cfg(any(Py_LIMITED_API, PyPy))]
+            let item = self.list.get(self.index).expect("tuple.get failed");


It's a list here

davidhewitt · 2021-07-25T08:54:44Z

The new name makes the API asymmetric (get vs set_item, and btw why is there is no del_item or remove...) Are we still allowed to break APIs if the result is a clear compile error?

Yes, we could change get_item in 0.15. I do agree we should keep consistent names; maybe it is better to have get_item and get_item_unchecked here. I think I prefer having isize because it's more similar to the Python semantics, but I could be persuaded either way. IIRC the underlying C-API only accepts unsigned integers, so we'd need to do the conversion (but that's not too bad)...

Sorry @aviramha you picked a ticket which has a lot of open design questions 🙃

I definitely think if we call it .get() then we should accept usize only.

aviramha · 2021-07-25T12:35:23Z

Sorry @aviramha you picked a ticket which has a lot of open design questions 🙃

That's OK. the get_item of Python is actually usize, so it might be more confusing that it works with negative indices. I guess it's a question whether you want the API to feel like a Rust wrapper to the CPython or a rust wrapper to the Python API.
Let me know which you prefer and I'll fix the PR.

davidhewitt · 2021-07-25T15:58:39Z

That's a fair question. I think I'm now leaning towards this matching the C API, so would vote for get_item(usize) -> PyResult<&PyAny> and unsafe get_item_unchecked(usize) -> &PyAny.

aviramha · 2021-07-25T16:02:27Z

That's a fair question. I think I'm now leaning towards this matching the C API, so would vote for get_item(usize) -> PyResult<&PyAny> and unsafe get_item_unchecked(usize) -> &PyAny.

Goes without saying that I am too. How do we make this call? 🙃

davidhewitt · 2021-07-25T16:11:45Z

If it looks like a majority of us agree, I'm happy for us to commit to this plan (cc @birkenfeld @mejrs @messense).

birkenfeld · 2021-07-25T16:21:54Z

Where would isize be allowed then? This should really be designed keeping all places with indices in mind (see #1667), not decided on a case-by-case basis just looking at the C API, which is inconsistent. For example, PyList_Insert accepts negative indices.

birkenfeld · 2021-07-25T16:23:47Z

To be clear, I'm not opposed to doing away with isize everywhere and only accepting "real" indices, Rust-std-style. I just want it to be consistent.

aviramha · 2021-07-25T16:25:51Z

@birkenfeld I understand your concern but I believe that's CPython inconsistency issue. It depends on what we value more - being consistent with it or improving it.

aviramha · 2021-07-25T17:08:44Z

To make myself more clear (previous comment was from mobile) - when coding in PyO3 I often use the CPython docs to understand how stuff works - having different API (i.e get_item of PyO3 accepts negative indices vs CPython not) can easily cause confusion for anyone.

birkenfeld · 2021-07-25T17:19:04Z

The expectation of each PyO3 method mapping exactly to one Python C-API call is misguided, though. If you are missing info from the PyO3 docs which is integral to understanding the API in question, it is a bug and should be fixed; please report that here.

And of course, in the case we're discussing here, the documentation would need to state that negative indices are supported and how they are interpreted. (That this is not documented currently is one of the points of #1667.)

davidhewitt · 2021-07-25T17:22:47Z

I definitely agree that we should go for all-isize, or all-usize. It's not fun as a user to have to look up argument types for every function. (And the right document source should definitely be PyO3 docs, not CPython docs.)

Here's the arguments I can see either way:

usize

Good:

Matches std
Means we can use the SliceIndex trait to allow doing list[0..5] etc

Bad:

PySequence and PyList_Insert APIs work correctly with negative indices, but we'd be preventing users from doing so.

isize

Good:

Matches python usage, e.g. list[-5]
We'd have to work around APIs that don't support negative indexing. It's probably as simple as using PySequence_GetItem instead of PyList_GetItem, and similar, but not sure. Some edge cases might be annoying.

Bad:

Supporting negative indexing presumably has a (tiny) performance overhead, because negative indices must be converted to actual positive ones.
To have slices with negative indices such as list[-4..-2] we'd probably need to create a PySliceIndex trait which is implemented for various Range<isize>, RangeFrom<isize>, RangeFull<isize>. Could be a lot of work.
PyList::get_item_unchecked would almost certainly want to take usize. Either that or it could have a simple check if idx < self.len() { idx += self.len() }, But that would again imply performance overhead, which kinda sucks in an unsafe low-level API.

An alternative... what if we made PyList take usize and PySequence take isize? That way we'd get the simplicity of usize for most usage but also give users the "full API" if they chose to treat the list as a sequence. We'd have to document this really clearly, but at least there's then a clear pattern to it.

I think this "mixed-mode" would match the C-API in every way except PyList::insert, which we would make only take usize.

mejrs · 2021-07-25T22:15:12Z

@davidhewitt thanks for the recap

Indexing and slicing with negative numbers doesn't seem like a good idea to me:

For someone not coming from Python, having negative indices work when indexing/slicing is something that I feel will mostly just be surprising and unexpected.
For someone who is coming from Python (to Rust), I think it'll be confusing if negative indexing works somewhere (pyo3) but not in other places (the rest of the Rust ecosystem).
It will only be a convenience for people making Python libraries rather than the people using said libraries. After all, authors can implement negative slicing and indexing for their own types.
I'm not a fan of doing a bunch of math tricks in what should be a low level api.

aviramha · 2021-08-07T13:38:23Z

Cool. @davidhewitt Waiting for your review :

davidhewitt · 2021-08-07T23:27:20Z

👍 will try to give a full read within the next few days!

davidhewitt

Thanks for pushing forward on this! I'm afraid there's a few bits and pieces which still need tidying up. Comments scattered below.

@davidhewitt should I change get_slice to also use usize?

Before we release 0.15 we'll need to change all these tuple / list / sequence APIs which currently take isize to take usize. This PR is already big enough, so maybe it's better to make a list of what still needs to change in #1667 and then make follow-up PRs.

CHANGELOG.md

benches/bench_list.rs

benches/bench_tuple.rs

pyo3-macros-backend/src/from_pyobject.rs

src/types/list.rs

src/types/tuple.rs

aviramha · 2021-08-13T13:08:53Z

Thanks for the review. I think I fixed all of the comments :)

davidhewitt

The changes are looking reasonable to me... but CI has a segfault?

CHANGELOG.md

src/types/list.rs

src/types/tuple.rs

src/types/list.rs

aviramha · 2021-08-14T14:44:07Z

Oopsie, I thought the CI failure was main's fault

davidhewitt

Ahh I think I've found another cause of segfault... Fingers crossed CI green after 🤞

src/types/list.rs

davidhewitt

👍 looks brilliant! Thanks again for your patience and many iterations as we worked out the design for this.

I'll merge this shortly (just going to cherry-pick some fixes for a 0.14.3 bugfix release first).

davidhewitt · 2021-08-15T23:02:14Z

(See #1796 - as soon as that release is live I'll merge this. It just makes my life easier if I try to cherry-pick other stuff and there's no more breaking changes 😉)

birkenfeld

LGTM.

aviramha · 2021-08-16T05:56:56Z

👍 looks brilliant! Thanks again for your patience and many iterations as we worked out the design for this.

I'll merge this shortly (just going to cherry-pick some fixes for a 0.14.3 bugfix release first).

You're welcome! Glad to contribute to an awesome framework.
Where do we wish to track progress on this so I might pick up the next task?

davidhewitt · 2021-08-16T07:22:22Z

You're welcome! Glad to contribute to an awesome framework.
Where do we wish to track progress on this so I might pick up the next task?

🚀

If you want to own this, that would be awesome! The easiest way might be to create a new issue where you can own a list of checkboxes and close them as we make progress?

birkenfeld · 2021-08-17T12:48:47Z

Since 0.14.x is going to be in a branch, can we merge this?

davidhewitt · 2021-08-17T13:03:59Z

Ok 👍 I'm getting more used to doing the cherry-picking onto the 0.14.3 branch now, so let's proceed with this.

I see I created a merge conflict by merging #1802, so I'll fixup this quickly now...

…`usize` indices instead of `isize`. - `PyList` and `PyTuple`'s `get_item` now always uses the safe API. See `get_item_unchecked` for retrieving index without checks.

davidhewitt reviewed Jul 20, 2021

View reviewed changes

src/types/tuple.rs Outdated Show resolved Hide resolved

src/types/tuple.rs Outdated Show resolved Hide resolved

aviramha force-pushed the list_stuff branch from cc21dbe to ea9b7e0 Compare July 22, 2021 17:22

aviramha force-pushed the list_stuff branch 4 times, most recently from 8e5484e to c4216a1 Compare July 22, 2021 17:49

aviramha requested a review from davidhewitt July 22, 2021 17:49

aviramha force-pushed the list_stuff branch from c4216a1 to 86e6f0b Compare July 22, 2021 17:54

davidhewitt reviewed Jul 22, 2021

View reviewed changes

src/types/list.rs Outdated Show resolved Hide resolved

src/types/list.rs Outdated Show resolved Hide resolved

src/types/tuple.rs Outdated Show resolved Hide resolved

src/types/tuple.rs Outdated Show resolved Hide resolved

src/types/tuple.rs Outdated Show resolved Hide resolved

aviramha force-pushed the list_stuff branch 2 times, most recently from c598465 to 996b677 Compare July 22, 2021 20:43

aviramha requested a review from davidhewitt July 22, 2021 20:43

mejrs reviewed Jul 22, 2021

View reviewed changes

aviramha force-pushed the list_stuff branch from 996b677 to 9f73128 Compare July 23, 2021 03:49

aviramha requested a review from mejrs July 23, 2021 04:29

mejrs approved these changes Jul 24, 2021

View reviewed changes

birkenfeld requested changes Jul 25, 2021

View reviewed changes

davidhewitt requested changes Aug 13, 2021

View reviewed changes

aviramha force-pushed the list_stuff branch 2 times, most recently from 9880cb4 to 8cbf9f9 Compare August 13, 2021 13:08

aviramha requested a review from davidhewitt August 13, 2021 13:08

aviramha force-pushed the list_stuff branch from 8cbf9f9 to 68d3761 Compare August 13, 2021 13:36

davidhewitt reviewed Aug 14, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/types/list.rs Show resolved Hide resolved

src/types/tuple.rs Show resolved Hide resolved

davidhewitt reviewed Aug 14, 2021

View reviewed changes

src/types/list.rs Outdated Show resolved Hide resolved

aviramha force-pushed the list_stuff branch from 68d3761 to 3c497fc Compare August 14, 2021 14:43

aviramha force-pushed the list_stuff branch from 3c497fc to b53ef65 Compare August 14, 2021 14:48

davidhewitt reviewed Aug 14, 2021

View reviewed changes

src/types/list.rs Outdated Show resolved Hide resolved

aviramha force-pushed the list_stuff branch from b53ef65 to e716276 Compare August 15, 2021 05:34

davidhewitt approved these changes Aug 15, 2021

View reviewed changes

birkenfeld approved these changes Aug 16, 2021

View reviewed changes

birkenfeld mentioned this pull request Aug 17, 2021

Some PyList index fixes #1802

Merged

davidhewitt force-pushed the list_stuff branch 2 times, most recently from e32641a to 33b056b Compare August 17, 2021 13:23

- PyList, PyTuple and PySequence's get_item now accepts only …

c6255e6

…`usize` indices instead of `isize`. - `PyList` and `PyTuple`'s `get_item` now always uses the safe API. See `get_item_unchecked` for retrieving index without checks.

davidhewitt force-pushed the list_stuff branch from 33b056b to c6255e6 Compare August 17, 2021 14:01

davidhewitt merged commit fc90092 into PyO3:main Aug 17, 2021

aviramha deleted the list_stuff branch August 21, 2021 22:01

davidhewitt mentioned this pull request Aug 24, 2021

list,tuple,sequence: add slice indexing #1829

Merged

Conversation

aviramha commented Jul 19, 2021

Uh oh!

davidhewitt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aviramha commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mejrs left a comment

Choose a reason for hiding this comment

Uh oh!

mejrs Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

aviramha Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

mejrs Jul 24, 2021

Choose a reason for hiding this comment

Uh oh!

birkenfeld Jul 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

birkenfeld left a comment

Choose a reason for hiding this comment

Uh oh!

birkenfeld Jul 25, 2021

Choose a reason for hiding this comment

Uh oh!

birkenfeld Jul 25, 2021

Choose a reason for hiding this comment

Uh oh!

davidhewitt commented Jul 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviramha commented Jul 25, 2021

Uh oh!

davidhewitt commented Jul 25, 2021

Uh oh!

aviramha commented Jul 25, 2021

Uh oh!

davidhewitt commented Jul 25, 2021

Uh oh!

birkenfeld commented Jul 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

birkenfeld commented Jul 25, 2021

Uh oh!

aviramha commented Jul 25, 2021

Uh oh!

aviramha commented Jul 25, 2021

Uh oh!

birkenfeld commented Jul 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidhewitt commented Jul 25, 2021

usize

isize

Uh oh!

mejrs commented Jul 25, 2021

Uh oh!

aviramha commented Aug 7, 2021

Uh oh!

davidhewitt commented Aug 7, 2021

Uh oh!

davidhewitt left a comment •

edited

Loading

aviramha commented Jul 22, 2021 •

edited

Loading

davidhewitt commented Jul 25, 2021 •

edited

Loading

birkenfeld commented Jul 25, 2021 •

edited

Loading

birkenfeld commented Jul 25, 2021 •

edited

Loading