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

Invalid handling type narrowing with inf/float due to duck typing #12824

Closed
CarliJoy opened this issue May 20, 2022 · 4 comments · Fixed by #13781
Closed

Invalid handling type narrowing with inf/float due to duck typing #12824

CarliJoy opened this issue May 20, 2022 · 4 comments · Fixed by #13781
Labels
bug mypy got something wrong topic-type-narrowing Conditional type narrowing / binder

Comments

@CarliJoy
Copy link

CarliJoy commented May 20, 2022

Bug Report

Due to duck typing handling type narrowing within a function accepting both int and float (even explicitly) doesn't work correctly.

To Reproduce
Consider this code, that actually leads to a type error

from typing import NewType

Euro = NewType("Euro", float)

def to_eur(user_in: int | float | str) -> Euro:
    reveal_type(user_in) # int | float | str as expected
    if isinstance(user_in, float):
        reveal_type(user_in) # float
        return Euro(user_in)
    reveal_type(user_in) # str, but expected str | int
    converted = float(user_in.rstrip("€"))
    return Euro(converted)
    

print(to_eur(10))  # Raises type error as int has no method rstrip

Expected Behavior
I would expect that MyPy would warn me and that the expected reveal_type after the the isinstance call is int | float
This should be even the case, if int is not explicitly given.

Consider the modification of above:

from typing import NewType

Euro = NewType("Euro", float)

def to_eur(user_in: float | str) -> Euro:
    reveal_type(user_in) # float | str as expected
    if isinstance(user_in, float):
        reveal_type(user_in) # float
        return Euro(user_in)
    reveal_type(user_in) # str
    converted = float(user_in.rstrip("€"))
    return Euro(converted)
    

print(to_eur(10))   # Raises type error as int has no method rstrip

would still raise a runtime type error and no typing error at all.

Also handling all duck types explicitly as unions when type narrowing would probably handle the non reachable bugs described in
#12643.
This is also suggested in #11516

Actual Behavior
No error / warning raised by MyPy

Your Environment

See MyPy Playground

  • Mypy version used: 0.950
  • Mypy command-line flags: no
  • Python version used: 3.10
  • Operating system and version: Fedora/Online

Maybe related issues:
#11511

@CarliJoy CarliJoy added the bug mypy got something wrong label May 20, 2022
@AlexWaygood AlexWaygood added the topic-type-narrowing Conditional type narrowing / binder label May 20, 2022
@ethanhs
Copy link
Collaborator

ethanhs commented May 20, 2022

This behavior is specified in PEP 484. You may also want to read #3186.

@ethanhs ethanhs closed this as completed May 20, 2022
@CarliJoy
Copy link
Author

This behavior is specified in PEP 484. You may also want to read #3186.

Sorry but how is this defined there?
How can it be defined, that the typed narrowing doesn't work, even so it is explicitly defined as union?

How can't this be a bug?
The PEP only says:

Rather than requiring that users write import numbers and then use numbers.Float etc., this PEP proposes a straightforward shortcut that is almost as effective: when an argument is annotated as having type float, an argument of type int is acceptable; similar, for an argument annotated as having type complex, arguments of type float or int are acceptable

It doesn't say: If I accept int and float please silently ignore the int when doing type narrowing?

IMHO this is still a bug (at least when declaring explicitly!). So I kindly ask you @ethanhs to re-open.

@ethanhs
Copy link
Collaborator

ethanhs commented May 20, 2022

Sorry, I think I was confused as to what you were expecting. Let me back up a bit more.

If I have an argument that takes Union[T1, T2], I can pass subtypes of either T1 or T2 (note that T is a subtype of itself).

However, if T2 is a subtype of T1, then Union[T1, T2] is equivalent to Union[T1], because any subtype of T2, including T2 itself, is a subtype of T1. Therefore, since in Python int a subtype of float and complex, Union[int, float] is equivalent to Union[float].

I think the real bug here is the first reveal_type should probably simplify int and float, if nothing else to be consistent.

@ethanhs ethanhs reopened this May 20, 2022
@erictraut
Copy link

erictraut commented May 20, 2022

I agree this is a bug.

@ethanhs, you're correct that PEP 484 specifies that int should be treated as a subtype of float (part of the so-called "numeric tower") for purposes of type compatibility. But the PEP doesn't specify the behavior of type narrowing. In fact, it doesn't even mention the concept of "type narrowing".

Mypy is applying type narrowing for user_in based on an isinstance check, but it's doing so in a way that does not match runtime behaviors. The call isinstance(user_in, float) will return False at runtime if user_in is an int. In the negative ("else") narrowing case, user_in should be narrowed to str | int, but mypy is incorrectly narrowing it to str. For comparison, pyright gets this right. Fixing this will probably require some special casing within the isinstance narrowing logic.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-type-narrowing Conditional type narrowing / binder
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants