@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 thePyErr_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 doint(val), and the exception value happens to be something, that causes that to raise another exception?
2. If we doPyErr_NormalizeException, and completely discard the original values, subsequentPythonException.Restore would not return the error state to be exactly the same as beforePythonException.

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 ofPyErr_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 inPythonException 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 inFormat, what should we do in that case?

Codecov Report

Merging#1265 (04c8b66) intomaster (c81c3c3) willnot change coverage.
The diff coverage isn/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 byCodecov. Last updatec81c3c3...04c8b66. Read thecomment 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 andhttps://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 pointersbefore sending them to_PyErr_NormalizeException, and thenafter the function you will own whatever is returned, so no need to incref after.

For example, they decrefvaluehere, and then replace it withfixed_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 win32Type "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 doPyErr_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!