Created on 2014-06-11 13:28 by davidszotten@gmail.com, last changed 2017-10-26 09:03 by serhiy.storchaka. This issue is now closed.
```
>>> __import__('fabric.', fromlist=[u'api'])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
```
accidentally ended up with something like this via some module that was using `unicode_literals`. stumped me for a second until i realised that my variable was a string, but not `str`. would be nice with a custom error message if this is a unicode string, explicitly mentioning that these must not be unicode or similar
Do you want to propose a patch? I think the standard message in these cases is along the lines of "TypeError: fromlist argument X must be str, not unicode"
I'm trying to replicate this issue. I do not get an error when I run
```
>>> __import__('datetime', fromlist=[u'datetime'])
```
any more information you could provide to help move this issue forward?
after some trial and error it only appears to break for 3rd party packages (all 20 or so i happened to have installed), whereas everything i tried importing from the standard library worked fine
```
>>> __import__('requests.', fromlist=[u'get'])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: Item in ``from list'' not a string
>>> __import__('os.', fromlist=[u'path'])
<module 'os' from '<snip (virtualenv)>/lib/python2.7/os.pyc'>
```
Interesting...I'll try to dig in and see what's going on.
first ever patch to python, so advice on the patch would be appreciated
found an example in the stdlib that triggers bug (used in test):
`__import__('encodings', fromlist=[u'aliases'])`
Thanks for the patch, David! + def test_fromlist_error_messages(self): + # Test for issue #21720: fromlist unicode error messages + try: + __import__('encodings', fromlist=[u'aliases']) + except TypeError as exc: + self.assertIn("must be str, not unicode", str(exc)) You could use assertRaises here: with self.assertRaises(TypeError) as cm: # ... self.assertIn('foo', str(cm.exception)) + if (PyUnicode_Check(item)) { + PyErr_SetString(PyExc_TypeError, + "Item in ``from list'' must be str, not unicode"); + Py_DECREF(item); + return 0; + } I think it would be better to improve the error message in Python/import.c: http://hg.python.org/cpython/file/2.7/Python/import.c#l2571 So you can safely remove this check.
not sure i follow. we need a different message if e.g. an integer is passed in updated the patch to only run the unicode check for non-strings or do you have a suggestion for an error message that works nicely in both cases?
Is there room for a better resolution: fix the API so that Unicode objects are accepted in the ‘fromlist’ items?
As far as I can tell, this isn't an issue on Python 3. Can this be closed since Python 2 is only receiving bug fixes now?
I think we can classify this one as a usability bug and improve the exception message.
Here's a patch to demonstrate what I meant in msg226047. Example from the REPL: >>> __import__('encodings', fromlist=[u'aliases']) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: Item in ``from list'' must be str, not unicode
Berker's fix for Python 2.7 looks good to me.
However, Python 3 has a comparably vague error message, it's just inverted to complain about bytes rather than unicode due to the change in the native str type:
>>> __import__('encodings', fromlist=[b'aliases'])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<frozen importlib._bootstrap>", line 1013, in _handle_fromlist
TypeError: hasattr(): attribute name must be string
hasattr() in Python 2.7 is similarly unhelpful regarding what type it actually got when you give it something it doesn't expect.
Good catch, thanks Nick. Here's a Python 3 version of the patch. I excluded Python/importlib.h from the patch to make review easier.
Thanks Berker - looks good to me! Should we file a separate issue regarding the similarly vague error message from hasattr() itself?
If run Python 3 with -bb:
>>> __import__('encodings', fromlist=[b'aliases'])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<frozen importlib._bootstrap>", line 1000, in _handle_fromlist
BytesWarning: Comparison between bytes and string
> Should we file a separate issue regarding the similarly vague error message from hasattr() itself? +1 from me. It would be good to show users a user friendly message :) > BytesWarning: Comparison between bytes and string How about raising a TypeError if ``all(isinstance(l, str) for l in fromlist)`` is False? That would make the exception message less clearer since we can't include the "[...] not 'bytes'" part though.
> How about raising a TypeError if ``all(isinstance(l, str) for l in
> fromlist)`` is False? That would make the exception message less clearer
> since we can't include the "[...] not 'bytes'" part though.
for l in fromlist:
if not isinstance(l, str):
raise TypeError(...)
Note also that you can get an arbitrary error if fromlist is not a sequence.
Actually this issue doesn't look very important for Python 3, since it is
unlikely that non-string is passed in fromlist. Unlike to Python 2 where you
can pass unicode if just use unicode_literals. Other solution for Python 2 is
allowing unicode in fromlist. Perhaps this would increase compatibility with
Python 3.
Well, I find using a for loop is a bit verbose in this case :)
In Python 3.2:
>>> __import__('encodings', fromlist=[b'aliases'])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: Item in ``from list'' not a string
BytesWarning is there since Python 3.3 and I couldn't find any report on the tracker. I'd be fine with either committing the current patch or using pre-importlib exception message from 3.2 in 3.5+ (assuming the chance of passing a non-str item is low in Python 3)
> Other solution for Python 2 is allowing unicode in fromlist.
My preference is to improve the exception message and move on. Accepting both str and unicode would make the code harder to maintain and it would be better to avoid potential regressions in 2.7 (note that we already introduced regressions in the previous bugfix releases :))
Right, Python 2.7 import is what it is at this point - if folks want something more modern, they can take a look at importlib2 :) In this case, I think just reporting the first failing item is fine, and mentioning the type of that item in the error message is the most useful additional information we can provide to make things easier to debug.
On 12-Oct-2016, Nick Coghlan wrote: > In this case, I think just reporting the first failing item is fine, > and mentioning the type of that item in the error message is the > most useful additional information we can provide to make things > easier to debug. Yes; also, the type expected, so the user knows what's different from expected. That is, the error message should say exactly what type is expected *and* exactly what type failed that check.
New changeset 7dd0910e8fbf by Berker Peksag in branch '2.7': Issue #21720: Improve exception message when the type of fromlist is unicode https://hg.python.org/cpython/rev/7dd0910e8fbf
Thanks for the reviews! I pushed the patch for 2.7. Nick, what do you think about the case Serhiy mentioned in msg278515? Should we fix it or is issue21720_python3.diff good to go?
On 16-Oct-2016, Roundup Robot wrote: > New changeset 7dd0910e8fbf by Berker Peksag in branch '2.7': > Issue #21720: Improve exception message when the type of fromlist is unicode > https://hg.python.org/cpython/rev/7dd0910e8fbf This is an improvement, but it still should IMO show *which* item caused the error. Can it state “Item in from list must be str, not {type}: {item!r}”? That would show the exact item so the reader has a better chance at finding where it came from.
@Berker: the warning under "-bb" is a separate issue related to the handling of wildcard imports (_handle_fromlist searches for '*' and then pops it from a copy of the input list, replacing it with __all__ if that's defined)
@Ben: As a general principle, we don't give value information in type errors, since the error is structural rather than value based. The closest equivalent to us doing that that I'm aware of is the sequence index being given in str.join's TypeError:
>>> "".join(["a", "b", b"c"])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: sequence item 2: expected str instance, bytes found
By contrast, when sorting, we don't give *any* indication as to where in the sequence the problem was found or the specific values involved:
>>> sorted(["a", "b", b"c"])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: unorderable types: bytes() < str()
That doesn't make it a bad idea (as I think you're right that it would often make debugging easier), I'd just prefer to consider that as a separate question rather than introducing a one-off inconsistency with the general policy here (in particular, encountering TypeError is far more likely with str.join and sorted than it is with __import__, so it would be genuinely weird for the latter to have the most helpful error message).
Is it all with this issue?
On 23-Oct-2017, Serhiy Storchaka wrote: > Is it all with this issue? I accept Nick's reasoning: > As a general principle, we don't give value information in type > errors, since the error is structural rather than value based. as sufficient to reject my requested improvement to the message. The change in <URL:https://hg.python.org/cpython/rev/7dd0910e8fbf> is good enough, IMO.
issue21720_python3.diff hasn't been applied. I'll convert my issue21720_python3.diff patch to a pull request. I like the format of Nick's "".join() example in msg278794. Here's my proposal for Python 3: f"Item {i} in 'from list' must be str, not {type(x).__name__!r}"
I don't think the index in error message is needed. Unlike to str.join() which accepts arbitrary iterables of arbitrary names, the fromlist usually is a short tuple.
Interesting, what happen if the fromlist is not a list or tuple?
>>> __import__('encodings', fromlist=iter(('aliases', b'codecs')))
<module 'encodings' from '/home/serhiy/py/cpython/Lib/encodings/__init__.py'>
Import is successful because the iterator was exhausted by "'*' in fromlist".
>>> __import__('encodings', fromlist=iter(('aliases', b'foobar')))
<module 'encodings' from '/home/serhiy/py/cpython/Lib/encodings/__init__.py'>
> I don't think the index in error message is needed. I'm fine with either format. It's ultimately up to Nick. Should I switch back to the 2.7 version? > Import is successful because the iterator was exhausted by "'*' in fromlist". This shouldn't be a problem in the latest version of PR 4113. While I don't think anyone would pass ``fromlist=iter(('aliases', b'foobar'))`` in real life, I can add it to the test. Let me know what do you think.
I'm fine with the approach in the latest version of the PR - it does make "from x import y" slightly slower due to the extra error checking, but folks should be avoiding doing that in performance critical loops or functions anyway. It would be nice if we could avoid that overhead for the import statement case, but we can't readily tell the difference between "__import__ called via syntax" and "__import__ called directly", and I don't think this is going to be performance critical enough to be worth introducing that complexity. The question of how best to handle passing a consumable iterator as the from list would be a separate question, if we decided to do anything about it at all (the current implementation implicitly assumes the input is reiterable, but passing a non-container seems like a harder mistake to make than accidentally passing bytes instead of a string).
The from import already is much slower than simple import: $ ./python -m timeit 'from encodings import aliases' 500000 loops, best of 5: 475 nsec per loop $ ./python -m timeit 'import encodings.aliases as aliases' 1000000 loops, best of 5: 289 nsec per loop The latter executes only C code if the module already is imported, but the former executes Python code. It may be worth to add the C acceleration for this case too. PR 4113 makes it yet slower: $ ./python -m timeit 'from encodings import aliases' 500000 loops, best of 5: 793 nsec per loop
There are other differences between Python 2.7 and Python 3. PR 4118 restores the Python 2.7 logic. It adds type checking, but its overhead is smaller. $ ./python -m timeit 'from encodings import aliases' 500000 loops, best of 5: 542 nsec per loop Actually in some cases (with '*') the new code is even slightly faster. I don't know whether these differences were intentional, but all tests are passed. The type of items in __all__ also is checked.
I can't say I agree that the performance here is practically insignificant. This will affect the startup time of Python process, and adding even 10% to that in some cases is significant. In some of the larger codebases I've worked on, even simple scripts would import large portions of the system, and there would be thousands of such imports done in the process. There are "basic" utilities in the stdlib which are imported very often in different modules, so the performance of the import statement is not necessarily insignificant compared to that of actually loading the modules. That being said I'm all for getting this in and implementing an optimization of the slower path in time for 3.7.
As Nick said, if the overhead of an import statement is that critical, then you should NOT use the `from ... import ...` form at all and just stick with `import ...` and if necessary, bind local names to objects off of the final module or a local name for the overall module.
I understand that there is a workaround. I'm just thinking about the many existing large codebases where re-writing thousands of imports because of this is unlikely to be done, yet having somewhat longer process launch times would be surprising and unwanted. Anyways I do think it's a very small price to pay for better error messages, and there's a good chance nobody will actually feel the difference, so let's definitely move forward with this.
Serhiy's PR avoids the cryptic BytesWarning on Py3 while minimising the overhead of the new typecheck, so I've closed Berker's PR in favour of that one (which now has approved reviews from both Brett and I, so Serhiy will merge it when he's ready to do so). Thanks for both patches!
New changeset 41c56940c6edf3ea169332a6b039b6c8796f0475 by Serhiy Storchaka in branch 'master': bpo-21720: Restore the Python 2.7 logic in handling a fromlist. (#4118) https://github.com/python/cpython/commit/41c56940c6edf3ea169332a6b039b6c8796f0475
Is it worth to backport PR 4118 to 3.6?
Given that the automated cherry-pick failed, I'd consider a 3.6 backport nice to have, but definitely not essential. My rationale for that is that "from __future__ import unicode_literals" makes it fairly easy to stumble over the 2.7 variant of this error message, but we're not aware of a similarly implicit way of encountering the 3.x variant.
New changeset 2b5cbbb13c6c9138d04c3ca4eb7431f8c65d8e65 by Serhiy Storchaka in branch '3.6': [3.6] bpo-21720: Restore the Python 2.7 logic in handling a fromlist. (GH-4118) (#4128) https://github.com/python/cpython/commit/2b5cbbb13c6c9138d04c3ca4eb7431f8c65d8e65
resolution: fixed -> (no value)
stage: resolved -> needs patch
messages: + msg304868
messages: + msg304792
messages: + msg278522
messages:
+ msg278506
components:
+ Interpreter Core
versions:
+ Python 3.5, Python 3.6, Python 3.7
messages: + msg278270
messages: + msg226055
nosy: + rhettinger
keywords:
+ easy
stage: needs patch