Skip to content

Pass *args and **kwargs to superclass in Generic.__new__#517

Merged
ilevkivskyi merged 2 commits into
python:masterfrom
chadrik:generic_dunder_new
Jan 27, 2018
Merged

Pass *args and **kwargs to superclass in Generic.__new__#517
ilevkivskyi merged 2 commits into
python:masterfrom
chadrik:generic_dunder_new

Conversation

@chadrik

@chadrik chadrik commented Dec 10, 2017

Copy link
Copy Markdown
Contributor

In cases with multiple inheritance, Generic's super may not be object, and so its __new__ requires the same args as __init__.

Note that there's special treatment required for the case where the next class in the mro is object, otherwise every subclass of Generic that takes an argument would need to override __new__.

In cases with multiple inheritance, Generic's super may not be object, and so its __new__ requires the same args as __init__.
@the-knights-who-say-ni

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@ilevkivskyi ilevkivskyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please sign the CLA (and be sure that tests trigger both branches in your special-casing of object, I didn't check this, you can just run coverage locally).

Comment thread src/typing.py
if cls.__origin__ is None:
return base_cls.__new__(cls)
if base_cls is object:
return base_cls.__new__(cls)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This special-casing of object worries me a bit. But anyway, this will be necessary only for the backport (with PEP 560 all these hacks will be removed). So if @gvanrossum is fine with this, then it is also OK with me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any viable alternatives. Any suggestions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other alternative is just to keep the status quo. I remember we have a similar situation with GenericMeta.__new__. You can special-case for object, but it looks like that the same can happen with any other class that doesn't expect additional arguments in its __new__.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can special-case for object, it looks like that the same can happen with any other class that doesn't expect additional arguments in its new.

The existing code is already a special-case for object: by assuming there are no args in base_cls.__new__ it's assuming base_cls is object. I'm simply adding another case that covers multiple inheritance while preserving the existing special object case.

When using multiple inheritance with Generic and __new__ users will simply have to be responsible and only pass args that are compatible with super's __new__. This is what they would do normally anyway, so it's not asking anything additional of them. Also, note that in my tests I've covered the case where no args are expected.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you don't special-case object and always pass *args, **kwds? I find it disturbing that by inheriting directly from object you end up with a class that ignores all arguments passed to it. (Though I realize that there is a problem here when you override one of __init__ and __new__ but not the other.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you don't special-case object and always pass *args, **kwds?

Any class inheriting from Generic whose __init__ takes an arg will fail.

Here's an example of a test case that fails if we pass *args, **kwds to base_cls.__new__:

T = TypeVar('T')
class C(Generic[T]):
    def __init__(self, attr):
        self.attr = attr

c = C(42)
======================================================================
ERROR: test_copy_generic_instances (test_typing.GenericTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/chad/dev/typing/src/test_typing.py", line 1113, in test_copy_generic_instances
    c = C(42)
  File "/Users/chad/dev/typing/src/typing.py", line 1223, in __new__
    return _generic_new(cls.__next_in_mro__, cls, *args, **kwds)
  File "/Users/chad/dev/typing/src/typing.py", line 1184, in _generic_new
    return base_cls.__new__(cls, *args, **kwds)
TypeError: object() takes no parameters

@chadrik chadrik Dec 15, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I realize that there is a problem here when you override one of __init__ and __new__ but not the other.

I think this is exactly the point. typing.Generic has created this situation for all subclasses, so to avoid inconveniencing users by forcing subclasses with args to implement __new__ and pass the correct args (almost always none), it deals with the problem transparently, by special-casing object. And I'm speaking now about the status-quo, prior to this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I am beginning to understand the situation. It is indeed a problem (we don't want to require that both __init__ and __new__ process all arguments). But I can defeat your hack by having C subclass a non-generic class D that also override __init__ -- then the __init__ call works fine, but the __new__ call still fails because the superclass isn't object (but still doesn't implement __new__). So I think we should really check whether base_cls.__new__ is object.__new__.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think we should really check whether base_cls.new is object.new.

Deal!

@chadrik

chadrik commented Dec 11, 2017

Copy link
Copy Markdown
Contributor Author

Could you please sign the CLA

I've signed the CLA and am waiting for the Human on the other end (sorry, Human, I did this twice)

(and be sure that tests trigger both branches in your special-casing of object, I didn't check this, you can just run coverage locally).

All four cases are hit by the tests (with and without __origin__ * with and without object).

@ilevkivskyi

Copy link
Copy Markdown
Member

All four cases are hit by the tests (with and without __origin__ * with and without object).

OK, thanks!

@chadrik

chadrik commented Dec 15, 2017

Copy link
Copy Markdown
Contributor Author

Fix pushed.

@chadrik

chadrik commented Dec 15, 2017

Copy link
Copy Markdown
Contributor Author

And test added.

@ilevkivskyi ilevkivskyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I will merge this soon unless Guido wants to wait until after Python 3.6.4 final.

@gvanrossum

gvanrossum commented Dec 15, 2017 via email

Copy link
Copy Markdown
Member

@ilevkivskyi

Copy link
Copy Markdown
Member

It looks like master coincides with CPython master now, so I will merge this after typing 3.6.4 is published on PyPI.

@ilevkivskyi ilevkivskyi self-assigned this Dec 16, 2017
@ilevkivskyi

Copy link
Copy Markdown
Member

New typing was just released, so that we can merge this now.

@ilevkivskyi
ilevkivskyi merged commit 7d7ffcd into python:master Jan 27, 2018
@chadrik

chadrik commented Jul 23, 2018

Copy link
Copy Markdown
Contributor Author

Hi, when will this change make it onto PyPI? I was under the impression that typing was in sync with Python releases, so I was expecting to see this change in a typing 3.6.5 awhile back.

@gvanrossum

Copy link
Copy Markdown
Member

Hm, we should do a release. By now it should probably be 3.6.6. But I'm not sure when I'll have time, and Ivan's on holiday until the end of July (good for him!).

@ilevkivskyi

Copy link
Copy Markdown
Member

Hi, when will this change make it onto PyPI?

I will try to make a release this week (if no other urgent things will appear).

@ilevkivskyi

Copy link
Copy Markdown
Member

If there are no objections I will release typing-3.6.6 in few hours.

@ilevkivskyi

Copy link
Copy Markdown
Member

Hm, I prepared the release but it turns out I don't have rights to upload typing. I just checked on PyPI and indeed I am not in the maintainers list. @gvanrossum @ambv @JukkaL @vlasovskikh could any of you please add me as a maintainer?

@gvanrossum

gvanrossum commented Aug 26, 2018 via email

Copy link
Copy Markdown
Member

@ilevkivskyi

Copy link
Copy Markdown
Member

Great, thanks! I just uploaded typing wheels and source to PyPI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants