-
-
Notifications
You must be signed in to change notification settings - Fork 35.2k
gh-152315: Add a suggestion for self #152326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
87ffa90
b73f5cd
dfa5f6f
02dfa50
6165cd1
118a24c
0a52b0b
aaf51cf
887456a
b9c2e70
f61cf66
a67750f
db5f2a4
6721c67
c246bad
5cd94fe
5519866
a8bf860
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1605,12 +1605,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); | ||
|
|
@@ -1648,18 +1650,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, | ||
|
|
@@ -1752,6 +1793,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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
| } | ||
|
|
@@ -1895,7 +1937,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; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argcount == 0 condition is redundant, right?