This patch makes sure that the function name always appears in the error

message, and tries to make the messages more consistent and helpful when
the wrong number of arguments or duplicate keyword arguments are supplied.
Comes with more tests for test_extcall.py and and an update to an error
message in test/output/test_pyexpat.

2 files changed, 89 insertions, 55 deletions

diff --git a/Python/ceval.c b/Python/ceval.c
index ed201b390f..36f0017c3a 100644
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -48,8 +48,8 @@ static PyObject *fast_function(PyObject *, PyObject ***, int, int, int);
 static PyObject *fast_cfunction(PyObject *, PyObject ***, int);
 static PyObject *do_call(PyObject *, PyObject ***, int, int);
 static PyObject *ext_do_call(PyObject *, PyObject ***, int, int, int);
-static PyObject *update_keyword_args(PyObject *, int, PyObject ***);
-static PyObject *update_star_args(int, int, PyObject *,	PyObject ***);
+static PyObject *update_keyword_args(PyObject *, int, PyObject ***, PyObject *);
+static PyObject *update_star_args(int, int, PyObject *, PyObject ***);
 static PyObject *load_args(PyObject ***, int);
 #define CALL_FLAG_VAR 1
 #define CALL_FLAG_KW 2
@@ -451,10 +451,14 @@ eval_code2(PyCodeObject *co, PyObject *globals, PyObject *locals,
 		if (argcount > co->co_argcount) {
 			if (!(co->co_flags & CO_VARARGS)) {
 				PyErr_Format(PyExc_TypeError,
-				    "too many arguments to %s(); "
-				    "expected %d, got %d",
+				    "%.200s() takes %s %d "
+				    "%sargument%s (%d given)",
 				    PyString_AsString(co->co_name),
-				    co->co_argcount, argcount);
+				    defcount ? "at most" : "exactly",
+				    co->co_argcount,
+				    kwcount ? "non-keyword " : "",
+				    co->co_argcount == 1 ? "" : "s",
+				    argcount);
 				goto fail;
 			}
 			n = co->co_argcount;
@@ -480,8 +484,9 @@ eval_code2(PyCodeObject *co, PyObject *globals, PyObject *locals,
 			PyObject *value = kws[2*i + 1];
 			int j;
 			if (keyword == NULL || !PyString_Check(keyword)) {
-				PyErr_SetString(PyExc_TypeError,
-						"keywords must be strings");
+				PyErr_Format(PyExc_TypeError,
+				    "%.200s() keywords must be strings",
+				    PyString_AsString(co->co_name));
 				goto fail;
 			}
 			/* XXX slow -- speed up using dictionary? */
@@ -508,10 +513,11 @@ eval_code2(PyCodeObject *co, PyObject *globals, PyObject *locals,
 			else {
 				if (GETLOCAL(j) != NULL) {
 					PyErr_Format(PyExc_TypeError, 
-					     "keyword parameter '%.400s' "
-					     "redefined in call to %.200s()",
-					     PyString_AsString(keyword),
-					     PyString_AsString(co->co_name));
+					     "%.200s() got multiple "
+					     "values for keyword "
+					     "argument '%.400s'",
+					     PyString_AsString(co->co_name),
+					     PyString_AsString(keyword));
 					goto fail;
 				}
 				Py_INCREF(value);
@@ -523,10 +529,14 @@ eval_code2(PyCodeObject *co, PyObject *globals, PyObject *locals,
 			for (i = argcount; i < m; i++) {
 				if (GETLOCAL(i) == NULL) {
 					PyErr_Format(PyExc_TypeError,
-					    "not enough arguments to "
-					    "%.200s(); expected %d, got %d",
+					    "%.200s() takes %s %d "
+					    "%sargument%s (%d given)",
 					    PyString_AsString(co->co_name),
-					    m, i);
+					    ((co->co_flags & CO_VARARGS) ||
+					     defcount) ? "at least"
+						       : "exactly",
+					    m, kwcount ? "non-keyword " : "",
+					    m == 1 ? "" : "s", i);
 					goto fail;
 				}
 			}
@@ -546,8 +556,9 @@ eval_code2(PyCodeObject *co, PyObject *globals, PyObject *locals,
 	else {
 		if (argcount > 0 || kwcount > 0) {
 			PyErr_Format(PyExc_TypeError,
-				     "%.200s() expected no arguments",
-				     PyString_AsString(co->co_name));
+				     "%.200s() takes no arguments (%d given)",
+				     PyString_AsString(co->co_name),
+				     argcount + kwcount);
 			goto fail;
 		}
 	}
@@ -2669,8 +2680,12 @@ call_method(PyObject *func, PyObject *arg, PyObject *kw)
 		    && PyClass_IsSubclass((PyObject *)
 				  (((PyInstanceObject *)self)->in_class),
 					  class))) {
-		PyErr_SetString(PyExc_TypeError,
-	"unbound method must be called with instance as first argument");
+                PyObject* fn = ((PyFunctionObject*) func)->func_name;
+		PyErr_Format(PyExc_TypeError,
+                             "unbound method %s%smust be "
+                             "called with instance as first argument",
+                             fn ? PyString_AsString(fn) : "",
+                             fn ? "() " : "");
 			return NULL;
 		}
 		Py_INCREF(arg);
@@ -2793,7 +2808,8 @@ fast_function(PyObject *func, PyObject ***pp_stack, int n, int na, int nk)
 }
 
 static PyObject *
-update_keyword_args(PyObject *orig_kwdict, int nk, PyObject ***pp_stack)
+update_keyword_args(PyObject *orig_kwdict, int nk, PyObject ***pp_stack,
+                    PyObject *func)
 {
 	PyObject *kwdict = NULL;
 	if (orig_kwdict == NULL)
@@ -2809,10 +2825,12 @@ update_keyword_args(PyObject *orig_kwdict, int nk, PyObject ***pp_stack)
 		PyObject *value = EXT_POP(*pp_stack);
 		PyObject *key = EXT_POP(*pp_stack);
 		if (PyDict_GetItem(kwdict, key) != NULL) {
-			PyErr_Format(PyExc_TypeError, 
-				     "keyword parameter '%.400s' "
-				     "redefined in function call",
-				     PyString_AsString(key));
+                        PyObject* fn = ((PyFunctionObject*) func)->func_name;
+                        PyErr_Format(PyExc_TypeError, 
+                                     "%.200s%s got multiple values "
+                                     "for keyword argument '%.400s'",
+                                     fn ? PyString_AsString(fn) : "function",
+                                     fn ? "()" : "", PyString_AsString(key));
 			Py_DECREF(key);
 			Py_DECREF(value);
 			Py_DECREF(kwdict);
@@ -2877,7 +2895,7 @@ do_call(PyObject *func, PyObject ***pp_stack, int na, int nk)
 	PyObject *result = NULL;
 
 	if (nk > 0) {
-		kwdict = update_keyword_args(NULL, nk, pp_stack);
+		kwdict = update_keyword_args(NULL, nk, pp_stack, func);
 		if (kwdict == NULL)
 			goto call_fail;
 	}
@@ -2903,8 +2921,11 @@ ext_do_call(PyObject *func, PyObject ***pp_stack, int flags, int na, int nk)
 	if (flags & CALL_FLAG_KW) {
 		kwdict = EXT_POP(*pp_stack);
 		if (!(kwdict && PyDict_Check(kwdict))) {
-			PyErr_SetString(PyExc_TypeError,
-					"** argument must be a dictionary");
+                        PyObject* fn = ((PyFunctionObject*) func)->func_name;
+			PyErr_Format(PyExc_TypeError,
+                            "%s%s argument after ** must be a dictionary",
+                            fn ? PyString_AsString(fn) : "function",
+                            fn ? "()" : "");
 			goto ext_call_fail;
 		}
 	}
@@ -2915,8 +2936,12 @@ ext_do_call(PyObject *func, PyObject ***pp_stack, int flags, int na, int nk)
 			t = PySequence_Tuple(stararg);
 			if (t == NULL) {
 			    if (PyErr_ExceptionMatches(PyExc_TypeError)) {
-				PyErr_SetString(PyExc_TypeError,
-					"* argument must be a sequence");
+                                PyObject* fn =
+                                    ((PyFunctionObject*) func)->func_name;
+				PyErr_Format(PyExc_TypeError,
+                                    "%s%s argument after * must be a sequence",
+                                    fn ? PyString_AsString(fn) : "function",
+                                    fn ? "()" : "");
 			    }
 				goto ext_call_fail;
 			}
@@ -2926,7 +2951,7 @@ ext_do_call(PyObject *func, PyObject ***pp_stack, int flags, int na, int nk)
 		nstar = PyTuple_GET_SIZE(stararg);
 	}
 	if (nk > 0) {
-		kwdict = update_keyword_args(kwdict, nk, pp_stack);
+		kwdict = update_keyword_args(kwdict, nk, pp_stack, func);
 		if (kwdict == NULL)
 			goto ext_call_fail;
 	}
diff --git a/Python/getargs.c b/Python/getargs.c
index cf1c6b72a2..5a9418380b 100644
--- a/Python/getargs.c
+++ b/Python/getargs.c
@@ -130,16 +130,18 @@ vgetargs1(PyObject *args, char *format, va_list *p_va, int compat)
 		if (max == 0) {
 			if (args == NULL)
 				return 1;
-			sprintf(msgbuf, "%s requires no arguments",
-				fname==NULL ? "function" : fname);
+			sprintf(msgbuf, "%s%s takes no arguments",
+				fname==NULL ? "function" : fname,
+				fname==NULL ? "" : "()");
 			PyErr_SetString(PyExc_TypeError, msgbuf);
 			return 0;
 		}
 		else if (min == 1 && max == 1) {
 			if (args == NULL) {
 				sprintf(msgbuf,
-					"%s requires at least one argument",
-					fname==NULL ? "function" : fname);
+					"%s%s takes at least one argument",
+					fname==NULL ? "function" : fname,
+					fname==NULL ? "" : "()");
 				PyErr_SetString(PyExc_TypeError, msgbuf);
 				return 0;
 			}
@@ -167,8 +169,9 @@ vgetargs1(PyObject *args, char *format, va_list *p_va, int compat)
 	if (len < min || max < len) {
 		if (message == NULL) {
 			sprintf(msgbuf,
-				"%s requires %s %d argument%s; %d given",
+				"%s%s takes %s %d argument%s (%d given)",
 				fname==NULL ? "function" : fname,
+				fname==NULL ? "" : "()",
 				min==max ? "exactly"
 				         : len < min ? "at least" : "at most",
 				len < min ? min : max,
@@ -213,22 +216,26 @@ seterror(int iarg, char *msg, int *levels, char *fname, char *message)
 
 	if (PyErr_Occurred())
 		return;
-	if (iarg == 0 && message == NULL)
-		message = msg;
 	else if (message == NULL) {
 		if (fname != NULL) {
-			sprintf(p, "%s, ", fname);
+			sprintf(p, "%s() ", fname);
 			p += strlen(p);
 		}
-		sprintf(p, "argument %d", iarg);
-		i = 0;
-		p += strlen(p);
-		while (levels[i] > 0) {
-			sprintf(p, ", item %d", levels[i]-1);
+		if (iarg != 0) {
+			sprintf(p, "argument %d", iarg);
+			i = 0;
+			p += strlen(p);
+			while (levels[i] > 0) {
+				sprintf(p, ", item %d", levels[i]-1);
+				p += strlen(p);
+				i++;
+			}
+		}
+		else {
+			sprintf(p, "argument");
 			p += strlen(p);
-			i++;
 		}
-		sprintf(p, ": expected %s found", msg);
+		sprintf(p, " %s", msg);
 		message = buf;
 	}
 	PyErr_SetString(PyExc_TypeError, message);
@@ -247,10 +254,9 @@ seterror(int iarg, char *msg, int *levels, char *fname, char *message)
       *p_va is undefined,
       *levels is a 0-terminated list of item numbers,
       *msgbuf contains an error message, whose format is:
-         "<typename1>, <typename2>", where:
+         "must be <typename1>, not <typename2>", where:
             <typename1> is the name of the expected type, and
             <typename2> is the name of the actual type,
-         (so you can surround it by "expected ... found"),
       and msgbuf is returned.
 */
 
@@ -281,10 +287,11 @@ converttuple(PyObject *arg, char **p_format, va_list *p_va, int *levels,
 			n++;
 	}
 	
-	if (!PySequence_Check(arg)) {
+	if (!PySequence_Check(arg) || PyString_Check(arg)) {
 		levels[0] = 0;
 		sprintf(msgbuf,
-			toplevel ? "%d arguments, %s" : "%d-sequence, %s",
+			toplevel ? "expected %d arguments, not %s" :
+				   "must be %d-item sequence, not %s",
 			n, arg == Py_None ? "None" : arg->ob_type->tp_name);
 		return msgbuf;
 	}
@@ -292,11 +299,12 @@ converttuple(PyObject *arg, char **p_format, va_list *p_va, int *levels,
 	if ((i = PySequence_Size(arg)) != n) {
 		levels[0] = 0;
 		sprintf(msgbuf,
-		    toplevel ? "%d arguments, %d" : "%d-sequence, %d-sequence",
-		    n, i);
+			toplevel ? "expected %d arguments, not %d" :
+				   "must be sequence of length %d, not %d",
+			n, i);
 		return msgbuf;
 	}
-	
+
 	format = *p_format;
 	for (i = 0; i < n; i++) {
 		char *msg;
@@ -310,7 +318,7 @@ converttuple(PyObject *arg, char **p_format, va_list *p_va, int *levels,
 			return msg;
 		}
 	}
-	
+
 	*p_format = format;
 	return NULL;
 }
@@ -343,14 +351,14 @@ convertitem(PyObject *arg, char **p_format, va_list *p_va, int *levels,
 
 
 /* Convert a non-tuple argument.  Adds to convertsimple1 functionality
-   by appending ", <actual argument type>" to error message. */
+   by formatting messages as "must be <desired type>, not <actual type>". */
 
 static char *
 convertsimple(PyObject *arg, char **p_format, va_list *p_va, char *msgbuf)
 {
 	char *msg = convertsimple1(arg, p_format, p_va);
 	if (msg != NULL) {
-		sprintf(msgbuf, "%.50s, %.50s", msg,
+		sprintf(msgbuf, "must be %.50s, not %.50s", msg,
 			arg == Py_None ? "None" : arg->ob_type->tp_name);
 		msg = msgbuf;
 	}
@@ -1063,8 +1071,9 @@ vgetargskeywords(PyObject *args, PyObject *keywords, char *format,
 	if (len < min || max < len) {
 		if (message == NULL) {
 			sprintf(msgbuf,
-				"%s requires %s %d argument%s; %d given",
+				"%s%s takes %s %d argument%s (%d given)",
 				fname==NULL ? "function" : fname,
+				fname==NULL ? "" : "()",
 				min==max ? "exactly"
 				         : len < min ? "at least" : "at most",
 				len < min ? min : max,