@slide tbh, I think it would be better to be fixed by checking if __cause__ attribute exists or available via some other means.

I am not sure if the PyErr_NormalizeException always does the right thing. I have two issues with it:

1. What if the exception class has __init__ method, that performs something totally not expected when a value is passed to it? For example, tries to do int(val), and the exception value happens to be something, that causes that to raise another exception?
2. If we do PyErr_NormalizeException, and completely discard the original values, subsequent PythonException.Restore would not return the error state to be exactly the same as before PythonException.

This function is called in PyErr_Print, which is why I thought it would be safe. Would you rather the call be moved to the Format method itself?

Can you clarify on behavior of PyErr_NormalizeException in regards to reference counts? Does it decref objects passed to it? Do the objects it returns have their refcount > 0? For example, if it can under some circumstances decref the objects passed to it, then you have to ensure to incref them, otherwise pointers in PythonException will become invalid.

A test for scenario 1 is still required (e.g. when exception class has __init__ that throws an exception of its own). If that throws in Format, what should we do in that case?

Codecov Report

Merging #1265 (04c8b66) into master (c81c3c3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1265   +/-   ##
=======================================
  Coverage   87.97%   87.97%           
=======================================
  Files           1        1           
  Lines         291      291           
=======================================
  Hits          256      256           
  Misses         35       35           

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c81c3c3...04c8b66. Read the comment docs.

The reference count stuff is something I tend to struggle with in regards to C# vs. Python stuff. :-) From looking at the source it looks like things would only be DECREF'd in case of an error or if the value is updated to be the correct value (https://github.com/python/cpython/blob/98c4433a81a4cd88c7438adbee1f2aa486188ca3/Python/errors.c#L353 and https://github.com/python/cpython/blob/98c4433a81a4cd88c7438adbee1f2aa486188ca3/Python/errors.c#L336).

In the case of an error, it looks like PyErr_NormalizeException will chain the new exception and try and normalize it. https://github.com/python/cpython/blob/98c4433a81a4cd88c7438adbee1f2aa486188ca3/Python/errors.c#L366. So, it seems like case 1 is covered internally in the normalization process. I can add something though just to make sure.

From my reading the code you linked, it looks like you need to incref pointers before sending them to _PyErr_NormalizeException, and then after the function you will own whatever is returned, so no need to incref after.

For example, they decref value here, and then replace it with fixed_value, which is a new reference from _PyErr_CreateException.

I have NOT abandoned this PR, just had some work stuff come up. I will revisit for item 1 above shortly.

I ran the same code on C Python 3.7.7 interpreter and got this, so the behavior matches:

Python 3.7.7 (tags/v3.7.7:d7c567b08f, Mar 10 2020, 10:41:24) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> class TestException(NameError):
...     def __init__(self, val):
...             super().__init__(val)
...             x = int(val)
...
>>> raise TestException('foo')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 4, in __init__
ValueError: invalid literal for int() with base 10: 'foo'

@slide I don't think the new test with __init__ actually uses normalization. What you need is to do PyErr_NormalizeException(TestException, "hello", null).

You're absolutely right, I was thinking of something else, this makes complete sense.

closing and reopening to get a new build

Thanks!