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

Ensure python errors are propogated for Python->C++ #3671

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented Jan 31, 2022

Description

  • Ensures that any other exceptions that occur during function overload resolution propogates to the user if possible

Suggested changelog entry:

* Propagate Python exception traceback using ``raise_from`` if a pybind11 function runs out of overloads.

@Skylion007 Skylion007 changed the title Ensure python errors are propogated for C++->Python Ensure python errors are propogated for Python->C++ Jan 31, 2022
@Skylion007 Skylion007 merged commit ce18721 into pybind:master Jan 31, 2022
@Skylion007 Skylion007 deleted the more-type-error-info-2 branch January 31, 2022 20:13
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 31, 2022
@rwgk
Copy link
Collaborator

rwgk commented Jan 31, 2022

Looks great, thanks Aaron!
How much trouble is it to update a test to exercise this change? (The dispatcher code is very complex. Would be great to have an assurance that refactoring doesn't break this feature.)

@Skylion007
Copy link
Collaborator Author

@rwgk We would need an example to trigger it easily. (Maybe a dummy caster just sets an error and then fails to load)? One of the tests could already be triggering it, but I would need to check the __cause__ value on all the exceptions to verify.

@rwgk
Copy link
Collaborator

rwgk commented Jan 31, 2022

@rwgk We would need an example to trigger it easily. (Maybe a dummy caster just sets an error and then fails to load)? One of the tests could already be triggering it, but I would need to check the __cause__ value on all the exceptions to verify.

I played around a few minutes, adding assert()s. It turns out no existing test reaches the new branch. (To be sure I'm not fooling myself, I also put the assert() in the older similar code a few lines down, and that fired.)

Based on @Skylion007's comment, it seems to be quite a bit of trouble adding a test. I think it's fine to not invest the time, but it would be best to leave some low-key warning for people working on the code in the future. Here is a super simple idea:

#3673

What do you think?

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Feb 2, 2022
Sign up for free to 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.

3 participants