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
4 changes: 4 additions & 0 deletions Doc/whatsnew/3.16.rst
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ New features
Other language changes
======================

* :exc:`TypeError` messages for some instance methods called with too many
positional arguments now suggest checking whether the method definition is
missing the ``self`` parameter. (Contributed by Suren Nihalani in
:gh:`152315`.)


New modules
Expand Down
148 changes: 133 additions & 15 deletions Lib/test/test_call.py
Original file line number Diff line number Diff line change
Expand Up @@ -898,51 +898,169 @@ def f(self, *args, **kwargs):
self.assertEqual(args_captured, [("foo",)])
self.assertEqual(kwargs_captured, [{"baz": "bar"}])

class A:
def method_two_args(self, x, y):
class Dog:
def bark(self, volume, pitch):
pass

@staticmethod
def static_no_args():
def wag_tail_no_args():
pass

@staticmethod
def positional_only(arg, /):
def wag_tail(times):
pass

@staticmethod
def fetch(toy, /):
pass

def bark_with_self(self, sound):
pass

def bark_missing_self(sound):
pass

def wag_missing_self():
pass

def bark_with_self_arg(sound, self):
pass

def sit(self):
pass

@classmethod
def register_breed(dog_type):
pass

class AnimalMeta(type):
def register_pack(dog_type):
pass

def track_pack(cls, scent):
pass

@classmethod
def classify_pack(dog_type):
pass

@staticmethod
def tag_pack(label):
pass

class Poodle(metaclass=AnimalMeta):
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_too_many_positional_with_self_does_not_suggest_missing_self(self):
"""A regular method with self should keep the normal too-many-args error."""
msg = "Dog.bark_with_self() takes 2 positional arguments but 3 were given"
with self.check_raises_type_error(msg):
Dog().bark_with_self("woof", "loud")

def test_too_many_positional_but_missing_self(self):
"""A bound instance method missing self should get the targeted hint."""
msg = "Dog.bark_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):
Dog().bark_missing_self("woof")

def test_too_many_positional_but_missing_self_no_args(self):
"""A zero-argument method called through an instance should get the hint."""
msg = "Dog.wag_missing_self() 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):
Dog().wag_missing_self()

def test_missing_arguments(self):
msg = "A.method_two_args() missing 1 required positional argument: 'y'"
msg = "Dog.bark() missing 1 required positional argument: 'pitch'"
with self.check_raises_type_error(msg):
A().method_two_args("x")
Dog().bark("quiet")

def test_too_many_positional(self):
msg = "A.static_no_args() takes 0 positional arguments but 1 was given"
msg = "Dog.wag_tail_no_args() takes 0 positional arguments but 1 was given"
with self.check_raises_type_error(msg):
A.static_no_args("oops it's an arg")
Dog.wag_tail_no_args("oops it's an arg")

def test_positional_only_passed_as_keyword(self):
msg = "A.positional_only() got some positional-only arguments passed as keyword arguments: 'arg'"
msg = "Dog.fetch() got some positional-only arguments passed as keyword arguments: 'toy'"
with self.check_raises_type_error(msg):
A.positional_only(arg="x")
Dog.fetch(toy="ball")

def test_unexpected_keyword(self):
msg = "A.method_two_args() got an unexpected keyword argument 'bad'"
msg = "Dog.bark() got an unexpected keyword argument 'bad'"
with self.check_raises_type_error(msg):
A().method_two_args(bad="x")
Dog().bark(bad="x")

def test_multiple_values(self):
msg = "A.method_two_args() got multiple values for argument 'x'"
msg = "Dog.bark() got multiple values for argument 'volume'"
with self.check_raises_type_error(msg):
Dog().bark("quiet", "low", volume="oops")

def test_self_in_wrong_position_keeps_missing_argument_error(self):
"""A parameter named self in the wrong position is a missing-arg error."""
msg = "Dog.bark_with_self_arg() missing 1 required positional argument: 'self'"
with self.check_raises_type_error(msg):
Dog().bark_with_self_arg()

def test_unbound_method_with_self_keeps_missing_argument_error(self):
"""Calling an unbound method without self should keep the missing-arg error."""
msg = "Dog.sit() missing 1 required positional argument: 'self'"
with self.check_raises_type_error(msg):
Dog.sit()

def test_classmethod_missing_cls_does_not_suggest_missing_self(self):
"""A classmethod missing cls conceptually should not suggest self."""
msg = "Dog.register_breed() takes 1 positional argument but 2 were given"
with self.check_raises_type_error(msg):
Dog.register_breed("poodle")

def test_classmethod_missing_cls_via_instance_does_not_suggest_missing_self(self):
"""A classmethod called through an instance should not suggest self."""
msg = "Dog.register_breed() takes 1 positional argument but 2 were given"
with self.check_raises_type_error(msg):
Dog().register_breed("poodle")

def test_staticmethod_too_many_args_does_not_suggest_missing_self(self):
"""A staticmethod with too many arguments should not suggest self."""
msg = "Dog.wag_tail() takes 1 positional argument but 2 were given"
with self.check_raises_type_error(msg):
Dog.wag_tail(1, 2)

def test_staticmethod_too_many_args_via_instance_does_not_suggest_missing_self(self):
"""A staticmethod called through an instance should not suggest self."""
msg = "Dog.wag_tail() takes 1 positional argument but 2 were given"
with self.check_raises_type_error(msg):
Dog().wag_tail(1, 2)

def test_metaclass_missing_receiver_does_not_suggest_missing_self(self):
"""A metaclass receiver error should not suggest an instance self."""
msg = "AnimalMeta.register_pack() takes 1 positional argument but 2 were given"
with self.check_raises_type_error(msg):
Poodle.register_pack("standard")

def test_metaclass_method_too_many_args_does_not_suggest_missing_self(self):
"""A metaclass method with too many arguments should not suggest self."""
msg = "AnimalMeta.track_pack() takes 2 positional arguments but 3 were given"
with self.check_raises_type_error(msg):
Poodle.track_pack("trail", "river")

def test_metaclass_classmethod_does_not_suggest_missing_self(self):
"""A classmethod on a metaclass should not suggest instance self."""
msg = "AnimalMeta.classify_pack() takes 1 positional argument but 2 were given"
with self.check_raises_type_error(msg):
Poodle.classify_pack("standard")

def test_metaclass_staticmethod_does_not_suggest_missing_self(self):
"""A staticmethod on a metaclass should not suggest instance self."""
msg = "AnimalMeta.tag_pack() takes 1 positional argument but 2 were given"
with self.check_raises_type_error(msg):
A().method_two_args("x", "y", x="oops")
Poodle.tag_pack("show", "working")

@cpython_only
class TestErrorMessagesSuggestions(unittest.TestCase):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
:exc:`TypeError` messages for some instance methods called with too many
positional arguments now suggest checking whether the method definition is
missing the ``self`` parameter.
50 changes: 46 additions & 4 deletions Python/ceval.c
Original file line number Diff line number Diff line change
Expand Up @@ -1604,12 +1604,14 @@ 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;
const char *self_hint = "";
Py_ssize_t co_argcount = co->co_argcount;

assert((co->co_flags & CO_VARARGS) == 0);
Expand Down Expand Up @@ -1647,18 +1649,57 @@ 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 = ". Did you forget the 'self' parameter "
"in the function definition?";
}
_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%s",
qualname,
sig,
plural ? "s" : "",
given,
kwonly_sig,
given == 1 && !kwonly_given ? "was" : "were");
given == 1 && !kwonly_given ? "was" : "were",
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)
{
/* Missing self shows up as exactly one extra positional argument. */
if ((co->co_argcount + 1) != argcount || argcount == 0) {
return 0;
}

PyObject *first_argument = PyStackRef_AsPyObjectBorrow(args[0]);
if (first_argument == NULL || PyType_Check(first_argument)) {
// When first arg is NULL, it's not really about self
// If its a type object, then its a classmethod.
return 0;
}

if (co->co_argcount > 0) {
// don't confuse the user when they've already declared a common convention of cls/self
PyObject *first_parameter_name = PyTuple_GET_ITEM(co->co_localsplusnames, 0);
/* If the receiver parameter is already declared, another hint would be misleading. */
if (PyUnicode_CompareWithASCIIString(first_parameter_name, "self") == 0 ||
PyUnicode_CompareWithASCIIString(first_parameter_name, "cls") == 0)
{
return 0;
}
}
// If the current function matches on the type, its likely worth adding the hint
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 @@ -1751,6 +1792,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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've switched it to checking function name matching the self dictionary. Please take a fresh look at the PR and lmk if it suffices.

if (argcount > co->co_argcount) {
n = co->co_argcount;
}
Expand Down Expand Up @@ -1894,7 +1936,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