-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
Restore compatibility with PyPy <3.9 #262
Conversation
and __tp is not Protocol | ||
and __tp is not getattr(typing, "Protocol", object()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_protocol(typing.Protocol)
was evaluating to True
on PyPy-3.8. I don't know why that is, really, but this change fixes it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I used !=
on purpose here so that Protocol can have a custom __eq__
that helps us out. What you're doing here should work too, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I have no idea why _ProtocolMeta.__eq__
doesn't seem to be doing what it should be doing on PyPy 3.8. I suspect another bug in PyPy. It's easy enough for us to work around it here, though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This looks good, but Matti's response in https://foss.heptapod.net/pypy/pypy/-/issues/3958#note_299926 makes me think that maybe we should drop support for pypy3.7/3.8 instead, so let's wait to see what he responds.
Yeah... I guess the question is whether we're okay with mandating that black, trio, pydantic, cattrs and pylint all drop support for PyPy-3.7 and PyPy-3.8 as well? Seems like quite a few projects that depend on typing_extensions still test with PyPy-3.8 in CI :// |
Matti suggested we should drop support for pypy3.7/3.8. However, given that this is a regression and so many downstream packages still depend on pypy3.7/3.8, I'm inclined to merge this and release 4.7.1, then in 4.8.0 formally drop support for pypy3.7/3.8. We can also ask downstream packages to drop support. I know at least one maintainer of Black will agree. |
That sounds like a reasonable plan. My personal opinion -- I'm fully on board with dropping support for PyPy-3.7, since Python 3.7 is EOL. For PyPy-3.8, before dropping support, I'd prefer to first have a conversation with packages who depend on us and who we know test with PyPy-3.8 in CI. As you say, since PyPy-3.8 is no longer supported, maybe they shouldn't be testing with it in CI anymore -- but for packages that we know we'd be breaking, I'd still prefer to have the conversation first, I think. And whatever the case, I agree that it's a good idea to restore compatibility in a patch release now, then consider what to do in the next feature release after that.
👀 |
Fixes #258.
It looks like the problem is that PyPy on <3.9 looks up
__mro_entries__
on the class rather than the instance. That's the correct behaviour for 99% of dunders, but__mro_entries__
is extremely special in that it needs to be looked up on the instance rather than the class; it's this quality that allows us to monkey-patch__mro_entries__
methods onto functions.It also looks like some of our tests have been broken on PyPy <3.9 for quite a while -- the
NamedTuple
function-based implementation also didn't work on PyPy 3.8.