Skip to content

bpo-45711: use exc_value instead of exc_type to determine if exc_info is valid. Add more assertions. #29627

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

Merged
merged 7 commits into from
Nov 25, 2021

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Nov 18, 2021

This brings us closer to removing exc_type from exc_info.

Where exc_type is used, I made it use exc_value instead and added assertions that this is ok. Then when we remove exc_type it will just be a matter of removing the assertions.

https://bugs.python.org/issue45711

@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 22, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit bc889bc 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 22, 2021
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I noticed that in some places you require type and value to be both "nullish" or both "not nullish" (where "nullish" means either NULL or Py_None), and in other places (in particular in _assert_exception_type_is_redundant()) they are required to both be the same nullishness (i.e., both NULL or both Py_None or neither of them nullish). Is there a reason for the difference?

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
@iritkatriel
Copy link
Member Author

iritkatriel commented Nov 24, 2021

I noticed that in some places you require type and value to be both "nullish" or both "not nullish" (where "nullish" means either NULL or Py_None), and in other places (in particular in _assert_exception_type_is_redundant()) they are required to both be the same nullishness (i.e., both NULL or both Py_None or neither of them nullish). Is there a reason for the difference?

Yes. Most of these assertions are verifying that the change right next to them is valid. So if I replace if(exc_type is nullish) by if(exc_value is nullish) then I only care that their nullishness is the same. In the case of the triplets pushed to the stack, it used to be that no-exc was pushed as "NULL, NULL, None" (None for type, NULL for value and tb). I change that in this PR (ceval.c lines 5907-9 sorry - 4182-88) so now it's "NULL, None, None". There are places where "None at the top of the stack" means there is no exc_info. So when we will remove the type we need the value to be None and not NULL, otherwise we'll get segfaults.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Okay, I'm through now. (Sorry for the premature send on the first batch.)

Python/errors.c Outdated
@@ -586,7 +633,18 @@ _PyErr_ChainStackItem(_PyErr_StackItem *exc_info)
exc2 = exc_info->exc_type;
Copy link
Member

Choose a reason for hiding this comment

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

While you're here could you change 'exc' and 'exc2' to 'type' and 'type2' (or 'typ' and 'typ2')? It drives me nuts that we mix "exc[exption]" and "typ[e]" for the same concept (this must date back to Python 1 when the value was not an exception instance and the exception could be any object).

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

All good! (Alas, I'm still stuck trying to wrap my head around the except* implementation, so that one will probably have to wait until next week.)

@iritkatriel iritkatriel merged commit c456dfa into python:main Nov 25, 2021
remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021
@iritkatriel iritkatriel deleted the bpo-45711-remove-exc_type_field branch January 13, 2022 15:34
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.

4 participants