| msg312704 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
Date: 2018-02-24 08:51 |
| I see people wrongly write non-str objects in __all__ and the error message for this case is simply a AttributeError which doesn't reveal the cause directly. >>> from test import * Traceback (most recent call last): File "", line 1, in TypeError: attribute name must be string, not 'C' It would be better to make the cause more obvious, like importlib._bootstrap._handle_fromlist does: Traceback (most recent call last): File "/root/cpython/Lib/test/test_importlib/import_/test_fromlist.py", line 166, in test_invalid_type_in_all self.__import__('pkg', fromlist=['*']) File "/root/cpython/Lib/importlib/_bootstrap.py", line 1094, in __import__ return _handle_fromlist(module, fromlist, _gcd_import) File "/root/cpython/Lib/importlib/_bootstrap.py", line 1019, in _handle_fromlist recursive=True) File "/root/cpython/Lib/importlib/_bootstrap.py", line 1014, in _handle_fromlist raise TypeError(f"Item in {where} must be str, " TypeError: Item in pkg.__all__ must be str, not bytes |
|
|
| msg312705 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2018-02-24 08:53 |
| s/AttributeError/TypeError |
|
|
| msg312707 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-02-24 09:36 |
| Agreed. Seems this was an oversight. Do you want to write a patch yourself or left it on to me? |
|
|
| msg312716 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-02-24 14:09 |
| Added import experts since this issue can be not so trivial as looked at first glance. |
|
|
| msg312743 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2018-02-24 19:05 |
| Fixing the message makes sense. I assume this is happening in ceval.c or import.c since https://github.com/python/cpython/blob/42c35d9c0c8175332f50fbe034a001fe52f057b9/Lib/importlib/_bootstrap.py#L1021 has the appropriate message? |
|
|
| msg312756 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-02-24 21:12 |
| I was wondering why the error is not raised by the IMPORT_NAME opcode which predates IMPORT_FROM. It calls _handle_fromlist() from _bootstrap. But in this case the module doesn't have the __path__ attribute and the sanity check was skipped. I'm wondering if it is enough to add the sanity check in _handle_fromlist() for the case when the module doesn't have the __path__ attribute. The Python code could be simpler than the C code. |
|
|
| msg312769 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2018-02-25 01:34 |
| I believe the original rationale for the `__path__` check was to restrict that branch to the case where we may need to import a not-yet-imported submodule in order to get the attribute set appropriately. However, giving a better error message for __all__ in ordinary modules also seems like a good reason to follow that branch. |
|
|
| msg312792 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-02-25 09:53 |
| On other hand, adding checks in Python code will add a slowdown. See which moves in contrary direction. |
|
|
| msg312832 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2018-02-25 18:38 |
| This is only for `import *`, though, right? So I would argue you're already tossing import perf out the window if you're willing to pollute your namespace like that. |
|
|
| msg312901 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2018-02-26 09:11 |
| I +1'ed Serhiy's patch for issue 32946, so we'll have to take that micro-optimisation into account if we decide to rely on the Python level `_handle_fromlist` to cover the "*" import case. Given that optimisation, it's probably simpler to just enhance the C error path to use the same error message as the Python version, though. |
|
|
| msg312906 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-02-26 09:44 |
| I was fooled by similarity of Python and C code, but actually Python and C code are not different implementations of the same algorithm, they have different purposes. The purpose of _bootstrap._handle_fromlist() is importing requested submodules first than `from pkg import submod1, submod2` change the outer locals/globals namespace. In the case of the star import it imports submodules enumerated in the package's __all__. The purpose of the IMPORT_STAR opcode is updating the globals namespace by the content of already imported module/package. Both codes iterate __all__ and use its items. The additional check in Python code was needed to prevent a BytesWarning. The additional check in IMPORT_STAR proposed in this issue is not strongly required. The exception of the correct type is already raised, and its message is not incorrect. But it can be improved to help fixing an obscure user error. I'm not very happy that we adds so much C code in ceval.c for handling a subtle error, but seems there is no other way (except keeping all as it is). |
|
|
| msg312983 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2018-02-27 08:51 |
| I would like the error message improved. The Python message is obviously much better than the C message. It sends the cause to your eyes. Different messages issued from similar statements are confusing. And due to the non-ideal message is generated by C, the traceback is more limited. tmpdir/ - __init__.py . __all__ = [1] - a.py . __all__ = [1] >>> from tmpdir import * Traceback (most recent call last): File "", line 1, in File "<frozen importlib._bootstrap>", line 1031, in _handle_fromlist File "<frozen importlib._bootstrap>", line 1026, in _handle_fromlist TypeError: Item in tmpdir.__all__ must be str, not int >>> from tmpdir.a import * Traceback (most recent call last): File "", line 1, in TypeError: attribute name must be string, not 'float' |
|
|
| msg314364 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2018-03-24 10:39 |
| New changeset d8b291a74284307610946f1b5801aa95d7f1e052 by Xiang Zhang in branch 'master': bpo-32932: More revealing error message when non-str objects in __all__ (GH-5848) https://github.com/python/cpython/commit/d8b291a74284307610946f1b5801aa95d7f1e052 |
|
|