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

Type issue in a protocol method if ... is missing from the body #6487

Closed
kkom opened this issue Nov 18, 2023 · 4 comments
Closed

Type issue in a protocol method if ... is missing from the body #6487

kkom opened this issue Nov 18, 2023 · 4 comments
Labels
as designed Not a bug, working as intended bug Something isn't working

Comments

@kkom
Copy link

kkom commented Nov 18, 2023

Describe the bug

Pyright finds an issue in the following code snippet:

from typing import Protocol

class Repro(Protocol):
    def foo(self) -> str:
        """docblock"""
error: Function with declared return type "str" must return value on all code paths
    "None" is incompatible with "str" (reportGeneralTypeIssues)

Things are fine if the code looks like this:

from typing import Protocol

class Repro(Protocol):
    def foo(self) -> str:
        """docblock"""
        ...

However after changes to PIE790 in Ruff v0.1.6, Ruff wants to remove the ... - resulting the Pyright error.

@zanieb from @astral-sh thinks that nothing in PEP 544 requires the ... and that Pyright shouldn't see a problem here. See astral-sh/ruff#8756 (comment)

@erictraut – what's your view? It would be great if Pyright and Ruff were to agree on what's legal here.

VS Code extension or command-line

Pyright 1.1.336 CLI

@kkom kkom added the bug Something isn't working label Nov 18, 2023
@erictraut
Copy link
Collaborator

Protocols are allowed to provide default implementations of methods, and classes that derive from these protocols inherit these implementations. A method that does not include a ... placeholder is assumed by pyright to be a method implementation, and it is type checked accordingly. This has been well established by pyright for several years now. Ruff should be updated according to this convention. I'll also respond to the thread in the ruff issue tracker.

@kkom
Copy link
Author

kkom commented Nov 18, 2023

Thank you for the quick response Eric! This makes sense, and I hope Ruff resolves it soon.

@erictraut erictraut added the as designed Not a bug, working as intended label Nov 18, 2023
@erictraut erictraut closed this as not planned Won't fix, can't repro, duplicate, stale Nov 18, 2023
@sg495
Copy link

sg495 commented Jan 19, 2024

Just making a note that this behaviour is not (yet) aligned with Pylint, which raises W2301:unnecessary-ellipsis].

@kalfa
Copy link

kalfa commented Aug 19, 2024

aligning with pylint is also important. unless it is envisioned that if one uses pyright should not use pylint or other linters.

@runtime_checkable
class MyProto(Protocol):
    """An object able to do stuff (protocol)"""

    def f1(self) -> str:
        """annoys pylint because there is an ellipsis where there is already a docstring
        pyright is ok
        """
        ...

    # annoys pylint because there is no docstring, pyright is ok
    def f2(self) -> str:
        ...
        
    def f2(self) -> str:
        """annoys pyright since there is no ellipsis and thereof it expects an implementation.
        it is considered normal with pylint
        """

the only way to make them both happy is

class MyProto(Protocol):
    """An object able to do stuff (protocol)"""

    def f1(self) -> Any:
       """both happy"""

since Any can be none I guess, and pyright is happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants