False positive "is abstract" flagging when using abstract properties

[yehorb]

Environment data

Pylance language server

• Language Server version: 2020.7.3
• OS and version: Windows 10 (Version 2004)
• Python version (& distribution if applicable, e.g. Anaconda): Python 3.8.3

Pylance's Type Checking Mode is set to basic

Expected behaviour

Provided code should work just fine, with no errors reported.

# abstract_property.py

import abc

class NamedObject(abc.ABC):
    @property
    @abc.abstractmethod
    def name(self) -> str:
        pass

class Python(NamedObject):
    name = "name"

Python()

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:

> mypy .\abstract_property.py
Success: no issues found in 1 source file

And it is valid type check, because if I change name = "name" to name = 42, mypy will complain:

> mypy .\abstract_property.py
abstract_property.py:12: error: Incompatible types in assignment (expression has type "int", base class "NamedObject" defined the type as "str")
Found 1 error in 1 file (checked 1 source file)

I expect Pylance to at least recognize this snippet as a valid Python code, and at best report an issue if there are type mismatch (as mypy does).

Actual behaviour

Pylance identifies the Python class as an abstract one and reports an error:

[erictraut]

We've discussed this issue previously. It was a conscious decision not to allow an abstract property to be considered properly "overridden" by a simple instance variable. They have different behaviors. A property is an object and is not settable or deletable (unless it also has a setter and deleter). That's very different from a simple instance variable.

A type checker should flag this difference. If you define an abstract property, Pyright requires that you override it with a non-abstract property.

Incidentally, you can use the abc.abstractproperty decorator rather than a combination of property and abc.abstractmethod decorators.

[yehorb]

Okay. I can understand this decision, thank you for explaining.

And I also checked abc.abstractproperty, and It is not producing an error. At least, not this exact error.

However, I am now faced with a conundrum: for Pylance not to report an error, I need to use deprecated Python function. Because, according to Python docs, abc.abstractproperty is deprecated since version 3.3.

And Use 'property' with 'abstractmethod' instead. shows up in help and is even in a hover over abc.abstractproperty:

Using abc.abstractproperty also produces a different kind of error message: Function with declared type of "str" must return value Type "None" cannot be assigned to type "str"

Which makes even less sense in this context. Not recognizing abstract property being overwritten by class variable is understandable. But telling me that I should provide an implementation for and abstract property is outright wrong.

I would like to use Pylance typechecking in my daily workflow. It is really powerful and on-point, and I like it being somehow more strict than mypy. But issues like it make the experience much less enjoyable.

No because this behavior from Pylance is wrong. But because it contradicts the Python programming rules. Which is kind of not desirable in a Language Server for a Python language.

[erictraut]

Ah, I didn't realize that @abstractproperty was deprecated. Interesting. Yeah, then disregard that suggestion.

Pyright does suppress the return type validation in a method that is decorated with @AbstractMethod.

When you say "contradicts the Python programming rules", what are you referring to?

[yehorb]

Well, maybe "contradicts the Python programming rules" is a too strong of a statement, sorry.

I referred to your suggestion of using abc.abstractproperty, which is deprecated. I mean, using deprecated is not inherently wrong, but a bad practice nevertheless.

I also mean that using combination of property and abc.abstractmethod is the only way (well, except abc.abstractproperty) to enforce the "child classes should have this variable defined, of this exact type, and acessible without explicit function call" behavior by the interpreter itself. If use code like this, I have specific reasons not to wrap this information in a function call.

And when Pylance told me that I am wrong, I was a bit upset.

[erictraut]

The "proper" way to indicate that child classes should have a variable defined is to use a Protocol (as defined in PEP 544). This requires Python 3.8, which may not be feasible for you. Is that an option?

[yehorb]

Should really read that PEP, thank you.
But, regarding this conversation, no this is not feasible, as I need to stick to Python 3.6.
I reported 3.8.3 as my Python version because I created and tested example code using the latest available Python, but I first encountered it in a different, Python 3.6.10 codebase.

[yehorb]

There is other side of the issue: the issuer reported by Pylance is misleading at best.

Provided snippet IS a valid Python, and it is the only (at least the only I know of) way to force a presence of a variable in subclasses (prior to 3.8). Is it good or bad code is not for me to decide, but it works and forces my intention on interpreter level.

Pylance reports it as an invalid Python. Moreover, if I remove the name = "name" part and substitute it with the plain pass, the Pylance will report the same exact error:

Having abstract property being overridden by a class variable IS NOT the same as not having it defined at all. The first one leads to a runnable code, and the other leads to the TypeError: Can't instantiate abstract class Python with abstract methods name.

There should be distinction between the two.

I also looked through mypy issues regarding "abstract property" for reference.
There is an issue, which is describes almost exactly my use case. The author defined abstract property and satisfied it in a child class and mypy flagged it as incompatible with definition in base class. The mypy also accepted the PR that fixes that issue. So flagging overridden abstract property as incompatible with definition in base class considered a bug by mypy team.
Sadly, it is the only issue that mentions this topic.

Either way, the I consider the current Pylance behavior (treating the abstract property overriden with a class variable as not implemented at all) not correct.

Moreover, I think that there is no issue with overriding abstract properties with class variables.

Pylance should at least treat such case differently. Something along the lines

Cannot assign member "name" for type "Python"
  Expression of type "Literal['name']" cannot be assigned to member "name" of class "Python"
    "Literal['name']" is incompatible with "property"  # or "Callable[[NamedObject], str]"

still would be upsetting, but at least it will not be misleading.

[erictraut]

Thanks for your well reasoned and well researched response.

A few thoughts...

First, I don't automatically equate "raises a runtime exception" with "valid Python". I can provide many examples where a runtime exception is valid, and I can provide many other examples where code doesn't raise an exception but is still worthy of being flagged as incorrect. Most type violations fall into this latter category, since Python doesn't perform any runtime type checks and therefore won't raise exceptions in those cases.

Second, the behavior of mypy is interesting to consider, but there are many places where Pylance/Pyright deviates from mypy — most of them intended and for good reasons. This is a case where I think an incorrect decision was made in mypy. A property is different from a variable in important ways, and it's the job of a type checker to point out inconsistencies like this.

There are several points that I agree with you on, and I think if I address these, it will satisfy your use case. First, I agree that the diagnostic message provided by Pylance/Pyright in this case is misleading. Second, the mismatch between a property and a variable should be controllable via a diagnostic rule other than reportGeneralTypeIssues. It would logically be covered by the reportIncompatibleMethodOverride rule. This rule is disabled by default when using the "basic" type checking mode, but is on for "strict".

How does that sound to you?

[yehorb]

I agree that reporting property override with a variable as reportIncompatibleMehtodOverride is a viable solution.

On the other hand, I still believe that described case is a bit different.

reportIncompatibleMehtodOverride should be triggered if override is interface breaking, like this:

class Parent:
    def reverse(self, x: int) -> int:
        return -x

class Child(Parent):
    def reverse(self, x: str) -> str:
        return x[::-1]

This is obviously a violation of the Parent interface and should not be allowed.

With properties it is a bit more subtle.

When I do

class Parent:
    @property
    def name(self) -> str:
        return "parent"

class Child(Parent):
    name = "child"

I am not overriding a function name(self: Parent) -> str function, I am overriding a property object.

From my point of view, the Parent class defines an interface: a name attribute of type str. Either it is calculated dynamically or provided statically does not really matter: the signature is the same.

It is kind of the same with property backed by an abstractmethod -- I only define an interface, and also say that base class provides no implementation. The child classes must take care of it, ether defining a static variable or defining a property backed by a concrete method -- it is all the same interface-wise.

You should also consider what happens when the situation is reverse: overriding a variable with a property.

from typing import Optional

class StaticallyNamedObject:
    name = "my name is StaticallyNamedObject"

class DynamicallyNamedObject:
    def __init__(self, name: Optional[str] = None):
        self._name = name

    @property
    def name(self):
        if self._name:
            return "my name is " + self._name

        return "my name is " + self.__class__.__name__

    @name.setter
    def name(self, new_name: str):
        self._name = new_name

Pylance is totally fine with provided code snippet, even with strict type checking. But it should not be allowed, if we consider variables and properties being different in this regard... Or is it? Again, in terms of class interfaces, if I override the class variable with a property with getter and setter, I am kind of good to go (I know that there are also a deleter, but I omitted it intentionaly)

mypy has an open issue regarding this case. It is considered a valid solution if both getter and setter of the property are defined and have correct types.

I don't know, the more I think about it, the more confused I get. I totally agree that properties and variables are not the same.

But in terms of class interfaces they can act the same...

Even though there are cases where even in terms of defined interfaces they are not the same:

class LockedName:
    @property
    def name(self) -> str:
        return "locked"

    @name.setter
    def name(self, new_name: str) -> None:
        raise NotImplementedError

class UnlockedName(LockedName):
    name = "unlocked"

Where LockedName class explicitly states that the name could not be set, but UnlockedName overrides that constraint right away, without any complaints.

Thinking about examples like this I agree that overriding property with a variable should be reported. But not with reportIncompatibleMehtodOverride. Ideally, there should be a separate set of warnings regarding properties.
For now, at least reporting reportIncompatibleMehtodOverride is a valid solution.

[jakebailey]

This issue has been fixed in version 2020.7.4, which we've just released. You can find the changelog here: https://github.com/microsoft/pylance-release/blob/master/CHANGELOG.md#202074-29-july-2020