Skip to content

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

Merged
hauntsaninja merged 4 commits intopython:masterfrom
hauntsaninja:check-none-no-return
Jul 27, 2022
Merged

Check implicit None return is valid when using --no-warn-no-return#13219
hauntsaninja merged 4 commits intopython:masterfrom
hauntsaninja:check-none-no-return

Conversation

@hauntsaninja
Copy link
Copy Markdown
Collaborator

@hauntsaninja hauntsaninja commented Jul 23, 2022

Fixes #7511

@hauntsaninja hauntsaninja mentioned this pull request Jul 23, 2022
Closed
@github-actions

This comment has been minimized.

Sign in to view
1 similar comment
@github-actions

This comment has been minimized.

Sign in to view
@ofek
Copy link
Copy Markdown

ofek commented Jul 23, 2022

Awesome!

@github-actions

This comment has been minimized.

Sign in to view
sobolevn
sobolevn reviewed Jul 23, 2022
View reviewed changes
Copy link
Copy Markdown
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 🙂

test-data/unit/check-flags.test
# flags: --no-warn-no-return --strict-optional
import typing

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

@sobolevn sobolevn Jul 23, 2022

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
Copy Markdown
Member

@sobolevn sobolevn Jul 23, 2022

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
Copy Markdown
Collaborator Author

@hauntsaninja hauntsaninja Jul 23, 2022

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.

test-data/unit/check-flags.test
[builtins fixtures/dict.pyi]
[typing fixtures/typing-async.pyi]

[case testNoWarnNoReturn]
Copy link
Copy Markdown
Member

@sobolevn sobolevn Jul 23, 2022

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
Copy Markdown
Collaborator Author

@hauntsaninja hauntsaninja Jul 23, 2022 •
edited
Loading

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
Copy Markdown
Collaborator Author

hauntsaninja commented Jul 23, 2022

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
Copy Markdown
Collaborator Author

hauntsaninja commented Jul 23, 2022 •
edited
Loading

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
Copy Markdown
Contributor

github-actions bot commented Jul 27, 2022

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
@dimaqq dimaqq mentioned this pull request Sep 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sobolevn sobolevn sobolevn left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@hauntsaninja @ofek @sobolevn