bpo-33237: Improve AttributeError message for partially initialized module. by serhiy-storchaka · Pull Request #6398

bpo-33237: Improve AttributeError message for partially initialized module. by serhiy-storchaka · Pull Request #6398 · python/cpython

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor thing to fix, otherwise LGTM!

		initializing = PyObject_IsTrue(value);
		Py_DECREF(value);
		if (initializing < 0)
		PyErr_Clear();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curly braces around the statement.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

See an example on the tracker.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I really like the much better error message!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it makes sense.

		int
		_PyModuleSpec_IsInitializing(PyObject *spec)
		{
		if (spec != NULL && spec != Py_None) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

See an example on the tracker.

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

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