Skip to content

asyncio: make AbstractServer abstract and remove unnecessary metaclass=ABCMeta #7185

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

Merged
merged 12 commits into from
Feb 13, 2022

Conversation

AlexWaygood
Copy link
Member

I've been fiddling about with a script to identify functions that:

  1. from an AST analysis of the runtime source code, appear to return None at runtime.
  2. Are annotated as returning something other than None at runtime.

One thing the script has identified is that there seem to be a bunch of classes in asyncio that are not marked as being abstract in typeshed, but look awfully like they're intended to be abstract at runtime.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood marked this pull request as ready for review February 13, 2022 20:29
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood changed the title asyncio: make several classes abstract asyncio: make AbstractServer abstract Feb 13, 2022
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood requested a review from Akuli February 13, 2022 21:14
@Akuli
Copy link
Collaborator

Akuli commented Feb 13, 2022

$ grep -r metaclass=ABCMeta stdlib/asyncio/
stdlib/asyncio/events.pyi:class BaseDefaultEventLoopPolicy(AbstractEventLoopPolicy, metaclass=ABCMeta):
stdlib/asyncio/base_events.pyi:class BaseEventLoop(AbstractEventLoop, metaclass=ABCMeta):

Neither of these seems to be necessary.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Feb 13, 2022

$ grep -r metaclass=ABCMeta stdlib/asyncio/
stdlib/asyncio/events.pyi:class BaseDefaultEventLoopPolicy(AbstractEventLoopPolicy, metaclass=ABCMeta):
stdlib/asyncio/base_events.pyi:class BaseEventLoop(AbstractEventLoop, metaclass=ABCMeta):

Neither of these seems to be necessary.

I think the metaclass lie is actually necessary for both of these classes, unfortunately. If we do not lie about the metaclass, then mypy will complain about not all abstractmethods being implemented -- because there are no abstractmethods defined in the body of either of these classes, mypy will assume that the classes are meant to be a concrete implementation of the abstract classes they inherit from.

The way to get mypy to accept that the subclass is also meant to be abstract, even though it defines no abstractmethods itself, is explicitly re-specify the metaclass in the subclass. Unfortunately, in this case that means lying about the metaclass.

But, let me check...

@Akuli
Copy link
Collaborator

Akuli commented Feb 13, 2022

tests/mypy_test.py passes to me with metaclass=ABCMeta removed from BaseEventLoop, although the one in BaseDefaultEventLoopPolicy seems to be necessary.

@AlexWaygood
Copy link
Member Author

tests/mypy_test.py passes to me with metaclass=ABCMeta removed from BaseEventLoop, although the one in BaseDefaultEventLoopPolicy seems to be necessary.

Okay, let's try it

@Akuli Akuli changed the title asyncio: make AbstractServer abstract asyncio: make AbstractServer abstract and remove unnecessary metaclass=ABCMeta Feb 13, 2022
@github-actions

This comment has been minimized.

1 similar comment
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

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