From b204a42383642cd7a4ff756b0d5de8b31e65644d Mon Sep 17 00:00:00 2001 From: Benjamin Peterson Date: Sun, 5 Jun 2011 22:04:07 -0500 Subject: [PATCH] greatly improve argument parsing error messages (closes #12265) --- Lib/inspect.py | 109 ++++++----- Lib/test/test_extcall.py | 82 ++++++++- Lib/test/test_keywordonlyarg.py | 2 +- Misc/NEWS | 3 + Python/ceval.c | 315 +++++++++++++++++--------------- 5 files changed, 307 insertions(+), 204 deletions(-) diff --git a/Lib/inspect.py b/Lib/inspect.py index 4899cbf9f05..f200a82f333 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 - 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 + n = min(num_pos, num_args) + for i in range(n): + arg2value[args[i]] = positional[i] + if varargs: + 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 1f7f63042e7..49d5441345a 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 . >>> 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 d7f7541837d..44be32c0498 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 8485b9c3556..650ae608c1e 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 5627e839993..f1f4c70da18 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); + /* 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; - } - /* 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); - 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'", + } + if (j >= total_args && kwdict == NULL) { + PyErr_Format(PyExc_TypeError, + "%U() got an unexpected " + "keyword 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); - 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); - } - } + PyDict_SetItem(kwdict, keyword, value); + continue; + kw_found: + if (GETLOCAL(j) != NULL) { + PyErr_Format(PyExc_TypeError, + "%U() got multiple " + "values for argument '%S'", + co->co_name, + keyword); + goto fail; } + Py_INCREF(value); + SETLOCAL(j, value); } - else if (argcount > 0 || kwcount > 0) { - PyErr_Format(PyExc_TypeError, - "%U() takes no arguments (%d given)", - co->co_name, - argcount + kwcount); + 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 (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 (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() requires keyword-only argument '%S'", + co->co_name, name); + 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)) {