classification
Title: error message for incorrect call degraded in 3.7
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: brett.cannon, rhettinger, scoder, serhiy.storchaka, vstinner
Priority: normal Keywords:

Created on 2017-06-01 06:32 by scoder, last changed 2017-06-08 11:41 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1901 merged serhiy.storchaka, 2017-06-01 07:38
PR 1996 merged serhiy.storchaka, 2017-06-08 10:33
Messages (14)
msg294906 - (view) Author: Stefan Behnel (scoder) * (Python committer) 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 <module>
        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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2017-06-01 09:17
The details can be different. For example:

>>> 1 .__add__(x=2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (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
History
Date User Action Args
2017-06-08 11:41:21serhiy.storchakasetmessages: + msg295414
2017-06-08 10:33:43serhiy.storchakasetpull_requests: + pull_request2063
2017-06-06 15:56:44serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-06-06 15:45:31serhiy.storchakasetmessages: + msg295269
2017-06-03 18:52:04serhiy.storchakasetmessages: + msg295097
2017-06-03 18:33:47brett.cannonsetmessages: + msg295096
2017-06-03 15:04:55serhiy.storchakasetmessages: + msg295079
2017-06-02 18:26:11brett.cannonsetnosy: + brett.cannon
messages: + msg295050
2017-06-01 09:17:40serhiy.storchakasetmessages: + msg294920
2017-06-01 08:02:28vstinnersetmessages: + msg294917
2017-06-01 07:45:02serhiy.storchakasetmessages: + msg294916
stage: patch review
2017-06-01 07:43:34vstinnersetmessages: + msg294915
2017-06-01 07:39:49vstinnersetmessages: + msg294914
2017-06-01 07:38:05serhiy.storchakasetpull_requests: + pull_request1980
2017-06-01 07:36:03vstinnersetmessages: + msg294913
2017-06-01 07:35:24vstinnersetnosy: + rhettinger
messages: + msg294912
2017-06-01 07:03:00serhiy.storchakasetassignee: serhiy.storchaka

nosy: + vstinner, serhiy.storchaka
2017-06-01 06:32:30scodercreate