Unintended consequences of eliminating promotions with isinstance? #6180
Description
Previously, the following code:
def foo(x):
# type: (unicode) -> None
if isinstance(x, str):
reveal_type(x)
else:
reveal_type(x)
would emit the expected types (str, unicode) in each branch
Post #6114, which removed the use of promotions when doing isinstance,
it only emits that the type is unicode and skips checking the first branch.
Is this bad? Should we be doing something else? Obviously str is not really a subtype of unicode, but there are a lot of places where things declared unicode can actually be str.
I think the intention with the first fix was to mostly to fix Union-related issues, so maybe we need a special case for non-unions? Argh.
Thoughts?
Activity
msullivan commentedon Jan 11, 2019
And, this is a regression---albeit a false negative regression. It might be worth thinking about before the release (#6130)
msullivan commentedon Jan 11, 2019
(Thoughts, @Michael0x2a ?)
Michael0x2a commentedon Jan 11, 2019
Honestly, at this stage, I think we should just revert the pull request I made and re-open #6060. I think it's pretty clear this has more ramifications then intended, and it doesn't seem like the original issue is really that glaring of an issue anyways.
We can then look into landing a more targeted and less disruptive fix after the release.
Really, the only reason why I went ahead and landed this was because it didn't seem to make a difference when I tried testing that fix internally -- but clearly, I messed up some part of that process.
msullivan commentedon Jan 11, 2019
That sounds reasonable to me
Re-enable promtions in isinstance checks
Re-enable promotions in isinstance checks (#6181)
ilevkivskyi commentedon Jan 11, 2019
Do we need to keep both this and #6060 open? Maybe we can continue the discussion in #6060 and close this one?