Misleading error message when ImportError called with invalid keyword args

[ericsnowcurrently]

BPO	21578
Nosy	@brettcannon, @bitdancer, @ericsnowcurrently, @berkerpeksag, @serhiy-storchaka, @zhangyangyu
PRs	[Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	issue21578_v2.diff issue21578_v3.diff import_error_parse_args.patch: Use PyArg_ParseTupleAndKeywords()

^{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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-09-27.18:03:54.495>
created_at = <Date 2014-05-26.04:31:35.238>
labels = ['interpreter-core', 'type-bug', '3.7']
title = 'Misleading error message when ImportError called with invalid keyword args'
updated_at = <Date 2017-03-31.16:36:11.929>
user = 'https://github.com/ericsnowcurrently'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:11.929>
actor = 'dstufft'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-09-27.18:03:54.495>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2014-05-26.04:31:35.238>
creator = 'eric.snow'
dependencies = []
files = ['35381', '35794', '44010']
hgrepos = []
issue_num = 21578
keywords = ['patch']
message_count = 17.0
messages = ['219125', '219148', '219235', '219240', '221640', '221755', '227735', '228527', '228540', '271995', '271996', '272005', '277516', '277519', '277521', '277531', '277534']
nosy_count = 7.0
nosy_names = ['brett.cannon', 'r.david.murray', 'python-dev', 'eric.snow', 'berker.peksag', 'serhiy.storchaka', 'xiang.zhang']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21578'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[ericsnowcurrently]

>>> ImportError(spam='spam')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ImportError does not take keyword arguments

However, it *does* take keyword arguments:

>>> ImportError(name='spam', path='spam')
ImportError()

[berkerpeksag]

Here's a patch with a simple test case.

[berkerpeksag]

Thanks for the review, Eric. Here's a new patch.

[ericsnowcurrently]

Looks good to me. Thanks for doing this. If no one objects in the meantime, I'll commit this in a few days.

[berkerpeksag]

Eric, do you want me to commit the patch? Should this also be committed to the 3.4 branch?

[berkerpeksag]

Here's a new patch which uses assertRaisesRegex instead of assertRaises.

[bitdancer]

The standard error message for this case is:

xxx() got an unexpected keyword argument 'foo'

I have no idea where that gets generated (a grep didn't teach me anything useful), but I think it would make sense to use that form for the message.

I think the fix can be applied to 3.4.

[berkerpeksag]

Thanks for the review, David.

The standard error message for this case is:

xxx() got an unexpected keyword argument 'foo'

I found two similar messages in the codebase:

• In Modules/itertoolsmodule.c:

  PyErr_SetString(PyExc_TypeError,
  "zip_longest() got an unexpected keyword argument");

• In Python/ceval.c:

  PyErr_Format(PyExc_TypeError,
  "%U() got an unexpected "
  "keyword argument '%S'",
  co->co_name,
  keyword);

But, in ImportError case it can take more than one keyword arguments:

   ImportError(spam="SPAM", eggs=True)

What error message should be printed for the above case?

[bitdancer]

Just the first unexpected keyword. That's what happens if you pass more than one unexpected keyword to a python function.

[brettcannon]

Assigning to Berker to apply his own patch since Eric has not gotten around to this.

[serhiy-storchaka]

Why not use PyArg_ParseTupleAndKeywords()?

[zhangyangyu]

I am a little uncomfortable with the empty tuple. It's only used as a placeholder for PyArg_ParseTupleAndKeywords. But this way we can achieve consistent error message. Don't know how to choose.

[serhiy-storchaka]

Ping.

[berkerpeksag]

Serhiy's patch looks good to me. It would be nice to add a test for multiple invalid keyword arguments:

    with self.assertRaisesRegex(TypeError, msg):
        ImportError('test', invalid='keyword', another=True)

Using empty_tuple seems reasonable to me. Brett and Eric, what do you think?

[brettcannon]

Left a review, but basically LGTM.

[python-dev]

New changeset 9b8f0db1944f by Serhiy Storchaka in branch '3.5':
Issue bpo-21578: Fixed misleading error message when ImportError called with
https://hg.python.org/cpython/rev/9b8f0db1944f

New changeset 95549f4970d0 by Serhiy Storchaka in branch '3.6':
Issue bpo-21578: Fixed misleading error message when ImportError called with
https://hg.python.org/cpython/rev/95549f4970d0

New changeset 59268ac85f4e by Serhiy Storchaka in branch 'default':
Issue bpo-21578: Fixed misleading error message when ImportError called with
https://hg.python.org/cpython/rev/59268ac85f4e

[serhiy-storchaka]

Added a test for multiple invalid keyword arguments, added braces, fixed a leak.

But there is other oddity in ImportError constructor (bpo-28289).