GeezerVison

Curly braces around the statement.

What if test value == Py_True instead of PyObject_IsTrue(value)? This would simplify the code, and I don't think that we should expect any true value except True.

I'm torn. I do agree the code is simpler and you shouldn't expect it, but I also don't know if it's worth restricting the expectations that tightly. Then again, who is going to set this other than import itself?

IOW I don't have an opinion so whatever you prefer. 😄

And please note, that this PR changes the behavior of PyImport_ImportModuleLevelObject(). I have extracted the common code into _PyModule_IsInitializing(), but the code in PyImport_ImportModuleLevelObject() is not exactly the same as in module_getattro(). It used _PyObject_GetAttrId(mod, &PyId___spec__), but now it uses _PyDict_GetItemId(((PyModuleObject *)m)->md_dict, &PyId___spec__), and supports only the module type instances. I.e. __getattr__() will be now bypassed when looking up __spec__.

That is a breaking changing due to the fact that people could return any object for their module thanks to importlib.abc.Loader.create_module().

Why do you always clear the current exception? What if the function is called with an exception set? Maybe start with a "assert(!PyErr_Occurred());".

I prefer the old code which only clears the exception on error.

It is not wrong to call this function with NULL and an exception set. Checking this here saves several lines of code and/or indentation level at caller places.

The exception is cleared on error in most cases. The only exception from this rule is for _PyDict_GetItemId() returned NULL.

Ok, it makes sense.

Maybe just return 0 (without calling PyErr_Clear) if this condition is false? It would avoid one identation level :-)

We need to call PyErr_Clear() in all cases except spec == Py_None. The latter condition was added for mistake (left from old versions). I'll remove it.