From f78eaeb0f2d17dcc66e2f51a3b36938d5a017b5f Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Wed, 31 Jan 2024 01:05:07 -0800 Subject: [PATCH 1/6] gh-107944: Improve error message for getargs with bad keyword arguments --- Lib/test/test_call.py | 12 +++++++++--- Python/getargs.c | 32 ++++++++++++++++++++++++++------ 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_call.py b/Lib/test/test_call.py index 3c8fc35e3c116d..a4e3a26e2ce900 100644 --- a/Lib/test/test_call.py +++ b/Lib/test/test_call.py @@ -155,7 +155,7 @@ def test_varargs16_kw(self): min, 0, default=1, key=2, foo=3) def test_varargs17_kw(self): - msg = r"'foo' is an invalid keyword argument for print\(\)$" + msg = r"print\(\) got an unexpected keyword argument 'foo'$" self.assertRaisesRegex(TypeError, msg, print, 0, sep=1, end=2, file=3, flush=4, foo=5) @@ -928,7 +928,7 @@ def check_suggestion_includes(self, message): self.assertIn(f"Did you mean '{message}'?", str(cm.exception)) @contextlib.contextmanager - def check_suggestion_not_pressent(self): + def check_suggestion_not_present(self): with self.assertRaises(TypeError) as cm: yield self.assertNotIn("Did you mean", str(cm.exception)) @@ -946,7 +946,7 @@ def foo(blech=None, /, aaa=None, *args, late1=None): for keyword, suggestion in cases: with self.subTest(keyword): - ctx = self.check_suggestion_includes(suggestion) if suggestion else self.check_suggestion_not_pressent() + ctx = self.check_suggestion_includes(suggestion) if suggestion else self.check_suggestion_not_present() with ctx: foo(**{keyword:None}) @@ -987,6 +987,12 @@ def case_change_over_substitution(BLuch=None, Luch = None, fluch = None): with self.check_suggestion_includes(suggestion): func(bluch=None) + def test_unexpected_keyword_suggestion_via_getargs(self): + with self.check_suggestion_includes("maxsplit"): + "foo".split(maxsplt=1) + with self.check_suggestion_not_present(): + "foo".split(more_noise=1) + @cpython_only class TestRecursion(unittest.TestCase): diff --git a/Python/getargs.c b/Python/getargs.c index 0c4ce282f48764..8dfaa5e4dc60c5 100644 --- a/Python/getargs.c +++ b/Python/getargs.c @@ -8,6 +8,7 @@ #include "pycore_modsupport.h" // export _PyArg_NoKeywords() #include "pycore_pylifecycle.h" // _PyArg_Fini #include "pycore_tuple.h" // _PyTuple_ITEMS() +#include "pycore_pyerrors.h" // _Py_CalculateSuggestions() /* Export Stable ABIs (abi only) */ PyAPI_FUNC(int) _PyArg_Parse_SizeT(PyObject *, const char *, ...); @@ -1424,12 +1425,31 @@ error_unexpected_keyword_arg(PyObject *kwargs, PyObject *kwnames, PyObject *kwtu int match = PySequence_Contains(kwtuple, keyword); if (match <= 0) { if (!match) { - PyErr_Format(PyExc_TypeError, - "'%S' is an invalid keyword " - "argument for %.200s%s", - keyword, - (fname == NULL) ? "this function" : fname, - (fname == NULL) ? "" : "()"); + PyObject *kwlist = PySequence_List(kwtuple); + if (kwlist == NULL) { + return; + } + PyObject *suggestion_keyword = _Py_CalculateSuggestions(kwlist, keyword); + Py_DECREF(kwlist); + + if (suggestion_keyword) { + PyErr_Format(PyExc_TypeError, + "%.200s%s got an unexpected keyword argument '%S'." + " Did you mean '%S'?", + (fname == NULL) ? "this function" : fname, + (fname == NULL) ? "" : "()", + keyword, + suggestion_keyword); + Py_DECREF(suggestion_keyword); + } + else { + PyErr_Format(PyExc_TypeError, + "%.200s%s got an unexpected keyword argument '%S'", + (fname == NULL) ? "this function" : fname, + (fname == NULL) ? "" : "()", + keyword); + } + } return; } From 713f85ade4c28461160d44b5545abeddf4485466 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Wed, 31 Jan 2024 09:10:15 +0000 Subject: [PATCH 2/6] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2024-01-31-09-10-10.gh-issue-107944.XWm1B-.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-01-31-09-10-10.gh-issue-107944.XWm1B-.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-01-31-09-10-10.gh-issue-107944.XWm1B-.rst b/Misc/NEWS.d/next/Core and Builtins/2024-01-31-09-10-10.gh-issue-107944.XWm1B-.rst new file mode 100644 index 00000000000000..8e3fb786c11055 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-01-31-09-10-10.gh-issue-107944.XWm1B-.rst @@ -0,0 +1 @@ +Improve error message for function calls with bad keyword arguments via getargs From a3039531c4eabd8002e3d64c6e260e7f559aec62 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Wed, 31 Jan 2024 01:13:45 -0800 Subject: [PATCH 3/6] add a what's new --- Doc/whatsnew/3.13.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index 985e34b453f63a..118c6163f0ddfd 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -94,6 +94,17 @@ Improved Error Messages variables. See also :ref:`using-on-controlling-color`. (Contributed by Pablo Galindo Salgado in :gh:`112730`.) +* When an incorrect keyword argument is passed to a function, the error message + now potentially suggests the correct keyword argument. + (Contributed by Pablo Galindo Salgado and Shantanu Jain in :gh:`107944`.) + + >>> "better error messages!".split(max_split=1) + Traceback (most recent call last): + File "", line 1, in + "better error messages!".split(max_split=1) + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^ + TypeError: split() got an unexpected keyword argument 'max_split'. Did you mean 'maxsplit'? + Other Language Changes ====================== From 7a3804d767f30b295dad4e1637dff56c4bc36da1 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Wed, 31 Jan 2024 01:18:32 -0800 Subject: [PATCH 4/6] add another test case --- Lib/test/test_call.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/test/test_call.py b/Lib/test/test_call.py index a4e3a26e2ce900..f399626844eca8 100644 --- a/Lib/test/test_call.py +++ b/Lib/test/test_call.py @@ -992,6 +992,8 @@ def test_unexpected_keyword_suggestion_via_getargs(self): "foo".split(maxsplt=1) with self.check_suggestion_not_present(): "foo".split(more_noise=1) + with self.check_suggestion_not_present(): + "foo".split(more_noise=1, maxsplt=1) @cpython_only class TestRecursion(unittest.TestCase): From d1785414ac36f0a843cbcc8b558a2415e4ba65cc Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Wed, 31 Jan 2024 01:54:02 -0800 Subject: [PATCH 5/6] Fix vgetargskeywords path, explicit error test --- Lib/test/test_call.py | 19 +++++++++++++- Lib/test/test_capi/test_getargs.py | 26 +++++++++---------- Lib/test/test_exceptions.py | 2 +- Python/getargs.c | 40 ++++++++++++++++++++++++------ 4 files changed, 65 insertions(+), 22 deletions(-) diff --git a/Lib/test/test_call.py b/Lib/test/test_call.py index f399626844eca8..97e0ac02dae316 100644 --- a/Lib/test/test_call.py +++ b/Lib/test/test_call.py @@ -990,11 +990,28 @@ def case_change_over_substitution(BLuch=None, Luch = None, fluch = None): def test_unexpected_keyword_suggestion_via_getargs(self): with self.check_suggestion_includes("maxsplit"): "foo".split(maxsplt=1) + + self.assertRaisesRegex( + TypeError, r"split\(\) got an unexpected keyword argument 'blech'$", + "foo".split, blech=1 + ) with self.check_suggestion_not_present(): - "foo".split(more_noise=1) + "foo".split(blech=1) with self.check_suggestion_not_present(): "foo".split(more_noise=1, maxsplt=1) + # Also test the vgetargskeywords path + self.assertRaisesRegex( + TypeError, r"ImportError\(\) got an unexpected keyword argument 'blech'$", + ImportError, blech=1 + ) + with self.check_suggestion_includes("name"): + ImportError(namez="oops") + with self.check_suggestion_not_present(): + ImportError(blech=1) + with self.check_suggestion_not_present(): + ImportError(blech=1, namez="oops") + @cpython_only class TestRecursion(unittest.TestCase): diff --git a/Lib/test/test_capi/test_getargs.py b/Lib/test/test_capi/test_getargs.py index 9b6aef27625ad0..12039803ba543e 100644 --- a/Lib/test/test_capi/test_getargs.py +++ b/Lib/test/test_capi/test_getargs.py @@ -667,7 +667,7 @@ def test_invalid_keyword(self): try: getargs_keywords((1,2),3,arg5=10,arg666=666) except TypeError as err: - self.assertEqual(str(err), "'arg666' is an invalid keyword argument for this function") + self.assertEqual(str(err), "this function got an unexpected keyword argument 'arg666'") else: self.fail('TypeError should have been raised') @@ -675,7 +675,7 @@ def test_surrogate_keyword(self): try: getargs_keywords((1,2), 3, (4,(5,6)), (7,8,9), **{'\uDC80': 10}) except TypeError as err: - self.assertEqual(str(err), "'\udc80' is an invalid keyword argument for this function") + self.assertEqual(str(err), "this function got an unexpected keyword argument '\udc80'") else: self.fail('TypeError should have been raised') @@ -742,12 +742,12 @@ def test_too_many_args(self): def test_invalid_keyword(self): # extraneous keyword arg with self.assertRaisesRegex(TypeError, - "'monster' is an invalid keyword argument for this function"): + "this function got an unexpected keyword argument 'monster'"): getargs_keyword_only(1, 2, monster=666) def test_surrogate_keyword(self): with self.assertRaisesRegex(TypeError, - "'\udc80' is an invalid keyword argument for this function"): + "this function got an unexpected keyword argument '\udc80'"): getargs_keyword_only(1, 2, **{'\uDC80': 10}) def test_weird_str_subclass(self): @@ -761,7 +761,7 @@ def __hash__(self): "invalid keyword argument for this function"): getargs_keyword_only(1, 2, **{BadStr("keyword_only"): 3}) with self.assertRaisesRegex(TypeError, - "invalid keyword argument for this function"): + "this function got an unexpected keyword argument"): getargs_keyword_only(1, 2, **{BadStr("monster"): 666}) def test_weird_str_subclass2(self): @@ -774,7 +774,7 @@ def __hash__(self): "invalid keyword argument for this function"): getargs_keyword_only(1, 2, **{BadStr("keyword_only"): 3}) with self.assertRaisesRegex(TypeError, - "invalid keyword argument for this function"): + "this function got an unexpected keyword argument"): getargs_keyword_only(1, 2, **{BadStr("monster"): 666}) @@ -807,7 +807,7 @@ def test_required_args(self): def test_empty_keyword(self): with self.assertRaisesRegex(TypeError, - "'' is an invalid keyword argument for this function"): + "this function got an unexpected keyword argument ''"): self.getargs(1, 2, **{'': 666}) @@ -1204,7 +1204,7 @@ def test_basic(self): "function missing required argument 'a'"): parse((), {}, 'O', ['a']) with self.assertRaisesRegex(TypeError, - "'b' is an invalid keyword argument"): + "this function got an unexpected keyword argument 'b'"): parse((), {'b': 1}, '|O', ['a']) with self.assertRaisesRegex(TypeError, fr"argument for function given by name \('a'\) " @@ -1278,10 +1278,10 @@ def test_nonascii_keywords(self): fr"and position \(1\)"): parse((1,), {name: 2}, 'O|O', [name, 'b']) with self.assertRaisesRegex(TypeError, - f"'{name}' is an invalid keyword argument"): + f"this function got an unexpected keyword argument '{name}'"): parse((), {name: 1}, '|O', ['b']) with self.assertRaisesRegex(TypeError, - "'b' is an invalid keyword argument"): + "this function got an unexpected keyword argument 'b'"): parse((), {'b': 1}, '|O', [name]) invalid = name.encode() + (name.encode()[:-1] or b'\x80') @@ -1301,17 +1301,17 @@ def test_nonascii_keywords(self): for name2 in ('b', 'ë', 'ĉ', 'Ɐ', '𐀁'): with self.subTest(name2=name2): with self.assertRaisesRegex(TypeError, - f"'{name2}' is an invalid keyword argument"): + f"this function got an unexpected keyword argument '{name2}'"): parse((), {name2: 1}, '|O', [name]) name2 = name.encode().decode('latin1') if name2 != name: with self.assertRaisesRegex(TypeError, - f"'{name2}' is an invalid keyword argument"): + f"this function got an unexpected keyword argument '{name2}'"): parse((), {name2: 1}, '|O', [name]) name3 = name + '3' with self.assertRaisesRegex(TypeError, - f"'{name2}' is an invalid keyword argument"): + f"this function got an unexpected keyword argument '{name2}'"): parse((), {name2: 1, name3: 2}, '|OO', [name, name3]) def test_nested_tuple(self): diff --git a/Lib/test/test_exceptions.py b/Lib/test/test_exceptions.py index c57488e44aecc6..c7e76414ff0715 100644 --- a/Lib/test/test_exceptions.py +++ b/Lib/test/test_exceptions.py @@ -1917,7 +1917,7 @@ def test_attributes(self): self.assertEqual(exc.name, 'somename') self.assertEqual(exc.path, 'somepath') - msg = "'invalid' is an invalid keyword argument for ImportError" + msg = r"ImportError\(\) got an unexpected keyword argument 'invalid'" with self.assertRaisesRegex(TypeError, msg): ImportError('test', invalid='keyword') diff --git a/Python/getargs.c b/Python/getargs.c index 8dfaa5e4dc60c5..08e97ee3e627b5 100644 --- a/Python/getargs.c +++ b/Python/getargs.c @@ -1426,7 +1426,7 @@ error_unexpected_keyword_arg(PyObject *kwargs, PyObject *kwnames, PyObject *kwtu if (match <= 0) { if (!match) { PyObject *kwlist = PySequence_List(kwtuple); - if (kwlist == NULL) { + if (!kwlist) { return; } PyObject *suggestion_keyword = _Py_CalculateSuggestions(kwlist, keyword); @@ -1477,6 +1477,9 @@ PyArg_ValidateKeywordArguments(PyObject *kwargs) return 1; } +static PyObject * +new_kwtuple(const char * const *keywords, int total, int pos); + #define IS_END_OF_FORMAT(c) (c == '\0' || c == ';' || c == ':') static int @@ -1742,12 +1745,35 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format, } } if (!match) { - PyErr_Format(PyExc_TypeError, - "'%U' is an invalid keyword " - "argument for %.200s%s", - key, - (fname == NULL) ? "this function" : fname, - (fname == NULL) ? "" : "()"); + PyObject *_pykwtuple = new_kwtuple(kwlist, len, pos); + if (!_pykwtuple) { + return cleanreturn(0, &freelist); + } + PyObject *pykwlist = PySequence_List(_pykwtuple); + Py_DECREF(_pykwtuple); + if (!pykwlist) { + return cleanreturn(0, &freelist); + } + PyObject *suggestion_keyword = _Py_CalculateSuggestions(pykwlist, key); + Py_DECREF(pykwlist); + + if (suggestion_keyword) { + PyErr_Format(PyExc_TypeError, + "%.200s%s got an unexpected keyword argument '%S'." + " Did you mean '%S'?", + (fname == NULL) ? "this function" : fname, + (fname == NULL) ? "" : "()", + key, + suggestion_keyword); + Py_DECREF(suggestion_keyword); + } + else { + PyErr_Format(PyExc_TypeError, + "%.200s%s got an unexpected keyword argument '%S'", + (fname == NULL) ? "this function" : fname, + (fname == NULL) ? "" : "()", + key); + } return cleanreturn(0, &freelist); } } From ec06393b13361e10e1e21c0a2fcf45bcba271133 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Wed, 31 Jan 2024 01:58:29 -0800 Subject: [PATCH 6/6] nit: change test order --- Lib/test/test_call.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_call.py b/Lib/test/test_call.py index 97e0ac02dae316..2a6a5d287b04ee 100644 --- a/Lib/test/test_call.py +++ b/Lib/test/test_call.py @@ -1001,12 +1001,13 @@ def test_unexpected_keyword_suggestion_via_getargs(self): "foo".split(more_noise=1, maxsplt=1) # Also test the vgetargskeywords path + with self.check_suggestion_includes("name"): + ImportError(namez="oops") + self.assertRaisesRegex( TypeError, r"ImportError\(\) got an unexpected keyword argument 'blech'$", ImportError, blech=1 ) - with self.check_suggestion_includes("name"): - ImportError(namez="oops") with self.check_suggestion_not_present(): ImportError(blech=1) with self.check_suggestion_not_present():