-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
"TypeError: Item in ``from list'' not a string" message #65919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
accidentally ended up with something like this via some module that was using |
Do you want to propose a patch? |
I'm trying to replicate this issue. I do not get an error when I run
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
|
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):
|
Thanks for the patch, David! + def test_fromlist_error_messages(self): You could use assertRaises here: with self.assertRaises(TypeError) as cm:
# ...
self.assertIn('foo', str(cm.exception))
I think it would be better to improve the error message in Python/import.c:
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 |
+1 from me. It would be good to show users a user friendly message :)
How about raising a TypeError if |
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 |
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)
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:
Yes; also, the type expected, so the user knows what's different from That is, the error message should say exactly what type is expected |
New changeset 7dd0910e8fbf by Berker Peksag in branch '2.7': |
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:
This is an improvement, but it still should IMO show *which* item Can it state “Item in from list must be str, not {type}: {item!r}”? |
@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:
I accept Nick's reasoning:
as sufficient to reject my requested improvement to the message. The change in <URL:https://hg.python.org/cpython/rev/7dd0910e8fbf\> is |
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:
|
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'm fine with either format. It's ultimately up to Nick. Should I switch back to the 2.7 version?
This shouldn't be a problem in the latest version of PR 4113. While I don't think anyone would pass |
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' |
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 |
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! |
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. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: