Non-informative error message in index() and remove() functions

[akheron]

BPO	13349
Nosy	@brettcannon, @rhettinger, @terryjreedy, @ncoghlan, @vstinner, @ezio-melotti, @merwok, @Julian, @akheron, @serhiy-storchaka, @DimitrisJim, @eamanu
PRs	bpo-13349: Fix error reporting for index and remove methods #876 bpo-13349: Improve the error message for tuple.index #1309 bpo-13349: repr object in tuple.index ValueError message #1820
Dependencies	bpo-7330: PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A
Files	issue-13349.patch issue13349.patch.1: First update to issue patch. issue13349.patch.2 issue13349.patch.3 issue13349.patch.4 issue13349.patch.6 issue13349.patch.7

^{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 = None
closed_at = <Date 2017-05-27.21:57:54.212>
created_at = <Date 2011-11-05.20:49:37.320>
labels = ['extension-modules', 'interpreter-core', 'type-bug', '3.7']
title = 'Non-informative error message in index() and remove() functions'
updated_at = <Date 2017-05-28.15:33:09.523>
user = 'https://github.com/akheron'

bugs.python.org fields:

activity = <Date 2017-05-28.15:33:09.523>
actor = 'Jim Fasarakis-Hilliard'
assignee = 'none'
closed = True
closed_date = <Date 2017-05-27.21:57:54.212>
closer = 'rhettinger'
components = ['Extension Modules', 'Interpreter Core']
creation = <Date 2011-11-05.20:49:37.320>
creator = 'petri.lehtinen'
dependencies = ['7330']
files = ['27884', '27907', '27940', '27949', '27975', '28265', '28267']
hgrepos = []
issue_num = 13349
keywords = ['patch']
message_count = 46.0
messages = ['147109', '147215', '147237', '147238', '147431', '147546', '147795', '147824', '147834', '147836', '147837', '147853', '147859', '174666', '174716', '174744', '174759', '174820', '174821', '174958', '175134', '175265', '175266', '175319', '175325', '175328', '175489', '175491', '177198', '177203', '188598', '289856', '290020', '290492', '290742', '290745', '290747', '290748', '294552', '294563', '294603', '294609', '294613', '294627', '294631', '294649']
nosy_count = 13.0
nosy_names = ['brett.cannon', 'rhettinger', 'terry.reedy', 'ncoghlan', 'vstinner', 'ezio.melotti', 'eric.araujo', 'Julian', 'petri.lehtinen', 'serhiy.storchaka', 'Sean.Ochoa', 'Jim Fasarakis-Hilliard', 'eamanu']
pr_nums = ['876', '1309', '1820']
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue13349'
versions = ['Python 3.7']

[akheron]

For example:

>>> (1, 2, 3).index(4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: tuple.index(x): x not in tuple

The "x not in tuple" error message should be replaced with "4 is not in tuple". list.index() already does this (see bpo-7252):

>>> [1, 2, 3].index(4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: 4 is not in list

Although in bpo-7252 it was claimed that no other place in stdlib has this error message, I found many occurences:

Modules/_collectionsmodule.c: PyErr_SetString(PyExc_ValueError, "deque.remove(x): x not in deque"
Modules/_elementtree.c: "list.remove(x): x not in list"
Modules/_elementtree.c: "list.remove(x): x not in list"
Modules/arraymodule.c: PyErr_SetString(PyExc_ValueError, "array.index(x): x not in list");
Modules/arraymodule.c: PyErr_SetString(PyExc_ValueError, "array.remove(x): x not in list");
Objects/abstract.c: "sequence.index(x): x not in sequence");
Objects/listobject.c: PyErr_SetString(PyExc_ValueError, "list.remove(x): x not in list");
Objects/tupleobject.c: PyErr_SetString(PyExc_ValueError, "tuple.index(x): x not in tuple");

There's also documentation and tests that depend on this actual error message:

Doc/library/doctest.rst: ValueError: list.remove(x): x not in list
Lib/test/test_xml_etree.py: ValueError: list.remove(x): x not in list

bpo-7252 was fixed in r76058, and it's quite a lot of code. I think it could be done more easily using PyUnicode_FromFormat() and the %R format.

[akheron]

The good thing about this is ease of debugging. You can see which is the offending value that was not found.

On the other hand, the repr of a value might be very long:

>>> [].index(list(range(1000)))
ValueError: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, ...
(many lines of numbers)
997, 998, 999] is not in list

Also, all values don't have a very informal repr:

>>> class Foo: pass
...
>>> [].index(Foo())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: <__main__.Foo object at 0xb736f92c> is not in list

[merwok]

The good thing about this is ease of debugging.
Exactly! +1 for the idea.

On the other hand, the repr of a value might be very long:
You can restrict the length with % formats.

Also, all values don't have a very informal repr:
Not your problem. This change will still be much more useful than the current 'x'. Some reprs are very helpful, other ones give the ID so can be used in debugging, other ones are not helpful at all so authors will have to make them more helpful or debug their code otherwise.

[merwok]

There's also documentation and tests that depend on this actual error message:
Doc/library/doctest.rst: ValueError: list.remove(x): x not in list
Lib/test>/test_xml_etree.py: ValueError: list.remove(x): x not in list
That’s a well-known doctest problem. Just update the doc. Writing robust doctests is an art:

>>> str(someobject)
'output that can change'
>>> 'something I want' in str(someobject)  # more robust, but less useful if it fails
True

>>> something.index(spam)
traceback blah:
ValueError: output that can change
>>> try: something.index(spam)
... except ValueError: print('spam not in something')  # more robust, but ugly
spam not in something

[ncoghlan]

doctests by their very nature tend to overspecify things - that's why actual regression tests should be written with unittest, while doctest is kept for its originally intended purpose of testing examples included in docstrings and other documentation.

Still, there's also a reason why IGNORE_EXCEPTION_DETAIL is an available doctest option.

[terryjreedy]

Improving error messages has been a long, slow process as people are irritated enough to make a change. Please go ahead.

Guido has explicitly excluded exception detail from the language spec multiple times. Test that depend on details are broken. Doc examples are updated as such details change. (So if you do upgrade the message, change the tests and examples ;-).

[akheron]

Éric Araujo wrote:

> On the other hand, the repr of a value might be very long:
You can restrict the length with % formats.

Seems you can't with %R. I'd also like to somehow indicate that
the output is truncated.

[terryjreedy]

The test suite has code (functions) that restrict the length on AssertEqual (and other) failure messages. (I do not know the location though.) Perhaps that can be reused. This almost seems like something that should be more easily available.

[akheron]

I found safe_repr() from Lib/unittest/util.py. We would require a similar function, just implemented in C.

What is a good place to define such C helpers that could be used everywhere?

[ezio-melotti]

I found safe_repr() from Lib/unittest/util.py.

The functions in Lib/unittest/util.py shouldn't be used outside unittest.

We would require a similar function, just implemented in C.
What is a good place to define such C helpers that could be used everywhere?

You could start by adding it in the file where you need it. If it starts becoming useful elsewhere too, it can then be moved somewhere else. I would expect such function to be private, so we are free to move it whenever we need it.

[akheron]

Ezio Melotti wrote:

You could start by adding it in the file where you need it. If it
starts becoming useful elsewhere too, it can then be moved somewhere
else. I would expect such function to be private, so we are free to
move it whenever we need it.

Well, msg147109 already lists 6 files that would use this function.
Some of them are under Objects&, some are under Modules/.

[ncoghlan]

Please don't stress too much about providing an indication that the repr has been truncated - it's an error message, not part of the normal program output. Besides, the lack of a closing ')', ']', '}' or '>' will usually indicate something is amiss in long reprs.

More useful would be to raise a separate feature request about the lack of width and precision handling for string formatting in PyUnicode_FromFormatV - the common format code handling means it *accepts* the width and precision information for string codes, but then proceeds to completely ignore it. It should be using them to pad short strings (width) and truncate long ones (precision), just like printf() (only in terms of code points rather than bytes, since those codes work with Unicode text).