| msg294906 - (view) |
Author: Stefan Behnel (scoder) *  |
Date: 2017-06-01 06:32 |
| I'm seeing doctest failures in Cython's test suite with Py3.7 due to the change of an error message: Failed example: func1(arg=None) Expected: Traceback (most recent call last): ... TypeError: func1() takes no keyword arguments Got: Traceback (most recent call last): File "/opt/python/3.7-dev/lib/python3.7/doctest.py", line 1329, in __run compileflags, 1), test.globs) File "<doctest always_allow_keywords_T295.func1[2]>", line 1, in func1(arg=None) TypeError: func1() takes exactly one argument (0 given) I'd normally just adapt the doctest and move on, but this seems like the error message is worse than before, so I thought I'd bring it up here. func1() is implemented as a METH_O C function. Suggesting that it does not accept keyword arguments seems better than saying that it expects "exactly one argument" instead of the exactly one argument that was passed. |
|
|
| msg294912 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-06-01 07:35 |
| Example with a builtin function, abs(), which uses METH_O. Before: haypo@selma$ python3 Python 3.5.3 (default, Apr 24 2017, 13:32:13) >>> abs(x=5) TypeError: abs() takes no keyword arguments After: haypo@selma$ ./python Python 3.7.0a0 (heads/master:85aba23, May 31 2017, 10:29:03) >>> abs(x=5) TypeError: abs() takes exactly one argument (0 given) In Python 3.5, PyCFunction_Call() starts by checking keyword arguments: if (flags == (METH_VARARGS | METH_KEYWORDS)) { ... } else { if (kwds != NULL && PyDict_Size(kwds) != 0) { PyErr_Format(PyExc_TypeError, "%.200s() takes no keyword arguments", f->m_ml->ml_name); return NULL; } switch (flags) { case METH_NOARGS: size = PyTuple_GET_SIZE(args); if (size != 0) { PyErr_Format(PyExc_TypeError, "%.200s() takes no arguments (%zd given)", f->m_ml->ml_name, size); return NULL; } ... In Python 3.7, _PyMethodDef_RawFastCallKeywords() first check positional arguments: switch (flags) { case METH_NOARGS: if (nargs != 0) { PyErr_Format(PyExc_TypeError, "%.200s() takes no arguments (%zd given)", method->ml_name, nargs); goto exit; } if (nkwargs) { goto no_keyword_error; } ... We can easily exchange the two checks, but IMHO we need an unit test (at least decorated by @cpython_only) to avoid regressions in the future. |
|
|
| msg294913 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-06-01 07:36 |
| Ah, Python 3.6 is not affected: haypo@selma$ ./python Python 3.6.1+ (heads/unpack_dict:ba4fb35, May 31 2017, 16:31:51) >>> abs(x=5) TypeError: abs() takes no keyword arguments |
|
|
| msg294914 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-06-01 07:39 |
| I introduced the regression with the commit 7fc252adfbedece75f2330bcfdadbf84dee7836f: "Optimize _PyCFunction_FastCallKeywords(): Issue #29259: Write fast path in _PyCFunction_FastCallKeywords() for METH_FASTCALL, (...) Cleanup also _PyCFunction_FastCallDict(): (...) Move code to raise the "no keyword argument" exception into a new no_keyword_error label." |
|
|
| msg294915 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-06-01 07:43 |
| See also "[Python-ideas] Positional-only parameters" discussion of last March: https://mail.python.org/pipermail/python-ideas/2017-March/044956.html Brett Cannon: "It seems all the core devs who have commented on this are in the positive (Victor, Yury, Ethan, Yury, Guido, Terry, and Steven; MAL didn't explicitly vote). So to me that suggests there's enough support to warrant writing a PEP. (...)" (No, I didn't write such PEP yet :-p) Maybe we can also enhance the error message to explain that the function accepts exactly one positional-only argument? |
|
|
| msg294916 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-06-01 07:45 |
| Note that some error messages are "takes no keyword arguments", but others are "does not take keyword arguments" or "doesn't take keyword arguments". They should be unified. What wording is better? |
|
|
| msg294917 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-06-01 08:02 |
| > Note that some error messages are "takes no keyword arguments", but others are "does not take keyword arguments" or "doesn't take keyword arguments". We already have a _PyArg_NoKeywords() function. Maybe we could add a new _PyErr_NoKeywords() function which would just raise the error? |
|
|
| msg294920 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-06-01 09:17 |
| The details can be different. For example: >>> 1 .__add__(x=2) Traceback (most recent call last): File "", line 1, in TypeError: wrapper __add__ doesn't take keyword arguments The message contains the word "wrapper" and doesn't contains parenthesis. I think it can be extended for containing the type name ("int.__add__()" or "wrapper __add__ of in objects"), and this hard to do with general _PyErr_NoKeywords(). Let first unify messages, then try to add more useful information in messages and create a general API if this is possible. |
|
|
| msg295050 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2017-06-02 18:26 |
| > Note that some error messages are "takes no keyword arguments", but others are "does not take keyword arguments" or "doesn't take keyword arguments". They should be unified. What wording is better? I vote for "takes no keyword arguments". |
|
|
| msg295079 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-06-03 15:04 |
| Updated error messages. Later I'm going to unify error messages for Python and C functions. But this is a different issue. I can wrote a test, but I don't know good place for it. test_builtin doesn't look a good place to me. |
|
|
| msg295096 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2017-06-03 18:33 |
| The tests could either go into the respective type's tests or a new test module like test_exception_messages could be added that's only to help keep messages in sync with each other. |
|
|
| msg295097 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-06-03 18:52 |
| There is no respective type. abs() is just an arbitrary example. There are perhaps tens functions affected by this bug. test_exception_messages looks an overkill. There are similar tests in test_capi and test_getargs2 that test public PyArg_* functions. But this bug is related to other code. Hmm, may be test_call is an appropriate test. |
|
|
| msg295269 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-06-06 15:45 |
| New changeset 5eb788bf7f54a8e04429e18fc332db858edd64b6 by Serhiy Storchaka in branch 'master': bpo-30534: Fixed error messages when pass keyword arguments (#1901) https://github.com/python/cpython/commit/5eb788bf7f54a8e04429e18fc332db858edd64b6 |
|
|
| msg295414 - (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 |
|
|