Skip to content

Use 'raise from' in initialization - #2112

Closed
virtuald wants to merge 2 commits into
pybind:masterfrom
robotpy:raise-from
Closed

Use 'raise from' in initialization#2112
virtuald wants to merge 2 commits into
pybind:masterfrom
robotpy:raise-from

Conversation

@virtuald

@virtualdvirtuald commented Feb 12, 2020

Copy link
Copy Markdown
Contributor

We ran into an issue where a unicode decode exception occurred during initialization, and it was difficult to figure out what was causing the exception without looking directly at the object. Unfortunately, pybind11 currently swallows all exceptions by only retaining the text. This would change it to use the equivalent of raise ImportError('something') from e.

There are several things to be considered here:

  • I think this mechanism was introduced in python 3.3?, need to add appropriate #ifdefs, not 100% sure the best way to go about this
  • Needs tests I assume?
  • Probably should apply this in other places where exceptions might be swallowed, but I think that's more appropriate for another PR?

Suggested changelog entry:

* Added ``py::raise_from`` to enable chaining exceptions

@virtuald

Copy link
Copy Markdown
ContributorAuthor

When y'all are reviewing things, I'd love some initial feedback on this idea.

@YannickJadoul

Copy link
Copy Markdown
Collaborator

We'll get to things in due time. Give us some time, please.

Comment threadinclude/pybind11/pytypes.h Outdated
@henryiii

Copy link
Copy Markdown
Collaborator

Can you rebase on master, to see what is failing?

@virtuald

Copy link
Copy Markdown
ContributorAuthor

Fails on Python 2.7, but as noted in the original description:

I think this mechanism was introduced in python 3.3?, need to add appropriate #ifdefs, not 100% sure the best way to go about this

@virtualdvirtuald changed the title WIP: Use 'raise from' in initializationUse 'raise from' in initializationFeb 13, 2021
@virtuald

virtuald commented Feb 13, 2021

Copy link
Copy Markdown
ContributorAuthor

I changed my mind about this implementation, I think it makes more sense to have a py::raise_from function instead of adding a causes function to error_already_set. Should be good to go now.

@virtuald
virtualdforce-pushed the raise-from branch 4 times, most recently from 91d80ee to f07809cCompareFebruary 13, 2021 23:21
@virtuald

Copy link
Copy Markdown
ContributorAuthor

Looks like all the tests will pass now except for the NVIDIA thing which failed because of a download issue.

@Skylion007
Skylion007 requested a review from henryiiiJuly 25, 2021 17:30
@virtuald
virtualdforce-pushed the raise-from branch 2 times, most recently from cd0eeff to 89c04e7CompareJuly 26, 2021 14:10
Comment threaddocs/advanced/exceptions.rst Outdated
Comment threaddocs/advanced/exceptions.rst Outdated
@henryiiihenryiii added this to the v2.8 milestone Jul 26, 2021
rwgk
rwgk approved these changes Aug 5, 2021

@rwgkrwgk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

/// Replaces the current Python error indicator with the chosen error, performing a
/// 'raise from' to indicate that the chosen error was caused by the original error
inline void raise_from(PyObject *type, const char *message) {
// from cpython/errors.c _PyErr_FormatVFromCause

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference it would be ideal to have a full link, even if it's long, e.g. (the code looks the same at first glance):

https://github.com/python/cpython/blob/8f010dc920e1f6dc6a357e7cc1460a7a567c05c6/Python/errors.c#L589

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked a bit a the history of errors.c, I think @virtuald's copy is most similar to the oldest version (467ab19 on Oct 21, 2016) that still has any blamed lines today:

https://github.com/python/cpython/blob/467ab194fc6189d9f7310c89937c51abeac56839/Python/errors.c#L405

The diff is:

--- commit_a_467ab194fc6 2021-08-23 16:31:10.128182694 -0700+++ copy_pr2112 2021-08-23 16:32:52.148562421 -0700@@ -1,26 +1,20 @@-static PyObject *-_PyErr_FormatVFromCause(PyObject *exception, const char *format, va_list vargs)-{- PyObject *exc, *val, *val2, *tb;-- assert(PyErr_Occurred());+inline void raise_from(PyObject *type, const char *message) {+ // from cpython/errors.c _PyErr_FormatVFromCause+ PyObject *exc = nullptr, *val = nullptr, *val2 = nullptr, *tb = nullptr;
PyErr_Fetch(&exc, &val, &tb);
+
PyErr_NormalizeException(&exc, &val, &tb);
- if (tb != NULL) {+ if (tb != nullptr) {
PyException_SetTraceback(val, tb);
Py_DECREF(tb);
}
Py_DECREF(exc);
- assert(!PyErr_Occurred());-- PyErr_FormatV(exception, format, vargs);+ PyErr_SetString(type, message);
PyErr_Fetch(&exc, &val2, &tb);
PyErr_NormalizeException(&exc, &val2, &tb);
Py_INCREF(val);
PyException_SetCause(val2, val);
PyException_SetContext(val2, val);
PyErr_Restore(exc, val2, tb);
-- return NULL;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For completeness, the 3 commits I looked at are:

467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 602) static PyObject *
b4bdecd0fc9 (Victor Stinner 2019-05-24 13:44:24 +0200 603) _PyErr_FormatVFromCause(PyThreadState *tstate, PyObject *exception,
b4bdecd0fc9 (Victor Stinner 2019-05-24 13:44:24 +0200 604) const char *format, va_list vargs)
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 605) {
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 606) PyObject *exc, *val, *val2, *tb;
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 607) b4bdecd0fc9 (Victor Stinner 2019-05-24 13:44:24 +0200 608) assert(_PyErr_Occurred(tstate));
b4bdecd0fc9 (Victor Stinner 2019-05-24 13:44:24 +0200 609) _PyErr_Fetch(tstate, &exc, &val, &tb);
b4bdecd0fc9 (Victor Stinner 2019-05-24 13:44:24 +0200 610) _PyErr_NormalizeException(tstate, &exc, &val, &tb);
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 611) if (tb != NULL) {
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 612) PyException_SetTraceback(val, tb);
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 613) Py_DECREF(tb);
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 614) }
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 615) Py_DECREF(exc);
b4bdecd0fc9 (Victor Stinner 2019-05-24 13:44:24 +0200 616) assert(!_PyErr_Occurred(tstate));
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 617) 438a12dd9d8 (Victor Stinner 2019-05-24 17:01:38 +0200 618) _PyErr_FormatV(tstate, exception, format, vargs);
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 619) b4bdecd0fc9 (Victor Stinner 2019-05-24 13:44:24 +0200 620) _PyErr_Fetch(tstate, &exc, &val2, &tb);
b4bdecd0fc9 (Victor Stinner 2019-05-24 13:44:24 +0200 621) _PyErr_NormalizeException(tstate, &exc, &val2, &tb);
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 622) Py_INCREF(val);
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 623) PyException_SetCause(val2, val);
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 624) PyException_SetContext(val2, val);
b4bdecd0fc9 (Victor Stinner 2019-05-24 13:44:24 +0200 625) _PyErr_Restore(tstate, exc, val2, tb);
467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 626) 467ab194fc6 (Serhiy Storchaka 2016-10-21 17:09:17 +0300 627) return NULL;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For easy future reference, this is the merged PR #3215 diff relative to the base version:

--- commit_a_467ab194fc6 2021-08-23 16:31:10.128182694 -0700+++ copy_final 2021-08-23 17:33:04.173103658 -0700@@ -1,19 +1,20 @@-static PyObject *-_PyErr_FormatVFromCause(PyObject *exception, const char *format, va_list vargs)-{- PyObject *exc, *val, *val2, *tb;+inline void raise_from(PyObject *type, const char *message) {+ // Based on _PyErr_FormatVFromCause:+ // https://github.com/python/cpython/blob/467ab194fc6189d9f7310c89937c51abeac56839/Python/errors.c#L405+ // See https://github.com/pybind/pybind11/pull/2112 for details.+ PyObject *exc = nullptr, *val = nullptr, *val2 = nullptr, *tb = nullptr;
assert(PyErr_Occurred());
PyErr_Fetch(&exc, &val, &tb);
PyErr_NormalizeException(&exc, &val, &tb);
- if (tb != NULL) {+ if (tb != nullptr) {
PyException_SetTraceback(val, tb);
Py_DECREF(tb);
}
Py_DECREF(exc);
assert(!PyErr_Occurred());
- PyErr_FormatV(exception, format, vargs);+ PyErr_SetString(type, message);
PyErr_Fetch(&exc, &val2, &tb);
PyErr_NormalizeException(&exc, &val2, &tb);
@@ -21,6 +22,4 @@
PyException_SetCause(val2, val);
PyException_SetContext(val2, val);
PyErr_Restore(exc, val2, tb);
-- return NULL;
}

@rwgk

rwgk commented Aug 23, 2021

Copy link
Copy Markdown
Collaborator

Hi all, I'll try to fix up the comment now as per my own suggestion as of a couple weeks ago. Hang on ...

@rwgk

rwgk commented Aug 24, 2021

Copy link
Copy Markdown
Collaborator

Unfortunately I cannot push to this branch, therefore I created a helper PR #3215.

rwgk added a commit that referenced this pull request Aug 24, 2021
* Add py::raise_from to enable chaining exceptions on Python 3.3+
* Use 'raise from' in initialization
* Documenting the exact base version of _PyErr_FormatVFromCause, adding back `assert`s.
Co-authored-by: Dustin Spicuzza <dustin@virtualroadside.com>
@rwgk

rwgk commented Aug 24, 2021

Copy link
Copy Markdown
Collaborator

Merged via #3215.

@rwgkrwgk closed this Aug 24, 2021
@virtuald
virtuald deleted the raise-from branch August 24, 2021 03:23
@rwgkrwgk mentioned this pull request Feb 10, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@virtuald@YannickJadoul@henryiii@rwgk@Skylion007