classification
Title: better error message when __all__ contains non-str objects
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: barry, brett.cannon, eric.snow, ncoghlan, serhiy.storchaka, xiang.zhang
Priority: normal Keywords: patch

Created on 2018-02-24 08:51 by xiang.zhang, last changed 2018-03-24 10:41 by xiang.zhang. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 5848 merged xiang.zhang, 2018-02-24 13:49
Messages (13)
msg312704 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) 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 "<stdin>", line 1, in <module>
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) * (Python committer) Date: 2018-02-24 08:53
s/AttributeError/TypeError
msg312707 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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: Nick Coghlan (ncoghlan) * (Python committer) 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) * (Python committer) Date: 2018-02-25 09:53
On other hand, adding checks in Python code will add a slowdown. See issue32946 which moves in contrary direction.
msg312832 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) 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: Nick Coghlan (ncoghlan) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 "<stdin>", line 1, in <module>
  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 "<stdin>", line 1, in <module>
TypeError: attribute name must be string, not 'float'
msg314364 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) 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
History
Date User Action Args
2018-03-24 10:41:16xiang.zhangsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-03-24 10:39:43xiang.zhangsetmessages: + msg314364
2018-02-27 08:51:54xiang.zhangsetmessages: + msg312983
2018-02-26 09:44:07serhiy.storchakasetmessages: + msg312906
2018-02-26 09:11:50ncoghlansetmessages: + msg312901
2018-02-25 18:38:20brett.cannonsetmessages: + msg312832
2018-02-25 09:53:21serhiy.storchakasetmessages: + msg312792
2018-02-25 01:34:02ncoghlansetmessages: + msg312769
2018-02-24 21:12:19serhiy.storchakasetmessages: + msg312756
2018-02-24 19:05:19brett.cannonsetnosy: + barry
messages: + msg312743
2018-02-24 14:09:54serhiy.storchakasetnosy: + brett.cannon, ncoghlan, eric.snow
messages: + msg312716
2018-02-24 13:49:45xiang.zhangsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request5624
2018-02-24 09:36:05serhiy.storchakasetnosy: + serhiy.storchaka

messages: + msg312707
stage: needs patch
2018-02-24 08:53:11xiang.zhangsetmessages: + msg312705
2018-02-24 08:51:48xiang.zhangcreate