classification
Title: Bad error message 'bool()() takes no keyword arguments'
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: SylvainDe, serhiy.storchaka, vstinner
Priority: normal Keywords:

Created on 2017-06-07 20:18 by SylvainDe, last changed 2017-06-08 12:50 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1996 merged serhiy.storchaka, 2017-06-08 10:33
Messages (15)
msg295366 - (view) Author: SylvainDe (SylvainDe) * Date: 2017-06-07 20:18
Very recent "regression". Issue found because of a pet project trying to parse error messages using regexps : more on https://github.com/SylvainDe/DidYouMean-Python/issues/31 .

On recent Cron builds on Jenkins, running the following code

    bool(this_doesnt_exist=2)

fails with the following error message

    'bool()() takes no keyword arguments'

instead of the expected error message

    'bool() takes no keyword arguments'.


Having a quick look at the recent commits, I suspect with no guarantee whatsoever the issue got introduced with https://github.com/python/cpython/commit/5eb788bf7f54a8e04429e18fc332db858edd64b6 / http://bugs.python.org/issue30534 .

I haven't tried to reproduce the issue locally yet and add the findinds if any later on.
msg295370 - (view) Author: SylvainDe (SylvainDe) * Date: 2017-06-07 21:09
Reproduced locally using unit tests:

+    def test_varargs0_kw(self):
+        msg = r"bool\(\) takes no keyword arguments"
+        self.assertRaisesRegex(TypeError, msg, bool, x=2)
+


giving:

Traceback (most recent call last):
  File "/home/josay/Geekage/PythonRgr/cpython/Lib/test/test_call.py", line 136, in test_varargs0_kw
    self.assertRaisesRegex(TypeError, msg, bool, x=2)
AssertionError: "bool\(\) takes no keyword arguments" does not match "bool()() takes no keyword arguments"
msg295372 - (view) Author: SylvainDe (SylvainDe) * Date: 2017-06-07 21:18
The issue seems to be in:

int
_PyArg_NoKeywords(const char *funcname, PyObject *kwargs)


I'm happy to submit a PR if needed.
msg295373 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-07 21:24
Thank you for catching this bug SylvainDe. This is a regression caused by issue30534. I added "()" after the function name for unifying with other error messages but missed that _PyArg_NoKeywords() often is called with argument containing "()", e.g. _PyArg_NoKeywords("bool()", kwds).

There are two ways to solve this issue:

1. Remove "()" from all calls of _PyArg_NoKeywords(), _PyArg_NoStackKeywords() and _PyArg_NoPositional() and let these functions to add "()" automatically. This will change almost all manually calls of _PyArg_NoKeywords(). And may be in some cases "()" shouldn't be added.

2. Make _PyArg_NoKeywords(), _PyArg_NoStackKeywords() and _PyArg_NoPositional() not adding "()" automatically, but always pass a name with "()" if it is needed. Argument Clinic should be changed to add "()" in arguments of these functions. This is large patch, but mostly generated.

I don't know what the way is better.
msg295389 - (view) Author: SylvainDe (SylvainDe) * Date: 2017-06-08 10:01
As I was trying to test coverage for a few places where the same error was built, I have discovered another issue where the error message is very misleading:

 AssertionError: "^index\(\) takes no keyword arguments$" does not match "index() takes at least 1 argument (0 given)"
 def test_varargs4_kw(self):
     msg = r"^index\(\) takes no keyword arguments$"
     self.assertRaisesRegex(TypeError, msg, [].index, x=2)


Should I open another ticket to track this ?



Anyway, so far, I have reached the following conclusion regarding the test coverage:

Objects/call.c:551:                 "%.200s() takes no keyword arguments" => TESTED
(In _PyMethodDef_RawFastCallDict)

Objects/call.c:690:                 "%.200s() takes no keyword arguments" => Not tested
(In _PyMethodDef_RawFastCallKeywords)

Objects/call.c:737:            PyErr_Format(PyExc_TypeError, "%.200s() takes no keyword arguments" => Not tested
(In cfunction_call_varargs)


Python/getargs.c:2508:    PyErr_Format(PyExc_TypeError, "%.200s takes no keyword arguments" => TESTED (Now)
(In _PyArg_NoKeywords)

Python/getargs.c:2525:    PyErr_Format(PyExc_TypeError, "%.200s() takes no keyword arguments" => Not tested
(In _PyArg_NoStackKeywords)


Any suggestion regarding how to test what is not tested is more than welcome.
msg295398 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-08 10:44
Sorry, SylvainDe, but by the time you expose your desire to work on this issue, I already had written two alternate patches. I needed only a time for deciding what of them is the more appropriate solution.

PR 1996 implements the first way. It removes explicit "()" from arguments passed to _PyArg_NoKeywords(). Fixed 23 functions and methods.
msg295415 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-08 11:41
New changeset 6cca5c8459cc439cb050010ffa762a03859d3051 by Serhiy Storchaka in branch 'master':
bpo-30592: Fixed error messages for some builtins. (#1996)
https://github.com/python/cpython/commit/6cca5c8459cc439cb050010ffa762a03859d3051
msg295418 - (view) Author: SylvainDe (SylvainDe) * Date: 2017-06-08 11:45
Hi serhiy.storchaka, this is fine by me! I've added a corresponding comment with questions/suggestions on the Github PR.

Do you reckon I should open another ticket for the issue mentionned in my previous message ? I'm happy to take care of it :)
msg295419 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-08 11:47
If you know how to write tests for these cases a patch is welcome. But this may be not easy.
msg295423 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 12:06
Would it be possible to add an assertion to _PyArg_NoKeywords() to prevent regressions? Fail if funcname contains "(" or ")". For example:

/* funcname must not contain ( or ), since "()" suffix is added in the error message */
assert(!strchr(funcname, '(') && !strchr(funcname, ')'));
msg295424 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-08 12:13
It would be possible to make _PyArg_NoKeywords() adding "()" only if the passed argument don't contain them. But since this is a private API, it was easier to just remove "()" from arguments. In any case most uses of _PyArg_NoKeywords() are generated by Argument Clinic and they don't pass "()" in the argument.
msg295426 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 12:19
Oh no, I don't expect anything smart :-) Just an assertion to catch obvious mistakes when the function is called manually (not using Argument Clinic).
msg295429 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-08 12:31
If add an assertion, the user code could provoke a crash by creating an exception type with a name containing "()": type('E()', (BaseException,), {}). Thus this should be a runtime check that raises an exception. What the type of an exception should be raised?
msg295436 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 12:44
> If add an assertion, the user code could provoke a crash by creating an exception type with a name containing "()": type('E()', (BaseException,), {}).

Right. Do we have such code currently? If not, we can remove the assertion later if we want to use such error message.

> Thus this should be a runtime check that raises an exception. What the type of an exception should be raised?

Sorry, I don't understand. funcname is not always a constant string like "func"?

Well, it was just an idea. Ignore it if you see practical issues.
msg295438 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-08 12:50
We don't have such code currently. The function name containing "()" looks silly. But funcname is not always a constant string, you can set it to arbitrary string when create a BaseException subclass.
History
Date User Action Args
2017-06-08 12:50:49serhiy.storchakasetmessages: + msg295438
2017-06-08 12:44:26vstinnersetmessages: + msg295436
2017-06-08 12:31:13serhiy.storchakasetmessages: + msg295429
2017-06-08 12:19:29vstinnersetmessages: + msg295426
2017-06-08 12:13:07serhiy.storchakasetmessages: + msg295424
2017-06-08 12:06:52vstinnersetnosy: + vstinner
messages: + msg295423
2017-06-08 11:47:19serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg295419

stage: patch review -> resolved
2017-06-08 11:45:32SylvainDesetmessages: + msg295418
2017-06-08 11:41:21serhiy.storchakasetmessages: + msg295415
2017-06-08 10:44:47serhiy.storchakasetmessages: + msg295398
stage: patch review
2017-06-08 10:33:43serhiy.storchakasetpull_requests: + pull_request2062
2017-06-08 10:01:43SylvainDesetmessages: + msg295389
2017-06-07 21:24:55serhiy.storchakasetmessages: + msg295373
2017-06-07 21:18:50SylvainDesetmessages: + msg295372
2017-06-07 21:09:09SylvainDesetmessages: + msg295370
2017-06-07 20:50:06serhiy.storchakasetversions: + Python 3.7
nosy: + serhiy.storchaka

assignee: serhiy.storchaka
components: + Interpreter Core
type: enhancement -> behavior
2017-06-07 20:18:16SylvainDecreate