New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Strings passed to KeyError do not round trip #46903
Comments
Here is a bug in Python 2.5 which would be nice to fix for Py3k (since Take a string: Create a KeyError exception with that string: Counterintuitively, casting the exception to a string doesn't return the Instead, when KeyError is cast to a string it affixes single-quotes I have create a test which shows that the other built-in exceptions 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 |
Here is a relevant comment inside the KeyError_str function: Why is it so important to round trip? |
I think it is important to round-trip for at least two reasons:
To quote PEP-20: "Special cases aren't special enough to break the rules"
Ensuring intuitive round-trip behavior is an important enough issue that Why can't intuitiveness be restored to KeyError in Py3K? |
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 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 3- like IOError, KeyError has "msg" and "key" attributes. then dict Choice 1 is not an improvement. |
Attached patch changes KeyError: when it has two arguments, they are Standard objects (dict, set, UserDict, namedtuple, defaultdict, weak At least one place in the stdlib relied on the key being the first I don't know if this incompatibility will break much code. At least we |
Wouldn't it be nice to also store the offending key as a "key" attribute? |
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. |
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? |
@Łukasz: please provide an updated patch. |
Patch for py3k ready. Includes patches for the stdlib where tests actually failed. |
Looks like you didn’t narrow your diff command, since unrelated changes to configparser appear there. svn diff file1 path/file2 :) |
Corrected patch attached. You're right, I left in ReST doc changes for configparser. Sorry for that. |
In KeyError_str, I think the following code shouldn't be deleted:
|
Patch updated to include a roundtrip test in test_exceptions. |
FTR regarding for Antoine's comment above: that code should in fact be removed :) |
Latest patch looks good. Note that you could use PyUnicode_FromFormat() instead of building the format string manually. |
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. |
For the record, I am -1 for this change after discussion on #python-dev. There are three major arguments against the proposed approach:
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. |
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. |
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. |
Agreed that this can be addressed now for Python 3.5. |
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. |
What are you suggesting? Something like: KeyError: key 'foobar' not found in dict ? If so, +1 from me. |
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 |
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' |
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 ;) |
A new Stackoverflow question gives a better illustration of how special-casing KeyError can be a nuisance. >>> 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. |
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. |
Even if we fixed stdlib, there are many KeyError(missing) idiom used in 3rd party code. KeyError: '42' All of the above are looks different. I think it's good. |
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. |
(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 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) |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: