Unless I am missing somethingPyUnicode_FromFormat can fail if the _PyUnicodeWriter fails to write the string. Please, add error checking around all calls toPyUnicode_FromFormat

There is no mention thatPyUnicode_FromFormat can fail athttps://docs.python.org/3/c-api/unicode.html#c.PyUnicode_FromFormat

Should I add a note about this?

Typo: dictionnary -> dictionary

This assignment should only be done ifkwds is a non-empty dict, otherwise we're risking increasing the size of exception instances bysys.getsizeof({}) bytes for no compelling reason (that's an extra 288 bytes on my machine, vs the current 88 bytes for aBaseException instance).

While calls likeSomeException() would getNULL for kwds, consider code likesuper().__new__(*some_args, **maybe_some_kwds) in exception subclasses.

Ifkwds is NULL, or an empty dict, thenself->kwargs should be set to theNone singleton.

Wouldn't it be good to do that forargs too?

val needs to be checked to make sure it is a mapping (withPyMapping_Check) orNone.

I thought about doing that but it seems to me thatPyMapping_Check is only a loose check, for example, wouldn't we be able to do:

>>> b = BaseException()>>> b.kwargs = ()

since tuples support slicing?

If so, is the check still worth it?

This reduce implementation concerns me, as it looks like it will make everything much slower, even for exception instances whereself->kwargs isn't set.

Instead, I'd recommend migratingBaseException away from implementing__reduce__ directly, and instead have it implement__getnewargs_ex__:https://docs.python.org/3/library/pickle.html#object.__getnewargs_ex__

That way the pickle machinery will take care of calling__new__ with the correct arguments, and you wouldn't need to introduce a weird dependency from a core builtin into a standard library module.

(That would have potential backwards compatibility implications for subclasses implementing reduce based on the parent class implementation, but the same would hold true for introduce a partial object in place of a direct reference to the class - either way, there'll need to be a note in the Porting section of the What's New guide, and switching to__get_newargs_ex__ will at least have the virtue of simplifying the code rather than making it more complicated)

I tried to do that, I removed__reduce__ and added__getnewargs_ex__ to the methods as:

staticPyObject*BaseException_getnewargs_ex(PyBaseExceptionObject*self,PyObject*Py_UNUSED(ignored)){PyObject*args=PyObject_GetAttrString((PyObject*)self,"args");PyObject*kwargs=PyObject_GetAttrString((PyObject*)self,"kwargs");if (args==NULL||kwargs==NULL) {returnNULL;    }returnPy_BuildValue("(OO)",args,kwargs);}

but it brocke pickling. Did I miss something?

Oh, found my mistake, using__getnewargs_ex__ broke pickling for protocols 0 and 1. Is this expected?

I don't think this happened when using a partial reference on the the constructor of the class.

Maybe it's ok to broke pickling support for protocols 0 and 1 since it was broken for keyword args anyway?

Defining__reduce_ex__ would let you restore the old behaviour for those protocols, but I'm not sure__getnewargs_ex__ will still be called if you do that (we're reaching the limits of my ownpickle knowledge).

@pitrou Do you have any recommendations here? (Context: trying to getBaseException to pickle keyword args properly, wanting to use__getnewargs_ex__ for more recent pickle protocols, but wondering how to handle the older protocols that don't use that)

How should I callobject.__reduce_ex__?

It seems to me that calling the builtin super is not done anywhere in the source code but I don't find the right way to do it.

Do I need to callobject___reduce_ex__ directly?

@ncoghlan Well, I'm not sure why you wouldn't implement the entire logic in__reduce_ex__, instead of also defining__getnewargs_ex__?

Or, rather, you could just define__getnewargs_ex__ and stop caring about protocols 0 and 1 (which are extremely obsolete by now, so we want to maintain compatibility, but fixing bugs in them is not important).

@pitrou I only suggested delegating to__getnewargs_ex__ because I wasn't sure how to mimic that behaviour from inside a custom__reduce_ex__ implementation.

But if__reduce__ still gets called for protocols 0 and 1 even when__getnewargs_ex__ is defined, then that's even better.

Hi@pitrou@ncoghlan, thanks for you input. I pushed a new commit that implement__getnewargs_ex__ but it seems that__reduce_ex__ does not check it and call__reduce__ no matter what the protocol is:

>>> BaseException().__reduce_ex__(0)(<class 'BaseException'>, ())>>> BaseException().__reduce_ex__(1)(<class 'BaseException'>, ())>>> BaseException().__reduce_ex__(2)(<class 'BaseException'>, ())>>> BaseException().__reduce_ex__(3)(<class 'BaseException'>, ())>>> BaseException().__reduce_ex__(4)(<class 'BaseException'>, ())>>> BaseException().__getnewargs_ex__()((), {})

If I remove the__reduce__, then it breaks pickling for protocols 0 and 1:

>>> BaseException().__reduce_ex__(0)Traceback (most recent call last):  File "<stdin>", line 1, in <module>  File "/Users/remi/src/cpython/Lib/copyreg.py", line 66, in _reduce_ex    raise TypeError(f"cannot pickle {cls.__name__!r} object")TypeError: cannot pickle 'BaseException' object>>> BaseException().__reduce_ex__(1)Traceback (most recent call last):  File "<stdin>", line 1, in <module>  File "/Users/remi/src/cpython/Lib/copyreg.py", line 66, in _reduce_ex    raise TypeError(f"cannot pickle {cls.__name__!r} object")TypeError: cannot pickle 'BaseException' object>>> BaseException().__reduce_ex__(2)(<function __newobj__ at 0x105c63040>, (<class 'BaseException'>,), None, None, None)>>> BaseException().__reduce_ex__(3)(<function __newobj__ at 0x105c63040>, (<class 'BaseException'>,), None, None, None)>>> BaseException().__reduce_ex__(4)(<function __newobj__ at 0x105c63040>, (<class 'BaseException'>,), None, None, None)

Do I need to define a custom__reduce_ex__ as well?

I dug further and it seems my issue comes fromhttps://github.com/python/cpython/blob/master/Lib/copyreg.py#L66, I will look into the details tomorrow.