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

Protocols and ... if ... else ... #5392

Closed
srittau opened this issue Jul 26, 2018 · 9 comments
Closed

Protocols and ... if ... else ... #5392

srittau opened this issue Jul 26, 2018 · 9 comments
Labels

Comments

@srittau
Copy link
Contributor

srittau commented Jul 26, 2018

python/typeshed#2323 exposed a problem with mypy's (lack of) handling of protocols in ... if ... else ... constructs. Please consider:

from typing import SupportsFloat

class Floaty:
    def __float__(self): ...

class Supporter(SupportsFloat):
    def __float__(self): ...

reveal_type(Floaty() if bool() else 0)
reveal_type(Supporter() if bool() else 0)

This will reveal the type of the first expression to be builtins.object, while the latter is typing.SupportsFloat. This means that mypy (tested with 0.620, Python 3.7.0) will reject, for example float(Floaty() if bool() else 0).

@srittau
Copy link
Contributor Author

srittau commented Jul 26, 2018

On further thought, I wonder why mypy tries to infer the base type for if ... else ... constructs, instead of using an Union like with or. This makes it reject other valid code like this:

class Foo:
    def x(self) -> None: ...
    
class Bar:
    def x(self) -> None: ...
    

o = Foo() if bool() else Bar()
o.x()

@srittau
Copy link
Contributor Author

srittau commented Jul 26, 2018

Continuing my train of thoughts: Changing if ... else ... to return an Union would make the following code not check:

class Base: ...
class Foo(Base): ...
class Bar(Base): ...
class Baz(Base): ...

x = Foo() if bool() else Bar()
...
x = Baz()

Although I'd argue that in this case an explicit annotation for x is better style anyway.


Strangely enough, the following code does type check:

from typing import Union

def foo(x: Union[int, str]) -> None: ...

foo("" if bool() else 0)

while the following does not:

"" if bool() else 0
foo(x)

@gvanrossum
Copy link
Member

Thanks for opening this issue.

There's been discussion about this. There are pros and cons for returning unions for conditional expressions. It's my day off and I don't recall where the larger discussion lives. :-(

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 30, 2018

We looked at changing if/else expressions to always have union types, but it caused a lot of issues with code that relied on the existing semantics. Instead, we only modified it to infer a union type if the type context is a union type.

Similarly, we could infer a union/protocol type when the type context is a protocol type. Example:

from typing_extensions import Protocol
class P(Protocol):
    x: int
class A:
    x: int
class B:
    x: int
def f(p: P) -> None: pass
f(A() if bool() else B())  # Should probably be okay
y: P = A() if bool() else B()  # Should probably be okay

This change wouldn't directly fix the original issue, but it would at least provide a somewhat simple workaround:

from typing import SupportsFloat

class Floaty:
    def __float__(self): ...

x: SupportsFloat = reveal_type(Floaty() if bool() else 0)  # Would be SupportsFloat

This would clearly still be a compromise, but marginally better than what we currently have. It would also be consistent with how type inference issues are generally resolved -- by adding explicit type annotations. The fundamental problem is that sometimes there are multiple reasonable types we could infer, and we usually try to pick one of the options that we expect to be usually (but not always) a good one.

@srittau
Copy link
Contributor Author

srittau commented Jul 30, 2018

This compromise seems reasonable, although I'd argue that a "correct" solution would be better. I understand that that means changes to existing code bases, but I feel that typing and mypy are still in semi-experimental states (the typing module is still provisional, for example). Decisions that are made now will affect every Python user (that opt to use type hints) for the next decades.

This particular case irks me not only because of the practical problem discussed in the referenced issue, but also because of the inconsistency with or, which is used in many similar situations:

from typing import Protocol

class P(Protocol):
    x: int
class Foo:
    x: int
class Bar:
    x: int

reveal_type(Foo() or Bar())  # Union[foo.Foo, foo.Bar]
reveal_type(Foo() if bool() else Bar())  # builtins.object

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 30, 2018

I agree that this is something that would be nice to fix, but the core team doesn't have the bandwidth to deal with the fallout right now :-(

The mypy fix is actually trivial, but fixing our internal Dropbox codebases would be a lot of work. The situation would likely be similar for many other mypy users with large codebases.

Another alternative would be to allow the old behavior to be retained with a configuration option.

@mthuurne
Copy link
Contributor

I encountered this issue and made a test case when figuring out whether my code was incorrectly hinted or mypy was rejecting it unnecessarily. Perhaps the test case will be useful in implementing or discussing the handling of conditional expressions.

from typing_extensions import Protocol

class Unary(Protocol):
    def process(self, n: int) -> int:
        pass

class Square:
    def process(self, n: int) -> int:
        return n ** 2
    
class Negate:
    def process(self, n: int) -> int:
        return -n
    
def f(cond: bool) -> Unary:
    return Square() if cond else Negate()

def g(cond: bool) -> Unary:
    if cond:
        return Square()
    else:
        return Negate()

What is interesting about this test case, is that for f mypy reports "Incompatible return value type", but for g it doesn't report any errors, while the two functions are equivalent in execution.

@FFY00
Copy link
Member

FFY00 commented Aug 14, 2020

I would be open to work on this issue if someone can give me some pointers 😊

@hauntsaninja
Copy link
Collaborator

Fixed by #17427

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

No branches or pull requests

7 participants