classification
Title: Non-informative error message in index() and remove() functions
Type: behavior Stage: patch review
Components: Extension Modules, Interpreter Core Versions: Python 3.7
process
Status: open Resolution:
Dependencies: 7330 Superseder:
Assigned To: Nosy List: Jim Fasarakis-Hilliard, Julian, Sean.Ochoa, eamanu, ezio.melotti, haypo, merwok, ncoghlan, petri.lehtinen, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2011-11-05 20:49 by petri.lehtinen, last changed 2017-03-28 17:18 by serhiy.storchaka.

Files
File name Uploaded Description Edit
issue-13349.patch Sean.Ochoa, 2012-11-04 17:29 review
issue13349.patch.1 Sean.Ochoa, 2012-11-06 07:45 First update to issue patch. review
issue13349.patch.2 Sean.Ochoa, 2012-11-10 07:29 review
issue13349.patch.3 Sean.Ochoa, 2012-11-11 05:01 review
issue13349.patch.4 Sean.Ochoa, 2012-11-13 06:52 review
issue13349.patch.6 Sean.Ochoa, 2012-12-09 06:30 review
issue13349.patch.7 Sean.Ochoa, 2012-12-09 07:16 review
Pull Requests
URL Status Linked Edit
PR 876 open Jim Fasarakis-Hilliard, 2017-03-28 16:03
Messages (38)
msg147109 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-11-05 20:49
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 #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 #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

#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.
msg147215 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-11-07 10:08
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
msg147237 - (view) Author: Éric Araujo (merwok) * (Python committer) Date: 2011-11-07 16:42
> 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.
msg147238 - (view) Author: Éric Araujo (merwok) * (Python committer) Date: 2011-11-07 16:48
> 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
msg147431 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-11-11 12:37
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.
msg147546 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-11-13 03:11
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 ;-).
msg147795 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-11-17 12:11
É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.
msg147824 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-11-17 21:53
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.
msg147834 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-11-18 07:26
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?
msg147836 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-11-18 09:22
> 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.
msg147837 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-11-18 09:26
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/.
msg147853 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-11-18 11:41
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).
msg147859 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-11-18 12:01
Nick Coghlan wrote:
> 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.

Ok. This makes things easier.

> 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).

Created #13428.
msg174666 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-11-03 18:41
Working on issue as part of Python Bug Day, Oct 2012.
msg174716 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-11-03 22:37
It seems that this has been fixed.  Using simple tests from msg147215:  

~/hg/cpython/working $ env-py3.3/bin/python
Python 3.3.0 (default, Nov  3 2012, 15:28:29) 
[GCC 4.7.2] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> [].index(list(range(1000)))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: [0, 1, 2, 3, 4, 5, ... 995, 996, 997, 998, 999] is not in list
[60649 refs]
>>> class Foo: pass
... 
[60679 refs]
>>> [].index(Foo())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: <__main__.Foo object at 0x7f7fad31bc30> is not in list
[60677 refs]
msg174744 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2012-11-04 02:52
The original example was about tuples and displaying 'x' instead of '4'. Have that and *all* the others been fixed?
msg174759 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-11-04 06:54
After discussing with folks on the #python-dev tonight, I learned that I was testing with a list and I should've been using a set.  I'm working on a patch now, and I'm almost ready to have it reviewed.
msg174820 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-11-04 17:29
Ready for review.
msg174821 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-11-04 17:48
You should use assertRaises/assertRaisesRegex to check the error message and add more tests to cover all the changes.  I also noticed an inconsistence between 'element not in container' and 'element is not in container' (note the extra 'is').  FWIW I prefer the version without 'is'.
msg174958 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-11-06 07:45
Tests updated for coverage and to use assertRaisesRegex.
msg175134 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-11-08 05:51
From Taggnostr on #python-dev:  

1.) Use assertRaises+assertIn instead of assertRaisesRegex
2.) Add or change an existing test that you've already updated to use  elements with a repr longer than 100 chars.
msg175265 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-11-10 06:54
Truncating repr strings to 100chars will require the patch from #7330.  After applying the patch from that issue, my tests work fine.
msg175266 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-11-10 07:29
Updated patch after taking into account Ezio's (aka Taggnostr on #python-dev) latest feedback.
msg175319 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-11-11 05:01
Updates after feedback from Serhiy.
msg175325 - (view) Author: Julian Berman (Julian) * Date: 2012-11-11 06:19
test_issuewhatever is a bit indescriptive.

It'd be easier for someone glancing at the test to see what it claims to be testing if there were a more descriptive name given, like perhaps test_exception_message (or something even more verbose).
msg175328 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-11 07:58
Or test_index()/test_remove().  In this case positive test cases (and may be other exception types) should be added.
msg175489 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-11-13 06:52
Lib/test/test_array.py
  -- Moved index test (for this issue) to test_index (existing test method).
  -- Added remove test (for this issue) to test_remove (existing test method)

Lib/test/test_deque.py
  -- Moved remove test (for this issue) to test_index (existing test method).

Lib/test/test_list.py
  -- Added test_remove instead of test_issue13349, and test_index as a new test method; both with positive tests and a negative test to for this issue.

Lib/test/test_tuple.py
  -- Added test_remove instead of test_issue13349 as a new test method with a positive test and a negative test case for this issue to cover both items longer than 100chars and shorter items.

Lib/test/test_xml_etree.py
  -- Added test_remove as a new test method to TreeBasicOperationTest testcase class instead of test_issue13349, with a positive test case and the negative test to cover this issue.
msg175491 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-13 08:35
See also my comments to previous patch about repr() and error message checking.
msg177198 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-12-09 06:30
Update based on Taggnostr's (Ezio on IRC, if I recall correctly) feedback from 11/12/2012 around 11:57 PST.  

* Added check for index result in positive tests.
* Added assertIn check for remove result in positive tests.
* Removed extra whitespace.
* Formatted comments to be more concise.
msg177203 - (view) Author: Sean Ochoa (Sean.Ochoa) Date: 2012-12-09 07:16
* Fixed issue with test name in Lib/test/test_tuple.py
* Fixed issue with test_remove in Lib/test/test_list.py to assertNotIn instead of assertIn for positive case.

Confirmed with Ezio that issue #7330 will need to be fixed/approved before this issue can be closed.
msg188598 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-05-06 21:36
> Confirmed with Ezio that issue #7330 will need to be fixed/approved before this issue can be closed.

FYI I just fixed this issue. PyUnicode_FromFormat() now supports width and precision in the format string.
msg289856 - (view) Author: Jim Fasarakis-Hilliard (Jim Fasarakis-Hilliard) * Date: 2017-03-19 15:31
@Sean Ochoa, do you want to make this into a PR? The only tweak I would suggest would be to change all error messages to either be:

    "object.method(repr(x)): element not in object" 

or:

    "repr(x) not in object"

also, this probably needs to be changed to version 3.7 now.
msg290020 - (view) Author: Emmanuel Arias (eamanu) * Date: 2017-03-23 02:44
I agree with Jim Fasarakis-Hilliard this message may be change in new 3.7 version
msg290492 - (view) Author: Jim Fasarakis-Hilliard (Jim Fasarakis-Hilliard) * Date: 2017-03-25 17:40
Additional instances of this:

 - function indexOf of operator.py.
 - function _PySequence_IterSearch of abstract.c
msg290742 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-28 16:54
>>> [1, 2, 3].index("'")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: ""'"" not in list

Aren't too much quotes?
msg290745 - (view) Author: Jim Fasarakis-Hilliard (Jim Fasarakis-Hilliard) * Date: 2017-03-28 17:06
Yes, that does look like too much. My rationale for adding quotes around the value was in order to make it more clear in cases where the repr exceeds 100 characters. 

Instead of:

   Traceback (most recent call last):
     File "<stdin>", line 1, in <module>
   ValueError: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 2 not in list

Have it clearly distinguished by using "":

   Traceback (most recent call last):
     File "<stdin>", line 1, in <module>
   ValueError: "[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 2" not in list

I'm not sure if there's a trivial way to not display so many quotes in the case you supplied, you're better suited to decide if this can be done somehow.
msg290747 - (view) Author: Jim Fasarakis-Hilliard (Jim Fasarakis-Hilliard) * Date: 2017-03-28 17:14
Could use `%.100S` instead of `%.100R` but I'm not sure of the downsides that might entail.
msg290748 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-28 17:18
Error messages should use repr(). For example str() of bytes object emits a warning or error.

See issue26090 for the issue with truncated reprs.
History
Date User Action Args
2017-03-28 17:18:44serhiy.storchakasetmessages: + msg290748
2017-03-28 17:14:08Jim Fasarakis-Hilliardsetmessages: + msg290747
2017-03-28 17:06:49Jim Fasarakis-Hilliardsetmessages: + msg290745
versions: + Python 3.7, - Python 3.3
2017-03-28 16:54:33serhiy.storchakasetmessages: + msg290742
2017-03-28 16:03:19Jim Fasarakis-Hilliardsetpull_requests: + pull_request775
2017-03-25 17:40:29Jim Fasarakis-Hilliardsetmessages: + msg290492
2017-03-23 02:44:14eamanusetnosy: + eamanu
messages: + msg290020
2017-03-19 15:31:32Jim Fasarakis-Hilliardsetnosy: + Jim Fasarakis-Hilliard
messages: + msg289856
2017-03-19 15:19:47serhiy.storchakalinkissue29853 superseder
2013-05-06 21:36:30hayposetnosy: + haypo
messages: + msg188598
2012-12-09 07:16:18Sean.Ochoasetfiles: + issue13349.patch.7

messages: + msg177203
2012-12-09 06:30:48Sean.Ochoasetfiles: + issue13349.patch.6

messages: + msg177198
2012-11-13 08:35:09serhiy.storchakasetmessages: + msg175491
2012-11-13 06:52:36Sean.Ochoasetfiles: + issue13349.patch.4

messages: + msg175489
2012-11-11 07:58:30serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg175328
2012-11-11 06:19:53Juliansetnosy: + Julian
messages: + msg175325
2012-11-11 05:01:57Sean.Ochoasetfiles: + issue13349.patch.3

messages: + msg175319
2012-11-10 16:59:30serhiy.storchakasetdependencies: + PyUnicode_FromFormat: implement width and precision for %s, %S, %R, %V, %U, %A
2012-11-10 07:29:36Sean.Ochoasetfiles: + issue13349.patch.2

messages: + msg175266
2012-11-10 06:54:47Sean.Ochoasetmessages: + msg175265
2012-11-08 05:51:26Sean.Ochoasetmessages: + msg175134
2012-11-06 07:45:18Sean.Ochoasetfiles: + issue13349.patch.1

messages: + msg174958
2012-11-04 17:48:01ezio.melottisetmessages: + msg174821
stage: needs patch -> patch review
2012-11-04 17:29:24Sean.Ochoasetfiles: + issue-13349.patch
keywords: + patch
messages: + msg174820
2012-11-04 06:54:59Sean.Ochoasetmessages: + msg174759
2012-11-04 02:52:48terry.reedysetmessages: + msg174744
2012-11-03 22:37:03Sean.Ochoasetmessages: + msg174716
2012-11-03 18:41:57Sean.Ochoasetnosy: + Sean.Ochoa
messages: + msg174666
2012-11-03 18:39:04merwoksettitle: Uninformal error message in index() and remove() functions -> Non-informative error message in index() and remove() functions
2011-11-18 12:01:46petri.lehtinensetmessages: + msg147859
2011-11-18 11:41:21ncoghlansetmessages: + msg147853
2011-11-18 09:26:40petri.lehtinensetmessages: + msg147837
2011-11-18 09:22:11ezio.melottisetmessages: + msg147836
2011-11-18 07:26:15petri.lehtinensetmessages: + msg147834
2011-11-17 21:53:43terry.reedysetmessages: + msg147824
2011-11-17 12:11:29petri.lehtinensetmessages: + msg147795
2011-11-13 03:11:10terry.reedysetnosy: + terry.reedy
messages: + msg147546
2011-11-11 12:37:34ncoghlansetnosy: + ncoghlan
messages: + msg147431
2011-11-07 16:48:59merwoksetmessages: + msg147238
2011-11-07 16:43:00merwoksetnosy: + merwok
messages: + msg147237
2011-11-07 10:08:51petri.lehtinensetmessages: + msg147215
2011-11-07 07:19:05ezio.melottisetnosy: + ezio.melotti

stage: needs patch
2011-11-05 20:50:45petri.lehtinensettype: behavior
components: + Extension Modules, Interpreter Core
versions: + Python 3.3
2011-11-05 20:49:37petri.lehtinencreate