assert False makes rest of function not checked by mypy

[Hnasar]

It seems when you have assert False, the rest of the function isn't checked.

x = 0
def foo_bad1():  # type: () -> str
    assert False
    return x
def foo_good():  # type: () -> str
    assert x
    return x
def foo_bad2():  # type: () -> str
    assert 0
    return x

• What is the actual behavior/output?

$ mypy  foo2.py
foo2.py:7: error: Incompatible return value type (got "int", expected "str")

• What is the behavior/output you expect?

$ mypy  foo2.py
foo2.py:4: error: Incompatible return value type (got "int", expected "str")
foo2.py:7: error: Incompatible return value type (got "int", expected "str")
foo2.py:10: error: Incompatible return value type (got "int", expected "str")

• What are the versions of mypy and Python you are using?
  I tried with mypy 0.501, all the way up to 0.720, and also from git master.
  python 3.7.4 from Fedora 30.

• What are the mypy flags you are using? (For example --strict-optional)
  none

[thomaslee]

Quick observation before I pick on this specific issue: the "prune everything after an assert False" semantics seem to be intentional for top-level definitions/statements, but at the block/statement level the intent is a bit less clear. With that out of the way ...

An even simpler repro case:

def expecting_check_error() -> str:
  assert False
  return 123

Pretty sure what's happening here is that TypeChecker.find_isinstance_check() returns None, {} here. None is then passed to push_type_map which then marks the rest of the function unreachable.

Proof: the issue is "fixed" by a hacky little change like this:

diff --git a/mypy/checker.py b/mypy/checker.py
index 1de0876e..07ae749b 100644
--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -3028,7 +3028,7 @@ class TypeChecker(NodeVisitor[None], CheckerPluginInterface):
         true_map, else_map = self.find_isinstance_check(s.expr)
         if s.msg is not None:
             self.expr_checker.analyze_cond_branch(else_map, s.msg, None)
-        self.push_type_map(true_map)
+        self.push_type_map(true_map or {})
 
     def visit_raise_stmt(self, s: RaiseStmt) -> None:
         """Type check a raise statement."""

If you look at find_isinstance_check(), the empty dict is what we'd get for assert True.

IMO this is a bug (or at best surprising/"unsafe" behavior), but I think one of the core folks will need to weigh in on the actual expected semantics.

[thomaslee]

From my perspective the options seem to be one of the following:

1. mypy always assumes that code after assert False is unreachable. This is the "surprising" current semantics we see here.
2. mypy always assumes that code after assert False is reachable. This is the "safest possible" semantics, but with possible ergonomic issues for users who rely on the current semantics.
3. some kind of mypy option(s) to choose some balance between these semantics at runtime.

Defer to the experts but I'd personally prefer we default to (2) and maybe hook up (3) later to let folks opt-out of the stricter semantics. Just seems a bit safer.

[emmatyping]

Thank you for digging into this @thomaslee!

I view mypy as doing the correct thing here. assert False means that the code following it is unreachable, why would you want mypy to check code that never gets executed? Furthermore, if mypy did check that code, it could give spurious errors that would just add noise.

Is there a real-world example where the current behavior causes issues?

[ilevkivskyi]

This behavior is by design, mypy doesn't type check code it can prove unreachable. There is also a new flag to issue a warning in this case --warn-unreachable.

[thomaslee]

@ethanhs (cc @ilevkivskyi) in an earlier draft I mentioned this but I guess it got lost somewhere along the way: the obvious one is -O/PYTHONOPTIMIZE, right?

Example:

# my_package/__init__.py

def do_stuff():
  assert False
  print('I did stuff!')

# driver.py

from my_package import do_stuff

do_stuff()

Then, without -O/PYTHONOPTIMIZE (the "normal" case) we get the semantics you folks expect:

$ python driver.py 
Traceback (most recent call last):
  File "driver.py", line 3, in <module>
    do_stuff()
  File "/Users/tom/src/repro/my_package/__init__.py", line 2, in do_stuff
    assert False
AssertionError

But the same code, unmodified, with -O/PYTHONOPTIMIZE:

$ python -O driver.py 
I did stuff!
$ PYTHONOPTIMIZE=1 python driver.py
I did stuff!

Hopefully that makes the issue clear.

EDIT: The above was using Python 3.7.4, but same results with a recent-ish 3.8 build.

[Hnasar]

Thanks for the quick replies, everyone! And thanks @ilevkivskyi for clarifying that this is by design. Still, I think this is an incorrect design choice to make these errors/warnings silent by default and I would encourage we reopen this issue to improve the usability of this functionality.

(For example, by doing things like making the warning default, or revisiting this design choice entirely due to the optimize case as above.)

As a slightly less minimal example which mimics some code I was editing at work:

from typing import Any

class Foo(object):
    def __init__(self):  # type: () -> None
        self.data = None
    def set_data(self, data):  # type: (Any) -> None
        self.data = data
    def to_str(self):  # type: () -> str
        assert self.data
        return self.data  # should be a type error -- this is missing str()

foo = Foo()
foo.set_data(5)
mystr = ''  # type: str
mystr = foo.to_str()  # missing type error here

mypy and mypy --strict don't complain about any issues with the above code.

Clearly, one small solution is to add an annotation like self.data = None # type: Optional[Any] which at least prints an error in the --strict case. Still, I only noticed this issue by accident when my reveal_type lines weren't working for some reason and I imagine there's plenty of code out there that is silently hiding errors.

[thomaslee]

As a slightly less minimal example

Hm this would be a different issue to the one you originally raised I think. Suspect it works because mypy is inferring self.data is the Any type. All bets are off where Any is involved in type checking -- suggest you look at the --disallow-any family of flags and see if you can find something that works for you.

I personally think the "assume dead code for the assert False thing" is a bug because the assumption breaks in potentially scary ways under PYTHONOPTIMIZE, but the Any thing is likely expected (and unrelated) behavior.

[ilevkivskyi]

@thomaslee assert False is a commonly accepted way to mark a branch as unreachable, and people do actually use it. I don't believe anyone would deploy some optimized code in prod that fails on assert False in dev. Also actually not treating such branches as unreachable will trigger false positives about missing return statements and possibly other problems.

@Hnasar Your problem is not forgetting annotation but using Any where you should use object.
Also --warn-unreachable was added exactly for the kind of situations you describe. The only thing we can do is to add --warn-unreachable to --strict when the former is more polished.

[thomaslee]

No worries @ilevkivskyi. I guess it just seems like forcing folks to modify their code to use something that ensures the code is truly unreachable under all circumstances (such as raise) would be more "predictable". I mean, picking up mypy to begin with often means changing the way you write Python to adapt to the type checker -- given that and assuming PYTHONOPTIMIZE is a first-class feature of Python that people also may be using/relying on, why is assert given special status?

Anyway -- I won't argue the point further, feel free to ignore me if I'm being annoying. 😉 Thanks for the clarification!