Skip to content
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

Fix type guard crashes #11061

Merged
merged 9 commits into from Sep 14, 2021
Merged

Fix type guard crashes #11061

merged 9 commits into from Sep 14, 2021

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Sep 6, 2021

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.

@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks like a good start to clean-up the existing mess. I left few comments where things were not clear to me. There is no need to address them all in this PR, but it would be good to have a clearer/detailed long-term plan.

@@ -254,9 +254,6 @@ def visit_union_type(self, ut: UnionType) -> None:
for it in ut.items:
it.accept(self)

def visit_type_guard_type(self, t: TypeGuardType) -> None:
t.type_guard.accept(self)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it's not a proper type, we still need to fixup the deserialized target type. Do we have an incremental test with an import cycle?

mypy/meet.py Show resolved Hide resolved
@@ -335,9 +335,6 @@ def visit_union_type(self, typ: UnionType) -> SnapshotItem:
normalized = tuple(sorted(items))
return ('UnionType', normalized)

def visit_type_guard_type(self, typ: TypeGuardType) -> SnapshotItem:
return ('TypeGuardType', snapshot_type(typ.type_guard))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may be wrong. We still need to have some way to propagate the information on updated type guard function. (Btw most likely the old way was wrong as well, do we actually have good test cover for type guards in daemon mode?)

return self._is_proper_subtype(left.type_guard, self.right.type_guard)
else:
# TypeGuards aren't a subtype of anything else for now (but see #10489)
return False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just to clarify, what is the view on subtyping between type guard functions? Can a type guard method be overridden in a subclass? Can a type guard function be expected as a callback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe yes to both.

But note that mypy.types.TypeGuardedType doesn't actually appear in the signature of def f(x: object) -> TypeGuard[str]. The return type is just treated as a bool

elif self.anal_type_guard_arg(t, fullname) is not None:

mypy.types.TypeGuardedType is only created in find_isinstance_check:
return {expr: TypeGuardedType(node.callee.type_guard)}, {}

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

tornado (https://github.com/tornadoweb/tornado.git)
- tornado/testing.py:587: error: Incompatible types in assignment (expression has type "Union[Generator[Any, Any, Any], CoroutineType]", variable has type "Union[Generator[Any, Any, Any], Coroutine[Any, Any, Any], None]")
- tornado/testing.py:590: error: Incompatible return value type (got "Union[Generator[Any, Any, Any], CoroutineType, Coroutine[Any, Any, Any]]", expected "Union[Generator[Any, Any, Any], Coroutine[Any, Any, Any]]")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants