Repository navigation
Improve error message when "self" is missing from the method definition #87144
Description
| BPO | 42978 |
|---|---|
| Nosy | @gvanrossum, @rhettinger, @serhiy-storchaka, @pablogsal |
| PRs |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2021-01-21.00:45:55.135>
created_at = <Date 2021-01-20.15:40:27.504>
labels = []
title = 'Improve error message when "self" is missing from the method definition'
updated_at = <Date 2021-01-21.00:45:55.135>
user = 'https://github.com/pablogsal'bugs.python.org fields:
activity = <Date 2021-01-21.00:45:55.135>
actor = 'pablogsal'
assignee = 'none'
closed = True
closed_date = <Date 2021-01-21.00:45:55.135>
closer = 'pablogsal'
components = []
creation = <Date 2021-01-20.15:40:27.504>
creator = 'pablogsal'
dependencies = []
files = []
hgrepos = []
issue_num = 42978
keywords = ['patch']
message_count = 12.0
messages = ['385349', '385354', '385358', '385361', '385363', '385371', '385372', '385375', '385378', '385381', '385384', '385385']
nosy_count = 4.0
nosy_names = ['gvanrossum', 'rhettinger', 'serhiy.storchaka', 'pablogsal']
pr_nums = ['24272']
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue42978'
versions = []
Activity
pablogsal commentedon Jan 20, 2021
We currently say:
gvanrossum commentedon Jan 20, 2021
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).
serhiy-storchaka commentedon Jan 20, 2021
Concur with Guido. Seems that that guess will be wrong in most cases.
pablogsal commentedon Jan 20, 2021
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
pablogsal commentedon Jan 20, 2021
Maybe I am missing something. Could you list some cases where this will give the wrong advice?
gvanrossum commentedon Jan 20, 2021
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'.
pablogsal commentedon Jan 20, 2021
Yup, there is a test in the PR for that IIRC
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.
serhiy-storchaka commentedon Jan 20, 2021
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).
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.
pablogsal commentedon Jan 20, 2021
PR 24272 will work on all those cases.
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.
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).
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.
pablogsal commentedon Jan 21, 2021
Of course, being said all this, if the general consensus is that is not worth the trouble, I am happy to retract the PR :)
rhettinger commentedon Jan 21, 2021
-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.
pablogsal commentedon Jan 21, 2021
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.