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

gh-104050: Argument clinic: enable mypy's --warn-return-any setting #107405

Merged
merged 4 commits into from
Jul 29, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jul 28, 2023

Mypy's --warn-return-any check flags when a function is declared to return a specific type, but mypy can't verify whether it's actually returning that type or not -- it can only infer a vague, unsafe Any type as the returned type. This can often lead to bugs slipping beneath the radar.

In the case of argument clinic, the check only flags a single function. But, turns out that it's a true positive! The function is incorrectly annotated at the moment -- the annotation says that it returns a FunctionType, but that's not true. The function creates a FunctionType instance fn, and then returns whatever calling fn returns returns. Therefore, the proper return annotation for this function is -> Any, not -> FunctionType.

Closes #104050!

@erlend-aasland
Copy link
Contributor

Good sleuthing!

@@ -4288,7 +4288,7 @@ def eval_ast_expr(
globals: dict[str, Any],
*,
filename: str = '-'
) -> FunctionType:
) -> Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a comment as to why Any is the correct annotation here?

Copy link
Member Author

Choose a reason for hiding this comment

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

You weren't going to make finally closing that issue easy, were you? ;)

Let me know if a5103be is too verbose...!

Copy link
Member Author

@AlexWaygood AlexWaygood Jul 28, 2023

Choose a reason for hiding this comment

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

Hmm, actually, now that I look at it again, the comment feels like it duplicates the docstring a little bit... not sure if it's worth it? I'll defer to you on whether it's helpful or not!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, you're right. The docstring should be enough!

Copy link
Member Author

@AlexWaygood AlexWaygood Jul 29, 2023

Choose a reason for hiding this comment

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

Okay -- removed the comment again and tweaked the docstring slightly! How's it look now?

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Yay!

@AlexWaygood AlexWaygood merged commit 87de2fb into python:main Jul 29, 2023
18 checks passed
@AlexWaygood AlexWaygood deleted the clinic-return-any branch July 29, 2023 12:39
@bedevere-bot

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add type annotations to clinic.py
3 participants