gh-152315: Add a suggestion for self#152326
Conversation
9ef597f to
5c6f5c1
Compare
lul-cas
left a comment
There was a problem hiding this comment.
My two cents, if they're valid <3
| } | ||
| if (suggest_missing_self) { | ||
| self_hint = PyUnicode_FromString( | ||
| ". Did you forget to declare 'self' as the first parameter?"); |
There was a problem hiding this comment.
I agree with @edvilme: self is only a naming convention. What matters is that the first parameter receives the instance. Consider something like:
". Did you forget the instance parameter (e.g. self) in the function definition?"This avoids misleading beginners into adding a parameter named self in the wrong position.
There was a problem hiding this comment.
This avoids misleading beginners into adding a parameter named self in the wrong position.
I'm not so sure that "instance parameter" means anything more to a beginner than a "self" parameter. "Self" has the advantage of being a very, very common pattern in Python, and I think that's more easily searchable than "instance parameter". For reference, our docs use "self" hundreds of times, whereas "instance parameter" does not appear once.
I would lean more towards this:
Did you forget the 'self' parameter in the function definition?
"self parameter python" on Google will yield millions of results describing exactly what the user wants, whereas "instance parameter python" returns some unrelated results about class methods vs. instance methods and similar.
There was a problem hiding this comment.
I must agree with
'"self parameter python"' on Google will yield millions of results'.
| if (suggest_missing_self) { | ||
| self_hint = PyUnicode_FromString( | ||
| ". Did you forget to declare 'self' as the first parameter?"); | ||
| if (self_hint == NULL) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We should actually propagate the exception in this case, and not suggest anything at all. This is already the case when creating kwonly_sig
| def positional_only(arg, /): | ||
| pass | ||
|
|
||
| def missing_self(another_arg): |
There was a problem hiding this comment.
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.
|
|
||
| /* Copy all positional arguments into local variables */ | ||
| Py_ssize_t j, n; | ||
| int missing_self_hint = suggest_missing_self(func, co, args, argcount); |
There was a problem hiding this comment.
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.
| suggest_missing_self(PyFunctionObject *func, PyCodeObject *co, | ||
| _PyStackRef const *args, Py_ssize_t argcount) | ||
| { | ||
| if (co->co_argcount >= argcount) { |
There was a problem hiding this comment.
Hmm, this fires whenever argcount > co_argcount and the first argument's type matches the class in qualname. That's broader than the beginner mistake this PR targets.
Ex:
This should hint
def lol(a): ...
A().lol(1) but his should not:
def method(self, x): ...
A().method(1, 2, 3) I'd suggest combining argcount == co_argcount + 1 with a second check, like, the first declared parameter is not named "self", or verifying via _PyType_Lookup that this code object is the function bound on the instance's type.
ZeroIntensity
left a comment
There was a problem hiding this comment.
This needs a news entry and a note in "What's New in Python 3.16".
|
|
||
| PyObject *self = PyStackRef_AsPyObjectBorrow(args[0]); | ||
| if (self == NULL) { | ||
| // When first arg is NULL, its not really about self |
There was a problem hiding this comment.
| // When first arg is NULL, its not really about self | |
| // When first arg is NULL, it's not really about self |
| Py_ssize_t qualname_len; | ||
| const char *qualname = PyUnicode_AsUTF8AndSize( | ||
| func->func_qualname, &qualname_len); | ||
| if (qualname == NULL) { | ||
| PyErr_Clear(); | ||
| return 0; | ||
| } | ||
|
|
||
| const char *method_dot = strrchr(qualname, '.'); | ||
| if (method_dot == NULL) { | ||
| return 0; | ||
| } | ||
|
|
||
| const char *class_start = qualname; | ||
| for (const char *p = qualname; p < method_dot; p++) { | ||
| if (*p == '.') { | ||
| class_start = p + 1; | ||
| } | ||
| } | ||
| Py_ssize_t class_len = method_dot - class_start; | ||
| const char *type_name = Py_TYPE(self)->tp_name; | ||
|
|
||
| return (strlen(type_name) == (size_t)class_len | ||
| && strncmp(type_name, class_start, (size_t)class_len) == 0); | ||
| } |
There was a problem hiding this comment.
This feels like a nasty hack. Relying on the __qualname__ feels very icky. I need to look into this more, but it's probably possible to determine whether this is an instance method earlier in the eval loop. That would be much cleaner.
…the error message
b2cb0bd to
febd510
Compare
sobolevn
left a comment
There was a problem hiding this comment.
(not a full review)
I think the important part of this issue is to find cases where self (as a concept, not as a name) is really missing from the func definition.
So, we can clearly tell apart cases where we just forgot a parameter vs where we forgot to add self to the function definition.
| self.assertEqual(str(cm.exception), message) | ||
|
|
||
| def test_happy_path(self): | ||
| self.assertIs(None, A().method_with_self(1, kwarg=2)) |
There was a problem hiding this comment.
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 missing_self(another_arg): | ||
| pass | ||
|
|
||
| def missing_self_no_args(): |
There was a problem hiding this comment.
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.
picnixz
left a comment
There was a problem hiding this comment.
You should add tests with classmethods, staticmethods (whether on regular classes or metaclasses)
| assert(kwonly_sig != NULL); | ||
| } | ||
| if (should_suggest_missing_self) { | ||
| self_hint = PyUnicode_FromString( |
There was a problem hiding this comment.
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.
| if (suggest_missing_self) { | ||
| self_hint = PyUnicode_FromString( | ||
| ". Did you forget to declare 'self' as the first parameter?"); | ||
| if (self_hint == NULL) { |
There was a problem hiding this comment.
We should actually propagate the exception in this case, and not suggest anything at all. This is already the case when creating kwonly_sig
| } | ||
| if (should_suggest_missing_self) { | ||
| self_hint = PyUnicode_FromString( | ||
| ". Did you forget the 'self' parameter in the function definition?"); |
There was a problem hiding this comment.
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.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Uh oh!
There was an error while loading. Please reload this page.