bpo-31956: Add start and stop parameters to array.index() #4435

Phaqui · 2017-11-17T07:27:57Z

https://bugs.python.org/issue31956

serhiy-storchaka

Just take list.index as an example.

serhiy-storchaka · 2017-11-17T07:33:14Z

Modules/arraymodule.c

@@ -1115,18 +1115,42 @@ array_array_count(arrayobject *self, PyObject *v)
 array.array.index

    v: object
+    start: Py_ssize_t = 0
+    stop: Py_ssize_t = 9223372036854775807


What does this 9223372036854775807 mean? I have idea, but this looks totally confusing.

And there is an integer overflow on 32-bit platform.

I found it somewhere (in cpython), though I can't remember where. I'll try to readjust with clinic, taking list_index() as an example, and commit the updates asap.

EDIT: Updated.

bedevere-bot · 2017-11-17T07:34:27Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Phaqui · 2017-11-17T08:00:53Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-17T08:00:54Z

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

serhiy-storchaka · 2017-11-17T08:47:10Z

Modules/arraymodule.c

+        stop = len;
+    }
+
+    for (i = start; i < stop; i++) {


An arbitrary Python code can be executed in PyObject_RichCompareBool(). It can change the size of the array. As result, the index can become out of the array limits.

Use the same loop condition as in list.index to prevent this.

Yes, I was able to reproduce this, using an object that contained a reference to the array, which in its __eq__ mutates the array, causing an assertion failure. At this point in time, my code has boiled down to an almost identical copy of list_index, except I do not see the reason for truncating the stop index, as is done there.

serhiy-storchaka · 2017-11-17T08:47:44Z

Modules/arraymodule.c


-    for (i = 0; i < Py_SIZE(self); i++) {
+    len = Py_SIZE(self);
+


Remove redundant empty lines.

serhiy-storchaka · 2017-11-17T08:50:11Z

Modules/arraymodule.c

+
+    if (stop < 0) {
+        stop += len;
+    } else if (stop > len) {


"else if" should be on a separate line (PEP 7). But actually this branch is not needed due to the comment below.

serhiy-storchaka · 2017-11-17T09:01:29Z

Doc/library/array.rst

-   *x* in the array.
+   *x* in the array. The optional arguments *start* and *stop* can be specified to
+   search for *x* within a subsection of the array, but the returned index is
+   still relative to the start of the array, not the subsection.


I think the words "but the returned index is still relative to the start of the array, not the subsection" are redundant since this directly follows from the first sentence.

serhiy-storchaka

LGTM, but somebody other should review the documentation.

serhiy-storchaka · 2017-11-17T10:26:12Z

Doc/library/array.rst

+   Raises :exc:`ValueError` if *x* is not found.
+
+   .. versionchanged:: 3.7
+      Added optional start and stop parameters.


*start* and *stop*

serhiy-storchaka · 2017-11-17T10:27:51Z

Misc/NEWS.d/next/Library/2017-11-17-06-18-05.bpo-31956.m3Ylau.rst

@@ -0,0 +1,4 @@
+array.index() now supports optional start and stop arguments.
+As with general sequences, they can be negative, and the


Perhaps this is redundant too. This is common behavior of index() methods.

…omatically confirm to the standard, no need to point that out

vstinner · 2017-11-17T14:35:33Z

Doc/library/array.rst

@@ -188,11 +188,16 @@ The following data items and methods are also supported:
   array of some other type.


-.. method:: array.index(x)
+.. method:: array.index(x[, start[, stop]])


You can document the default start value:

array.index(x, start=0[, stop])

I would prefer to specify start=0, stop=sys.maxsize, but there were objections against this, because this looks like start and stop are keyword parameters. There were issues about changing the documentation back to [, start[, stop]].

vstinner · 2017-11-17T14:36:30Z

Modules/arraymodule.c

@@ -1115,18 +1115,31 @@ array_array_count(arrayobject *self, PyObject *v)
 array.array.index

    v: object
+    start: slice_index(accept={int}) = 0
+    stop: slice_index(accept={int}, c_default="PY_SSIZE_T_MAX") = sys.maxsize


Is it possible to not document the default value at Python level? Try to remove "= sys.maxsize".

Maybe you should put "/" (currently the following line) between start and stop?

It would be better to not make stop the keyword argument until other index() methods accept keyword arguments.

The signature of array.index() matches the signature of list.index() and this is the best that we can to do now. Due to limitations of pydoc, inspect and Argument Clinic the default value for stop is displayed as a concrete number, but this is a separate issue.

Oh, right, I didn't notice that other methods don't accept keywords.

>>> "abc".index("c", stop=3) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: index() takes no keyword arguments

I like the idea of accepting keyword arguments in other index() methods :-)

I think there is an open issue for this. But first I want unify the code for all index() methods. Currently index() methods of str/bytes/bytearray use slightly different code.

vstinner · 2017-11-17T14:37:13Z

Modules/arraymodule.c

    /

 Return index of first occurrence of v in the array.
+
+Raises ValueError if the value is not present.


Usually we don't document the exact exception raised by a method. Please remove this addition.

Maybe we should start to document them, but I would prefer to have a discussion about that, before starting to document them.

Docstrings of other index() methods document this.

vstinner · 2017-11-17T14:39:28Z

Modules/arraymodule.c

+            start = 0;
+    }
+    if (stop < 0) {
+        stop += Py_SIZE(self);


What is stop is still negative here?

Maybe you can use PySlice_AdjustIndices() here with step=1? What do you think @serhiy-storchaka?

All correct if stop is still negative. The following loop just will not be executed.

vstinner · 2017-11-17T14:40:35Z

Modules/arraymodule.c

+    if (stop < 0) {
+        stop += Py_SIZE(self);
+    }
+    for (i = start; i < stop && i < Py_SIZE(self); i++) {


I guess that you kept the "< Py_SIZE()" check if the array is mutated during the PyObject_RichCompareBool() call? If yes, please add a short comment to explain that, since it can be surprising.

I suggested to do this. This is an exact copy from list.index(). Do you want to add a comment in list.index()?

bedevere-bot · 2017-11-17T14:41:55Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2019-08-26T22:15:22Z

Ping @Phaqui: do you still plan to work on this PR? https://bugs.python.org/issue31956#msg350405 cc @nanjekyejoannah

nanjekyejoannah

@vstinner @serhiy-storchaka , I think the author of this PR is waiting for consesus from either or both of you following the discussion from BPO here : https://bugs.python.org/issue31956#msg350405

kamilturek · 2021-03-13T19:19:43Z

@Phaqui @vstinner @serhiy-storchaka
Is there any work planned to complete this PR? I am happy to take it over if necessary.

Phaqui · 2021-03-26T12:50:40Z

Sure, the code is mostly written. There were arguments about documentation, and details, and thus did not go in, but as far as I can remember, the code itself is pretty much list.index(), and should therefore in essence be ready to go.

Phaqui added 3 commits November 17, 2017 05:57

Implementation and first test cases

55d18ca

Updated documentation for array.index()

f9ddd58

Added news entry in NEWS.d/next/

94118b2

the-knights-who-say-ni added the CLA signed label Nov 17, 2017

bedevere-bot added the awaiting review label Nov 17, 2017

serhiy-storchaka requested changes Nov 17, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Nov 17, 2017

Clean up according to PR review

34692ba

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 17, 2017

serhiy-storchaka reviewed Nov 17, 2017

View reviewed changes

Phaqui added 2 commits November 17, 2017 11:20

Simplified documentation

a34e960

Fix crash when an object mutates the array in its __eq__ method

3ca98de

serhiy-storchaka approved these changes Nov 17, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 17, 2017

Phaqui added 2 commits November 17, 2017 11:31

*start* and *stop*

e6d75ae

Simplified news blurb. Having start and stop arguments means they aut…

d2b2f9f

…omatically confirm to the standard, no need to point that out

vstinner requested changes Nov 17, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Nov 17, 2017

serhiy-storchaka requested a review from rhettinger February 26, 2018 20:11

Merge branch 'master' into fix-issue-31956

d2755af

serhiy-storchaka added the type-feature A feature request or enhancement label Feb 26, 2018

nanjekyejoannah reviewed Jan 21, 2020

View reviewed changes

vstinner closed this May 3, 2021

vstinner deleted the branch python:master May 3, 2021 21:29

		@@ -0,0 +1,4 @@
		array.index() now supports optional start and stop arguments.
		As with general sequences, they can be negative, and the

Uh oh!

bpo-31956: Add start and stop parameters to array.index() #4435

bpo-31956: Add start and stop parameters to array.index() #4435

Uh oh!

Conversation

Phaqui commented Nov 17, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Phaqui Nov 17, 2017 • 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!

bedevere-bot commented Nov 17, 2017

Uh oh!

Phaqui commented Nov 17, 2017

Uh oh!

bedevere-bot commented Nov 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 17, 2017

Uh oh!

vstinner commented Aug 26, 2019

Uh oh!

nanjekyejoannah 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!

kamilturek commented Mar 13, 2021

Uh oh!

Phaqui commented Mar 26, 2021

Uh oh!

Uh oh!

Phaqui commented Nov 17, 2017 •

edited by bedevere-bot

Loading

Phaqui Nov 17, 2017 •

edited

Loading

nanjekyejoannah left a comment •

edited

Loading