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.

JukkaL
JukkaL approved these changes Jan 15, 2021
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

@97littleleaf11 97littleleaf11 added this to Review in progress in mypy Feb 20, 2022
@97littleleaf11 97littleleaf11 moved this from Review in progress to Reviewer approved in mypy Feb 20, 2022
@JelleZijlstra
Copy link
Member

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

@JelleZijlstra JelleZijlstra merged commit b22c4e4 into python:master Feb 20, 2022
23 of 25 checks passed
mypy automation moved this from Reviewer approved to Done Feb 20, 2022
@BvB93 BvB93 deleted the issubclass branch Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
mypy
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants