Fix crash on TypeGuard plus "and" #10496

JelleZijlstra · 2021-05-18T04:49:51Z

In python/typeshed#5473, I tried to switch a number of inspect functions to use the new TypeGuard functionality. Unfortunately, mypy-primer found a number of crashes in third-party libraries in places where a TypeGuard function was ANDed together with some other check. Examples:

The problems trace back to the decision in #9865 to make TypeGuardType not inherit from ProperType: in various conditions that are more complicated than a simple if check, mypy wants everything to become a ProperType. Therefore, to fix the crashes I had to make TypeGuardType a ProperType and support it in various visitors.

JukkaL

Thanks! This looks good to me (just one comment).

I wonder if we can "leak" type guard types more generally during type checking. This could plausibly trigger some of the other visitors that we don't currently support, resulting in a crash. Can you give a link to the code where the crash happened? Maybe it's clear from the context that we won't perform arbitrary type operations on type guard types.

JukkaL · 2021-05-19T08:41:03Z

mypy/subtypes.py

@@ -1374,6 +1377,10 @@ def visit_overloaded(self, left: Overloaded) -> bool:
    def visit_union_type(self, left: UnionType) -> bool:
        return all([self._is_proper_subtype(item, self.orig_right) for item in left.items])

+    def visit_type_guard_type(self, left: TypeGuardType) -> bool:
+        # TODO: What's the right thing to do here?


I guess since we don't expect type guard types to be used for subtype checks, we can raise an error here as well?

Thanks for reviewing! I'll make this one throw an error.

Oops, that actually causes it to crash again.

File "/Users/jelle/py/mypy/mypy/checker.py", line 5232, in or_conditional_maps result[n1] = make_simplified_union([m1[n1], m2[n2]]) File "/Users/jelle/py/mypy/mypy/typeops.py", line 370, in make_simplified_union if i != j and is_proper_subtype(tj, ti, keep_erased_types=keep_erased) and \ File "/Users/jelle/py/mypy/mypy/subtypes.py", line 1178, in is_proper_subtype return _is_proper_subtype(left, right, File "/Users/jelle/py/mypy/mypy/subtypes.py", line 1199, in _is_proper_subtype return left.accept(ProperSubtypeVisitor(orig_right, File "/Users/jelle/py/mypy/mypy/types.py", line 290, in accept return visitor.visit_type_guard_type(self) File "/Users/jelle/py/mypy/mypy/subtypes.py", line 1381, in visit_type_guard_type raise RuntimeError("TypeGuard should not be used in subtype checks") RuntimeError: TypeGuard should not be used in subtype checks sphinx/ext/coverage.py:212: : note: use --pdb to drop into pdb

Going to write a proper implementation instead.

JelleZijlstra · 2021-05-19T13:59:39Z

The first crash was:

sphinx/util/inspect.py:252: error: INTERNAL ERROR -- Please try using mypy master on Github:
https://mypy.readthedocs.io/en/stable/common_issues.html#using-a-development-mypy-build
Please report a bug at https://github.com/python/mypy/issues
version: 0.820+dev.1e220b107db1868b3b039cf9299bfceba8046741
Traceback (most recent call last):
  File "/Users/jelle/py/venvs/mypy/bin/mypy", line 33, in <module>
    sys.exit(load_entry_point('mypy', 'console_scripts', 'mypy')())
  File "/Users/jelle/py/mypy/mypy/__main__.py", line 11, in console_entry
    main(None, sys.stdout, sys.stderr)
  File "/Users/jelle/py/mypy/mypy/main.py", line 98, in main
    res = build.build(sources, options, None, flush_errors, fscache, stdout, stderr)
  File "/Users/jelle/py/mypy/mypy/build.py", line 179, in build
    result = _build(
  File "/Users/jelle/py/mypy/mypy/build.py", line 253, in _build
    graph = dispatch(sources, manager, stdout)
  File "/Users/jelle/py/mypy/mypy/build.py", line 2689, in dispatch
    process_graph(graph, manager)
  File "/Users/jelle/py/mypy/mypy/build.py", line 3013, in process_graph
    process_stale_scc(graph, scc, manager)
  File "/Users/jelle/py/mypy/mypy/build.py", line 3111, in process_stale_scc
    graph[id].type_check_first_pass()
  File "/Users/jelle/py/mypy/mypy/build.py", line 2164, in type_check_first_pass
    self.type_checker().check_first_pass()
  File "/Users/jelle/py/mypy/mypy/checker.py", line 294, in check_first_pass
    self.accept(d)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 687, in accept
    return visitor.visit_func_def(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 726, in visit_func_def
    self._visit_func_def(defn)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 730, in _visit_func_def
    self.check_func_item(defn, name=defn.name)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 792, in check_func_item
    self.check_func_def(defn, typ, name)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 975, in check_func_def
    self.accept(item.body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1015, in accept
    return visitor.visit_block(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1975, in visit_block
    self.accept(s)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1206, in accept
    return visitor.visit_if_stmt(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 3266, in visit_if_stmt
    self.accept(s.else_body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1015, in accept
    return visitor.visit_block(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1975, in visit_block
    self.accept(s)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1206, in accept
    return visitor.visit_if_stmt(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 3249, in visit_if_stmt
    t = get_proper_type(self.expr_checker.accept(e))
  File "/Users/jelle/py/mypy/mypy/checkexpr.py", line 3911, in accept
    typ = node.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1750, in accept
    return visitor.visit_op_expr(self)
  File "/Users/jelle/py/mypy/mypy/checkexpr.py", line 2148, in visit_op_expr
    return self.check_boolean_op(e, e)
  File "/Users/jelle/py/mypy/mypy/checkexpr.py", line 2821, in check_boolean_op
    right_type = self.analyze_cond_branch(right_map, e.right, left_type)
  File "/Users/jelle/py/mypy/mypy/checkexpr.py", line 3880, in analyze_cond_branch
    return self.accept(node, type_context=context, allow_none_return=allow_none_return)
  File "/Users/jelle/py/mypy/mypy/checkexpr.py", line 3911, in accept
    typ = node.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1750, in accept
    return visitor.visit_op_expr(self)
  File "/Users/jelle/py/mypy/mypy/checkexpr.py", line 2148, in visit_op_expr
    return self.check_boolean_op(e, e)
  File "/Users/jelle/py/mypy/mypy/checkexpr.py", line 2821, in check_boolean_op
    right_type = self.analyze_cond_branch(right_map, e.right, left_type)
  File "/Users/jelle/py/mypy/mypy/checkexpr.py", line 3880, in analyze_cond_branch
    return self.accept(node, type_context=context, allow_none_return=allow_none_return)
  File "/Users/jelle/.pyenv/versions/3.9.4/lib/python3.9/contextlib.py", line 124, in __exit__
    next(self.gen)
  File "/Users/jelle/py/mypy/mypy/binder.py", line 407, in frame_context
    self.pop_frame(can_skip, fall_through)
  File "/Users/jelle/py/mypy/mypy/binder.py", line 229, in pop_frame
    self.last_pop_changed = self.update_from_options(options)
  File "/Users/jelle/py/mypy/mypy/binder.py", line 206, in update_from_options
    if current_value is None or not is_same_type(type, current_value):
  File "/Users/jelle/py/mypy/mypy/sametypes.py", line 13, in is_same_type
    left = get_proper_type(left)
  File "/Users/jelle/py/mypy/mypy/types.py", line 1989, in get_proper_type
    assert isinstance(typ, ProperType), typ
AssertionError: TypeGuard(types.MethodType)

I first tried fixing that one with a special case in sametypes.py but got a crash here instead:

sphinx/ext/coverage.py:212: error: INTERNAL ERROR -- Please try using mypy master on Github:
https://mypy.readthedocs.io/en/stable/common_issues.html#using-a-development-mypy-build
Please report a bug at https://github.com/python/mypy/issues
version: 0.820+dev.1e220b107db1868b3b039cf9299bfceba8046741.dirty
Traceback (most recent call last):
  File "/Users/jelle/py/venvs/mypy/bin/mypy", line 33, in <module>
    sys.exit(load_entry_point('mypy', 'console_scripts', 'mypy')())
  File "/Users/jelle/py/mypy/mypy/__main__.py", line 11, in console_entry
    main(None, sys.stdout, sys.stderr)
  File "/Users/jelle/py/mypy/mypy/main.py", line 98, in main
    res = build.build(sources, options, None, flush_errors, fscache, stdout, stderr)
  File "/Users/jelle/py/mypy/mypy/build.py", line 179, in build
    result = _build(
  File "/Users/jelle/py/mypy/mypy/build.py", line 253, in _build
    graph = dispatch(sources, manager, stdout)
  File "/Users/jelle/py/mypy/mypy/build.py", line 2689, in dispatch
    process_graph(graph, manager)
  File "/Users/jelle/py/mypy/mypy/build.py", line 3013, in process_graph
    process_stale_scc(graph, scc, manager)
  File "/Users/jelle/py/mypy/mypy/build.py", line 3111, in process_stale_scc
    graph[id].type_check_first_pass()
  File "/Users/jelle/py/mypy/mypy/build.py", line 2164, in type_check_first_pass
    self.type_checker().check_first_pass()
  File "/Users/jelle/py/mypy/mypy/checker.py", line 294, in check_first_pass
    self.accept(d)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 950, in accept
    return visitor.visit_class_def(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1726, in visit_class_def
    self.accept(defn.defs)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1015, in accept
    return visitor.visit_block(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1975, in visit_block
    self.accept(s)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 687, in accept
    return visitor.visit_func_def(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 726, in visit_func_def
    self._visit_func_def(defn)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 730, in _visit_func_def
    self.check_func_item(defn, name=defn.name)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 792, in check_func_item
    self.check_func_def(defn, typ, name)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 975, in check_func_def
    self.accept(item.body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1015, in accept
    return visitor.visit_block(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1975, in visit_block
    self.accept(s)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1140, in accept
    return visitor.visit_for_stmt(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 3486, in visit_for_stmt
    self.accept_loop(s.body, s.else_body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 417, in accept_loop
    self.accept(body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1015, in accept
    return visitor.visit_block(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1975, in visit_block
    self.accept(s)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1140, in accept
    return visitor.visit_for_stmt(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 3486, in visit_for_stmt
    self.accept_loop(s.body, s.else_body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 417, in accept_loop
    self.accept(body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1015, in accept
    return visitor.visit_block(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1975, in visit_block
    self.accept(s)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1206, in accept
    return visitor.visit_if_stmt(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 3266, in visit_if_stmt
    self.accept(s.else_body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1015, in accept
    return visitor.visit_block(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1975, in visit_block
    self.accept(s)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1206, in accept
    return visitor.visit_if_stmt(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 3259, in visit_if_stmt
    self.accept(b)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1015, in accept
    return visitor.visit_block(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1975, in visit_block
    self.accept(s)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1140, in accept
    return visitor.visit_for_stmt(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 3486, in visit_for_stmt
    self.accept_loop(s.body, s.else_body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 424, in accept_loop
    self.accept(else_body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1015, in accept
    return visitor.visit_block(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1975, in visit_block
    self.accept(s)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1140, in accept
    return visitor.visit_for_stmt(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 3486, in visit_for_stmt
    self.accept_loop(s.body, s.else_body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 417, in accept_loop
    self.accept(body)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1015, in accept
    return visitor.visit_block(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 1975, in visit_block
    self.accept(s)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 401, in accept
    stmt.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 1206, in accept
    return visitor.visit_if_stmt(self)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 3254, in visit_if_stmt
    if_map, else_map = self.find_isinstance_check(e)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 4050, in find_isinstance_check
    if_map, else_map = self.find_isinstance_check_helper(node)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 4270, in find_isinstance_check_helper
    left, right = self.find_isinstance_check_helper(node.expr)
  File "/Users/jelle/py/mypy/mypy/checker.py", line 4267, in find_isinstance_check_helper
    return (or_conditional_maps(left_if_vars, right_if_vars),
  File "/Users/jelle/py/mypy/mypy/checker.py", line 5232, in or_conditional_maps
    result[n1] = make_simplified_union([m1[n1], m2[n2]])
  File "/Users/jelle/py/mypy/mypy/typeops.py", line 338, in make_simplified_union
    items = get_proper_types(items)
  File "/Users/jelle/py/mypy/mypy/types.py", line 2002, in get_proper_types
    return [get_proper_type(t) for t in it]
  File "/Users/jelle/py/mypy/mypy/types.py", line 2002, in <listcomp>
    return [get_proper_type(t) for t in it]
  File "/Users/jelle/py/mypy/mypy/types.py", line 1989, in get_proper_type
    assert isinstance(typ, ProperType), typ
AssertionError: TypeGuard(types.MethodType)

I think the current code shouldn't let TypeGuard through in too many places because it's almost exclusively used in the binder, but fixing #10489 properly will probably involve making TypeGuard more of a real type, so it will appear in more places. I'm not proposing to do that now, but this PR will help create a foundation for that change.

Fixes #11007, fixes #10899, fixes #10647 Since the initial implementation of TypeGuard, there have been several fixes quickly applied to make mypy not crash on various TypeGuard things. This includes #10496, #10683 and #11015. We'll discuss how this PR relates to each of these three changes. In particular, #10496 seems incorrect. As A5rocks discusses in #10899 , it introduces confusion between a type guarded variable and a TypeGuard[T]. This PR basically walks back that change entirely and renames TypeGuardType to TypeGuardedType to reduce that possible confusion. Now, we still have the issue that TypeGuardedTypes are getting everywhere and causing unhappiness. I see two high level solutions to this: a) Make TypeGuardedType a proper type, then delegate to the wrapped type in a bunch of type visitors and arbitrary amounts of other places where multiple types interact, and hope that we got all of them, b) Make TypeGuardedType as an improper type (as it was in the original implementation)! Similar to TypeAliasType, it's just a wrapper for another type, so we unwrap it in get_proper_type. This is the approach this PR takes. This might feel controversial, but I think it could be the better option. It also means that if we type check we won't get type guard crashes. #10683 is basically "remove call that leads to crash from the stacktrace". I think the join here (that ends up being with the wrapped type of the TypeGuardedType) is actually fine: if it's different, it tells us that the type changed, which is what we want to know. So seems fine to remove the special casing. Finally, #11015. This is the other contentious part of this PR. I liked the idea of moving the core "type guard overrides narrowing" idea into meet.py, so I kept that. But my changes ended up regressing a reveal_type testTypeGuardNestedRestrictionAny test that was added. But it's not really clear to me how that worked or really, what it tested. I tried writing a simpler version of what I thought the test was meant to test (this is testTypeGuardMultipleCondition added in this PR), but that fails on master. Anyway, this should at least fix the type guard crashes that have been coming up.

JelleZijlstra added 4 commits May 17, 2021 20:58

failing test case

84395cf

fix the bug

acc5b9e

another case

2e5116a

bite the bullet and build the visitors

91e9364

JelleZijlstra requested a review from JukkaL May 18, 2021 04:50

JelleZijlstra added 2 commits May 18, 2021 05:48

fix lint

1724438

Merge remote-tracking branch 'origin/master' into tgand

832be41

JukkaL approved these changes May 19, 2021

View reviewed changes

JelleZijlstra added 2 commits May 19, 2021 07:01

error in subtype visitor

dace8af

proper implementation instead

892e656

JelleZijlstra merged commit 8e909e4 into master May 21, 2021

JelleZijlstra deleted the tgand branch May 21, 2021 13:42

JelleZijlstra mentioned this pull request May 29, 2021

bump various dependencies python/typeshed#5551

Merged

A5rocks mentioned this pull request Jun 19, 2021

Fix typeguards crash when assigning guarded value in if statement #10671

Closed

This was referenced Sep 6, 2021

type guarded value is same type as typing.TypeGuard[T]? #10899

Closed

Fix type guard crashes #11061

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash on TypeGuard plus "and" #10496

Fix crash on TypeGuard plus "and" #10496

JelleZijlstra commented May 18, 2021

JukkaL left a comment

JukkaL May 19, 2021

JelleZijlstra May 19, 2021

JelleZijlstra May 19, 2021

JelleZijlstra commented May 19, 2021

Fix crash on TypeGuard plus "and" #10496

Fix crash on TypeGuard plus "and" #10496

Conversation

JelleZijlstra commented May 18, 2021

JukkaL left a comment

Choose a reason for hiding this comment

JukkaL May 19, 2021

Choose a reason for hiding this comment

JelleZijlstra May 19, 2021

Choose a reason for hiding this comment

JelleZijlstra May 19, 2021

Choose a reason for hiding this comment

JelleZijlstra commented May 19, 2021