Error when checking function conditionally defined in try/except/else clause

[aiiie]

The following code causes mypy to crash:

def fun1():
    pass

try:
    pass
except:
    fun2 = fun1
else:
    def fun2():
        pass

Traceback:

Traceback (most recent call last):
  File "./scripts/mypy", line 6, in <module>
    main(__file__)
  File "mypy/mypy/main.py", line 50, in main
    type_check_only(sources, bin_dir, options)
  File "mypy/mypy/main.py", line 94, in type_check_only
    python_path=options.python_path)
  File "mypy/mypy/build.py", line 210, in build
    result = manager.process(initial_states)
  File "mypy/mypy/build.py", line 425, in process
    next.process()
  File "mypy/mypy/build.py", line 930, in process
    self.type_checker().visit_file(self.tree, self.tree.path)
  File "mypy/mypy/checker.py", line 409, in visit_file
    self.accept(d)
  File "mypy/mypy/checker.py", line 450, in accept
    typ = node.accept(self)
  File "mypy/mypy/nodes.py", line 707, in accept
    return visitor.visit_try_stmt(self)
  File "mypy/mypy/checker.py", line 1732, in visit_try_stmt
    self.accept(s.else_body)
  File "mypy/mypy/checker.py", line 450, in accept
    typ = node.accept(self)
  File "mypy/mypy/nodes.py", line 526, in accept
    return visitor.visit_block(self)
  File "mypy/mypy/checker.py", line 1113, in visit_block
    self.accept(s)
  File "mypy/mypy/checker.py", line 450, in accept
    typ = node.accept(self)
  File "mypy/mypy/nodes.py", line 382, in accept
    return visitor.visit_func_def(self)
  File "mypy/mypy/checker.py", line 594, in visit_func_def
    'redefinition with type')
  File "mypy/mypy/checker.py", line 2117, in check_subtype
    if not is_subtype(subtype, supertype):
  File "mypy/mypy/subtypes.py", line 49, in is_subtype
    return left.accept(SubtypeVisitor(right, type_parameter_checker))
AttributeError: 'NoneType' object has no attribute 'accept'

I'm still trying to understand the code and why it might be crashing. As a side issue, one thing I find odd is that presumably when mypy checks itself, it isn't seeing that TypeChecker.check_subtype() doesn't accept None for the first argument (it accepts Type but doesn't specify it using Optional). When it's called, the first argument is defined like this:

orig_type = defn.original_def.type

Where defn is of the type mypy.nodes.FuncDef. FuncDef.original_def is of the type Union[mypy.nodes.FuncDef, mypy.nodes.Var]. FuncDef.type and Var.type are of the type Optional[mypy.types.Type] (both fields are optional because they're set to None on their respective classes).

Shouldn't the checker, using type inference when orig_type is defined, catch that type mismatch when TypeChecker.check_subtype() is called?

Edit: I suppose my digression above might be invalid if all types are nullable, but while PEP 484 states, "[b]y default, None is an invalid value for any type, unless a default value of None has been provided in the function definition", the mypy docs say, "None is a valid value for every type, which resembles null in Java." Are the mypy docs out of date/incorrect, or is every type nullable?

[ddfisher]

You've hit the nail on the head: mypy doesn't do proper Optional/None checking yet (though we plan to add it soon!).

[gvanrossum]

This looks like a manifestation of one of the issues listed in #1297.

[gvanrossum]

(Well, except that #1297 is about imports, and this is about def. But there still seems to be a common theme.)

[aiiie]

You've hit the nail on the head: mypy doesn't do proper Optional/None checking yet (though we plan to add it soon!).

Awesome, thanks for clarifying that. That sounds like a great change. I was glad to see it make it into the PEP.

Regarding the crash, I'm still trying to figure my way around the codebase, but I was able to come up with a fix despite my not knowing exactly how everything works. Here's my proposed change:

diff --git a/mypy/checker.py b/mypy/checker.py
index 4df142b..53c69ac 100644
--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -1728,8 +1728,6 @@ class TypeChecker(NodeVisitor[Type]):
         self.binder.try_frames.add(len(self.binder.frames) - 2)
         self.accept(s.body)
         self.binder.try_frames.remove(len(self.binder.frames) - 2)
-        if s.else_body:
-            self.accept(s.else_body)
         self.breaking_out = False
         changed, frame_on_completion = self.binder.pop_frame()
         completed_frames.append(frame_on_completion)
@@ -1766,6 +1764,8 @@ class TypeChecker(NodeVisitor[Type]):

         self.binder.update_from_options(completed_frames)

+        if s.else_body:
+            self.accept(s.else_body)
         if s.finally_body:
             self.accept(s.finally_body)

diff --git a/mypy/test/data/check-statements.test b/mypy/test/data/check-statements.test
index 3b98693..9cb183f 100644
--- a/mypy/test/data/check-statements.test
+++ b/mypy/test/data/check-statements.test
@@ -516,6 +516,19 @@ else:
   object(None) # E: Too many arguments for "object"
 [builtins fixtures/exception.py]

+[case testRedefinedFunctionInTryWithElse]
+def f() -> None:
+    return None
+
+try: pass
+except BaseException: f2 = f
+else:
+    def f2() -> str:
+        return 'hi'
+[builtins fixtures/exception.py]
+[out]
+main:7: error: Incompatible redefinition (original type Callable[[], None], redefinition with type Callable[[], str])
+
 [case testExceptWithoutType]
 import typing
 try:

It seems like the type checker visitor, when it handles try blocks, checks the else block before the except block. That's at odds with how the other visitors in earlier passes run through the code. One of the previous code analysis passes populates information about the conditionally defined name/function, but because the checker handles them in the wrong order, it doesn't populate type information for the function defined in the except block, and that eventually causes the crash.

If that change seems reasonable, I can submit a PR. I do wonder if there's a larger issue here with how the different visitors/analysis passes are even able to get out of sync like that, but maybe it's unavoidable.

Also, I don't totally understand how the "binder" thing works. Would my change require changes to how it's invoked, or is it OK as is?

[JukkaL]

Thanks for looking at this!

A simple explanation of the binder is that it's tracking if variables or expressions have different types in different places due to things like assignments or isinstance tests. We may have to tweak the use of binder to get this fixed.

[JukkaL]

This crash also happens when type checking Lib/trace.py from Python 2.7 std lib.

[gvanrossum]

@Brodie: I looked into this and improved a bit on your diff, saving you the effort of submitting a PR. (But if you somehow want or need the full credit, feel free to submit a PR and I'll use that instead.)

@jukka: I'm not entirely sure whether deleting the "self.breaking_out = False" from near the top of visit_try_stmt() is the right thing to do? After looking at various places where this is set and used I still don't completely follow its meaning.