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

Check implicit None return is valid when using --no-warn-no-return #13219

Merged
merged 4 commits into from
Jul 27, 2022

Conversation

hauntsaninja
Copy link
Collaborator

Fixes #7511

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@ofek
Copy link
Sponsor

ofek commented Jul 23, 2022

Awesome!

@github-actions

This comment has been minimized.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Can you please add some docs please? Because it is not clear to me how this works. I assume it is an opposite of --warn-no-return, but not like a "full" opposite, but a partial one for an implicit None return? Kinda?

Anyway, it looks like a lot of people are happy about it 🙂

# flags: --no-warn-no-return --strict-optional
import typing

def implicit_optional_return(arg) -> typing.Optional[str]:
Copy link
Member

Choose a reason for hiding this comment

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

What will happen for a case like:

def implicit_optional_no_return() -> Optional[int]:
    print(1)  # or any other non-primitive body

Copy link
Member

Choose a reason for hiding this comment

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

And what will happen for just -> int?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No error for the first, error for the second Incompatible return value type (implicitly returns "None", expected "int"). Same as the existing test cases, since the if statement has no impact on reachability.

@@ -410,6 +410,44 @@ async def h() -> NoReturn: # E: Implicit return in function which does not retu
[builtins fixtures/dict.pyi]
[typing fixtures/typing-async.pyi]

[case testNoWarnNoReturn]
Copy link
Member

Choose a reason for hiding this comment

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

Do we need test cases for ... and pass?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's no change of behaviour here, so covered by existing tests. (Also note that every test would complain about errors in stubs if the trivial body logic breaks — I know because I broke this locally initially)

@hauntsaninja
Copy link
Collaborator Author

No, it continues to be a full opposite of --warn-no-return, but we just fix some unsoundness (so no docs change needed).

I suspect this got overlooked because the old behaviour was written back when --no-strict-optional was the default and the old behaviour is sound with --no-strict-optional. I added test cases with and without --strict-optional :-)

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Jul 23, 2022

Because it is not clear to me how this works.

The new error generated isn't really anything to do with explicit returns or implicit returns, it's just type checking the logic of the function per Python semantics.

To take your example from #13203 (comment)
On mypy's normal settings, these both error with "Missing return statement", which is great, but slightly opinionated.

def func_one(self) -> int:
        self.x += 1

def func_two(self) -> Optional[int]:
        self.x += 1

Previously, when using --no-warn-no-return, neither of these would complain. But that's unsound for func_one since it promises that it can never return None; with this PR you get Incompatible return value type (implicitly returns "None", expected "int")

@github-actions
Copy link
Contributor

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

@hauntsaninja hauntsaninja merged commit c02ec4a into python:master Jul 27, 2022
@hauntsaninja hauntsaninja deleted the check-none-no-return branch July 27, 2022 03:27
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.

warn_no_return = False: handle implicit "return None" (not ignoring return type)
3 participants