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

Make exception wrapping less intrusive for __set_name__ calls #77757

Closed
ncoghlan opened this issue May 19, 2018 · 7 comments · Fixed by #103402
Closed

Make exception wrapping less intrusive for __set_name__ calls #77757

ncoghlan opened this issue May 19, 2018 · 7 comments · Fixed by #103402
Labels
3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

ncoghlan commented May 19, 2018

BPO 33576
Nosy @ncoghlan, @carljm, @serhiy-storchaka, @sir-sigurd
PRs
  • bpo-33576: Remove exception wrapping from __set_name__ exceptions. #6983
  • 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 = None
    created_at = <Date 2018-05-19.05:41:19.566>
    labels = ['interpreter-core', 'type-feature', '3.8']
    title = 'Make exception wrapping less intrusive for __set_name__ calls'
    updated_at = <Date 2018-06-17.09:03:07.508>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2018-06-17.09:03:07.508>
    actor = 'sir-sigurd'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2018-05-19.05:41:19.566>
    creator = 'ncoghlan'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33576
    keywords = ['patch']
    message_count = 6.0
    messages = ['317098', '317104', '317115', '317123', '317126', '317143']
    nosy_count = 4.0
    nosy_names = ['ncoghlan', 'carljm', 'serhiy.storchaka', 'sir-sigurd']
    pr_nums = ['6983']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue33576'
    versions = ['Python 3.8']

    Linked PRs

    @ncoghlan
    Copy link
    Contributor Author

    Type creation currently wraps all exceptions raised by __set_name__ calls with a generic RuntimeError: https://github.com/python/cpython/blob/master/Objects/typeobject.c#L7263

    Unfortunately, this makes it difficult to use __set_name__ for descriptor validation operations, since it means the specific exception type gets lost, and it makes the traceback much harder to read (since the generic error messages appears at the end, while the actual error appears somewhere in the middle).

    See https://bugs.python.org/issue21145#msg317097 for a specific example.

    @ncoghlan ncoghlan added 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels May 19, 2018
    @carljm
    Copy link
    Member

    carljm commented May 19, 2018

    Nick, I think the reason this exception wrapping was added is because the stack trace for these exceptions is currently a bit lacking. The "caller" for the __set_name__ function is the class line for the class containing the descriptors.

    For exceptions raised inside __set_name__ this is kind of OK, although if a class has multiple instances of the same descriptor class on it, it doesn't give you an obvious clue which instance raised the exception ( though you can probably figure it out quickly enough by checking the value of the name argument).

    For cases where the exception is raised at the caller (e.g. a TypeError due to a __set_name__ method with wrong signature) it's worse; you get no pointer to either the problematic descriptor instance, or its name, or the class; all you get is the TypeError and the class that contains a broken descriptor.

    In practice I don't know how much of a problem this is; it doesn't seem likely that it would take too long to narrow down the source of the issue.

    Let me know what you think.

    @ncoghlan
    Copy link
    Contributor Author

    Hmm, I wonder if the UX problem with the current chaining might be solved in a different way, by doing something similar to what we did for codec exceptions (where we want to try to mention the codec name, but also don't want to change the exception type, and want to include the original exception text in the wrapper's message).

    The codecs related call is at

    /* Helper that tries to ensure the reported exception chain indicates the
    but most of the heavy lifting has since been refactored out into the _PyErr_TrySetFromCause helper function:
    _PyErr_TrySetFromCause(const char *format, ...)

    @serhiy-storchaka
    Copy link
    Member

    Unconditional replacing an exception is bad, because it can hide important exceptions like KeybordInterrupt or MemoryError.

    What if raise a new exception only if TypeError was raised? This will cover the case of a __set_name__() method with wrong signature.

    @carljm
    Copy link
    Member

    carljm commented May 19, 2018

    Awkwardly, the motivating use case in bpo-21145 is a TypeError that we wanted to raise within __set_name__, and not have replaced. It feels a little ugly to special case TypeError this way.

    I like the _PyErr_TrySetFromCause idea. That function is a bit ugly too, in the way it has to try and sniff out whether an exception has extra state or is safe to copy and add extra context to. But in practice I think the results would be pretty good here. Most of the time you’d get the original exception but with added useful context; occasionally for some exception types you might just not get the extra context. But as long as TypeError falls in the former category it would help with the worst case.

    I’ll look at using that in the PR.

    @ncoghlan
    Copy link
    Contributor Author

    Yeah, the "transparent exception chaining" code falls into the category of code that I'm both proud of (since it works really well in typical cases [1]) and somewhat horrified by (since the *way* it works tramples over several principles of good object oriented design and is completely non-portable to other Python implementations).

    Anyway, I've adjusted the issue title here to indicate that we don't want to remove the exception wrapping entirely, just make it less intrusive.

    [1] See the hex encoding and bz2 decoding failure examples in https://docs.python.org/3/whatsnew/3.4.html#codec-handling-improvements

    @ncoghlan ncoghlan changed the title Remove exception wrapping from __set_name__ calls Make exception wrapping less intrusive for __set_name__ calls May 19, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    iritkatriel added a commit to iritkatriel/cpython that referenced this issue Apr 9, 2023
    @iritkatriel
    Copy link
    Member

    The codecs wrapping was replaced by PEP-678 notes in #102407. We can do the same thing here. PR at #103402.

    @iritkatriel iritkatriel added 3.12 bugs and security fixes and removed 3.8 only security fixes labels Apr 9, 2023
    warsaw pushed a commit to warsaw/cpython that referenced this issue Apr 11, 2023
    aisk pushed a commit to aisk/cpython that referenced this issue Apr 18, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    4 participants