check for control-flow paths that fail to return a value

[sid-kap]

I noticed that for a function that should return a value, for example:

def foo() -> int:
  pass

mypy doesn't report an error if there is a pass (and no return statement) in the function. Is this intentional?

[gvanrossum]

It's because mypy doesn't do proper checking for Optional -- in effect
all types are optional. This is a known defect, you may be able to
find the issue that this is a duplicate of.
(Though honestly I think there's another issue too -- even if None is
a valid return value it should still not be allowed to fall off the
end of a function body if the return type isn't only None. Nor
should it be allowed to write "return" without a value. But this is
all difficult to enforce without causing a lot of spurious
complaints.)

[ddfisher]

I think this is more of the second issue. In order to ensure that functions return something, we'll need to do some control flow analysis, but I think it'll be quite some time before we get to that (because there's a lot else to do!).

[sid-kap]

Thanks for your quick replies. I'll leave it up to you whether you want to leave this issue open or close the issue.

[gvanrossum]

Added "needs discussion" label because the design will require some, um, discussion.

[rwbarton]

Whatever we decide about functions with a return type of None and return versus return None, I think it's pretty clear that we ought to treat falling off the end of a function the same way as an explicit return without a value. We currently don't do that, even without strict optional checking enabled:

def f() -> int:
    pass    # no error

def g() -> int:
    return  # E: Return value expected

Like @ddfisher says, this is a matter of doing control flow analysis. I wouldn't be surprised if we can cajole the ConditionalTypeBinder into doing it for us, by treating a return statement like an assignment to a special "return value" variable. @ecprice, does that sound plausible?

[rwbarton]

It turns out that (as suggested at #320) just tacking on a return statement to the body of every function sort of achieves this already, since mypy doesn't type check code it thinks is unreachable. I tried running mypy with this change on itself and found a number of functions missing return statements, though the signal-to-noise ratio is quite bad as mypy doesn't yet understand the flow control nature of some common constructs like while True: ... or assert False. At @ddfisher's suggestion I've been looking at the conditional type binder recently and I think it'll be pretty easy to have it handle most of these cases correctly.

The most interesting false positive cases were those in parse.py and fastparse.py that look like

    def expect(self, string: str) -> Token:
        if self.current_str() == string:
            self.ind += 1
            return self.tok[self.ind - 1]
        else:
            self.parse_error()

    def parse_error(self) -> None:
        self.parse_error_at(self.current())
        raise ParseError()

Here parse_error exits by raising an exception, so expect can't actually fall off the end of the function without returning. (In C, for example, gcc has a noreturn attribute that you can give a function like parse_error so that the compiler won't give you a warning in a case like this.) Interestingly we already have the means to do the same thing, we just have to give parse_error the type () -> UninhabitedType; if an expression statement has type UninhabitedType then it cannot return. The only question is how the user should mark parse_error as not returning.

[ddfisher]

This would probably require a typing change, but we could add a NoReturn annotation or similar (which seems like it should be reasonably understandable).

[JukkaL]

Yeah, having an annotation for this would be reasonable. There are several ways we could do it. Here are some random ideas:

• A special return type, as suggested above.
• A function decorator such as @does_not_return. This would make this more explicit, as the return type could perhaps be mistaken as some real return value with a funny name.
• A new mypy-specific comment annotation such as like this:

def error() -> None:
    # mypy: does_not_return
    raise Error('failure')

The last option might be reasonable if we don't want to rush a PEP 484 change (or it's hard to get the change through) or if we also want to add additional mypy-only annotations or directives, perhaps for things like enabling/disabling specific warnings. It would require a parser change.

[ecprice]

Whoa, for some reason github didn't send me an email for @rwbarton's comments, but did for @ddfisher's.

I've rewritten the type binder code to be cleaner. Once the pull request is merged, you can avoid the hack of tacking on return statements by doing something like if not frame.broken and not isinstance(self.return_types[-1], Void): at the end of check_func_def (and add as frame to the end of the with statement).

It looks to me that just special casing while True/assert False solves everything but parse_error() in the mypy codebase. Stdlib samples also have while 1 and assert 0, and after that I think everything it flags is a real bug.

[ecprice]

I've put a branch that does this at https://github.com/ecprice/mypy/tree/error-on-fall . The only tests that fail are due to parse_error() and a number of stdlib test suite __exit__ functions that don't return anything. __exit__ is supposed to return a bool, whether to suppress an exception, but lots of implementations don't return anything and rely on bool(None) == False.

[rwbarton]

It looks to me that just special casing while True/assert False solves everything but parse_error() in the mypy codebase.

Probably true due to your new commit "Keep track of whether a try/except block can fall off its end." as there were surprisingly many occurrences involving try statements. I already extracted all the false positive cases in mypy to unit test cases, so they will be good tests for your new branch.

It would be really nice to also have a way to test for false negatives on a real code base; maybe automatically adding an assert False to all functions that mypy thinks can't fall off the end? Not sure how to do that in practice, though.

[ecprice]

A complementary way of checking the control flow logic is to give errors on unused lines of code. I've added a couple commits to error-on-fall that do that; this caught a bug in while True/while 1 logic. I've set it up so unused code/missing return are options (warn-unreachable and warn-no-return).

It finds errors in several places that are like

def foo() -> None:
    raise NotImplementedError()  # the below is broken because X
    [lots of broken code]

def bar() -> None:
    assert false, "does this code ever get called?"
    [old dead code]

which should perhaps be special cased as OK?

[rwbarton]

Yeah, having an annotation for this would be reasonable. There are several ways we could do it. Here are some random ideas:

• A special return type, as suggested above.
• A function decorator such as @does_not_return. This would make this more explicit, as the return type could perhaps be mistaken as some real return value with a funny name.
• A new mypy-specific comment annotation such as like this: [...]

I sort of like the decorator idea (alternate name suggestion: @noreturn). I'm assuming that internally mypy would recognize the decorator, replace the function's return type by UninhabitedType and type check the function with that new type to make sure it really doesn't return (though note that this is not how we normally type check decorated functions, and interaction with other decorators may be tricky).

See also python/typing#165 which suggests a return type NoReturn. I think the decorator is more obvious, especially to people coming from other languages. The primary advantage of giving the user direct access to UninhabitedType (which is what NoReturn is) would be that it can be used in other contexts, like List[UninhabitedType] for a list that must be empty. However, those uses are fairly obscure, the name NoReturn is bad in the List[_] context, and the name UninhabitedType is bad in the def f() -> _: context, and I don't have a better name suggestion.

Another real use case: six.reraise should be marked as noreturn, since it always raises an exception. (This came up in one of our internal repos.)

[timabbott]

This was fixed in the last release, right?