Strings passed to KeyError do not round trip

[cc910417-cbb8-4329-ad1b-87b3b85acb64]

BPO	2651
Nosy	@rhettinger, @terryjreedy, @amauryfa, @abalkin, @pitrou, @merwok, @methane, @ambv, @Julian, @vadmium, @asottile, @bdoremus
Files	testExceptionStringRoundTrip.py: A test demonstrating the issue and showing that KeyError is arbitrarily different KeyError.patch issue2651.diff: Patch for py3k and stdlib

^{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: ```python assignee = 'https://github.com/ambv' closed_at = created_at = labels = ['interpreter-core', 'type-bug'] title = 'Strings passed to KeyError do not round trip' updated_at = user = 'https://bugs.python.org/rharris' ``` bugs.python.org fields: ```python activity = actor = 'Anthony Sottile' assignee = 'lukasz.langa' closed = True closed_date = closer = 'lukasz.langa' components = ['Interpreter Core'] creation = creator = 'rharris' dependencies = [] files = ['10048', '10069', '18372'] hgrepos = [] issue_num = 2651 keywords = ['patch'] message_count = 31.0 messages = ['65586', '65587', '65588', '65609', '65656', '66432', '107330', '111418', '111530', '112696', '112722', '112766', '112779', '112781', '112782', '112794', '112994', '113331', '154043', '240518', '242293', '274844', '274847', '274860', '274864', '275553', '304851', '322515', '322518', '323290', '376763'] nosy_count = 14.0 nosy_names = ['rhettinger', 'terry.reedy', 'amaury.forgeotdarc', 'belopolsky', 'pitrou', 'rharris', 'eric.araujo', 'methane', 'vencabot_teppoo', 'lukasz.langa', 'Julian', 'martin.panter', 'Anthony Sottile', 'bdoremus'] pr_nums = [] priority = 'normal' resolution = 'wont fix' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue2651' versions = ['Python 3.6'] ```

[cc910417-cbb8-4329-ad1b-87b3b85acb64]

Here is a bug in Python 2.5 which would be nice to fix for Py3k (since we are already breaking compatibility):

Take a string: s = "Hello"

Create a KeyError exception with that string: e = KeyError(s)

Counterintuitively, casting the exception to a string doesn't return the same string: str(e) != s

Instead, when KeyError is cast to a string it affixes single-quotes around the string.

I have create a test which shows that the other built-in exceptions (except for 3 Unicode Errors which seem to be unusual in that they don't accept just a string), do indeed round-trip the string unaltered.

This actually caused a bug (in an old version of zope.DocumentTemplate).

I am including the test case I wrote for now; I will begin looking into a solution shortly and hopefully whip up a patch.

[amauryfa]

Here is a relevant comment inside the KeyError_str function: / If args is a tuple of exactly one item, apply repr to args[0]. This is done so that e.g. the exception raised by {}[''] prints KeyError: '' rather than the confusing KeyError alone. The downside is that if KeyError is raised with an explanatory string, that string will be displayed in quotes. Too bad. If args is anything else, use the default BaseExceptionstr(). \/

Why is it so important to round trip?

[cc910417-cbb8-4329-ad1b-87b3b85acb64]

I think it is important to round-trip for at least two reasons:

1) Consistency. Other built-in exceptions behave this way, why should KeyError be any different? Okay, technically 3 UnicodeErrors don't allow just strings to be passed in (perhaps they should :-); but for common exception classes, they all behave the same way.

To quote PEP-20: "Special cases aren't special enough to break the rules"

2) Intuitiveness. Decorating the string with quotes is unexpected; it has already caused at least one bug and could cause more.

Ensuring intuitive round-trip behavior is an important enough issue that is explicitly discussed in PEP-327 for the decimal type.

Why can't intuitiveness be restored to KeyError in Py3K?

[amauryfa]

Here is another form of the same inconsistency:

>>> [].pop(0)
IndexError: pop from empty list
>>> {}.pop(0)
KeyError: 'pop(): dictionary is empty'

And my preferred one:
>>> unicodedata.lookup('"')
KeyError: 'undefined character name \'"\''

KeyError is special in that dict lookup raises the equivalent of KeyError(key). Since the key may be any kind of (hashable) object, it's preferable to repr() it.

I can see 3 solutions to the problem:

1- imitate IndexError for lists: the exception do not contain the key.

2- dict lookup builds the complete string message, and raise it raise KeyError("key not found: %r" % key) then KeyError.__str__ can be removed.

3- like IOError, KeyError has "msg" and "key" attributes. then dict lookup raises raise KeyError("key not found", key) and KeyError.__str__ is something like: if self.key is not None: return "%s: %r" % (self.msg, self.key) else return str(self.msg)

Choice 1 is not an improvement. Choice 2 has the correct behavior but leads to performance problems; KeyErrors are very very common in the interpreter (namespace lookups...) and formatting the message is costly. Choice 3 may cause regression for code that use exception.args[0], but otherwise seems the best to me. I'll try to come with a patch.

[amauryfa]

Attached patch changes KeyError: when it has two arguments, they are formatted with "%s: %r". Otherwise the base repr is called, and this allows the round trip.

Standard objects (dict, set, UserDict, namedtuple, defaultdict, weak dictionaries) now raise something like KeyError("not in dict", key).

At least one place in the stdlib relied on the key being the first argument to KeyError() (in ConfigParser.py)

I don't know if this incompatibility will break much code. At least we can say that the current behavior is not documented.

[pitrou]

Wouldn't it be nice to also store the offending key as a "key" attribute? Writing key_error.key is a lot intuitive than key_error.args[0] (or is it key_error.args[1] ? I've already forgotten :-)).

[abalkin]

KeyError.patch is out of date. Is anyone motivated enough to update it for py3k? I like the idea, but don't have spare cycles at the moment.

[ambv]

Alexander, Brett, I could update the patch but first I need thumbs up that this is going to be accepted and some eventual code breaks will be patched (again, I can do that but it has to be accepted on time).

Brett, what to do?

[83d2e70e-e599-4a04-b820-3814bbdb9bef]

@Łukasz: please provide an updated patch.

[ambv]

Patch for py3k ready. Includes patches for the stdlib where tests actually failed.

[merwok]

Looks like you didn’t narrow your diff command, since unrelated changes to configparser appear there. svn diff file1 path/file2 :)

[ambv]

Corrected patch attached. You're right, I left in ReST doc changes for configparser. Sorry for that.

[pitrou]

In KeyError_str, I think the following code shouldn't be deleted:

• if (PyTuple_GET_SIZE(self->args) == 1) {
• return PyObject_Repr(PyTuple_GET_ITEM(self->args, 0));

[ambv]

Patch updated to include a roundtrip test in test_exceptions.

[ambv]

FTR regarding for Antoine's comment above: that code should in fact be removed :)

[pitrou]

Latest patch looks good. Note that you could use PyUnicode_FromFormat() instead of building the format string manually.

[abalkin]

Unassigning because others seem to be ahead of me reviewing this patch. I will keep an eye on this, though.

Please note that I marked issue bpo-614557 to depend on this one. Adding 'key' attribute to exceptions is the subject of that issue.

While I support these features, I would really like to see a more comprehensive redesign as a part of PEP-3151. I think it would be a mistake to rush something in before PEP-3151 is implemented and then discover that it was not the best choice as a part of bigger picture.

[ambv]

For the record, I am -1 for this change after discussion on #python-dev. There are three major arguments against the proposed approach:

1. Moratorium. If we don't stick to the rules set by ourselves, nobody will take us seriously. No exceptions, this has to wait.

2. Backwards incompatibility. The current implementation is backwards incompatible and doesn't provide a reliable way of returning e.key. There was an alternative discussed about including key= and message= keyword arguments to the exception. This would require writing an additional helper routine in C for dealing with keyword arguments and is thus a bit of a broader problem.

3. PEP-3151. While still in draft form, this PEP prepares a solution for more generic and consistent behaviour of exceptions. It mentions IOError and OSError at the moment but Alexander is right: if the PEP-3151 approach chooses a new form of argument handling for exceptions, lookup errors should try to conform with that.

For the above reasons, I would resolve this issue as 'after moratorium' and prepare a superseder that gathers all PEP-3151 related issues in the tracker.

[cfc09b2d-670b-483f-9052-7f291f6b8939]

I'm +1 for fixing this behavior for the same reasons that are mentioned in the OP: consistency and predictability. I raised this issue as bpo-14086, and I was referred to this issue before closing mine as a duplicate.

It took me a while to figure out why I was getting unexpected escaped quotation marks in my strings, and it turned out that it was because I was passing strings back and forth as Exception arguments (tagging built-in Exceptions with a little bit of extra information when they occurred and re-raising), and every time that it occurred with a KeyError (and only with a KeyError), the string would grow another pair of quotation marks.

In my issue, I bring up the documentation in the Python Tutorial about Exception.args and Exception.__str(); it states very plainly and simply (as it should be) that the __str() method is there to be able to conveniently print Exception arguments without calling .args, and, when an unhandled Exception stops Python, the tail-end of the message (the details) of the exception will be the arguments that it was given. This is not the case with KeyError.

str(KeyError("Foo")) should be equal to "Foo", as it would be with any other Exception and as is the documented behavior of built-in Exceptions, at least in the tutorial (which I realize isn't the be-all, end-all document). The documented behavior makes more sense.

[vadmium]

The moratorium is over as far as I understand, and PEP-3151 (OSError changes) has already been implemented.

My understanding is that exception messages are not generally part of the API. I think that avoiding this surprising quirk is more important than retaining backwards compatibility with the formatted exception message. But if it cannot be changed, we can at least document the quirk.

[ambv]

Agreed that this can be addressed now for Python 3.5.

[rhettinger]

The current behavior seems to be a recurring source of confusion and bugs, so something needs to change. I'm thinking that the least egregious thing to do is to remove (in 3.6) the special case code for KeyError. The small downside is that KeyError('') wouldn't look as good. The benefit is that we eliminate an unexpected special case and make KeyErrors behave like all the other exceptions including LookupError.

I'm against the two-args solution as being too disruptive. Since args[0] is currently the only reliable way of extracting the key, prepending a message field will likely break much existing code that really wants to access the key.

[pitrou]

What are you suggesting? Something like:

KeyError: key 'foobar' not found in dict

? If so, +1 from me.

[ambv]

No, the suggestion is to only adopt the first part of the patch from 2010, which is to revert KeyError to behave like LookupError again:

  >>> print(LookupError('key'))
  key
  >>> print(KeyError('key'), 'now')
  'key' now
  >>> print(KeyError('key'), 'in 3.6')
  key in 3.6

In other words, there is no descriptive message while stringifying KeyError. Having an API with two arguments was disruptive because it moved the key from e.args[0] to e.args[1].

Raymond, would it be acceptable to create a two-argument form where the *second* argument is the message? That way we could make descriptive error messages for dicts, sets, etc. possible. In this world:

  >>> print(KeyError('key', 'key {!r} not found in dict'), 'in 3.6')
  key 'key' not found in dict in 3.6

Do you think any code depends on str(e) == str(e.args[0])?

[pitrou]

No, the suggestion is to only adopt the first part of the patch from 2010, which is to revert KeyError to behave like LookupError again

That ship has sailed long ago. 2.7, 3.4 and 3.5 (the three major Python versions currently in use) all have the same behaviour, and nobody seems to complain very loudly:

>>> {}['foo']
Traceback (most recent call last):
  File "<ipython-input-2-3761c7dc3711>", line 1, in <module>
    {}['foo']
KeyError: 'foo'

>>> KeyError('foo')
KeyError('foo')
>>> print(KeyError('foo'))
'foo'

[ambv]

So actually the issue long predates Python 2.5...

https://hg.python.org/cpython/rev/0401a0ead1eb

Now I'm not so sure it's worth touching it anymore ;)

[terryjreedy]

A new Stackoverflow question gives a better illustration of how special-casing KeyError can be a nuisance. https://stackoverflow.com/questions/46892261/new-line-on-error-message-in-idle-python-3-3/46899120#46899120 From a current repository build instead of 3.3:

>>> s = 'line\nbreak'
>>> raise Exception(s)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
Exception: line
break
>>> raise KeyError(s)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: 'line\nbreak'
>

The OP wanted to get the line break to break without fudging the code to catch Exception rather than KeyError. I proposed catching KeyError and then 'print(err.args[0]' instead of 'print(err)'.

Why this makes a difference, and why KeyError is unique in needing this, is obvious after I found this issue and read the code comment quoted by Amaury. But it sure wasn't before.

The rational for applying repr only applies when there is exactly one arg of value ''. So I think the fix should be to only apply it when args *is* ('',). There is no reason to quote a non-blank message -- and suppress any formatting a user supplies.

[8401f55c-0bc7-483b-ab7b-887f432fdfc6]

Did this patch die? I ran into the same issue noted in the SO post. It's bizarre that KeyError is the only error message to handle things this way.

[methane]

Even if we fixed stdlib, there are many KeyError(missing) idiom used in 3rd party code.

KeyError: '42' KeyError: '42\n' KeyError: 42

All of the above are looks different. I think it's good. Is it really worth enough to break it?

[ambv]

I agree with Inadasan. I was eager to fix this until I actually got to it at the '16 core sprint. I think there's too little value in fixing this versus possible backwards compatibility breakage.

[341ce0e2-bdbb-4bd4-99e6-746f11201a3f]

(I know this is old and closed but I've just run into the same thing with pwd.getpwnam in bpo-41767)

KeyError: "getpwnam(): name not found: 'test'"

would it be possible / make sense to extend KeyError with a named-only-argument which would preserve the raw string in this case?

something like (yeah the name is probably not great, I haven't had time to think much about this)

raise KeyError("getpwnam(): name not found 'test'", preserve_msg=True)