classification
Title: Improve the error message logic for object_new & object_init
Type: enhancement Stage: needs patch
Components: Interpreter Core Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: ncoghlan, serhiy.storchaka
Priority: normal Keywords: easy (C), patch

Created on 2017-09-18 09:44 by ncoghlan, last changed 2017-09-20 06:30 by ncoghlan.

Pull Requests
URL Status Linked Edit
PR 3650 merged serhiy.storchaka, 2017-09-19 07:34
Messages (12)
msg302439 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-09-18 09:44
As described in https://blog.lerner.co.il/favorite-terrible-python-error-message/, object_new and object_init currently have "object" hardcoded in the error messages they raise for excess parameters:


>>> class C: pass
... 
>>> C(10)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: object() takes no parameters
>>> c = C()
>>> c.__init__(10)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: object.__init__() takes no parameters

This hardcoding makes sense for the case where that particular method has been overridden, and the interpreter is reporting an error in the subclass's call up to the base class, rather than in the call to create an instance of the subclass:

>>> class D:
...     def __init__(self, *args):
...         return super().__init__(*args)
... 
>>> D(10)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in __init__
TypeError: object.__init__() takes no parameters


However, it's misleading in the case where object_new is reporting an error because it knows object_init hasn't been overridden (or vice-versa), and hence won't correctly accept any additional arguments: in those cases, it would be far more useful to report "type->tp_name" in the error message, rather than hardcoding "object".

If we split the error message logic that way, then the first two examples above would become:

>>> class C: pass
... 
>>> C(10)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: C() takes no parameters
>>> c = C()
>>> c.__init__(10)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: C.__init__() takes no parameters

while the subclassing cases would be left unchanged.
msg302445 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-18 11:51
Not sure this is easy issue. It requires taking to account many different cases and analyzing many arguments checking code scattered around many files.
msg302446 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-09-18 12:19
Fortunately, the logic is already well encapsulated: there's a "if (excess_args && (case A || case B)) {... report error ...}" check at the start of each of object_new and object_init, where "case A" = "the other function in the object_new/object_init pair has *not* been overriden" and "case B" is "this function *has* been overridden".

That means the only change needed is to include the type name in an updated error message in case A, while retaining the current error messages for case B.
msg302448 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-18 12:31
It is not so easy to make an error message conforming with error messages for similar types. This may require changing error messages in other code.

First, "takes no arguments" instead of "takes no parameters".

For normal __new__ and __init__ you never got "takes no arguments". They  take at least one argument -- a class or an instance.

>>> tuple.__new__(tuple, 1, 2, 3, 4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: tuple expected at most 1 arguments, got 4
>>> list.__init__([], 1, 2, 3, 4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: list expected at most 1 arguments, got 4
>>> class C:
...     def __new__(cls): return object.__new__(cls)
...     def __init__(self): pass
... 
>>> C.__new__(C, 1, 2, 3, 4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __new__() takes 1 positional argument but 5 were given
>>> C.__init__(C(), 1, 2, 3, 4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() takes 1 positional argument but 5 were given
msg302453 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-09-18 13:46
For this issue, I'm not proposing to make any change other than to solve the specific problem reported in the blog post: when the method itself isn't overridden, then the error message should report the name of the most derived class, not "object", to help users more readily find the likely source of their problem (a missing "__init__" method definition).

Making these custom errors consistent with Python 3's otherwise improved argument unpacking errors would be a separate issue (and I agree *that* change wouldn't qualify as being easy).
msg302462 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-18 15:18
What do you expect for:

class C: pass

object.__new__(C, 1)
C.__new__(C, 1)
msg302503 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-09-19 05:54
Those would both report "C() takes no parameters" without further enhancements (which would be out of scope for this issue).

The proposed improvement here isn't "Let's make the error message exactly correct in all cases" (that's probably impossible, since we've lost relevant information by the time the argument processing happens).

Instead, it's "let's make the error message more helpful in the most common case for beginners, and let the folks performing the more advanced operation of calling __new__ directly do the translation if they need to"
msg302586 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-09-20 03:44
New changeset a6c0c0695614177c8b6e1840465375eefcfee586 by Nick Coghlan (Serhiy Storchaka) in branch 'master':
bpo-31506: Improve the error message logic for object.__new__ and object.__init__. (GH-3650)
https://github.com/python/cpython/commit/a6c0c0695614177c8b6e1840465375eefcfee586
msg302587 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-09-20 04:05
Reopening, as I was a little hasty with the merge button: the merged PR *also* changed the `__init__` error message to drop the method name, but I don't think that's what we want.

I'm also wondering if we should change the up-call case to *always* report the method name.

That is, we'd implement the following revised behaviour:

    # Without any method overrides
    class C:
        pass

    C(42) -> "TypeError: C() takes no arguments"
    C.__new__(42) -> "TypeError: C() takes no arguments"
    C().__init__(42) -> "TypeError: C.__init__() takes no arguments"
    # These next two quirks are the price we pay for the nicer errors above
    object.__new__(C, 42) -> "TypeError: C() takes no arguments"
    object.__init__(C(), 42) -> "TypeError: C.__init__() takes no arguments"

    # With method overrides
    class D:
        def __new__(cls, *args, **kwds):
            super().__new__(cls, *args, **kwds)
        def __init__(self, *args, **kwds):
            super().__init__(*args, **kwds)

    D(42) -> "TypeError: object.__new__() takes no arguments"
    D.__new__(42) -> "TypeError: object.__new__() takes no arguments"
    D().__init__(42) -> "TypeError: object.__init__() takes no arguments"
    object.__new__(C, 42) -> "TypeError: object.__new__() takes no arguments"
    object.__init__(C(), 42) -> "TypeError: object.__init__() takes no arguments"
msg302593 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-09-20 05:20
I filed issue 31527 as a follow-up issue to see whether or not it might be possible to amend the way these custom errors are generated to benefit from the work that has gone into improving the error responses from PyArg_ParseTupleAndKeywords.
msg302596 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-20 06:07
C.__new__(42) emits different error, "TypeError: object.__new__(X): X is not a type object (int)". Perhaps you meant C.__new__(C, 42) which now emits "TypeError: C() takes no arguments".

Messages "object.__new__() takes no arguments" and "object.__init__() takes no arguments" are not correct since both object.__new__() and object.__init__() take one argument -- a class and an instance correspondingly.
msg302599 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-09-20 06:30
Aye, the "C.__new__" example omitting the first arg was just an error in that example.

And that's a good point about the current "object.__init__()" error message actually being incorrect, since the *methods* each take exactly one argument - it's only the "object(*args, **kwds)" form that genuinely expects zero arguments.

If we were to correct that error as well, we'd end up with the following:

    # Without any method overrides
    class C:
        pass

    C(42) -> "TypeError: C() takes no arguments"
    C.__new__(C, 42) -> "TypeError: C() takes no arguments"
    C().__init__(42) -> "TypeError: C.__init__() takes exactly one argument"
    # These next two quirks are the price we pay for the nicer errors above
    object.__new__(C, 42) -> "TypeError: C() takes no arguments"
    object.__init__(C(), 42) -> "TypeError: C.__init__() takes exactly one argument"

    # With method overrides
    class D:
        def __new__(cls, *args, **kwds):
            super().__new__(cls, *args, **kwds)
        def __init__(self, *args, **kwds):
            super().__init__(*args, **kwds)

    D(42) -> "TypeError: object.__new__() takes exactly one argument"
    D.__new__(D, 42) -> "TypeError: object.__new__() takes exactly one argument"
    D().__init__(42) -> "TypeError: object.__init__() takes exactly one argument"
    object.__new__(C, 42) -> "TypeError: object.__new__() takes exactly one argument"
    object.__init__(C(), 42) -> "TypeError: object.__init__() takes exactly one argument"
History
Date User Action Args
2017-09-20 06:30:01ncoghlansetmessages: + msg302599
2017-09-20 06:07:48serhiy.storchakasetmessages: + msg302596
2017-09-20 05:20:54ncoghlansetmessages: + msg302593
2017-09-20 05:19:45ncoghlanlinkissue31527 dependencies
2017-09-20 04:05:39ncoghlansetstatus: closed -> open
resolution: fixed ->
messages: + msg302587

stage: resolved -> needs patch
2017-09-20 03:47:11ncoghlansetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-09-20 03:44:35ncoghlansetmessages: + msg302586
2017-09-19 07:34:20serhiy.storchakasetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request3643
2017-09-19 05:54:53ncoghlansetmessages: + msg302503
2017-09-18 15:18:13serhiy.storchakasetmessages: + msg302462
2017-09-18 13:46:22ncoghlansetmessages: + msg302453
2017-09-18 12:31:16serhiy.storchakasetmessages: + msg302448
2017-09-18 12:19:40ncoghlansetmessages: + msg302446
2017-09-18 11:51:55serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg302445
2017-09-18 09:47:51ncoghlansetkeywords: + easy (C)
2017-09-18 09:44:34ncoghlansetstage: needs patch
type: enhancement
components: + Interpreter Core
versions: + Python 3.7
2017-09-18 09:44:09ncoghlancreate