greatly improve argument parsing error messages (closes #12265)

5 files changed, 301 insertions, 198 deletions

diff --git a/Lib/inspect.py b/Lib/inspect.py
index 4899cbf..f200a82 100644
--- a/Lib/inspect.py
+++ b/Lib/inspect.py
@@ -914,6 +914,29 @@ def formatargvalues(args, varargs, varkw, locals,
         specs.append(formatvarkw(varkw) + formatvalue(locals[varkw]))
     return '(' + ', '.join(specs) + ')'
 
+def _positional_error(f_name, args, kwonly, varargs, defcount, given, values):
+    atleast = len(args) - defcount
+    if given is None:
+        given = len([arg for arg in args if arg in values])
+    kwonly_given = len([arg for arg in kwonly if arg in values])
+    if varargs:
+        plural = atleast != 1
+        sig = "at least %d" % (atleast,)
+    elif defcount:
+        plural = True
+        sig = "from %d to %d" % (atleast, len(args))
+    else:
+        plural = len(args) != 1
+        sig = str(len(args))
+    kwonly_sig = ""
+    if kwonly_given:
+        msg = " positional argument%s (and %d keyword-only argument%s)"
+        kwonly_sig = (msg % ("s" if given != 1 else "", kwonly_given,
+                             "s" if kwonly_given != 1 else ""))
+    raise TypeError("%s() takes %s positional argument%s but %d%s %s given" %
+            (f_name, sig, "s" if plural else "", given, kwonly_sig,
+             "was" if given == 1 and not kwonly_given else "were"))
+
 def getcallargs(func, *positional, **named):
     """Get the mapping of arguments to values.
 
@@ -925,64 +948,50 @@ def getcallargs(func, *positional, **named):
     f_name = func.__name__
     arg2value = {}
 
+
     if ismethod(func) and func.__self__ is not None:
         # implicit 'self' (or 'cls' for classmethods) argument
         positional = (func.__self__,) + positional
     num_pos = len(positional)
-    num_total = num_pos + len(named)
     num_args = len(args)
     num_defaults = len(defaults) if defaults else 0
-    for arg, value in zip(args, positional):
-        arg2value[arg] = value
+
+    n = min(num_pos, num_args)
+    for i in range(n):
+        arg2value[args[i]] = positional[i]
     if varargs:
-        if num_pos > num_args:
-            arg2value[varargs] = positional[-(num_pos-num_args):]
-        else:
-            arg2value[varargs] = ()
-    elif 0 < num_args < num_pos:
-        raise TypeError('%s() takes %s %d positional %s (%d given)' % (
-            f_name, 'at most' if defaults else 'exactly', num_args,
-            'arguments' if num_args > 1 else 'argument', num_total))
-    elif num_args == 0 and num_total:
-        if varkw or kwonlyargs:
-            if num_pos:
-                # XXX: We should use num_pos, but Python also uses num_total:
-                raise TypeError('%s() takes exactly 0 positional arguments '
-                                '(%d given)' % (f_name, num_total))
-        else:
-            raise TypeError('%s() takes no arguments (%d given)' %
-                            (f_name, num_total))
-
-    for arg in itertools.chain(args, kwonlyargs):
-        if arg in named:
-            if arg in arg2value:
-                raise TypeError("%s() got multiple values for keyword "
-                                "argument '%s'" % (f_name, arg))
-            else:
-                arg2value[arg] = named.pop(arg)
-    for kwonlyarg in kwonlyargs:
-        if kwonlyarg not in arg2value:
-            try:
-                arg2value[kwonlyarg] = kwonlydefaults[kwonlyarg]
-            except KeyError:
-                raise TypeError("%s() needs keyword-only argument %s" %
-                                (f_name, kwonlyarg))
-    if defaults:    # fill in any missing values with the defaults
-        for arg, value in zip(args[-num_defaults:], defaults):
-            if arg not in arg2value:
-                arg2value[arg] = value
+        arg2value[varargs] = tuple(positional[n:])
+    possible_kwargs = set(args + kwonlyargs)
     if varkw:
-        arg2value[varkw] = named
-    elif named:
-        unexpected = next(iter(named))
-        raise TypeError("%s() got an unexpected keyword argument '%s'" %
-                        (f_name, unexpected))
-    unassigned = num_args - len([arg for arg in args if arg in arg2value])
-    if unassigned:
-        num_required = num_args - num_defaults
-        raise TypeError('%s() takes %s %d %s (%d given)' % (
-            f_name, 'at least' if defaults else 'exactly', num_required,
-            'arguments' if num_required > 1 else 'argument', num_total))
+        arg2value[varkw] = {}
+    for kw, value in named.items():
+        if kw not in possible_kwargs:
+            if not varkw:
+                raise TypeError("%s() got an unexpected keyword argument %r" %
+                                (f_name, kw))
+            arg2value[varkw][kw] = value
+            continue
+        if kw in arg2value:
+            raise TypeError("%s() got multiple values for argument %r" %
+                            (f_name, kw))
+        arg2value[kw] = value
+    if num_pos > num_args and not varargs:
+        _positional_error(f_name, args, kwonlyargs, varargs, num_defaults,
+                          num_pos, arg2value)
+    if num_pos < num_args:
+        for arg in args[:num_args - num_defaults]:
+            if arg not in arg2value:
+                _positional_error(f_name, args, kwonlyargs, varargs,
+                                  num_defaults, None, arg2value)
+        for i, arg in enumerate(args[num_args - num_defaults:]):
+            if arg not in arg2value:
+                arg2value[arg] = defaults[i]
+    for kwarg in kwonlyargs:
+        if kwarg not in arg2value:
+            if kwarg not in kwonlydefaults:
+                raise TypeError("%s() requires keyword-only argument %r" %
+                                (f_name, kwarg))
+            arg2value[kwarg] = kwonlydefaults[kwarg]
     return arg2value
 
 # -------------------------------------------------- stack frame extraction
diff --git a/Lib/test/test_extcall.py b/Lib/test/test_extcall.py
index 1f7f630..49d5441 100644
--- a/Lib/test/test_extcall.py
+++ b/Lib/test/test_extcall.py
@@ -66,17 +66,17 @@ Verify clearing of SF bug #733667
     >>> g()
     Traceback (most recent call last):
       ...
-    TypeError: g() takes at least 1 argument (0 given)
+    TypeError: g() takes at least 1 positional argument but 0 were given
 
     >>> g(*())
     Traceback (most recent call last):
       ...
-    TypeError: g() takes at least 1 argument (0 given)
+    TypeError: g() takes at least 1 positional argument but 0 were given
 
     >>> g(*(), **{})
     Traceback (most recent call last):
       ...
-    TypeError: g() takes at least 1 argument (0 given)
+    TypeError: g() takes at least 1 positional argument but 0 were given
 
     >>> g(1)
     1 () {}
@@ -151,7 +151,7 @@ What about willful misconduct?
     >>> g(1, 2, 3, **{'x': 4, 'y': 5})
     Traceback (most recent call last):
       ...
-    TypeError: g() got multiple values for keyword argument 'x'
+    TypeError: g() got multiple values for argument 'x'
 
     >>> f(**{1:2})
     Traceback (most recent call last):
@@ -263,29 +263,91 @@ the function call setup. See <http://bugs.python.org/issue2016>.
     >>> f(**x)
     1 2
 
-A obscure message:
+Some additional tests about positional argument errors:
 
     >>> def f(a, b):
     ...    pass
     >>> f(b=1)
     Traceback (most recent call last):
       ...
-    TypeError: f() takes exactly 2 arguments (1 given)
-
-The number of arguments passed in includes keywords:
+    TypeError: f() takes 2 positional arguments but 1 was given
 
     >>> def f(a):
     ...    pass
     >>> f(6, a=4, *(1, 2, 3))
     Traceback (most recent call last):
       ...
-    TypeError: f() takes exactly 1 positional argument (5 given)
+    TypeError: f() got multiple values for argument 'a'
     >>> def f(a, *, kw):
     ...    pass
     >>> f(6, 4, kw=4)
     Traceback (most recent call last):
       ...
-    TypeError: f() takes exactly 1 positional argument (3 given)
+    TypeError: f() takes 1 positional argument but 2 positional arguments (and 1 keyword-only argument) were given
+
+    >>> def f(a):
+    ...    pass
+    >>> f()
+    Traceback (most recent call last):
+      ...
+    TypeError: f() takes 1 positional argument but 0 were given
+
+    >>> def f(a, b):
+    ...    pass
+    >>> f(1)
+    Traceback (most recent call last):
+      ...
+    TypeError: f() takes 2 positional arguments but 1 was given
+
+    >>> def f(a, *b):
+    ...    pass
+    >>> f()
+    Traceback (most recent call last):
+      ...
+    TypeError: f() takes at least 1 positional argument but 0 were given
+
+    >>> def f(a, *, kw=4):
+    ...    pass
+    >>> f(kw=4)
+    Traceback (most recent call last):
+      ...
+    TypeError: f() takes 1 positional argument but 0 positional arguments (and 1 keyword-only argument) were given
+
+    >>> def f(a, b=2):
+    ...    pass
+    >>> f()
+    Traceback (most recent call last):
+      ...
+    TypeError: f() takes from 1 to 2 positional arguments but 0 were given
+
+    >>> def f(a, *b):
+    ...    pass
+    >>> f()
+    Traceback (most recent call last):
+      ...
+    TypeError: f() takes at least 1 positional argument but 0 were given
+
+    >>> def f(*, kw):
+    ...    pass
+    >>> f(3, kw=4)
+    Traceback (most recent call last):
+      ...
+    TypeError: f() takes 0 positional arguments but 1 positional argument (and 1 keyword-only argument) were given
+
+    >>> def f(a, c=3, *b, kw):
+    ...    pass
+    >>> f()
+    Traceback (most recent call last):
+     ...
+    TypeError: f() takes at least 1 positional argument but 0 were given
+    >>> f(kw=3)
+    Traceback (most recent call last):
+     ...
+    TypeError: f() takes at least 1 positional argument but 0 positional arguments (and 1 keyword-only argument) were given
+    >>> f(kw=3, c=4)
+    Traceback (most recent call last):
+     ...
+    TypeError: f() takes at least 1 positional argument but 1 positional argument (and 1 keyword-only argument) were given
 """
 
 import sys
diff --git a/Lib/test/test_keywordonlyarg.py b/Lib/test/test_keywordonlyarg.py
index d7f7541..44be32c 100644
--- a/Lib/test/test_keywordonlyarg.py
+++ b/Lib/test/test_keywordonlyarg.py
@@ -78,7 +78,7 @@ class KeywordOnlyArgTestCase(unittest.TestCase):
             pass
         with self.assertRaises(TypeError) as exc:
             f(1, 2, 3)
-        expected = "f() takes at most 2 positional arguments (3 given)"
+        expected = "f() takes from 1 to 2 positional arguments but 3 were given"
         self.assertEqual(str(exc.exception), expected)
 
     def testSyntaxErrorForFunctionCall(self):
diff --git a/Misc/NEWS b/Misc/NEWS
index 8485b9c..650ae60 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -10,6 +10,9 @@ What's New in Python 3.3 Alpha 1?
 Core and Builtins
 -----------------
 
+- Issue #12265: Make error messages produced by passing an invalid set of
+  arguments to a function more informative.
+
 - Issue #12225: Still allow Python to build if Python is not in its hg repo or
   mercurial is not installed.
 
diff --git a/Python/ceval.c b/Python/ceval.c
index 5627e83..f1f4c70 100644
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -3045,6 +3045,63 @@ exit_eval_frame:
     return retval;
 }
 
+static void
+positional_argument_error(PyCodeObject *co, int given, int defcount, PyObject **fastlocals)
+{
+    int plural;
+    int kwonly_given = 0;
+    int atleast = co->co_argcount - defcount;
+    int i;
+    PyObject *sig, *kwonly_sig;
+
+    if (given == -1) {
+        given = 0;
+        for (i = 0; i < co->co_argcount; i++)
+            if (GETLOCAL(i))
+                given++;
+    }
+    for (i = co->co_argcount; i < co->co_argcount + co->co_kwonlyargcount; i++)
+        if (GETLOCAL(i))
+            kwonly_given++;
+    if (co->co_flags & CO_VARARGS) {
+        plural = atleast != 1;
+        sig = PyUnicode_FromFormat("at least %d", atleast);
+    }
+    else if (defcount) {
+        plural = 1;
+        sig = PyUnicode_FromFormat("from %d to %d", atleast, co->co_argcount);
+    }
+    else {
+        plural = co->co_argcount != 1;
+        sig = PyUnicode_FromFormat("%d", co->co_argcount);
+    }
+    if (sig == NULL)
+        return;
+    if (kwonly_given) {
+        const char *format = " positional argument%s (and %d keyword-only argument%s)";
+        kwonly_sig = PyUnicode_FromFormat(format, given != 1 ? "s" : "", kwonly_given,
+                                              kwonly_given != 1 ? "s" : "");
+        if (kwonly_sig == NULL) {
+            Py_DECREF(sig);
+            return;
+        }
+    }
+    else {
+        /* This will not fail. */
+        kwonly_sig = PyUnicode_FromString("");
+    }
+    PyErr_Format(PyExc_TypeError,
+                 "%U() takes %U positional argument%s but %d%U %s given",
+                 co->co_name,
+                 sig,
+                 plural ? "s" : "",
+                 given,
+                 kwonly_sig,
+                 given == 1 && !kwonly_given ? "was" : "were");
+    Py_DECREF(sig);
+    Py_DECREF(kwonly_sig);
+}
+
 /* This is gonna seem *real weird*, but if you put some other code between
    PyEval_EvalFrame() and PyEval_EvalCodeEx() you will need to adjust
    the test in the if statements in Misc/gdbinit (pystack and pystackv). */
@@ -3061,6 +3118,9 @@ PyEval_EvalCodeEx(PyObject *_co, PyObject *globals, PyObject *locals,
     PyThreadState *tstate = PyThreadState_GET();
     PyObject *x, *u;
     int total_args = co->co_argcount + co->co_kwonlyargcount;
+    int i;
+    int n = argcount;
+    PyObject *kwdict = NULL;
 
     if (globals == NULL) {
         PyErr_SetString(PyExc_SystemError,
@@ -3077,161 +3137,130 @@ PyEval_EvalCodeEx(PyObject *_co, PyObject *globals, PyObject *locals,
     fastlocals = f->f_localsplus;
     freevars = f->f_localsplus + co->co_nlocals;
 
-    if (total_args || co->co_flags & (CO_VARARGS | CO_VARKEYWORDS)) {
-        int i;
-        int n = argcount;
-        PyObject *kwdict = NULL;
-        if (co->co_flags & CO_VARKEYWORDS) {
-            kwdict = PyDict_New();
-            if (kwdict == NULL)
-                goto fail;
-            i = total_args;
-            if (co->co_flags & CO_VARARGS)
-                i++;
-            SETLOCAL(i, kwdict);
-        }
-        if (argcount > co->co_argcount) {
-            if (!(co->co_flags & CO_VARARGS)) {
-                PyErr_Format(PyExc_TypeError,
-                    "%U() takes %s %d "
-                    "positional argument%s (%d given)",
-                    co->co_name,
-                    defcount ? "at most" : "exactly",
-                    co->co_argcount,
-                    co->co_argcount == 1 ? "" : "s",
-                    argcount + kwcount);
-                goto fail;
-            }
-            n = co->co_argcount;
-        }
-        for (i = 0; i < n; i++) {
+    /* Parse arguments. */
+    if (co->co_flags & CO_VARKEYWORDS) {
+        kwdict = PyDict_New();
+        if (kwdict == NULL)
+            goto fail;
+        i = total_args;
+        if (co->co_flags & CO_VARARGS)
+            i++;
+        SETLOCAL(i, kwdict);
+    }
+    if (argcount > co->co_argcount)
+        n = co->co_argcount;
+    for (i = 0; i < n; i++) {
+        x = args[i];
+        Py_INCREF(x);
+        SETLOCAL(i, x);
+    }
+    if (co->co_flags & CO_VARARGS) {
+        u = PyTuple_New(argcount - n);
+        if (u == NULL)
+            goto fail;
+        SETLOCAL(total_args, u);
+        for (i = n; i < argcount; i++) {
             x = args[i];
             Py_INCREF(x);
-            SETLOCAL(i, x);
+            PyTuple_SET_ITEM(u, i-n, x);
         }
-        if (co->co_flags & CO_VARARGS) {
-            u = PyTuple_New(argcount - n);
-            if (u == NULL)
-                goto fail;
-            SETLOCAL(total_args, u);
-            for (i = n; i < argcount; i++) {
-                x = args[i];
-                Py_INCREF(x);
-                PyTuple_SET_ITEM(u, i-n, x);
-            }
+    }
+    for (i = 0; i < kwcount; i++) {
+        PyObject **co_varnames;
+        PyObject *keyword = kws[2*i];
+        PyObject *value = kws[2*i + 1];
+        int j;
+        if (keyword == NULL || !PyUnicode_Check(keyword)) {
+            PyErr_Format(PyExc_TypeError,
+                         "%U() keywords must be strings",
+                         co->co_name);
+            goto fail;
         }
-        for (i = 0; i < kwcount; i++) {
-            PyObject **co_varnames;
-            PyObject *keyword = kws[2*i];
-            PyObject *value = kws[2*i + 1];
-            int j;
-            if (keyword == NULL || !PyUnicode_Check(keyword)) {
-                PyErr_Format(PyExc_TypeError,
-                    "%U() keywords must be strings",
-                    co->co_name);
-                goto fail;
-            }
-            /* Speed hack: do raw pointer compares. As names are
-               normally interned this should almost always hit. */
-            co_varnames = ((PyTupleObject *)(co->co_varnames))->ob_item;
-            for (j = 0; j < total_args; j++) {
-                PyObject *nm = co_varnames[j];
-                if (nm == keyword)
-                    goto kw_found;
-            }
-            /* Slow fallback, just in case */
-            for (j = 0; j < total_args; j++) {
-                PyObject *nm = co_varnames[j];
-                int cmp = PyObject_RichCompareBool(
-                    keyword, nm, Py_EQ);
-                if (cmp > 0)
-                    goto kw_found;
-                else if (cmp < 0)
-                    goto fail;
-            }
-            if (j >= total_args && kwdict == NULL) {
-                PyErr_Format(PyExc_TypeError,
-                             "%U() got an unexpected "
-                             "keyword argument '%S'",
-                             co->co_name,
-                             keyword);
+        /* Speed hack: do raw pointer compares. As names are
+           normally interned this should almost always hit. */
+        co_varnames = ((PyTupleObject *)(co->co_varnames))->ob_item;
+        for (j = 0; j < total_args; j++) {
+            PyObject *nm = co_varnames[j];
+            if (nm == keyword)
+                goto kw_found;
+        }
+        /* Slow fallback, just in case */
+        for (j = 0; j < total_args; j++) {
+            PyObject *nm = co_varnames[j];
+            int cmp = PyObject_RichCompareBool(
+                keyword, nm, Py_EQ);
+            if (cmp > 0)
+                goto kw_found;
+            else if (cmp < 0)
                 goto fail;
-            }
-            PyDict_SetItem(kwdict, keyword, value);
-            continue;
-          kw_found:
-            if (GETLOCAL(j) != NULL) {
-                PyErr_Format(PyExc_TypeError,
+        }
+        if (j >= total_args && kwdict == NULL) {
+            PyErr_Format(PyExc_TypeError,
+                         "%U() got an unexpected "
+                         "keyword argument '%S'",
+                         co->co_name,
+                         keyword);
+            goto fail;
+        }
+        PyDict_SetItem(kwdict, keyword, value);
+        continue;
+      kw_found:
+        if (GETLOCAL(j) != NULL) {
+            PyErr_Format(PyExc_TypeError,
                          "%U() got multiple "
-                         "values for keyword "
-                         "argument '%S'",
+                         "values for argument '%S'",
                          co->co_name,
                          keyword);
-                goto fail;
-            }
-            Py_INCREF(value);
-            SETLOCAL(j, value);
+            goto fail;
         }
-        if (co->co_kwonlyargcount > 0) {
-            for (i = co->co_argcount; i < total_args; i++) {
-                PyObject *name;
-                if (GETLOCAL(i) != NULL)
-                    continue;
-                name = PyTuple_GET_ITEM(co->co_varnames, i);
-                if (kwdefs != NULL) {
-                    PyObject *def = PyDict_GetItem(kwdefs, name);
-                    if (def) {
-                        Py_INCREF(def);
-                        SETLOCAL(i, def);
-                        continue;
-                    }
-                }
-                PyErr_Format(PyExc_TypeError,
-                    "%U() needs keyword-only argument %S",
-                    co->co_name, name);
+        Py_INCREF(value);
+        SETLOCAL(j, value);
+    }
+    if (argcount > co->co_argcount && !(co->co_flags & CO_VARARGS)) {
+        positional_argument_error(co, argcount, defcount, fastlocals);
+        goto fail;
+    }
+    if (argcount < co->co_argcount) {
+        int m = co->co_argcount - defcount;
+        for (i = argcount; i < m; i++) {
+            if (GETLOCAL(i) == NULL) {
+                positional_argument_error(co, -1, defcount, fastlocals);
                 goto fail;
             }
         }
-        if (argcount < co->co_argcount) {
-            int m = co->co_argcount - defcount;
-            for (i = argcount; i < m; i++) {
-                if (GETLOCAL(i) == NULL) {
-                    int j, given = 0;
-                    for (j = 0; j < co->co_argcount; j++)
-                        if (GETLOCAL(j))
-                            given++;
-                    PyErr_Format(PyExc_TypeError,
-                        "%U() takes %s %d "
-                        "argument%s "
-                        "(%d given)",
-                        co->co_name,
-                        ((co->co_flags & CO_VARARGS) ||
-                         defcount) ? "at least"
-                                   : "exactly",
-                             m, m == 1 ? "" : "s", given);
-                    goto fail;
-                }
+        if (n > m)
+            i = n - m;
+        else
+            i = 0;
+        for (; i < defcount; i++) {
+            if (GETLOCAL(m+i) == NULL) {
+                PyObject *def = defs[i];
+                Py_INCREF(def);
+                SETLOCAL(m+i, def);
             }
-            if (n > m)
-                i = n - m;
-            else
-                i = 0;
-            for (; i < defcount; i++) {
-                if (GETLOCAL(m+i) == NULL) {
-                    PyObject *def = defs[i];
+        }
+    }
+    if (co->co_kwonlyargcount > 0) {
+        for (i = co->co_argcount; i < total_args; i++) {
+            PyObject *name;
+            if (GETLOCAL(i) != NULL)
+                continue;
+            name = PyTuple_GET_ITEM(co->co_varnames, i);
+            if (kwdefs != NULL) {
+                PyObject *def = PyDict_GetItem(kwdefs, name);
+                if (def) {
                     Py_INCREF(def);
-                    SETLOCAL(m+i, def);
+                    SETLOCAL(i, def);
+                    continue;
                 }
             }
+            PyErr_Format(PyExc_TypeError,
+                         "%U() requires keyword-only argument '%S'",
+                         co->co_name, name);
+            goto fail;
         }
     }
-    else if (argcount > 0 || kwcount > 0) {
-        PyErr_Format(PyExc_TypeError,
-                     "%U() takes no arguments (%d given)",
-                     co->co_name,
-                     argcount + kwcount);
-        goto fail;
-    }
+
     /* Allocate and initialize storage for cell vars, and copy free
        vars into frame.  This isn't too efficient right now. */
     if (PyTuple_GET_SIZE(co->co_cellvars)) {