Very recent "regression". Issue found because of a pet project trying to parse error messages using regexps : more on SylvainDe/DidYouMean-Python#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 5eb788b / http://bugs.python.org/issue30534 .

I haven't tried to reproduce the issue locally yet and add the findinds if any later on.

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"

The issue seems to be in:

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

I'm happy to submit a PR if needed.

Thank you for catching this bug SylvainDe. This is a regression caused by bpo-30534. 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.

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.

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.

New changeset 6cca5c8 by Serhiy Storchaka in branch 'master':
bpo-30592: Fixed error messages for some builtins. (bpo-1996)
6cca5c8

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 :)

If you know how to write tests for these cases a patch is welcome. But this may be not easy.

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, ')'));

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.

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).

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?

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.

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.