reportIncompatibleVariableOverride regression when using abstract properties

[askurihin]

I found a good description and discussion in the pylance issue:

I use @Property in combination with @AbstractMethod to show that child classes should have a typed property defined. I usually use it to indicate the necessary class variables, such as "name", "id", "version" etc. It is valid Python, and mypy has no issues with this code

In the recent version (1.1.193) of pyright there is no way to turn off this check.

in [email protected] this reporting became reportIncompatibleMethodOverride:

12:5 - error: "test" incorrectly overrides property of same name in class "Interface" (reportIncompatibleMethodOverride)

in [email protected] this reporting became reportGeneralTypeIssues:

test.py:12:12 - error: Expression of type "Literal['hello']" cannot be assigned to declared type "property"   "Literal['hello']" is incompatible with "property" (reportGeneralTypeIssues)

Expected behavior
Error is reported as reportIncompatibleMethodOverride.

Code to reproduce

import abc

class Interface(abc.ABC):

    @property
    @abc.abstractmethod
    def test(self) -> str:
        ...


class Implementation(Interface):
    test = 'hello'

[erictraut]

The behavior in this case hasn't changed. I confirmed that the same error is reported across all of those versions.

The problem is that you are not redeclaring test (specifying a new type) in Implementation. Instead, you're simply assigning it a value with no type annotation. If you declare its type to be str, you will no longer see a reportGeneralTypeIssues error. You will still receive a reportIncompatibleMethodOverride error if that check is enabled.

[xliucs]

@erictraut Can you pleae provide an exmaple of this if we don't want to see the reportIncompatibleMethodOverride error? We just want to construct a class on an interface and modify a variable. Thanks!

[erictraut]

The reportIncompatibleMethodOverride check verifies that any overrides are compatible with the base class. If you want to leave this check enabled and don't want to see an error, you'll need to provide a compatible override in your child class. Since the base class defines test as a read-only property, you would need to declare a read-only property in the child class as well.

class Implementation(Interface):
    @property
    def test(self):
        return 'hello'

[xliucs]

The reportIncompatibleMethodOverride check verifies that any overrides are compatible with the base class. If you want to leave this check enabled and don't want to see an error, you'll need to provide a compatible override in your child class. Since the base class defines test as a read-only property, you would need to declare a read-only property in the child class as well.

class Implementation(Interface):
    @property
    def test(self):
        return 'hello'

That worked! Thanks so much, Eric!

[jjmachan]

hey @erictraut I still think pyright throwing reportIncompatibleMethodOverride is a bug from Pyright. The solution you suggested defeats the purpose of having abstract properties right? I I think ideally pyright should be able to detect that this is a special case of using abstract properties and not throw the error

I tested this solution with mypy and it works

import abc

class Interface(abc.ABC):

    @property
    @abc.abstractmethod
    def test(self) -> str:
        ...


class Implementation(Interface):
    test: str = 'hello'  # this is how I want others to use the Interface

The user of Interface ABC should not have to reimpliment a class that just return a str to fix this bug right? I would love to hear your thoughts

[erictraut]

The semantics for a property and and an attribute differ. They're similar, but not the same. If you access a property through the class (as opposed to a class instance), you will get back a property. If you access an attribute through a class, you'll get the attribute. If a type checker were to allow this overload, it would be unsafe from a type checking perspective.

In your particular example, it's doubly problematic because the ABC would seem to indicate that the value test must be read-only, but the implementation has implemented it as writable.

There is a new governance process for typing (defined in PEP 729) that aims to formalize the Python typing spec and reduce inconsistencies between type checkers. This is a good example of such an inconsistency. I happen to be a member of the initial five-person Typing Council that PEP 729 establishes. I have been gathering a long list of items to discuss in that forum. This particular issue is already on my list because I know it's a pain point for users of pyright and mypy. Once this aspect of the typing spec is clarified, pyright (and/or mypy) will be modified as necessary. Until then, I'm going to keep pyright as is because I think its current behavior is justified.

[sterliakov]

@erictraut sorry for bumping an old issue, feel free to ignore this. If this is already being discussed under PEP-729 governance, please point me to the right place - I strongly believe that mypy behaviour is correct here.

I have read your justification above and do believe that it makes sense. However, property on protocols is given a special meaning by PEP-544 (link to the section). To quote,

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.

Basically this means that a class that inherits from a typing.Protocol (and, arguably, abc.ABC) and defines such a property expects a PEP544-conformant behaviour for subtypes: anything that provides an attribute with this name, at least read only, should be treated as a subtype as long as prop types are assignable.

This was brought in this SO question.

Here's a mypy guidance section that illustrates exactly such application of PEP-544 protocol with respect to read-only vs writeable attribute distinction in protocols: https://mypy.readthedocs.io/en/stable/protocols.html#invariance-of-protocol-attributes.

[erictraut]

@sterliakov, you are correct that PEP 544 (and the typing spec) document a special case for protocol classes. Pyright honors this behavior. If you see a place where it is not doing so, please report it as a bug. I'll note that the typing spec indicates no such behavior for ABCs, which are different from (but related to) protocols.

When it comes to override enforcement for properties defined in non-protocol classes, the typing spec is currently silent on how type checkers should operate. I'd like to see that specified so type checkers can be consistent. I'm not yet convinced that mypy's behavior is correct here, but we should have that discussion. This isn't the right forum, however. It should be discussed in the typing forum, and I'd prefer if it was discussed as part of a broader discussion about override rules.

There has been good progress over the past six months in filling in missing pieces of the typing spec and implementing a conformance test suite for type checkers, but we still have quite a bit of work left. By my count, there are at least ten more chapters that need to be added to the typing spec. One of these should cover class override behaviors in detail. Once that chapter is written, reviewed, and approved by the five-person typing council (of which I'm currently a member), then I'll modify pyright's behavior to conform to it if changes are needed.

Another related topic is the typing.ReadOnly special form which was recently added to the type system as part of PEP 705. Currently, ReadOnly is allowed only in TypedDict definitions, but I think it's worth exploring its use more broadly, including in ABCs and protocol definitions.

[sterliakov]

Ough, this looks like a deeper inconsistency than I noticed initially. Pyright recognizes the subtyping relationship correctly when used somewhere where a protocol is expected, but rejects explicit inheritance. Should I report it as a new ticket?

The following passes (snippet from PEP-544):

from typing import Protocol 

class Box(Protocol):
    @property
    def content(self) -> object: ...

class IntBox:
    content: int

def takes_box(box: Box) -> None: ...

takes_box(IntBox())

On the other hand, the following fails:

from typing import Protocol 

class Box(Protocol):
    @property
    def content(self) -> object: ...

class IntBox(Box):
    content: int  # E: "content" incorrectly overrides property of same name in class "Box"  (reportIncompatibleMethodOverride)

However, explicit protocol inheritance should be interpreted as an extra indication of compatibility, and definitely not cause something compatible without inheritance to become incompatible, if I understand this correctly.

[erictraut]

...but rejects explicit inheritance. Should I report it as a new ticket?

No, this is the intended behavior in pyright currently. The typing spec is not clear on what should happen in the case of explicit inheritance. I think pyright's behavior is defensible, but it might need to change once we reach consensus in the typing community about the spec. I don't plan to make changes to pyright's behavior until we reach consensus because I don't want to create unnecessary churn for current pyright users. If this needs to change, I want to change it only once.