classification
Title: Misleading error message when ImportError called with invalid keyword args
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: berker.peksag, brett.cannon, eric.snow, python-dev, r.david.murray, serhiy.storchaka, xiang.zhang
Priority: normal Keywords: patch

Created on 2014-05-26 04:31 by eric.snow, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
issue21578_v2.diff berker.peksag, 2014-05-27 19:09 review
issue21578_v3.diff berker.peksag, 2014-06-27 23:53 review
import_error_parse_args.patch serhiy.storchaka, 2016-08-04 18:53 Use PyArg_ParseTupleAndKeywords() review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (17)
msg219125 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-05-26 04:31
>>> 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()
msg219148 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-05-26 09:33
Here's a patch with a simple test case.
msg219235 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-05-27 19:09
Thanks for the review, Eric. Here's a new patch.
msg219240 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-05-27 21:16
Looks good to me.  Thanks for doing this.  If no one objects in the meantime, I'll commit this in a few days.
msg221640 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-06-26 19:59
Eric, do you want me to commit the patch? Should this also be committed to the 3.4 branch?
msg221755 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-06-27 23:53
Here's a new patch which uses assertRaisesRegex instead of assertRaises.
msg227735 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-09-27 21:29
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.
msg228527 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-10-05 02:23
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?
msg228540 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-05 05:07
Just the first unexpected keyword.  That's what happens if you pass more than one unexpected keyword to a python function.
msg271995 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-04 18:11
Assigning to Berker to apply his own patch since Eric has not gotten around to this.
msg271996 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-04 18:53
Why not use PyArg_ParseTupleAndKeywords()?
msg272005 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-05 02:31
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.
msg277516 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-27 14:03
Ping.
msg277519 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-09-27 15:59
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?
msg277521 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-09-27 16:14
Left a review, but basically LGTM.
msg277531 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-27 17:52
New changeset 9b8f0db1944f by Serhiy Storchaka in branch '3.5':
Issue #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 #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 #21578: Fixed misleading error message when ImportError called with
https://hg.python.org/cpython/rev/59268ac85f4e
msg277534 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-27 18:03
Added a test for multiple invalid keyword arguments, added braces, fixed a leak.

But there is other oddity in ImportError constructor (issue28289).
History
Date User Action Args
2017-03-31 16:36:11dstufftsetpull_requests: + pull_request873
2016-09-27 18:03:54serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg277534

stage: patch review -> resolved
2016-09-27 17:52:20python-devsetnosy: + python-dev
messages: + msg277531
2016-09-27 16:14:52brett.cannonsetassignee: berker.peksag -> serhiy.storchaka
2016-09-27 16:14:41brett.cannonsetmessages: + msg277521
2016-09-27 15:59:18berker.peksagsetmessages: + msg277519
2016-09-27 14:03:30serhiy.storchakasetmessages: + msg277516
versions: + Python 3.7, - Python 3.4
2016-08-05 02:31:40xiang.zhangsetnosy: + xiang.zhang
messages: + msg272005
2016-08-04 18:53:18serhiy.storchakasetfiles: + import_error_parse_args.patch

messages: + msg271996
2016-08-04 18:40:52serhiy.storchakasetnosy: + serhiy.storchaka
2016-08-04 18:11:12brett.cannonsetassignee: eric.snow -> berker.peksag
messages: + msg271995
2016-08-04 17:26:40berker.peksaglinkissue27684 superseder
2015-09-16 17:21:23berker.peksagsetstage: needs patch -> patch review
versions: + Python 3.6
2015-09-16 17:20:42berker.peksaglinkissue25142 superseder
2014-10-05 05:07:34r.david.murraysetmessages: + msg228540
2014-10-05 02:24:00berker.peksagsetmessages: + msg228527
2014-09-27 21:29:52r.david.murraysettype: enhancement -> behavior
stage: commit review -> needs patch
messages: + msg227735
versions: + Python 3.4
2014-07-05 18:28:47berker.peksagsetversions: - Python 3.4
2014-06-27 23:54:01berker.peksagsetfiles: - issue21578.diff
2014-06-27 23:53:53berker.peksagsetfiles: + issue21578_v3.diff

messages: + msg221755
2014-06-26 19:59:05berker.peksagsetnosy: + r.david.murray
messages: + msg221640
2014-05-27 21:16:49eric.snowsetassignee: eric.snow
messages: + msg219240
stage: patch review -> commit review
2014-05-27 19:09:11berker.peksagsetfiles: + issue21578_v2.diff

messages: + msg219235
2014-05-26 09:33:05berker.peksagsetfiles: + issue21578.diff

nosy: + berker.peksag
messages: + msg219148

keywords: + patch
stage: needs patch -> patch review
2014-05-26 04:31:35eric.snowcreate