Unintended consequences of eliminating promotions with isinstance?

[msullivan]

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?

[msullivan]

And, this is a regression---albeit a false negative regression. It might be worth thinking about before the release (#6130)

[msullivan]

(Thoughts, @Michael0x2a ?)

[Michael0x2a]

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]

That sounds reasonable to me

[ilevkivskyi]

Do we need to keep both this and #6060 open? Maybe we can continue the discussion in #6060 and close this one?