Skip to content

gh-152315: Add a suggestion for self#152326

Open
SurenNihalani wants to merge 18 commits into
python:mainfrom
SurenNihalani:missing_self
Open

gh-152315: Add a suggestion for self#152326
SurenNihalani wants to merge 18 commits into
python:mainfrom
SurenNihalani:missing_self

Conversation

@SurenNihalani

@SurenNihalani SurenNihalani commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

@python-cla-bot

python-cla-bot Bot commented Jun 26, 2026

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@lul-cas lul-cas left a comment

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.

My two cents, if they're valid <3

Comment thread Python/ceval.c Outdated
}
if (suggest_missing_self) {
self_hint = PyUnicode_FromString(
". Did you forget to declare 'self' as the first parameter?");

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.

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.

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.

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.

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.

I must agree with
'"self parameter python"' on Google will yield millions of results'.

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 updated the error message to include the self parameter:

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

Comment thread Python/ceval.c Outdated
if (suggest_missing_self) {
self_hint = PyUnicode_FromString(
". Did you forget to declare 'self' as the first parameter?");
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

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 a char*. Does that suffice?

Comment thread Lib/test/test_call.py Outdated
def positional_only(arg, /):
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.

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.

Comment thread Python/ceval.c

/* 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.

Comment thread Python/ceval.c Outdated
suggest_missing_self(PyFunctionObject *func, PyCodeObject *co,
_PyStackRef const *args, Py_ssize_t argcount)
{
if (co->co_argcount >= 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.

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.

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.

ok, I've included your runtime arg count == defined arg count + 1 check. It also does both of your suggested checks on the first parameter. Please review it again

@ZeroIntensity ZeroIntensity left a comment

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.

This needs a news entry and a note in "What's New in Python 3.16".

Comment thread Python/ceval.c Outdated

PyObject *self = PyStackRef_AsPyObjectBorrow(args[0]);
if (self == NULL) {
// When first arg is NULL, its not really about self

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.

Suggested change
// When first arg is NULL, its not really about self
// When first arg is NULL, it's not really about self

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.

Added it in the HEAD

Comment thread Python/ceval.c Outdated
Comment on lines +1660 to +1684
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);
}

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.

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.

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 asked AI on other options. Please review this approach

@sobolevn sobolevn left a comment

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.

(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.

Comment thread Lib/test/test_call.py Outdated
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.

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.

This was my response to #152326 (comment) . I think I interpreted the happy path incorrectly

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.

Removed it.

Comment thread Lib/test/test_call.py Outdated
def missing_self(another_arg):
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.

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.

added bark_with_self_arg and test_unbound_method_with_self_keeps_missing_argument_error for this.

@picnixz picnixz left a comment

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.

You should add tests with classmethods, staticmethods (whether on regular classes or metaclasses)

Comment thread Python/ceval.c Outdated
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.

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 char*. Does that suffice?

Comment thread Python/ceval.c Outdated
if (suggest_missing_self) {
self_hint = PyUnicode_FromString(
". Did you forget to declare 'self' as the first parameter?");
if (self_hint == NULL) {

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

Comment thread Python/ceval.c Outdated
}
if (should_suggest_missing_self) {
self_hint = PyUnicode_FromString(
". 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.

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 added exemption for those cases.

@bedevere-app

bedevere-app Bot commented Jun 27, 2026

Copy link
Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@read-the-docs-community

read-the-docs-community Bot commented Jul 3, 2026

Copy link
Copy Markdown

Documentation build overview

📚 cpython-previews | 🛠️ Build #33422329 | 📁 Comparing 2b80cb2 against main (0a13efc)

  🔍 Preview build  

2 files changed
± whatsnew/3.16.html
± whatsnew/changelog.html

@SurenNihalani

Copy link
Copy Markdown
Contributor Author

@picnixz , I've added tests with classmethods, staticmethods and instancemethods with classes/metaclasses.

@sobolevn, I've added tests.

@ZeroIntensity , I've added news.

At a high level, I feel like I've taken on too much complexity in one PR. If people can chime on what feels good enough, that'd be great.

This is ready for next round of review.

@SurenNihalani

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

@bedevere-app

bedevere-app Bot commented Jul 3, 2026

Copy link
Copy Markdown

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants