Skip to content
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

Closed
rharris mannequin opened this issue Apr 17, 2008 · 31 comments
Closed

Strings passed to KeyError do not round trip #46903

rharris mannequin opened this issue Apr 17, 2008 · 31 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@rharris
Copy link
Mannequin

rharris mannequin commented Apr 17, 2008

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:

    assignee = 'https://github.com/ambv'
    closed_at = <Date 2018-08-08.15:10:39.497>
    created_at = <Date 2008-04-17.17:24:40.301>
    labels = ['interpreter-core', 'type-bug']
    title = 'Strings passed to KeyError do not round trip'
    updated_at = <Date 2020-09-12.02:12:36.168>
    user = 'https://bugs.python.org/rharris'

    bugs.python.org fields:

    activity = <Date 2020-09-12.02:12:36.168>
    actor = 'Anthony Sottile'
    assignee = 'lukasz.langa'
    closed = True
    closed_date = <Date 2018-08-08.15:10:39.497>
    closer = 'lukasz.langa'
    components = ['Interpreter Core']
    creation = <Date 2008-04-17.17:24:40.301>
    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']

    @rharris
    Copy link
    Mannequin Author

    rharris mannequin commented Apr 17, 2008

    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.

    @rharris rharris mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Apr 17, 2008
    @amauryfa
    Copy link
    Member

    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 BaseException__str__().
    */

    Why is it so important to round trip?

    @rharris
    Copy link
    Mannequin Author

    rharris mannequin commented Apr 17, 2008

    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"

    1. 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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    pitrou commented May 8, 2008

    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 abalkin self-assigned this May 28, 2010
    @abalkin
    Copy link
    Member

    abalkin commented Jun 8, 2010

    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
    Copy link
    Contributor

    ambv commented Jul 24, 2010

    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?

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 25, 2010

    @Łukasz: please provide an updated patch.

    @ambv
    Copy link
    Contributor

    ambv commented Aug 3, 2010

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

    @merwok
    Copy link
    Member

    merwok commented Aug 3, 2010

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

    @ambv
    Copy link
    Contributor

    ambv commented Aug 4, 2010

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

    @pitrou
    Copy link
    Member

    pitrou commented Aug 4, 2010

    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
    Copy link
    Contributor

    ambv commented Aug 4, 2010

    Patch updated to include a roundtrip test in test_exceptions.

    @ambv
    Copy link
    Contributor

    ambv commented Aug 4, 2010

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

    @pitrou
    Copy link
    Member

    pitrou commented Aug 4, 2010

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

    @abalkin
    Copy link
    Member

    abalkin commented Aug 5, 2010

    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.

    @abalkin abalkin removed their assignment Aug 5, 2010
    @ambv
    Copy link
    Contributor

    ambv commented Aug 8, 2010

    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.

    @vencabotteppoo
    Copy link
    Mannequin

    vencabotteppoo mannequin commented Feb 23, 2012

    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
    Copy link
    Member

    vadmium commented Apr 12, 2015

    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
    Copy link
    Contributor

    ambv commented Apr 30, 2015

    Agreed that this can be addressed now for Python 3.5.

    @ambv ambv self-assigned this Apr 30, 2015
    @rhettinger
    Copy link
    Contributor

    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
    Copy link
    Member

    pitrou commented Sep 7, 2016

    What are you suggesting? Something like:

    KeyError: key 'foobar' not found in dict

    ? If so, +1 from me.

    @ambv
    Copy link
    Contributor

    ambv commented Sep 7, 2016

    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
    Copy link
    Member

    pitrou commented Sep 7, 2016

    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
    Copy link
    Contributor

    ambv commented Sep 10, 2016

    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
    Copy link
    Member

    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.

    @bdoremus
    Copy link
    Mannequin

    bdoremus mannequin commented Jul 27, 2018

    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
    Copy link
    Member

    methane commented Jul 28, 2018

    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
    Copy link
    Contributor

    ambv commented Aug 8, 2018

    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.

    @ambv ambv closed this as completed Aug 8, 2018
    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Sep 12, 2020

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    9 participants