Skip to content

asyncio: remove overly specific protocols #10984

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 4 commits into from
Nov 8, 2023

Conversation

JelleZijlstra
Copy link
Member

The _warn parameter to these methods is just there to work around
some finalization issues and should never be used by users. In
addition, these protocols are out of date (the "stacklevel" argument
is not used by current CPython main). I don't think we gain anything
by trying to maintain these protocol definitions.

The _warn parameter to these methods is just there to work around
some finalization issues and should never be used by users. In
addition, these protocols are out of date (the "stacklevel" argument
is not used by current CPython main). I don't think we gain anything
by trying to maintain these protocol definitions.

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Nov 6, 2023

Maybe we can just remove __del__? Since the argument is for internal use only anyway, I don't think there is much value in keeping the methods. (Would need an allowlist entry, though, probably.)

@JelleZijlstra
Copy link
Member Author

That might be good, yes. I'm not sure what value we get from putting __del__ methods in typeshed. I think a slightly better approach could be to pretend it takes no arguments, which would still need a stubtest entry.

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 6, 2023

I think stubtest actually already doesn't care whether __del__ exists in a stub or not; I think it's specifically excluded

@srittau
Copy link
Collaborator

srittau commented Nov 6, 2023

After a quick grep: We have 16 instances of __del__ in stdlib, 8 of which are in asyncio. I'd say, let's get rid of them and see whether stubtest complains.

@AlexWaygood
Copy link
Member

After a quick grep: We have 16 instances of __del__ in stdlib, 8 of which are in asyncio. I'd say, let's get rid of them and see whether stubtest complains.

Sounds good! I'm pretty confident stubtest won't complain, because of https://github.com/python/mypy/blob/544e6ce119ec6bdd5eabab53a433264c98dc7d9c/mypy/stubtest.py#L1377

@JelleZijlstra
Copy link
Member Author

One reason to keep it: https://docs.python.org/3/reference/datamodel.html#object.__del__ says

If a base class has a del() method, the derived class’s del() method, if any, must explicitly call it to ensure proper deletion of the base class part of the instance.

A type checker or other kind of linter might want to enforce this rule, but to do so, it needs to know whether __del__ exists in the base class. So I do think there is value in having these methods in typeshed.

@JelleZijlstra
Copy link
Member Author

And relatedly, removing __del__ methods from typeshed would mean that a derived class that does super().__del__() would get a false positive from a type checker, because there is no object.__del__.

@AlexWaygood
Copy link
Member

One reason to keep it: https://docs.python.org/3/reference/datamodel.html#object.__del__ says

If a base class has a del() method, the derived class’s del() method, if any, must explicitly call it to ensure proper deletion of the base class part of the instance.

A type checker or other kind of linter might want to enforce this rule, but to do so, it needs to know whether __del__ exists in the base class. So I do think there is value in having these methods in typeshed.

In that case, we might want to remove it from stubtest's IGNORABLE_CLASS_DUNDERS frozenset

@JelleZijlstra
Copy link
Member Author

In that case, we might want to remove it from stubtest's IGNORABLE_CLASS_DUNDERS frozenset

Agree. For this PR though, I think I'd prefer to just pretend the _warn argument doesn't exist, since no users should be calling it and it exists only to early-bind the warn function. (The problem it guards against is that when __del__ is called, the module globals may already have been cleared.)

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Nov 8, 2023

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

@JelleZijlstra JelleZijlstra merged commit b36f3c5 into python:main Nov 8, 2023
@JelleZijlstra JelleZijlstra deleted the asynciodel branch November 8, 2023 03:29
@JelleZijlstra JelleZijlstra restored the asynciodel branch September 10, 2024 23:38
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.

3 participants