startswith and endswith don't accept None as slice index

[hkBst]

BPO	11828
Nosy	@gvanrossum, @rhettinger, @jcea, @ezio-melotti, @bitdancer
Files	issue-11828-v1.patch: Use _ParseTupleFinds() for count, startswith and endswith + tests issue-11828-v2.patch: Curly brackets adhere PEP 7 issue-11828-error-msg-tests.patch: Tests for wrong error messages in rfind(), etc. issue-11828-v3.patch: Complete patch for {unicode,bytes,bytearray}object issue-11828-v4-3.1.patch: Refined patch against 3.1 branch issue-11828-v4-2.7.patch: Refined patch against 2.7 branch 87be2a5e22ed.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/jcea'
closed_at = <Date 2011-04-20.15:59:51.700>
created_at = <Date 2011-04-11.15:09:08.234>
labels = ['type-bug']
title = "startswith and endswith don't accept None as slice index"
updated_at = <Date 2011-11-08.08:43:09.161>
user = 'https://bugs.python.org/hkBst'

bugs.python.org fields:

activity = <Date 2011-11-08.08:43:09.161>
actor = 'gvanrossum'
assignee = 'jcea'
closed = True
closed_date = <Date 2011-04-20.15:59:51.700>
closer = 'python-dev'
components = ['None']
creation = <Date 2011-04-11.15:09:08.234>
creator = 'hkBst'
dependencies = []
files = ['21620', '21626', '21627', '21629', '21655', '21656', '21739']
hgrepos = ['21', '22']
issue_num = 11828
keywords = ['patch']
message_count = 20.0
messages = ['133527', '133528', '133530', '133531', '133532', '133538', '133547', '133551', '133570', '133578', '133669', '133679', '133680', '133681', '133695', '133976', '134000', '134086', '134160', '147278']
nosy_count = 9.0
nosy_names = ['gvanrossum', 'rhettinger', 'jcea', 'ezio.melotti', 'r.david.murray', 'santoso.wijaya', 'python-dev', 'torsten.becker', 'hkBst']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue11828'
versions = ['Python 3.1', 'Python 2.7', 'Python 3.2', 'Python 3.3']

[hkBst]

startswith and endswith don't accept None as slice index, as shown by below interaction. Same behavior for python-3.1.3(with print()) and python-2.7.1. If instead this is intended behavior then the error message is wrong.

$ python -c "print 'abc'[-1:None]"
c
$ python -c "print 'abc'.endswith('c',-1,None)"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: slice indices must be integers or None or have an __index__ method

[hkBst]

I remark that find' and index' do accept None:

$ python3 -c "print('abc'.find('c',None,None))"
2

[jcea]

Since this is not a regression, this would be solved in 3.3 only. I guess.

[bitdancer]

Well, if it is judged a bug (and it seems to me that it is), then it can get fixed in 2.7 and 3.2 (and yes I did confirm that the same bug is present in 2.7).

It appears to be the result of startswith/endswith applying naive parsing to their arguments rather than the more sophisticated stringlib calls used by find and index.

[rhettinger]

Letting None be used as an index has always been treated as a feature request; however, in this case, it can be deemed a bug because the error message is making a promise that isn't kept.

It would be nice if once and for all this got solved by making argument parsing codes for indices and slices; otherwise, we're doomed to get some variant of this feature request over and over again.

FWIW, Google's code search shows that this slicing feature for startswith/endswith was of dubious worth -- no one seems to uses it. The feature should probably have not been added in the first place.

Now that it is in, we still need to handle this report because people will need to make method wrappers for string-like classes.

[jcea]

Anybody taking care of this, should check also "byte" and "bytearray".

[torstenbecker]

Hi, I started working on a first patch for this. A function _ParseTupleFinds() exists which does the proper parsing for this kind of arguments in unicodeobject.c, I adapted it to be usable for startswith() and endswith() besides find() and friends.

In issue-8282-v1.patch I fixed this for startswith() and endswith(). count() suffered from the same behavior and I updated it there as well.

[torstenbecker]

While working on this, I discovered anther problem. find(), etc. all use the same parsing function (_ParseTupleFinds()). So when an error occurs, the exception message will always start with "find()" even though index() or rfind() might have caused the error:

>>> "asd".index("x", None, None, None)
TypeError: find() takes at most 3 arguments (4 given)

I attached a patch (issue-8282-error-message-tests.patch) which adds test cases for the wrong error messages.

I was thinking about fixing this as well but wanted make sure my approach is correct first:

• I would like to add another argument to _ParseTupleFinds(): const char * function_name
• in _ParseTupleFinds(): allocate a buffer of 50 chars on the stack to hold "O|OO:" + function name
• copy "O|OO:" into buffer
• copy max(strlen(function_name), 44) chars from function_name into buffer
• use buffer as format argument of PyArg_ParseTuple()
• change all calls of _ParseTupleFinds to include the function name as first argument

Would that approach work with Python's C style or are there any Python-specific helper functions I could use?

[torstenbecker]

Just realized that part of my v1 patch did not conform to PEP-7, I hope, I fixed that in v2.

Please also excuse for the wrong name of the error message patch, it was supposed to be named "issue-11828-error-msg-tests.patch".

[torstenbecker]

Hi, since nobody stopped me by complaining about the approach or the first patch, I now fixed this for bytes and bytearray as well. :)

I renamed the old _ParseTupleFinds function to stringlib_parse_tuple_finds, added a parameter for function name, and another if it shall do unicode conversion. I used this helper function throughout all 3 files now.

I am new to writing C code for Python, so any comments on how to improve the patch are welcome.

[jcea]

Some comments posted in the review.

Could you possibly post a patch for 2.7 too?.

Thanks.

If any other reviewer agree, I will commit the patch.

[torstenbecker]

Some comments posted in the review.

I'm not sure if my review reply got mailed as I did not get a copy and nothing showed up here. I added some responses/follow up questions in the review.

Could you possibly post a patch for 2.7 too?.

Sure, I'll write the next version against 3.3 and 2.7

[jcea]

I got your comments, Torsten. I finds funny too that the tracker is not notified.

I wrote new comments too, but not using "the right way", so now I am the one not sure you got them... :-)

Go for the 3.3/2.7 patch. Thanks.

[jcea]

Better to have a 3.1/2.7 patch. The current workflow requires to patch the old version first (3.1), and up-port the change to 3.2 and 3.3.

So, 2.7 and 3.1 would be more useful. Al least if the patch applies to 3.2 and 3.3 easily. If major surgery is needed, let me know.

PS: If you use mercurial, try to upload the patch directly from it. See the "Remote hg repo" box.