Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the error message logic for object_new & object_init #75687

Closed
ncoghlan opened this issue Sep 18, 2017 · 28 comments
Closed

Improve the error message logic for object_new & object_init #75687

ncoghlan opened this issue Sep 18, 2017 · 28 comments
Labels
3.7 (EOL) end of life easy interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

BPO 31506
Nosy @ncoghlan, @serhiy-storchaka, @CuriousLearner, @miss-islington, @tirkarthi, @ppt000
PRs
  • bpo-31506: Improve the error message logic for object.__new__ and object.__init__. #3650
  • bpo-31506: Improve the error message logic for object_new & object_init #4740
  • bpo-31506: Clarify error messages for object.__new__ and object.__init__ #11641
  • bpo-31506: Clarify error messages for object.__new__ and object.__init__ #11641
  • bpo-31506: Clarify error messages for object.__new__ and object.__init__ #11641
  • [3.7] bpo-31506: Clarify error messages for object.__new__ and object.__init__ (GH-11641) #11939
  • 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 2019-02-19.13:48:34.176>
    created_at = <Date 2017-09-18.09:44:09.052>
    labels = ['interpreter-core', 'easy', 'type-feature', '3.7']
    title = 'Improve the error message logic for object_new & object_init'
    updated_at = <Date 2019-02-19.13:48:34.173>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2019-02-19.13:48:34.173>
    actor = 'ncoghlan'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-02-19.13:48:34.176>
    closer = 'ncoghlan'
    components = ['Interpreter Core']
    creation = <Date 2017-09-18.09:44:09.052>
    creator = 'ncoghlan'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31506
    keywords = ['patch', 'easy (C)']
    message_count = 28.0
    messages = ['302439', '302445', '302446', '302448', '302453', '302462', '302503', '302586', '302587', '302593', '302596', '302599', '307676', '307678', '307679', '307699', '307932', '307933', '308018', '308063', '309464', '326091', '326225', '330500', '335943', '335945', '335946', '335947']
    nosy_count = 6.0
    nosy_names = ['ncoghlan', 'serhiy.storchaka', 'CuriousLearner', 'miss-islington', 'xtreak', 'ppt000']
    pr_nums = ['3650', '4740', '11641', '11641', '11641', '11939']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue31506'
    versions = ['Python 3.7']

    @ncoghlan
    Copy link
    Contributor Author

    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.

    @ncoghlan ncoghlan added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement easy labels Sep 18, 2017
    @serhiy-storchaka
    Copy link
    Member

    Not sure this is easy issue. It requires taking to account many different cases and analyzing many arguments checking code scattered around many files.

    @ncoghlan
    Copy link
    Contributor Author

    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.

    @serhiy-storchaka
    Copy link
    Member

    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

    @ncoghlan
    Copy link
    Contributor Author

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

    @serhiy-storchaka
    Copy link
    Member

    What do you expect for:

    class C: pass
    
    object.__new__(C, 1)
    C.__new__(C, 1)

    @ncoghlan
    Copy link
    Contributor Author

    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"

    @ncoghlan
    Copy link
    Contributor Author

    New changeset a6c0c06 by Nick Coghlan (Serhiy Storchaka) in branch 'master':
    bpo-31506: Improve the error message logic for object.__new__ and object.__init__. (GH-3650)
    a6c0c06

    @ncoghlan
    Copy link
    Contributor Author

    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"
    

    @ncoghlan ncoghlan reopened this Sep 20, 2017
    @ncoghlan
    Copy link
    Contributor Author

    I filed bpo-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.

    @serhiy-storchaka
    Copy link
    Member

    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.

    @ncoghlan
    Copy link
    Contributor Author

    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"
    

    @CuriousLearner
    Copy link
    Member

    I'll work on a fix for this and issue a PR.

    @serhiy-storchaka
    Copy link
    Member

    I think the main problem is not with coding, but with design. And from this point of view this may be not so easy issue. Let wait until Nick has a time to work on it.

    @CuriousLearner
    Copy link
    Member

    Nick,

    I think the error messages are incorrect. We expect error message to be takes no argument rather than takes exactly one argument. Can you please confirm that?

    I think for the class without any method overrides, the functionality should be something like this:

         >>> class C:
        ...     pass
        ...
        >>> C(42)
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        TypeError: C() takes no arguments
        >>> C.__new__(C, 42)
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        TypeError: C() takes no arguments
        >>> C().__init__(42)
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        TypeError: C().__init__() takes no arguments
        >>> object.__new__(C, 42)
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        TypeError: C() takes no arguments
        >>> object.__init__(C(), 42)
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        TypeError: C().__init__() takes no arguments

    Is that correct?

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Dec 6, 2017

    Aye, I think Sanyam's proposed messages look good, and the "C().__init__() takes no arguments" wording is easier to follow than my suggested "C.__init__() takes exactly one argument" wording (as interpreting the latter currently requires noticing that it's referring to the *unbound* method taking one argument: the instance).

    @ncoghlan
    Copy link
    Contributor Author

    New changeset 780acc8 by Nick Coghlan (Sanyam Khurana) in branch 'master':
    bpo-31506: Improve the error message logic for class instantiation (GH-4740)
    780acc8

    @ncoghlan
    Copy link
    Contributor Author

    Thanks for the feedback and updates folks! If we decide to make any further changes, I think they will be best handled as a new issue :)

    @serhiy-storchaka
    Copy link
    Member

    780acc8 reintroduced the part of the original bug fixed in a6c0c06. object.__new__() and object.__init__() require an argument (cls and self correspondingly).

    @CuriousLearner
    Copy link
    Member

    Serhiy, can you please elaborate on that a bit? I'll try to fix this.

    @serhiy-storchaka
    Copy link
    Member

    Error messages "object.__init__() takes no arguments" and "object.__new__() takes no arguments" are wrong. They contradicts the following error messages:

    >>> object.__init__()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: descriptor '__init__' of 'object' object needs an argument
    >>> object.__new__()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: object.__new__(): not enough arguments

    @serhiy-storchaka
    Copy link
    Member

    I think this change should be reverted.

    @ncoghlan
    Copy link
    Contributor Author

    We added the method names to help provide a nudge that the issue is likely to be a missing method implementation in the subclassing case, so I'd like to keep them if we can find a way to make the messages accurate again.

    What if we updated the offending format strings in typeobject.c to state the exact nature of the expected argument that is missing?

        PyErr_SetString(PyExc_TypeError, "object.__init__() takes exactly one argument (the instance to initialize)");
    
        PyErr_Format(PyExc_TypeError, "%.200s.__init__() takes exactly one argument (the instance to initialize)", type->tp_name);
    PyErr_SetString(PyExc_TypeError, "object.__new__() takes exactly one argument (the type to instantiate)")
    

    @ppt000
    Copy link
    Mannequin

    ppt000 mannequin commented Nov 27, 2018

    I am not sure if the following is resolved by your proposal, I post it just in case:
    The following code works:
    1. class Singleton(object):
    2. def __new__(cls, *args, **kwargs):
    3. if not hasattr(cls, 'instance'):
    4. cls.instance = super(Singleton, cls).__new__(cls)
    5. cls.instance._init_pointer = cls.instance._init_properties
    6. else:
    7. cls.instance._init_pointer = lambda *args, **kwargs: None # do nothing
    8. return cls.instance
    9. def __init__(self, *args, **kwargs):
    10. super(Singleton, self).__init__()
    11. self._init_pointer(*args, **kwargs)
    12. def _init_properties(self, tag):
    13. self.info = tag
    14. #
    15. if __name__ == '__main__':
    16. S1 = Singleton('I am S1')
    17. print('S1 info is:' + S1.info)
    18. S2 = Singleton('Am I S2?')
    19. print('S2 info is:' + S2.info)
    However if I change line 4 into this code (which works in Python 2 by the way):
    cls.instance = super(Singleton, cls).__new__(cls, *args, **kwargs)
    I get:
    TypeError: object.__new__() takes no arguments
    But if I change line 4 into this (no arguments as suggested):
    cls.instance = super(Singleton, cls).__new__()
    I get:
    TypeError: object.__new__(): not enough arguments
    Line 10 has the same issue when changed to:
    super(Singleton, self).__init__(*args, **kwargs)

    @ncoghlan
    Copy link
    Contributor Author

    Paolo: it still won't be completely clear, since there's still the subtle issue that __new__ is a static method rather than a class method, so the correct calls up to the base class are respectively:

    super(Singleton, cls).__new__(cls) # Static method, cls needs to be passed explicitly
    
    super(Singleton, self).__init__() # Bound method, self filled in automatically
    

    @ncoghlan
    Copy link
    Contributor Author

    New changeset 5105483 by Nick Coghlan (Sanyam Khurana) in branch 'master':
    bpo-31506: Clarify error messages for object.__new__ and object.__init__ (GH-11641)
    5105483

    @miss-islington
    Copy link
    Contributor

    New changeset 64ca728 by Miss Islington (bot) in branch '3.7':
    bpo-31506: Clarify error messages for object.__new__ and object.__init__ (GH-11641)
    64ca728

    @ncoghlan
    Copy link
    Contributor Author

    The revised behaviour now makes the error messages consistent with each other:

    >>> class TooManyArgs():
    ...     def __new__(cls):
    ...         super().__new__(cls, 1)
    ... 
    >>> TooManyArgs()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "<stdin>", line 3, in __new__
    TypeError: object.__new__() takes exactly one argument (the type to instantiate)
    >>> class NotEnoughArgs():
    ...     def __new__(cls):
    ...         super().__new__()
    ... 
    >>> NotEnoughArgs()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "<stdin>", line 3, in __new__
    TypeError: object.__new__(): not enough arguments
    >>> class TooManyInitArgs():
    ...     def __init__(self):
    ...         super().__init__(1, 2, 3)
    ... 
    >>> TooManyInitArgs()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "<stdin>", line 3, in __init__
    TypeError: object.__init__() takes exactly one argument (the instance to initialize)
    >>> class NotEnoughInitArgs():
    ...     def __init__(self):
    ...         object.__init__()
    ... 
    >>> NotEnoughInitArgs()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "<stdin>", line 3, in __init__
    TypeError: descriptor '__init__' of 'object' object needs an argument

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life easy interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants