Skip to content

Disable promotions in isinstance checks #6114

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

Merged

Conversation

Michael0x2a
Copy link
Collaborator

This pull request resolves #6060 by modifying conditional_type_map function in checker.py to ignore promotions.

This pull request resolves python#6060
by modifying `conditional_type_map` function in `checker.py` to ignore
promotions.
@Michael0x2a
Copy link
Collaborator Author

Note: I am somewhat suspicious of this solution. I'll try testing it on our internal codebases later today or tomorrow.

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.

I think this is a right change. You can merge this after verifying this works well with internal code bases.

@Michael0x2a Michael0x2a merged commit 51e34fa into python:master Dec 31, 2018
@Michael0x2a Michael0x2a deleted the disable-promotions-in-isinstance-checks branch December 31, 2018 06:21
@msullivan
Copy link
Collaborator

This does actually cause some trouble in internal code bases (with str and unicode). See #6141

msullivan added a commit that referenced this pull request Jan 4, 2019
This fixes a bug left behind by #6114, where too much would get
subtracted out of a union type after an isinstance.

Fixes #6141.
msullivan added a commit that referenced this pull request Jan 4, 2019
This fixes a bug left behind by #6114, where too much would get
subtracted out of a union type after an isinstance.

Fixes #6141.
msullivan added a commit that referenced this pull request Jan 8, 2019
The type of Num.n is `complex`, though it can in practice also be an
`int` or a `float`, and we case on the actual type to determine which
sort of mypy ast node to use.

In #6114, promotions were disabled in `isinstance` checks, which
caused mypy to decide that the `isinstance` checks for int and float
won't match. Fix this by giving the value the type object instead of
complex.

Typeshed *does* actually annotate `n` as `Union[float, int, complex]`,
but mypy simplifies that into `complex`. (It does *not* do this
simplification for local variables with such an annotation; see
issue #6168.)
msullivan added a commit that referenced this pull request Jan 9, 2019
The type of Num.n is `complex`, though it can in practice also be an
`int` or a `float`, and we case on the actual type to determine which
sort of mypy ast node to use.

In #6114, promotions were disabled in `isinstance` checks, which
caused mypy to decide that the `isinstance` checks for int and float
won't match. Fix this by giving the value the type object instead of
complex.

Typeshed *does* actually annotate `n` as `Union[float, int, complex]`,
but mypy simplifies that into `complex`. (It does *not* do this
simplification for local variables with such an annotation; see
issue #6168.)
Michael0x2a added a commit to Michael0x2a/mypy that referenced this pull request Jan 11, 2019
This pull request reverts python#6114
and python#6142: see
python#6180 for rationale.

In short, the original fix ended up being more disruptive then
anticipated: it's modifying str and unicode semantics, and we should
put a little more thought into reasoning about that particular case
before moving ahead with any fix here.

This should also help unblock the upcoming 0.660 release.
Michael0x2a added a commit that referenced this pull request Jan 11, 2019
This pull request reverts #6114
and #6142: see
#6180 for rationale.

In short, the original fix ended up being more disruptive then
anticipated: it's modifying str and unicode semantics, and we should
put a little more thought into reasoning about that particular case
before moving ahead with any fix here.

This should also help unblock the upcoming 0.660 release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mypy fails to narrow Union[float,int] to float after isinstance(x, float)
3 participants