-
-
Notifications
You must be signed in to change notification settings - Fork 121
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
Union[str, int]
is treated as dynamic
#49
Comments
We also found that Static Python understands This bug suggests that some tests pass for a wrong reason. For example, the following test from the static python test suit is asserting that a union is a subtype of a broader union. But what really happened is that "the broader union" is read as def test_union_can_assign_to_broader_union(self):
self.assertReturns(
"""
from typing import Union
class B:
pass
def f(x: int, y: str) -> Union[int, str, B]:
return x or y
""",
"Union[int, str]",
) |
Hi! So the widening of It turns out there is still value in the compiler support, because if we infer a union from e.g. Thanks for pointing out the bad tests! Originally we supported union annotations until we realized the soundness hole, so these tests predate the widening of union annotations to dynamic. The path forward here is of course to add runtime support for casts to union, though we remain concerned about the performance with large union types. We might need to keep some arbitrary limit on union size, and above that limit widen the union to dynamic. Not sure what the best UX is for this; we could I suppose make it a type error to specify such a large union as an annotation? This might be better than silently widening to dynamic? Again this is a case where our desire for Pyre/mypy compatibility would push us away from the explicit approach and towards the silent widening. |
Summary: As noted in #49 some of these union tests became ineffective when we closed the union-annotations soundness hole by widening all non-optional union annotations to dynamic. The tests are passing but for the wrong reason; instead of testing union `can_assign_from`, they are effectively now testing that anything can be assigned to dynamic. Remove the bad tests and replace them with a single unit-test style test of `UnionType.can_assign_from` which covers the cases which (for now) aren't testable via real code. Reviewed By: DinoV Differential Revision: D31451895 fbshipit-source-id: f888f1b
The bad tests are addressed in 439b330 |
Unions should have static checks, right? @vivaan2006 and I just ran a small program. We expected a typechecker error, but no:
No error for |
@bennn This is explained in my comment above from Oct 4, 2021. It's not strictly a "static check" vs "runtime check" distinction. It's not enough to just say that we won't use union type information at runtime and when we emit a runtime check we will consider a union to be dynamic, because this can lead us to statically narrow a (not usable at runtime) union type to a simpler type that we do use at runtime, and then wrongly trust it. Consider:
Therefore we have to consider We could do better here if we had the ability in our compiler to track two parallel sets of types, "trusted" and "untrusted", and we used "untrusted" types for the purposes of emitting compile-time errors but never for runtime use. In that case we could record an "untrusted" type of So the only time our compiler support for tracking unions actually comes into play is when a union type arises "organically" in a way where we can trust it, e.g. in a case like this:
So you'll see that you do get a static error from e.g. this variant of your example:
Let me know if that doesn't make sense. I won't claim this behavior is ideal when SP is viewed in isolation, but it makes a bit more sense when SP is considered in tandem with use of e.g. mypy or Pyre. In an ideal future system, I would want the parallel tracking of trusted and untrusted types in a single SP compiler (essentially rolling the functionality of pyre/mypy into the static compiler also.) Also, more near term, we would ideally like to add support for arbitrary unions. The main question mark is the cost of verifying them at runtime, since that cost will be |
Ok, I was confused about the extent of compiler support for unions. The examples help: there's support for those organic unions but everything else is dynamic. The |
What version of Static Python are you using?
6862bbd
2021-10-03
What program did you run?
What happened?
The type of
x
isdynamic
What should have happened?
We expected the type of
x
to beUnion[int, str]
because union types are supported:The text was updated successfully, but these errors were encountered: