Skip to content

Teach mypy about NoReturn #2798

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
Feb 7, 2017
Merged

Teach mypy about NoReturn #2798

merged 4 commits into from
Feb 7, 2017

Conversation

ddfisher
Copy link
Collaborator

@ddfisher ddfisher commented Feb 3, 2017

Fixes #2474.

@ddfisher
Copy link
Collaborator Author

ddfisher commented Feb 3, 2017

One thing this doesn't do is disallow use of NoReturn in places other than as a return type, as there didn't seem to be a great way to do that, and it doesn't seem too important (because you'll get a reasonable-looking type error pretty quickly).

@JukkaL
Copy link
Collaborator

JukkaL commented Feb 6, 2017

Looks good!

It would be good to have more test cases. Here are some ideas:

  • Test assigning the return value from a NoReturn function to a variable (x = no_return_func()). This should be an error.
  • Test if no_return_func():. This should be an error.
  • Test using NoReturn in the middle of an expression where it probably doesn't make sense, e.g. [1, 2, no_return_func(), 4] and f(no_return_func()).
  • Test using NoReturn in an expression when it's okay. Here's one example: normal_func() or no_return_func() should have the type A if normal_func returns A.
  • Test what happens if you use NoReturn as an annotated argument or variable type and try to call the function or use the variable, respectively.

If some of these are non-trivial to support, you can create new issues for these.

@ddfisher
Copy link
Collaborator Author

ddfisher commented Feb 7, 2017

I'm not sure I agree that using NoReturn in those places should be an error. Sure, it's weird, but type-theoretically there's nothing wrong with it.

Added tests for the last two bullets.

@gvanrossum
Copy link
Member

I'd say that for the first three bullets, the expression containing the call itself inherits the "doesn't return" property. So then this should lead to some dead code, depending on the control flow. E.g. here I'd expect the final line of the function to be unreachable (given the above definition of no_return()):

def f() -> int:
    no_return()
    does_not_exist()  # Not a NameError because it's unreachable

I don't feel to strongly about all this though, so I personally am fine with however you and Jukka decide this.

@ddfisher
Copy link
Collaborator Author

ddfisher commented Feb 7, 2017

Currently we don't check for unreachable code, though. E.g.

def f() -> None:
  return
  does_not_exist()  # E: Name 'does_not_exist' is not defined

@JukkaL
Copy link
Collaborator

JukkaL commented Feb 7, 2017

Agreed that some of the cases I listed aren't real type errors, but they are at least pretty questionable and likely signs of code quality issues. I don't expect those cases to be common in the wild, so the current behavior is already okay.

(We already reject arguably similar cases, such as using the return value of a function with a None return type, even if None would be valid value in some context.)

@JukkaL JukkaL merged commit 9406886 into master Feb 7, 2017
@ddfisher ddfisher deleted the understand-noreturn branch February 7, 2017 19:08
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