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

inspect, asyncio: Use more TypeGuards #8057

Merged
merged 8 commits into from
Jul 19, 2022

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jun 11, 2022

Fixes #8009.

@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 June 12, 2022 11:50
@AlexWaygood AlexWaygood marked this pull request as draft June 12, 2022 11:54
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

Interesting. @erictraut, any idea why the test cases I've added in this PR are failing pyright? Mypy seems to have no trouble with them.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jun 12, 2022

Mypy primer analysis


Most of these are just noise: mypy is just emitting slightly different error messages for things that already failed to type check.


+ discord/ext/tasks/__init__.py:622: error: Incompatible return value type (got "Callable[..., CoroutineType[Any, Any, Any]]", expected "ET")

This one looks like a true positive to me. Source code here: https://github.com/Rapptz/discord.py/blob/334ef1d7facce9dbfba2a6924bf57fc59bc827b5/discord/ext/tasks/__init__.py#L598-L622


aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/helpers.py:126: error: Incompatible redefinition (redefinition with type "Callable[[Any], bool]", original type overloaded function)  [misc]

pyinstrument (https://github.com/joerick/pyinstrument)
+ pyinstrument/vendor/decorator.py:66: error: Incompatible redefinition (redefinition with type "Callable[[Any], Any]", original type overloaded function)

These are both a little unfortunate, but can't really be helped. I think they will simply have to add type: ignores to their code if this PR is merged.

@erictraut
Copy link
Contributor

@AlexWaygood, pyright doesn't currently support TypeGuard on overloaded functions. Evaluating overloads is very expensive in the type narrowing code path because all argument types need to be evaluated first, so it creates a bunch of dependencies between expressions. For that reason, I didn't initially add support for this case. I can think of ways to limit the performance impact, but it will take some work. I've filed this feature enhancement request to track this.

@hauntsaninja hauntsaninja added the status: deferred Issue or PR deferred until some precondition is fixed label Jun 13, 2022
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jun 13, 2022

I've filed this feature enhancement request to track this.

Thanks @erictraut!

I can think of ways to limit the performance impact, but it will take some work.

I'm in no particular rush with this PR :)

Evaluating overloads is very expensive in the type narrowing code path

If the performance impact is too large for pyright to tolerate, I suppose I could simply exclude the test cases from this PR -- these are the only bits that fail pyright. It seems that since pyright doesn't yet support combining overloads with TypeGuards, it just infers the return type of these overloaded functions as bool -- and that's what they are at the moment, anyway, so there wouldn't be a regression in pyright's understanding of these functions.

I think it would be unfeasible to consider a version of this PR without the overloads, however: the results from mypy-primer indicate to me that merging a version of this PR without the overloads would lead to an unacceptable number of false-positive errors when type-checking.

(Yet another option is obviously to close this PR and not bother too much about it.)

@erictraut
Copy link
Contributor

This is included in pyright 1.1.255, which I just published.

@AlexWaygood AlexWaygood mentioned this pull request Jun 20, 2022
@AlexWaygood AlexWaygood removed the status: deferred Issue or PR deferred until some precondition is fixed label Jun 20, 2022
@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jun 20, 2022

This is included in pyright 1.1.255, which I just published.

Thank you very much! Everything looks to be working well now; the tests I've added are passing CI nicely.

For reviewers: mypy-primer analysis is in this comment here: #8057 (comment)

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/tasks/__init__.py:564: error: Incompatible types in assignment (expression has type "FT", variable has type "None")
+ discord/ext/tasks/__init__.py:564: error: Incompatible types in assignment (expression has type "Callable[[VarArg(<nothing>), KwArg(<nothing>)], CoroutineType[Any, Any, <nothing>]]", variable has type "None")
+ discord/ext/tasks/__init__.py:565: error: Incompatible return value type (got "Callable[[VarArg(<nothing>), KwArg(<nothing>)], CoroutineType[Any, Any, <nothing>]]", expected "FT")
- discord/ext/tasks/__init__.py:592: error: Incompatible types in assignment (expression has type "FT", variable has type "None")
+ discord/ext/tasks/__init__.py:592: error: Incompatible types in assignment (expression has type "Callable[[VarArg(<nothing>), KwArg(<nothing>)], CoroutineType[Any, Any, <nothing>]]", variable has type "None")
+ discord/ext/tasks/__init__.py:593: error: Incompatible return value type (got "Callable[[VarArg(<nothing>), KwArg(<nothing>)], CoroutineType[Any, Any, <nothing>]]", expected "FT")
+ discord/ext/tasks/__init__.py:623: error: Incompatible return value type (got "Callable[..., CoroutineType[Any, Any, Any]]", expected "ET")
- discord/ext/commands/cog.py:215: error: Item "Group" of "Union[Any, Group, Command[Any, Any, Any]]" has no attribute "__cog_listener_names__"
+ discord/ext/commands/cog.py:215: error: "Callable[..., CoroutineType[Any, Any, Any]]" has no attribute "__cog_listener_names__"
- discord/ext/commands/cog.py:215: error: Item "Command[Any, Any, Any]" of "Union[Any, Group, Command[Any, Any, Any]]" has no attribute "__cog_listener_names__"
- discord/ext/commands/cog.py:218: error: Item "Group" of "Union[Any, Group, Command[Any, Any, Any]]" has no attribute "__name__"
- discord/ext/commands/cog.py:218: error: Item "Command[Any, Any, Any]" of "Union[Any, Group, Command[Any, Any, Any]]" has no attribute "__name__"

aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/helpers.py:126: error: Incompatible redefinition (redefinition with type "Callable[[Any], bool]", original type overloaded function)  [misc]

pyinstrument (https://github.com/joerick/pyinstrument)
+ pyinstrument/vendor/decorator.py:66: error: Incompatible redefinition (redefinition with type "Callable[[Any], Any]", original type overloaded function)

@JelleZijlstra JelleZijlstra merged commit e156c63 into python:master Jul 19, 2022
@AlexWaygood AlexWaygood deleted the iscoroutinefunction branch July 19, 2022 07:06
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.

Should asyncio.iscoroutinefunction return some kind of TypeGuard?
4 participants