False negatives on protocols with `ClassVar` attributes

[AlexWaygood]

Describe the bug

The following snippet of code should cause a type checker to emit an error. However, pyright sees no issue with it:

from typing import ClassVar, Protocol

class ClassVarProto(Protocol):
    x: ClassVar[str]

class Foo:
    x: ClassVar[str] = "x"

def expects_ClassVarProto(arg: ClassVarProto) -> None: ...

expects_ClassVarProto(Foo())  # should succeed
expects_ClassVarProto(Foo)  # should fail (the object passed in has type `type[ClassVarProto]`, not `ClassVarProto`)

Mypy correctly emits an error on the second call:

error: Argument 1 to "expects_ClassVarProto" has incompatible type "Type[Foo]"; expected "ClassVarProto"  [arg-type]
note: ClassVar protocol member ClassVarProto.x can never be matched by a class object

I've reproduced this on pyright 1.1.284. I discovered the bug in my typeshed PR python/typeshed#9362: several tests I added for that PR pass on mypy, but fail on pyright. (FYI I also discovered an edge case in that PR where pyright appears to have the correct behaviour, but mypy is buggy.)

[erictraut]

I'm not convinced this should be an error. A class instance can conform to a protocol, but so can a class or a module. I think this example demonstrates a case where a class meets the requirements of the protocol and should therefore be considered type compatible. The protocol indicates that the type must have a writable attribute called x that is of type str. The class Foo meets that requirement. Likewise, if you had a module that defined a top-level variable called x with type str, that module would be compatible with the protocol.

Is there something in PEP 544 that speaks to this case and explicitly excludes it from the normal protocol checks? I couldn't find anything, but I may have missed it.

[erictraut]

@AlexWaygood, I'm going to close this for now. If you still think it's a bug, let me know and I'll reconsider.

[AlexWaygood]

Thanks for the response @erictraut -- sorry I've been slow getting back to you. Been a little under the weather the last few days.

---

I still believe this is a bug, though I agree that it seems a little "underspecced". The only relevant passage I can find in PEP 544 is the following:

To distinguish between protocol class variables and protocol instance variables, the special ClassVar annotation should be used as specified by PEP 526. By default, protocol variables as defined above are considered readable and writable. To define a read-only protocol variable, one can use an (abstract) property.

I agree that a class or a module can conform to a protocol, but I disagree that the class Foo, in this case, conforms to the interface described by the protocol ClassVarProto. I would argue that an object x should only be considered to conform to a protocol y where y has a ClassVar member z if the type of the object x also has a ClassVar member z. Simply having a member z is insufficient for the object x to be considered to conform to the protocol y, in my view: a ClassVar is a distinct kind of variable that should be treated specially. I would argue PEP 544 supports my reading in the paragraph I quoted above: it specifically mentions that a property is the way to declare a read-only protocol variable, as opposed to a ClassVar (which is read-only on the instance, but not the class).

As an example, in the class Eggs below, the use of ClassVar signals to the type checker that the scrambled attribute should be considered by the type checker to be read-only on instances of Eggs, but read/write on the class Eggs itself:

class Eggs:
    scrambled: ClassVar[bool] = True

At runtime, all this behaviour checks out: the attribute is readable on instances, and is read-write on the class itself:

>>> e = Eggs()
>>> e.scrambled
True
>>> type(e).scrambled
True
>>> type(e).scrambled = False
>>>

But we don't get the same behaviour at all from the class Eggs itself, due to the fact that from the perspective of the class, scrambled is an instance variable, not a class variable (for it to be a class variable, the attribute would have to be present on the type of Eggs, which is the class type itself):

>>> e = Eggs
>>> e.scrambled
True
>>> type(e).scrambled
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'type' has no attribute 'scrambled'
>>> type(e).scrambled = False
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot set 'scrambled' attribute of immutable type 'type'

[erictraut]

In your second experiment above, e is assigned a value of Eggs. That means the type of e is type[Eggs], and the type of the expression type(e) is type[type[Eggs]].

The question we're trying to determine here is not whether type[type[Eggs]] is compatible with the protocol but whether type[Eggs] is. To test that against runtime assumptions, we should modify your second experiment as follows:

>>> e = Eggs
>>> e.scrambled
True
>>> e.scrambled = False
>>>

This works without any exceptions, as you'd expect.

[hauntsaninja]

Here is an example of a program that fails at runtime, but for which pyright does not issue a warning:

from typing import ClassVar, Protocol, reveal_type

class ClassVarProto(Protocol):
    x: ClassVar[str]

class Foo:
    x: ClassVar[str] = "x"

def expects_ClassVarProto(arg: ClassVarProto) -> None:
    reveal_type(type(arg).x)  # pyright and mypy reveal str

expects_ClassVarProto(Foo())  # succeeds
expects_ClassVarProto(Foo)  # fails

I agree that ClassVar's interaction with Protocol is underspecified, and typing of types is complicated and somewhat unsound anyway, so fine if this is a won't fix.
That said, I think I still prefer mypy's stricter behaviour here... If someone's gone to the effort of using ClassVar it seems useful to help differentiate classes from instances. If you do still think is a won't fix, I'd also be curious for your opinions on Alex's typeshed PR: https://github.com/python/typeshed/pull/9362/files

[erictraut]

By your argument, if the ClassVarProto protocol doesn't include a ClassVar but the class Foo does, then type[Foo] should match the protocol?

I think this is arguable. As you say, it's underspecified.

I think I'll leave the current behavior as is. I see this as being an extreme edge case anyway. It's unlikely that anyone will ever hit this case in practice. I will consider revisiting this decision if we get signal to the contrary.

[AlexWaygood]

By your argument, if the ClassVarProto protocol doesn't include a ClassVar but the class Foo does, then type[Foo] should match the protocol?

Yes, I'd agree with that.

It's unlikely that anyone will ever hit this case in practice.

Well, to be clear, I only opened this issue because I hit this edge case while trying to write tests for my typeshed PR :) but sure, I agree it's something of an edge case.

For my typeshed PR, I guess I'll comment out the tests that fail on pyright for now.

[AlexWaygood]

Thanks for your time and the discussion @erictraut — always appreciated!