Skip to content

bpo-31506: Improve the error message logic for object_new & object_init#4740

Merged
ncoghlan merged 3 commits into
python:masterfrom
CuriousLearner:fix-issue31506
Dec 10, 2017
Merged

bpo-31506: Improve the error message logic for object_new & object_init#4740
ncoghlan merged 3 commits into
python:masterfrom
CuriousLearner:fix-issue31506

Conversation

@CuriousLearner

@CuriousLearner CuriousLearner commented Dec 6, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

With this patch applied, the classes without any method overrides behaves 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

And for classes having method overrides, it behaves like this:

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

cc @ncoghlan . I'm still trying to figure out the changes to be done for cases of object.__init__(D(), 42) and D().__init__(42) because they don't report error right now.

https://bugs.python.org/issue31506

@ncoghlan

ncoghlan commented Dec 7, 2017

Copy link
Copy Markdown
Contributor

@CuriousLearner If I recall correctly, object.__init__() has a special case to make it permissive whenever __new__ is overridden. Rather than fiddling with that logic in this patch (since doing so would risk breaking backwards compatibility), I'd suggest defining a 3rd test class that only overrides __init__:

class E:
    def __init__(self, *args, **kwds):
        super().__init__(*args, **kwds)

Then E().__init__(42) and object.__init__(E(), 42) should both fail.

@CuriousLearner

CuriousLearner commented Dec 7, 2017

Copy link
Copy Markdown
Member Author

@ncoghlan Seems like you're right. That was a special case. I wanted to know what would be the best file to add these test cases in?

@ncoghlan

ncoghlan commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

@CuriousLearner I'd suggest adding a new def testConstructorErrorMessages(self): test case to https://github.com/python/cpython/blob/master/Lib/test/test_class.py, and then use self.assertRaisesRegex() to check for the desired error messages.

@CuriousLearner

CuriousLearner commented Dec 9, 2017

Copy link
Copy Markdown
Member Author

@ncoghlan I've updated the patch and added test cases. Can you please have a look?

ncoghlan
ncoghlan approved these changes Dec 10, 2017
View reviewed changes

@ncoghlan ncoghlan 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.

LGTM!

@ncoghlan ncoghlan merged commit 780acc8 into python:master Dec 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ncoghlan ncoghlan ncoghlan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@CuriousLearner @ncoghlan @the-knights-who-say-ni @bedevere-bot