better error message when __all__ contains non-str objects

[zhangyangyu]

BPO	32932
Nosy	@warsaw, @brettcannon, @ncoghlan, @ericsnowcurrently, @serhiy-storchaka, @zhangyangyu
PRs	bpo-32932: More revealing error message when non-str objects in __all__ #5848

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-03-24.10:41:16.825>
created_at = <Date 2018-02-24.08:51:48.133>
labels = ['interpreter-core', 'type-feature', '3.8']
title = 'better error message when __all__ contains non-str objects'
updated_at = <Date 2018-03-24.10:41:16.824>
user = 'https://github.com/zhangyangyu'

bugs.python.org fields:

activity = <Date 2018-03-24.10:41:16.824>
actor = 'xiang.zhang'
assignee = 'none'
closed = True
closed_date = <Date 2018-03-24.10:41:16.825>
closer = 'xiang.zhang'
components = ['Interpreter Core']
creation = <Date 2018-02-24.08:51:48.133>
creator = 'xiang.zhang'
dependencies = []
files = []
hgrepos = []
issue_num = 32932
keywords = ['patch']
message_count = 13.0
messages = ['312704', '312705', '312707', '312716', '312743', '312756', '312769', '312792', '312832', '312901', '312906', '312983', '314364']
nosy_count = 6.0
nosy_names = ['barry', 'brett.cannon', 'ncoghlan', 'eric.snow', 'serhiy.storchaka', 'xiang.zhang']
pr_nums = ['5848']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue32932'
versions = ['Python 3.8']

[zhangyangyu]

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

[zhangyangyu]

s/AttributeError/TypeError

[serhiy-storchaka]

Agreed. Seems this was an oversight. Do you want to write a patch yourself or left it on to me?

[serhiy-storchaka]

Added import experts since this issue can be not so trivial as looked at first glance.

[brettcannon]

Fixing the message makes sense. I assume this is happening in ceval.c or import.c since

cpython/Lib/importlib/_bootstrap.py

Line 1021 in 42c35d9

if not isinstance(x, str):

has the appropriate message?

[serhiy-storchaka]

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.

[ncoghlan]

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.

[serhiy-storchaka]

On other hand, adding checks in Python code will add a slowdown. See bpo-32946 which moves in contrary direction.

[brettcannon]

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.

[ncoghlan]

I +1'ed Serhiy's patch for bpo-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.

[serhiy-storchaka]

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

[zhangyangyu]

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'

[zhangyangyu]

New changeset d8b291a by Xiang Zhang in branch 'master':
bpo-32932: More revealing error message when non-str objects in __all__ (GH-5848)
d8b291a