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

Don't clobber user-defined __init__ methods on protocol classes, even when typing.Protocol is in the mro #247

Closed
wants to merge 2 commits into from

Conversation

AlexWaygood
Copy link
Member

Rather than trying to stop typing.Protocol.__init_subclass__ from replacing the __init__ method on a user-defined protocol, we let it go ahead with that. We just swap it back after typing.Protocol.__init_subclass__ has done so, to what the __init__ method was before.

The "fun" thing here is that we have to be able to figure out what the class's __init__ method is going to be before the class has actually been created. That requires knowing what the class's mro will be before it's been created. I used Steven D'Aprano's recipe here to accomplish this: https://code.activestate.com/recipes/577748-calculate-the-mro-of-a-class/.

This PR fixes the first problem I mentioned in #245. (I think we should still keep the docs note I added in #246, though -- the second problem I mentioned in #246 still exists, and in general it feels kind of unpredictable what kinds of issues might arise from mixing the two Protocol implementations.)

In general, while it was fun to write this PR, I'm not sure the extra complexity is actually worth it here, since this is something of an edge-case bug. (Adding this much complexity to the protocol-class creation process could definitely just lead to more bugs!)

Feedback is very much welcome. I think I definitely don't have enough tests right now.

@@ -1879,6 +1879,24 @@ class C: pass
P.__init__(c, 1)
self.assertEqual(c.x, 1)

@only_with_typing_Protocol
def test_protocol_defining_init_does_not_get_overridden_2(self):
Copy link
Member

Choose a reason for hiding this comment

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

Does this work if the positions of typing.Protocol and typing_extensions.Protocol in the MRO are reversed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! Added some tests for that in e68be1a

@AlexWaygood
Copy link
Member Author

Okay, this PR definitely shouldn't be merged as is. It would lead to a regression. Currently on main, the following snippet works fine, but breaks with this PR:

from typing_extensions import Protocol
import typing

class Foo(Protocol):
    def __init__(self):
        self.foo = 'foo'

class Bar(Protocol):
    def __init__(self):
        self.bar = 'bar'

class One(Foo, Bar, typing.Protocol): pass
class Three(One, Foo, typing.Protocol): pass
Traceback (most recent call last):
  File "<pyshell#9>", line 1, in <module>
    class Three(One, Foo, typing.Protocol): pass
  File "C:\Users\alexw\coding\typing_extensions\src\typing_extensions.py", line 684, in __new__
    for supercls in _calculate_mro(bases)
  File "C:\Users\alexw\coding\typing_extensions\src\typing_extensions.py", line 646, in _calculate_mro
    raise TypeError
TypeError

I've experimented locally using functools._c3_merge instead of Steven D'Aprano's ActiveState recipe, and I get the same results. Either the attempt to emulate the C3 resolution order in CPython is incorrect, or I'm misunderstanding how these functions are meant to be used.

Closing for now, as this probably isn't worth spending a huge amount of time on.

@AlexWaygood AlexWaygood deleted the play-nicely-please branch June 19, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants