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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:47 | admin | set | github: 74777 |
2017-06-08 12:50:49 | serhiy.storchaka | set | messages:
+ msg295438 |
2017-06-08 12:44:26 | vstinner | set | messages:
+ msg295436 |
2017-06-08 12:31:13 | serhiy.storchaka | set | messages:
+ msg295429 |
2017-06-08 12:19:29 | vstinner | set | messages:
+ msg295426 |
2017-06-08 12:13:07 | serhiy.storchaka | set | messages:
+ msg295424 |
2017-06-08 12:06:52 | vstinner | set | nosy:
+ vstinner messages:
+ msg295423
|
2017-06-08 11:47:19 | serhiy.storchaka | set | status: open -> closed resolution: fixed messages:
+ msg295419
stage: patch review -> resolved |
2017-06-08 11:45:32 | SylvainDe | set | messages:
+ msg295418 |
2017-06-08 11:41:21 | serhiy.storchaka | set | messages:
+ msg295415 |
2017-06-08 10:44:47 | serhiy.storchaka | set | messages:
+ msg295398 stage: patch review |
2017-06-08 10:33:43 | serhiy.storchaka | set | pull_requests:
+ pull_request2062 |
2017-06-08 10:01:43 | SylvainDe | set | messages:
+ msg295389 |
2017-06-07 21:24:55 | serhiy.storchaka | set | messages:
+ msg295373 |
2017-06-07 21:18:50 | SylvainDe | set | messages:
+ msg295372 |
2017-06-07 21:09:09 | SylvainDe | set | messages:
+ msg295370 |
2017-06-07 20:50:06 | serhiy.storchaka | set | versions:
+ Python 3.7 nosy:
+ serhiy.storchaka
assignee: serhiy.storchaka components:
+ Interpreter Core type: enhancement -> behavior |
2017-06-07 20:18:16 | SylvainDe | create | |