Spurious error when overriding property with constant value in base class

[matangover]

Environment data

• Language Server version: 2021.8.0
• OS and version: macOS 10.15.7
• Python version (& distribution if applicable, e.g. Anaconda): 3.7

Expected behaviour

No warning when overriding property with constant value.

Actual behaviour

Error:

"foo" incorrectly overrides property of same name in class "Base"
  Property method "fget" is incompatible
    Return type mismatch: base method returns type "Literal[1]", override returns type "Literal[2]"
      "Literal[2]" cannot be assigned to type "Literal[1]"

Code Snippet / Additional information

class Base:
    @property
    def foo(self):
        return 1

class Derived(Base):
    @property
    def foo(self): # Error here
        return 2

Discussion

This happens for method return types as well:

class Base2:
    def foo(self):
        return 1

class Derived2(Base2):
    def foo(self): # Error here
        return 2

But does not happen for attributes. E.g.:

class Base3:
    foo = 1

class Derived3(Base3):
    foo = 2 # No error

It feels inconsistent that int is inferred for Base3.foo but not for for Base.foo and Base2.foo. You could say that Base3.foo is mutable but then it seems Base.foo is also "mutable" in a sense (when overridden).

You might suggest to annotate foo -> int but in my opinion pyright could be smart enough to not warn, at least for the property, when types are inferred.

[erictraut]

I assume that you've enabled type checking along with the "reportIncompatibleMethodOverride" rule. This is one of the strictest checks. It is off by default because it may require you to add more type information for it to be useful.

In this case, you are relying on pylance to infer the return type of foo because you haven't provided a return type annotation. Generally, pylance will infer the narrowest possible type. In this case, it's inferring Literal[1]. You can override its inference rules by providing an explicit return type annotation — for example, int or float.

In the case of attributes, pylance's inference rules do not infer a literal type unless the attribute is constant. This is consistent with the guidance set forth in PEP 586.

So this is working as intended. You should either disable "reportIncompatibleMethodOverride" if you don't feel that it's providing sufficient value, or you should add an explicit return type annotation in this case.

[matangover]

Thanks for your reply. My use case is that I'm overriding a function from a base class that is in a third-party library, I do not control the annotations of that library. I do want reportIncompatibleMethodOverride on because it's useful for me in other cases. I still think inferring the literal here is wrong.

Regarding the guidance you referred to in PEP 586:

An alternative strategy that does maintain compatibility in every case would be to always assume expressions are not Literal types unless they are explicitly annotated otherwise. A type checker using this strategy would always infer that x is of type str in the first example above.
This is not the only viable strategy: type checkers should feel free to experiment with more sophisticated inference techniques. This PEP does not mandate any particular strategy; it only emphasizes the importance of backwards compatibility.

I think this is what you were referring to. This seems to support my suggestion of not inferring literal, in the interest of compatibility with valid unannotated code.

Also I notice that TypeScript does not infer literal - the following gives no error:

class Hi {
    foo() {
        return 1;
    }
}

class Hi2 extends Hi {
    foo() {
        return 2;
    }
}

[erictraut]

Which third-party library are you using? Is it properly typed? If not, I wouldn't recommend using the strictest diagnostic checks. These checks rely on complete and accurate types, and you're likely to see many false positives if you attempt to use them.

Type inference heuristics involve many tradeoffs. We've found that in most cases, it's best to infer the narrowest type. This results in the most accurate type checking, and it eliminates a class of false positive errors. But in some cases, inferring the narrowest type can result in false positives. We've tuned the inference heuristics in pylance to try to achieve the best balance in most Python sources. If we were to change one of these heuristics at this point, it would be a breaking change — not something we'd do without evidence that it would provide significant value.

[matangover]

Which third-party library are you using? Is it properly typed? If not, I wouldn't recommend using the strictest diagnostic checks. These checks rely on complete and accurate types, and you're likely to see many false positives if you attempt to use them.

The library is untyped. I don't think disabling strict diagnostic checks is a good recommendation. The checks are useful to me in other parts of my code. They are only problematic when combined with wrong type inference. A better workaround for me would be to patch the library or add an ignore comment in my code.

Type inference heuristics involve many tradeoffs. We've found that in most cases, it's best to infer the narrowest type. This results in the most accurate type checking, and it eliminates a class of false positive errors.

Can you give an example of a false positive error eliminated in this case by inferring Literal? I cannot think of a use case where inferring Literal[1] gives any advantage. As shown in the PEP 586 excerpt you referred to yourself, it results in false positives. If anything, maybe a union of Literals could be usefully inferred.

If we were to change one of these heuristics at this point, it would be a breaking change

OK, you might agree with my points but choose not to implement them due to this difficulty. But you seem to not agree with my points anyway?

evidence that it would provide significant value

Eliminating false positives

[midapsh]

You can find similar behavior in libraries like pika. For example:

import pika
# Boilerplate
p_rabbit = pika.credentials.PlainCredentials(
    "YOUR RABBIT LOGIN",
    "YOUR RABBIT PASSWORD",
)
params = pika.ConnectionParameters(
    host="YOUR RABBITMQ HOST",
    port="YOUR RABBITMQ PORT",
    virtual_host="YOUR RABBITMQ VIRTUAL PORT",
    credentials=p_rabbit,
)

#######################
# Here is the example #
#######################
params.socket_timeout = 10

#################
# Error message #
#################
# Cannot assign member "socket_timeout" for type 
# "ConnectionParameters"PylancereportGeneralTypeIssues
# (property) socket_timeout: Literal[10]

I agree with @erictraut:

Which third-party library are you using? Is it properly typed? If not, I wouldn't recommend using the strictest diagnostic checks.

The library is not properly typed and it's not possible(maybe?) to suggest a single answer to the typing problem.

[erictraut]

If you've enabled type checking, we also recommend disabling the useLibraryCodeForTypes option. This is on by default in pylance. It generates type information that's good enough to support type completions during editing but not for type checking. It's targeted at (the large majority of) pylance users who are not interested in type checking. This option tells pylance to not attempt to infer types from libraries that are not py.typed and instead treat symbols imported from them as Unknown. As I mentioned above, you won't be able to use the strictest type checking modes here, but you should see fewer false positives like the one you're reporting here. Other type checkers like mypy never attempt to infer types from libraries that are not py.typed.

[matangover]

I totally agree that useLibraryCodeForTypes and type checking don't go together. In any case, my report isn't specific to libraries, it's true for any code. It seems pointless to discuss it any further because as the PEP says there is not always a clear correct answer wrt inference. I gave an example where the current behavior leads to false positives. You didn't show an example where the current behavior is useful. Changing the behavior may cause backwards incompatibility.

[matangover]

@jakebailey is this closed as fixed or wontfix?

[jakebailey]

My impression was that this discussion had hit a terminus, where changing this behavior was undesired/potentially incompatible. I'm willing to be wrong.

[matangover]

I summarized the discussion in my last post, we haven't reached an agreement... If you want to close it as wontfix that's your choice but I personally was not convinced it is the right one.