Unclear whether __Pyx_PyDict_GetItemStr should clear exceptions

[jdemeyer]

We have

#if CYTHON_COMPILING_IN_CPYTHON && PY_VERSION_HEX >= 0x030500A1 && CYTHON_USE_UNICODE_INTERNALS
#define __Pyx_PyDict_GetItemStr(dict, name)  _PyDict_GetItem_KnownHash(dict, name, ((PyASCIIObject *) name)->hash)
#else
#define __Pyx_PyDict_GetItemStr(dict, name)  PyDict_GetItem(dict, name)
#endif

The problem is that the two variants have different semantics when an exceptions occurs during __eq__: the first variant sets an exception while the second clears it. So there are probably subtle bugs possible because of this difference.

So far, I'm just noting this issue, not fixing it. I do plan to add a new function __Pyx_PyDict_GetItemStrWithError which always keeps the exception.

[scoder]

I'll have to check, but I think it should only be used in cases where exceptions should get propagated. Swallowing exceptions is not a good idea for a general hash lookup. Sounds like we should use the PyObject_… variant as fallback instead.

[jdemeyer]

Some code is certainly written with the assumption that __Pyx_PyDict_GetItemStr won't raise an exception. For example, the old kwargs-parsing code (that's how I noticed the problem).

[scoder]

Hmm, right, I guess the usage follows the behaviour of PyDict_GetItem(). We should propagate the exception where we can, preferably without too much overhead for the "normal" cases (found or not, maybe also for KeyError).

[jdemeyer]

If you agree with the newly added function __Pyx_PyDict_GetItemStrWithError in #3021, this can be closed. Eventually, we should probably use __Pyx_PyDict_GetItemStrWithError everywhere (but that's certainly out of scope for #3021).

[scoder]

A reference leak due to calling PyObject_GetItem() was resolved in #3593.