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

PEP 678: Make it possible to enrich an exception's error message #89770

Closed
iritkatriel opened this issue Oct 25, 2021 · 12 comments · Fixed by #30441
Closed

PEP 678: Make it possible to enrich an exception's error message #89770

iritkatriel opened this issue Oct 25, 2021 · 12 comments · Fixed by #30441
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@iritkatriel
Copy link
Member

BPO 45607
Nosy @gvanrossum, @aroberge, @Zac-HD, @iritkatriel
PRs
  • bpo-45607: Make it possible to enrich exception displays via setting their __note__ field #29880
  • 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 = <Date 2021-12-03.22:02:13.816>
    created_at = <Date 2021-10-25.20:38:01.298>
    labels = ['interpreter-core', 'type-feature', '3.11']
    title = "Make it possible to enrich an exception's error message"
    updated_at = <Date 2021-12-03.22:02:13.816>
    user = 'https://github.com/iritkatriel'

    bugs.python.org fields:

    activity = <Date 2021-12-03.22:02:13.816>
    actor = 'iritkatriel'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-12-03.22:02:13.816>
    closer = 'iritkatriel'
    components = ['Interpreter Core']
    creation = <Date 2021-10-25.20:38:01.298>
    creator = 'iritkatriel'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45607
    keywords = ['patch']
    message_count = 4.0
    messages = ['404999', '405016', '407365', '407607']
    nosy_count = 4.0
    nosy_names = ['gvanrossum', 'aroberge', 'Zac Hatfield-Dodds', 'iritkatriel']
    pr_nums = ['29880']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue45607'
    versions = ['Python 3.11']

    @iritkatriel
    Copy link
    Member Author

    The requirement comes from Hypothesis, see
    #28569 (comment)

    It is necessary there to add a note to an exception describing which test case it comes from. The note should be printed by __str__ of this exception.

    class Explanation(Exception):
        __module__ = "builtins"
        def __str__(self) -> str:
            return f"\n{self.args[0]}"

    try:
    why = "Failed!"
    raise AssertionError(why)
    except Exception as e:
    msg = " You can reproduce this error by ...\n ..."
    raise Explanation(msg) from e

        # Ideally something more like:
        e.__note__ = msg
        raise

    @iritkatriel iritkatriel added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Oct 25, 2021
    @Zac-HD
    Copy link
    Mannequin

    Zac-HD mannequin commented Oct 26, 2021

    This code shows my current best workaround based on a wrapper exception, with the traceback below annotating the additional details that I'd prefer to omit for clarity:

    $ python example.py
    Traceback (most recent call last):
      File "example.py", line 8, in <module>
        raise AssertionError(why)
    AssertionError: Failed!
                                                                            # These lines are
    The above exception was the direct cause of the following exception:    # confusing for new 
                                                                            # users, and they
    Traceback (most recent call last):                                      # only exist due 
      File "example.py", line 10, in <module>                               # to implementation
        raise Explanation(msg) from e                                       # via the Explanation
    Explanation:                                                            # wrapper type :-(
        You can reproduce this error by ...
        ...

    The motivation for this is that we'd like to use ExceptionGroup to indicate that MultipleFailures is a group of exceptions, and replace our current print()-based method of reporting the details of the inner exceptions.

    @iritkatriel
    Copy link
    Member Author

    bpo-28953 is another use case for this feature.

    @iritkatriel
    Copy link
    Member Author

    New changeset 5bb7ef2 by Irit Katriel in branch 'main':
    bpo-45607: Make it possible to enrich exception displays via setting their __note__ field (GH-29880)
    5bb7ef2

    @iritkatriel iritkatriel added the 3.11 only security fixes label Dec 3, 2021
    @iritkatriel iritkatriel added the 3.11 only security fixes label Dec 3, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @iritkatriel iritkatriel reopened this Apr 12, 2022
    @iritkatriel
    Copy link
    Member Author

    Reopening to implement PEP 678.

    @iritkatriel iritkatriel changed the title Make it possible to enrich an exception's error message PEP 678: Make it possible to enrich an exception's error message Apr 13, 2022
    @iritkatriel
    Copy link
    Member Author

    The implementation is complete, I am leaving this open for one more documentation PR (but that should not block the release).

    @godlygeek
    Copy link
    Contributor

    Was it a deliberate choice to not add a C API for exception notes? It seems to not be trivial to safely and correctly call add_note from a C extension module. I think it requires all this code:

    PyObject *type, *exc, *tb;
    PyErr_Fetch(&type, &exc, &tb);
    PyErr_NormalizeException(&type, &exc, &tb);
    PyObject* ret = PyObject_CallMethod(exc, "add_note", "s", "Some message");
    Py_XDECREF(ret);
    if (ret) {
        // Note successfully added, restore the modified exception.
        PyErr_Restore(type, exc, tb);
    } else {
        // Failed to add the note, set `__context__` on the new exception.
        Py_XDECREF(type);
        Py_XDECREF(tb);
        PyObject *context = exc;
        PyErr_Fetch(&type, &exc, &tb);
        PyErr_NormalizeException(&type, &exc, &tb);
        PyException_SetContext(exc, context);
        PyErr_Restore(type, exc, tb);
    }

    @gvanrossum
    Copy link
    Member

    If we added a dedicated C API, you'd still need to do error checking, and such an API wouldn't be taking care of fetching/restoring the exception (you'd just be passing it an exception object), so the only line that would be simpler would be

    PyObject* ret = PyObject_CallMethod(exc, "add_note", "s", "Some message");
    

    @godlygeek
    Copy link
    Contributor

    godlygeek commented Jul 19, 2022

    Hm. I was hoping for a void PyException_AddNote(PyObject*, const char*) API that calls add_note and, on failure, instead sets __context__, which would shorten that to:

    PyObject *type, *exc, *tb;
    PyErr_Fetch(&type, &exc, &tb);
    PyErr_NormalizeException(&type, &exc, &tb);
    PyException_AddNote(exc, "Some message");
    PyErr_Restore(type, exc, tb);

    but perhaps I'm being too focused on my own use case, and that wouldn't be sufficiently generic. I'm picturing add_note almost always being used during exception handling, and I'm imagining that a failure to call add_note should always lead to exception chaining.

    @gvanrossum
    Copy link
    Member

    Setting exc.__context__ would reverse the relationship between the exception being handled and the one raised by add_note() though, right?

    @godlygeek
    Copy link
    Contributor

    Yes, you're right __context__ would need to be set on the newly raised exception rather than the one passed to add_note in the case where add_note fails. And making that work would mean making a function that takes a PyObject** for the exception, instead, and allowing it to replace it if another exception occurs. And that's getting weird...

    OK, I see your point. I still think this is quite hard to use, but maybe the complexity is irreducible.

    @gvanrossum
    Copy link
    Member

    Thanks for leaving a complete snippet showing how to do this. If in the future we find that many people are trying to do this from C (currently we only have a few use cases, all of which are Python code) we may consider adding a C API to reduce the complexity for those people, and your snippet will be helpful in designing the right API then.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    Development

    Successfully merging a pull request may close this issue.

    3 participants