bpo-33237: Improve AttributeError message for partially initialized module.

[serhiy-storchaka]

https://bugs.python.org/issue33237

[brettcannon]

Very minor thing to fix, otherwise LGTM!

[brettcannon]

Curly braces around the statement.

[serhiy-storchaka]

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.

[brettcannon]

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. 😄

[serhiy-storchaka]

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__.

[brettcannon]

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().

[vstinner]

How can I trigger the modified code? The following circular import error doesn't seem to be covered by this change:

vstinner@apu$ cat a.py from b import B
class A: pass
vstinner@apu$ cat b.py from a import A
class B: pass
vstinner@apu$ ./python -c 'import a'
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/home/vstinner/prog/python/master/a.py", line 1, in <module>
from b import B
File "/home/vstinner/prog/python/master/b.py", line 1, in <module>
from a import A
ImportError: cannot import name 'A' from 'a' (/home/vstinner/prog/python/master/a.py)

I get the same traceback and exception with this change.

[serhiy-storchaka]

See an example on the tracker.

[serhiy-storchaka]

Restored support of non-module subclasses and added tests. @brettcannon, please make review again.

[vstinner]

LGTM. I really like the much better error message!

I just a few minor remarks on the implementation and the NEWS entry.

[vstinner]

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.

[serhiy-storchaka]

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.

[vstinner]

Ok, it makes sense.

[vstinner]

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

[serhiy-storchaka]

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.

[vstinner]

See an example on the tracker.

Ah thanks, I succeded to reproduce the error message and I like it :-)

[asottile]

@vstinner I made a patch to improve the circular-from import as well: #15308