[issue32932] better error message when __all__ contains non-str objects

[Xiang Zhang]

Change by Xiang Zhang :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Xiang Zhang]

Xiang Zhang  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Xiang Zhang]

Xiang Zhang  added the comment:

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 "", line 1031, in _handle_fromlist
  File "", 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'

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Brett Cannon]

Brett Cannon  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Brett Cannon]

Brett Cannon  added the comment:

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?

--
nosy: +barry

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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

--
nosy: +brett.cannon, eric.snow, ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Xiang Zhang]

Change by Xiang Zhang :


--
keywords: +patch
pull_requests: +5624
stage: needs patch -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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

--
nosy: +serhiy.storchaka
stage:  -> needs patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Xiang Zhang]

Xiang Zhang  added the comment:

s/AttributeError/TypeError

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32932] better error message when __all__ contains non-str objects

[Xiang Zhang]

New submission from Xiang Zhang :

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

--
components: Interpreter Core
messages: 312704
nosy: xiang.zhang
priority: normal
severity: normal
status: open
title: better error message when __all__ contains non-str objects
type: enhancement
versions: Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com