This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Improve error message when "self" is missing from the method definition
Type: Stage: resolved
Components: Versions:
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: gvanrossum, pablogsal, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2021-01-20 15:40 by pablogsal, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 24272 closed pablogsal, 2021-01-20 15:41
Messages (12)
msg385349 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-01-20 15:40
We currently say:

Traceback (most recent call last):
  File "/Users/pgalindo3/github/python/master/lel.py", line 5, in <module>
    A().foo(1,2)
TypeError: foo() takes 2 positional arguments but 3 were given

but we should say:
Traceback (most recent call last):
  File "/Users/pgalindo3/github/python/master/lel.py", line 5, in <module>
    A().foo(1,2)
TypeError: A.foo() takes 2 positional arguments but 3 were given. Did you forget 'self' in the method definition?
msg385354 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-01-20 17:13
I don't think this makes sense. How do you know there's a missing 'self'? Surely it's just as likely that there is indeed an extra argument. Especially if A.foo is defined by a library (stdib or 3rd party).
msg385358 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-01-20 18:26
Concur with Guido. Seems that that guess will be wrong in most cases.
msg385361 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-01-20 19:11
> I don't think this makes sense. How do you know there's a missing 'self'? Surely it's just as likely that there is indeed an extra argument. Especially if A.foo is defined by a library (stdib or 3rd party).

I use the same heuristic as Pypy: the callable is a bound method, the first name in the definition is not named "self" and it was called with n+1 positional arguments
msg385363 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-01-20 19:12
> Seems that that guess will be wrong in most cases.

Maybe I am missing something. Could you list some cases where this will give the wrong advice?
msg385371 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-01-20 20:55
> I use the same heuristic as Pypy: the callable is a bound method, the first name in the definition is not named "self" and it was called with n+1 positional arguments.

Okay, that makes sense, you could have said so in the bug or PR description. :-)

Do you also exclude class and static methods?

I still worry that this might confuse users when a library for some reason uses a different name than 'self'.
msg385372 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-01-20 21:00
> Do you also exclude class and static methods?

Yup, there is a test in the PR for that IIRC

>I still worry that this might confuse users when a library for some reason uses a different name than 'self'.

That's a good point but I think it should be fine because the old error is still there and the new part is formulated as a question (did you forgot ...?). We have other errors where we are not completely sure and we formulate the suggestion as a question. Some examples are the new error for incomplete imports and some missing coma errors.
msg385375 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-01-20 22:34
> Could you list some cases where this will give the wrong advice?

Guido already listed them. Library functions, static and class methods, and names like slf and _self (used either in local classes and wrapper functions to avoid conflict with the self parameter of the outer method or as a poor positional-only parameter).

> We have other errors where we are not completely sure and we formulate the suggestion as a question. Some examples are the new error for incomplete imports and some missing coma errors.

There were reasons for adding that suggestions. An error with partial recursive import is pretty common, but the error message did not have any clue about the prevalent cause of error.

The problem with missing comma in a sequence is that since the code is always multiline, the traceback never contains enough information about the problem. And we are sure that the only case when the suggestion is wrong is when you intentionally wrote incorrect code.

The problem with missing "self" does not look so common and hard to correctly determine as the above examples. We want to keep the interpreter so simple as possible, and the proposed feature looks to me far the bar of compromise.
msg385378 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-01-20 23:21
> Guido already listed them. Library functions, static and class methods, 

PR 24272 will work on all those cases.

>  and names like slf and _self (used either in local classes and wrapper functions to avoid conflict with the self parameter of the outer method or as a poor positional-only parameter).

That's fine, the error is a suggestion (as we do in other parts of the stdlib), and the places where "self" is not used is normally rare. The old error is still there. The cases you mention are valid but I would say there are the exception and if someone is writing them they will probably still be able to make sense of the error.

> The problem with missing "self" does not look so common and hard to correctly determine as the above examples.

Is true, but I think that the cases were this will raise a false positive is very rare and make the benefits of the error message appealing (at least to me). 

>  We want to keep the interpreter so simple as possible, and the proposed feature looks to me far the bar of compromise.

This is an excellent point. THe only reason I am giving this a go is because this error message is super confusing to newcomers and educators are asking us, again and again, to include this hint as pypy is doing currently.
msg385381 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-01-21 00:09
Of course, being said all this, if the general consensus is that is not worth the trouble, I am happy to retract the PR :)
msg385384 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-01-21 00:38
-0 for this PR.  It would cause more problems than it solves, and traditionally we leave this sort of problem for linters and type checkers.  Also, this kind of error surfaces readily with even minimal testing.  So, I don't think this PR is worth it.

That said, don't stop looking for places where the parser can provide more useful error reporting.  Some parsing errors are truly inscrutable.
msg385385 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-01-21 00:40
Thanks, everyone for your valuable comments and for explaining your concerns!

I will close the PR as it seems that there is no clear consensus that will be a win after balancing the problems.
History
Date User Action Args
2022-04-11 14:59:40adminsetgithub: 87144
2021-01-21 00:45:55pablogsalsetstatus: open -> closed
resolution: rejected
stage: patch review -> resolved
2021-01-21 00:40:42pablogsalsetmessages: + msg385385
2021-01-21 00:38:48rhettingersetnosy: + rhettinger
messages: + msg385384
2021-01-21 00:09:55pablogsalsetmessages: + msg385381
2021-01-20 23:21:33pablogsalsetmessages: + msg385378
2021-01-20 22:34:51serhiy.storchakasetmessages: + msg385375
2021-01-20 21:00:35pablogsalsetmessages: + msg385372
2021-01-20 20:55:07gvanrossumsetmessages: + msg385371
2021-01-20 19:12:58pablogsalsetmessages: + msg385363
2021-01-20 19:11:52pablogsalsetmessages: + msg385361
2021-01-20 18:26:40serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg385358
2021-01-20 17:13:10gvanrossumsetnosy: + gvanrossum
messages: + msg385354
2021-01-20 15:41:11pablogsalsetkeywords: + patch
stage: patch review
pull_requests: + pull_request23095
2021-01-20 15:40:27pablogsalcreate