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

mypy 0.981 does not simplify unions for assert_type #13760

Closed
Dr-Irv opened this issue Sep 29, 2022 · 20 comments · Fixed by #13781
Closed

mypy 0.981 does not simplify unions for assert_type #13760

Dr-Irv opened this issue Sep 29, 2022 · 20 comments · Fixed by #13781
Labels
bug mypy got something wrong

Comments

@Dr-Irv
Copy link

Dr-Irv commented Sep 29, 2022

Bug Report

Code below works fine with 0.971, but fails with 0.981. It seems that 0.971 did type narrowing on contents of a Union, but 0.981 doesn't do it consistently.

To Reproduce

from typing import Union
from typing_extensions import reveal_type, assert_type

from datetime import date, datetime, timedelta

Scalar = Union[str, bytes, date, datetime, timedelta, bool, complex]


def reduce_it(s: Scalar) -> Scalar:
    return s


reveal_type(reduce_it(20))
reveal_type(reduce_it(True))

assert_type(reduce_it(True), Scalar)

Expected Behavior

Version 0.971 output:

mypysubclass.py:13: note: Revealed type is "Union[builtins.str, builtins.bytes, datetime.date, datetime.timedelta, builtins.complex]"
mypysubclass.py:14: note: Revealed type is "Union[builtins.str, builtins.bytes, datetime.date, datetime.timedelta, builtins.complex]"

Actual Behavior

Version 0.981 output:

mypysubclass.py:13: note: Revealed type is "Union[builtins.str, builtins.bytes, datetime.date, datetime.timedelta, builtins.complex]"
mypysubclass.py:14: note: Revealed type is "Union[builtins.str, builtins.bytes, datetime.date, datetime.timedelta, builtins.complex]"
mypysubclass.py:16: error: Expression is of type "Union[str, bytes, date, timedelta, complex]", not "Union[str, bytes, date, datetime, timedelta, bool, complex]"

Your Environment

  • Mypy version used: 0.981
  • Mypy command-line flags: None
  • Mypy configuration options from mypy.ini (and other config files): None
  • Python version used: 3.10.4
@Dr-Irv Dr-Irv added the bug mypy got something wrong label Sep 29, 2022
@hauntsaninja
Copy link
Collaborator

Thanks for the report! mypy_primer -p ~/dev/mypy_primer/test.py --bisect --new v0.981 --old v0.971 --debug bisects this to #13311

@JelleZijlstra JelleZijlstra changed the title mypy 0.981 does not narrow unions for assert_type mypy 0.981 does not simplify unions for assert_type Sep 29, 2022
@JelleZijlstra
Copy link
Member

Here is a reduced repro:

from typing import Union
from typing_extensions import assert_type

Scalar = Union[int, bool, bytes, bytearray]


def reduce_it(s: Scalar) -> Scalar:
    return s

assert_type(reduce_it(True), Scalar)

Interestingly, this requires two pairs of subclasses in the Union to repro; if I just put int, bool, bytes, the assert_type() passes.

@ilevkivskyi
Copy link
Member

Yeah, the change was kind of intentional. I wanted mypy to check something like type(x) is t instead of type(x) == t so people can do more precise/detailed type assertions. But I see how this can be inconvenient, because mypy itself does some union simplification (e.g. when type-checking a call expression).

How much inconvenience does this cause? This should be easy to change back, I just wanted to gather a bit more input before doing this.

@ilevkivskyi
Copy link
Member

Hmm... actually the fact that this requires two subtype pairs to trigger, means there may actually be some (unrelated) bug.

@twoertwein
Copy link

twoertwein commented Sep 29, 2022

If mypy would warn about redundant unions, this bug would actually be a feature ;)

edit: would be a generalization of flake8-pyi's int | float | complex check

@Dr-Irv
Copy link
Author

Dr-Irv commented Sep 29, 2022

How much inconvenience does this cause? This should be easy to change back, I just wanted to gather a bit more input before doing this.

We are using this pattern in pandas-stubs for testing our stubs, so changing this is a bit painful, as we can't test that a stub is returning the type we are expecting.

I find it confusing that mypy is reducing the types in the unions. We also use pyright, which doesn't do that, and it becomes easier to debug things when the unions aren't reduced.

@twoertwein
Copy link

We are using this pattern in pandas-stubs for testing our stubs, so changing this is a bit painful, as we can't test that a stub is returning the type we are expecting.

I think it would just mean that we need to de-duplicate all unions that occur in assert_type checks, such as _typing.Scalar which contains sub-classes. I believe that we have so far only one case where we hit this new behavior/regression.

@Dr-Irv
Copy link
Author

Dr-Irv commented Sep 29, 2022

I think it would just mean that we need to de-duplicate all unions that occur in assert_type checks, such as _typing.Scalar which contains sub-classes. I believe that we have so far only one case where we hit this new behavior/regression.

My only concern here is that from a user perspective, I think it is valuable to have Union cover "duplicate" items. For example, if you have a function that accepts Union[superclass, subclass, int, float], where subclass is a subclass of superclass, that tells the user that any of those classes are valid, whereas Union[superclass, float] isn't entirely clear that subclass or int are valid arguments, unless you know that subclass is a superclass, and that float is a superclass of int, and people reading code that specifies an API may not know the superclass/subclass relationships.

Another example is using complex, and you don't necessarily think that float and int are subclasses of complex.

For mypy, would be nice to have an option where the Union types were not consolidated.

@JelleZijlstra
Copy link
Member

My only concern here is that from a user perspective, I think it is valuable to have Union cover "duplicate" items.

The original example in this issue is a particularly pernicious example: it removed bool, because bool is a subclass of int, int is a subclass of float, float is a subclass of complex, and complex is also in the union. But none of these subclass relationships are particularly obvious, and a few of them exist only at type checking time.

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 29, 2022

It would be interesting to experiment with disabling complex union simplification. Duplicate entries should always be removed (e.g. int | int -> int), but simplifying float | int | bool to float doesn't sound very intuitive.

Also, as long as we perform union simplification, I think that assert_type should only care that the types are semantically the same, not identical. So int | float should be considered the same type as float, probably.

@ilevkivskyi
Copy link
Member

@JukkaL

Also, as long as we perform union simplification, I think that assert_type should only care that the types are semantically the same, not identical. So int | float should be considered the same type as float, probably.

Yeah, the one downside however, is if we use ignore_promotions = False, then int64 will pass assert_type() for int and vice versa (although maybe it is not a big deal).

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 30, 2022

What about not doing promotions during union simplification, i.e. ignore_promition = True, at least as a first step?

@ilevkivskyi
Copy link
Member

What about not doing promotions during union simplification, i.e. ignore_promition = True, at least as a first step?

I was thinking about this (and it will indeed fix this issue), but it may be a big change, we can of course try and see mypy_primer.

@ilevkivskyi
Copy link
Member

Btw not using promotions during union simplification is a generally right thing to do, since we ignore promotions when checking isinstance() etc. So maybe it is a good idea after all.

@ilevkivskyi
Copy link
Member

OK, so it is a bit more tricky than that. Previous attempt to ignore promotions in isinstance() was reverted because of some Python 2 str/unicode issue, see #6180. But now that Python 2 is gone, I think we cat try it again.

@ilevkivskyi
Copy link
Member

The mypy_primer in #13781 actually looks pretty good.

ilevkivskyi added a commit that referenced this issue Oct 3, 2022
Fixes #13760 
Fixes #6060
Fixes #12824

This is a right thing to do, but let's what `mypy_primer` will be. This
also required re-applying #6181 (which is also a right thing to do)
otherwise some tests fail.
@twoertwein
Copy link

Running mypy 0.982 on the example from the top of this issue still triggers an error:

test.py:13: note: Revealed type is "Union[builtins.str, builtins.bytes, datetime.date, datetime.timedelta, builtins.complex]"
test.py:14: note: Revealed type is "Union[builtins.str, builtins.bytes, datetime.date, datetime.timedelta, builtins.complex]"
test.py:16: error: Expression is of type "Union[str, bytes, date, timedelta, complex]", not "Union[str, bytes, date, datetime, timedelta, bool, complex]" [assert-type]

@ilevkivskyi
Copy link
Member

The fix was intentionally not included in 0.982, since it required a too big change for a point release. It will be included in 0.990

@ilevkivskyi
Copy link
Member

@twoertwein You can try running mypy from master to double-check that your issue is fixed.

@twoertwein
Copy link

@twoertwein You can try running mypy from master to double-check that your issue is fixed.

It works with mypy 0.990+dev.dc5c299aa190949f2300b163ccc10257a779006d :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants