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

gh-99947: Ensure unreported error is chained for SystemError during import #99946

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

seberg
Copy link
Contributor

@seberg seberg commented Dec 2, 2022

Before this change the triggering exception was dropped.

Debugging a small issue in SciPy was made a bit tricky by the error
being cleared seemingly unnecessary here.

This ensures that module load errors chain their exceptions when
this occurs (similar to function calls).

Expanded the test to check that the `__cause__` is set where it seems
that it should be.
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Dec 2, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@seberg seberg changed the title ENH: Ensure unreported error is chained for SystemError during import gh-99947: Ensure unreported error is chained for SystemError during import Dec 2, 2022
Copy link
Member

@brettcannon brettcannon left a comment

Some minor tweaks, but otherwise I'm good with the proposed change.

self.load_module_by_name(name)

# If there is an unreported exception, it should be chained
Copy link
Member

@brettcannon brettcannon Dec 2, 2022

Choose a reason for hiding this comment

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

Suggested change
# If there is an unreported exception, it should be chained
# If there is an unreported exception, it should be chained with the `SystemError`.

self.load_module_by_name(name)

# If there is an unreported exception, it should be chained
if "unreported_exception" in name_base:
self.assertIsNot(cm.exception.__cause__, None)
Copy link
Member

@brettcannon brettcannon Dec 2, 2022

Choose a reason for hiding this comment

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

Suggested change
self.assertIsNot(cm.exception.__cause__, None)
self.assertIsNotNone(cm.exception.__cause__)

@bedevere-bot
Copy link

bedevere-bot commented Dec 2, 2022

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@@ -0,0 +1 @@
SystemErrors on import now chain the original unexpected exception
Copy link
Member

@brettcannon brettcannon Dec 2, 2022

Choose a reason for hiding this comment

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

Suggested change
SystemErrors on import now chain the original unexpected exception
Raising SystemError on import will now have its cause be set to the original unexpected exception.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

I agree with Brett. :)

Co-authored-by: Brett Cannon <brett@python.org>
@seberg
Copy link
Contributor Author

seberg commented Dec 3, 2022

Adopted the improvements, thanks. Its a minor help with debugging, but would have saved me time twice in the past 1-2 months.

EDIT: I guess the bot maybe expects exact wording "I have made the requested changes; please review again"

@seberg
Copy link
Contributor Author

seberg commented Dec 7, 2022

"I have made the requested changes; please review again"

EDIT: Oops, missed the review request and thought the bot labelling was confusing, sorry.

@bedevere-bot
Copy link

bedevere-bot commented Dec 7, 2022

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants