Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion Lib/test/test_call.py
Original file line number Diff line number Diff line change
Expand Up @@ -910,15 +910,36 @@ def static_no_args():
def positional_only(arg, /):
pass

def method_with_self(self, arg, kwarg=1):
pass

def missing_self(another_arg):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good setup for the happy path. If possible, add a def method_with_self(self, x) on the same class A to cover the false positive case.

pass

def missing_self_no_args():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add a test case for def method_with_self_arg(x, self): ...

and then call it like inst.method_with_self().

Also: you can add def zero_args_self(self): ... and call it like: A.zero_args_self() and check that error message is also correct.

pass

@cpython_only
class TestErrorMessagesUseQualifiedName(unittest.TestCase):

@contextlib.contextmanager
def check_raises_type_error(self, message):
with self.assertRaises(TypeError) as cm:
yield
self.assertEqual(str(cm.exception), message)

def test_happy_path(self):
self.assertIs(None, A().method_with_self(1, kwarg=2))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no real need for this test. We test method calling in many other test cases in CPython. It does not add any real value.


def test_too_many_positional_but_missing_self(self):
msg = "A.missing_self() takes 1 positional argument but 2 were given. Did you forget the 'self' parameter in the function definition?"
with self.check_raises_type_error(msg):
A().missing_self("another_arg")

def test_too_many_positional_but_missing_self_no_args(self):
msg = "A.missing_self_no_args() takes 0 positional arguments but 1 was given. Did you forget the 'self' parameter in the function definition?"
with self.check_raises_type_error(msg):
A().missing_self_no_args()

def test_missing_arguments(self):
msg = "A.method_two_args() missing 1 required positional argument: 'y'"
with self.check_raises_type_error(msg):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
If number of arguments differs by one for bound methods and the number of
positional arg differs, we we add an additional hint in the TypeError: "Did
you forget the 'self' parameter in the function definition?"
34 changes: 29 additions & 5 deletions Python/ceval.c
Original file line number Diff line number Diff line change
Expand Up @@ -1574,12 +1574,13 @@ missing_arguments(PyThreadState *tstate, PyCodeObject *co,
static void
too_many_positional(PyThreadState *tstate, PyCodeObject *co,
Py_ssize_t given, PyObject *defaults,
_PyStackRef *localsplus, PyObject *qualname)
_PyStackRef *localsplus, PyObject *qualname,
int should_suggest_missing_self)
{
int plural;
Py_ssize_t kwonly_given = 0;
Py_ssize_t i;
PyObject *sig, *kwonly_sig;
PyObject *sig, *kwonly_sig, *self_hint = Py_GetConstant(Py_CONSTANT_EMPTY_STR);
Py_ssize_t co_argcount = co->co_argcount;

assert((co->co_flags & CO_VARARGS) == 0);
Expand Down Expand Up @@ -1617,18 +1618,40 @@ too_many_positional(PyThreadState *tstate, PyCodeObject *co,
kwonly_sig = Py_GetConstant(Py_CONSTANT_EMPTY_STR);
assert(kwonly_sig != NULL);
}
if (should_suggest_missing_self) {
self_hint = PyUnicode_FromString(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your self_hint is non NULL (before assignment). While empty strings are immortal, you still need to decref it if this ever changes. For that use Py_SETREF.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel super strongly about this, but I disagree with refcounting immortal objects. I've expressed before that too many users rely on certain things being immortal (such as empty strings), so changing that would be very difficult anyway. We might as well enjoy the performance boost rather than trying to maintain forward compatibility with a seemingly impossible case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with not changing this path though I would prefer to have it initialized to NULL instead in this case and before constructing the message we would put it to something. But I won't be strongly opposed to keep things as is.

". Did you forget the 'self' parameter in the function definition?");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this suggestion be also there if we are using classmethods? Or what about methodws on metaclasses? they can have cls as the first parameter name.

if (self_hint == NULL) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If PyUnicode_FromString fails, the original TypeError is never raised and the user gets no error message. Better to skip the hint and still emit the normal _PyErr_Format.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should actually propagate the exception in this case, and not suggest anything at all. This is already the case when creating kwonly_sig

self_hint = Py_GetConstant(Py_CONSTANT_EMPTY_STR);
}
}
_PyErr_Format(tstate, PyExc_TypeError,
"%U() takes %U positional argument%s but %zd%U %s given",
"%U() takes %U positional argument%s but %zd%U %s given%U",
qualname,
sig,
plural ? "s" : "",
given,
kwonly_sig,
given == 1 && !kwonly_given ? "was" : "were");
given == 1 && !kwonly_given ? "was" : "were",
self_hint
);
Py_DECREF(self_hint);
Py_DECREF(sig);
Py_DECREF(kwonly_sig);
}

static int
suggest_missing_self(PyFunctionObject *func, PyCodeObject *co, _PyStackRef const *args, Py_ssize_t argcount)
{
if ((co->co_argcount + 1) != argcount || argcount == 0) {
return 0;
}
PyObject *first_argument = PyStackRef_AsPyObjectBorrow(args[0]);
PyTypeObject *self_cls = Py_TYPE(first_argument);
PyFunctionObject *possibly_current_function = (PyFunctionObject *) _PyType_Lookup(self_cls, co->co_name);
return possibly_current_function == func;
}

static int
positional_only_passed_as_keyword(PyThreadState *tstate, PyCodeObject *co,
Py_ssize_t kwcount, PyObject* kwnames,
Expand Down Expand Up @@ -1721,6 +1744,7 @@ initialize_locals(PyThreadState *tstate, PyFunctionObject *func,

/* Copy all positional arguments into local variables */
Py_ssize_t j, n;
int missing_self_hint = suggest_missing_self(func, co, args, argcount);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The heuristic of matching tp_name against the class segment in qualname is solid. A negative test for a @classmethod missing cls would be useful, it probably shouldnt trigger the hint, and that would document the expected behavior.

if (argcount > co->co_argcount) {
n = co->co_argcount;
}
Expand Down Expand Up @@ -1864,7 +1888,7 @@ initialize_locals(PyThreadState *tstate, PyFunctionObject *func,
/* Check the number of positional arguments */
if ((argcount > co->co_argcount) && !(co->co_flags & CO_VARARGS)) {
too_many_positional(tstate, co, argcount, func->func_defaults, localsplus,
func->func_qualname);
func->func_qualname, missing_self_hint);
goto fail_post_args;
}

Expand Down
Loading