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

Fix an issubclass failure for protocols with overloaded methods #9904

Merged
merged 2 commits into from
Feb 20, 2022

Conversation

BvB93
Copy link
Contributor

@BvB93 BvB93 commented Jan 12, 2021

Description

Mypy currently rejects issubclass checks for protocols containing non-method members.
What I noticed is that this includes overloaded functions as well, the latter being incorrectly
identified as a "non-method" due to a missing isinstance(typ, mypy.types.Overloaded) check.

This PR attempts to remedy aforementioned issue.

Examples

The issubclass behavior prior to this PR:

from typing import Protocol, overload, Sequence, runtime_checkable

@runtime_checkable
class A(Protocol):
    @overload
    def get_item(self, a: int) -> int: ...
    @overload
    def get_item(self, a: slice) -> Sequence[int]: ...

# test.py:10:1: error: Only protocols that don't have non-method members can be used with issubclass()  [misc]
# test.py:10:1: note: Protocol "A" has non-method member(s): get_item
# Found 1 error in 1 file (checked 1 source file)
issubclass(int, A)

Test Plan

A new issubclass test has been added for overload-containing protocols.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

Left an (optional) request to update the test case, otherwise looks good.

def meth(self, a):
pass

reveal_type(issubclass(int, POverload)) # N: Revealed type is 'builtins.bool'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a concrete class (e.g., COverload) that has a suitable overloaded method, and test that issubclass can be used to narrow down from Type[Union[COverload, E]], similar to above around line 2218?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So while trying the add the test you proposed I seem to have stumbled across another bug.

The if block successfully narrows down the Union to Type[COverload] but the else block
on the other hand fails to do the same with E, revealing the type as the original Union.

Would you happen to know where in mypy the type inference of if / else statements is handled?
I imagine that change similar to non_method_protocol_members() will have to be made there,
but I'm not quite sure where to look.

Examples

class COverload:
    x: str
    @overload
    def meth(self, a: int) -> float: ...
    @overload
    def meth(self, a: str) -> Sequence[float]: ...
    def meth(self, a):
        pass

cls_overload: Type[Union[COverload, E]]
if issubclass(cls_overload, POverload):
    reveal_type(cls_overload)  # N: Revealed type is 'Type[__main__.COverload]'
else:
    reveal_type(cls_overload)  # N: Revealed type is 'Type[__main__.E]'

The test output:

Expected:
  ...
  main:45: note: Revealed type is 'Type[__main__.COverload]'
  main:47: note: Revealed type is 'Type[__main__.E]' (diff)
Actual:
  ...
  main:45: note: Revealed type is 'Type[__main__.COverload]'
  main:47: note: Revealed type is 'Union[Type[__main__.COverload], Type[__main__.E]]' (diff)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please open a new issue about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please open a new issue about it?

I just created an issue at #12228.

@hauntsaninja hauntsaninja reopened this Aug 4, 2021
@97littleleaf11 97littleleaf11 changed the title BUG: Fix an issubclass failure for protocols with overloaded methods Fix an issubclass failure for protocols with overloaded methods Nov 19, 2021
@97littleleaf11 97littleleaf11 reopened this Jan 5, 2022
@97littleleaf11
Copy link
Collaborator

Passes all the tests now. Shall we merge this one? @hauntsaninja

@JelleZijlstra
Copy link
Member

Jukka already approved it, so let's land it.

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.

5 participants