Pyright: Structural Type Checking -- Protocol bug

[kai-tub]

Describe the bug
The argument can't be assigned to bound variable, which is defined via structural type checking by inheriting from Protocol. PyRight highlights this as an error, but it shouldn't be.

To Reproduce
Simply add the following code to a python 3.7 file:

from typing import TypeVar, Any, Sequence
from typing_extensions import Protocol

class Comparable(Protocol):
    def __eq__(self, other: Any) -> bool:
        ...

C = TypeVar("C", bound="Comparable")

def contains(sequence: Sequence, key: C) -> bool:
    for elem in sequence:
        if elem == key:
            return True
    return False

if __name__ == "__main__":
    print(contains([1, 2, 3, 4], 4))
    print(contains(["a", "b", "c"], "a"))

Expected behavior
I would expect the tool to understand the structural type, as pyright also aims to hint these types. As a side note, I would also assume pyright to not highlight the -> bool part of
the Comparable class, as the ellipsis is the recommended way to create a method stub.

Screenshots or Code
Here is a screenshot of the error message under VS Code:

VS Code extension or command-line
As one can see, I am running it on VS Code 1.38.1 with the pyright extension 1.0.62.

Additional context
I am very new to type hinting in python and this was one of the first tests with structural typing. Mypy returns no errors. I also tried to use Python3.8, which allows one to use Protocol from the standard lib typing, but it didn't change the results.

PS: Thank you for your hard work, I really like this project!

[erictraut]

The problem is that you've specified a bound type of "Comparable", which is a literal str value. If you change "Comparable" to Comparable (remove the quotes), pyright will do what you expect. In contexts where a type annotation is used, you can place the type name in quotes (which is needed to support forward declarations), but in this particular instance, you're not using the type in the context of an annotation, you're using it as an argument to a call, so it can't be specified as a string. The bound parameter is allowed to be either a class or an object. In this case, you've specified a str object, which is why pyright doesn't complain when you specified the bound argument as a string literal.

As for the ellipses, pyright will ignore this error if it's in a type stub (".pyi") file, but it validates the return value for functions found within a ".py" file. I guess I could relax that rule if the entire implementation of a function consists of an ellipses only (or a doc string plus an ellipses).

[kai-tub]

First of all, thank you for the fast answer!
For the *.pyi file: As in my case, me as a beginner with type annotations am confused when I simply put it in the original python file for testing purposes. If you think it should be in an extra file, maybe pyright could also give this as a hint? I was simply looking into PEP544 for an example
where, for obvious reasons, everything is written together.

But if I remove the quotes, I get the following error at the top of the file:
An internal error occurred while performing type analysis

Regarding the quotes as an argument, I simply looked at an example from PEP544, where the example with a bound type also used quotes around the class. As seen here https://www.python.org/dev/peps/pep-0544/#self-types-in-protocols

After looking back at PEP484 https://www.python.org/dev/peps/pep-0484/#annotating-instance-and-class-methods does this line

Note that some type checkers may apply restrictions on this use, such as requiring an appropriate upper bound for the type variable used

explain the difference?

EDIT: It did work in the minimal-example, but not in my original code. I will try to get the next minimal-example as soon as possible

EDIT2: Ok, so this is the minimal example, which introduces the internal error:

from typing_extensions import Protocol
from typing import TypeVar, Any, Sequence

C = TypeVar("C", bound=Comparable)

class Comparable(Protocol):
    def __eq__(self: C, other: Any) -> bool:
        ...

def contains(sequence: Sequence, key: C) -> bool:
    for elem in sequence:
        if elem == key:
            return True
    return False

if __name__ == "__main__":
    print(contains(["a", "d", "e", "f", "z"], "f"))  # True
    print(contains(["john", "mark", "ronald", "sarah"], "sheila"))  # False

The difference is that I've introduced the type variable C in the Comparable class, which is mentinoed as an example in PEP544 https://www.python.org/dev/peps/pep-0544/#self-types-in-protocols and in the already mentioned PEP484 https://www.python.org/dev/peps/pep-0484/#annotating-instance-and-class-methods, which was why I used the string representation in the first place.

I assume there is a recursive look-up leading to the error.

[erictraut]

Thanks again for reporting the issue. There are three issues here:

1. I fixed the bug that caused the internal error. This will be addressed in the next release of pyright.
2. I added logic to detect empty function implementations (...) and suppress the error about a missing return value. This will be in the next version also.
3. You have some bugs in your code. You can't use Comparable before it is declared. You should move it below the declaration of Comparable. There's no need to provide a type C for self because the type of self is always implied.

[kai-tub]

Great!
Thanks for your hard work. :)

Yes, I did know that self always implies the type, but I was just following a given code template, where everything was given explicitly.

I was double-checking everything with mypy and mypy was returning the following error,
if I moved the declaration of Comparable after the class:
py_right_bug.py:6: note: Forward references to type variables are prohibited

And pyright will remain to expect the class and not a possible string representation of the class for the bound type, correct?

EDIT:
Sry, for doing another edit, but what happens for example with a function like __lt__(self, other: C), will this introduce an error or were you just pointing out that self implies the type?

[erictraut]

That's correct. Pyright supports string-based type annotations in all locations where a type annotation is expected. In other places (e.g. when passing a type as a parameter to a function), it will treat strings as strings. While it appears that mypy may have special-cased the "bound" parameter in this particular case, adding such a hack would require a pretty substantial change to the pyright code, and I can make a strong argument for it being the wrong behavior.

[kai-tub]

You answered so fast, maybe my last edit was too late for you to see.

May I ask what the behavior of the next pyright version is when the type variable C is declared after the stub, which for example uses a function like __lt__(self, other: C)

[erictraut]

This will produce an error if C is defined after the prototype class declaration because an attempt to execute that code will result in an exception. That's because the python interpreter doesn't support forward references.

However, you don't need to use the "C" TypeVar in this case. You can simply use "Comparable" as in the following. Note that it needs to be quoted in this case because it's considered a forward reference.

class Comparable(Protocol):
    def __eq__(self, other: 'Comparable') -> bool:
        ...

[kai-tub]

Oh okay, I see. Yes, that makes a lot of sense.

Like I wrote in the beginning, I am still quite new to type hinting, so thank you very much for answering my questions! :)
I am looking forward to the next revision of pyright. :)

[erictraut]

These issues are now fixed in pyright 1.0.63, which I just published.