microsoft / pyright

Static Type Checker for Python
Other
13.2k stars 1.42k forks source link

`reportIncompatibleVariableOverride` when narrowing type from `ClassVar[str]` to `ClassVar[Literal["bar"]]` in subclasses #5998

Closed ITProKyle closed 1 year ago

ITProKyle commented 1 year ago

EDIT: could be related to/fixed by #5993?

Describe the bug

error: "SOME_VAR" overrides symbol of same name in class "KlsA"
    Variable is mutable so its type is invariant
      Override type "Literal['bar']" is not the same as base type "str" (reportIncompatibleVariableOverride)

The above error is received for the code provided below where I would expect no error as I narrow the type in a subclass. Or, am I misunderstanding how I should be narrowing the type for a var that should be set by subclasses and not changed after that point?

from typing import ClassVar, Literal

class KlsA:
    SOME_VAR: ClassVar[str]

class KlsB(KlsA):  # error: "SOME_VAR" overrides symbol of same name in class "KlsA"
    SOME_VAR: ClassVar[Literal["bar"]] = "bar"

I checked the same snippet on https://mypy-play.net as a sanity check and it is passing there.

VS Code extension or command-line

command-line

$ npm exec -- pyright --version 
pyright 1.1.328
erictraut commented 1 year ago

This check was added after a long and in-depth discussion. See this thread if you're interested in the details.

A similar change will likely be added to mypy in the near future. See this thread.

Would this workaround work for you?

class KlsA:
    SOME_VAR: ClassVar[str]

class KlsB(KlsA):
    SOME_VAR: ClassVar = "bar"

Here's an alternative that has the added benefit of enforcing the immutability of SOME_VAR at runtime.

N = TypeVar("N", bound=LiteralString)

class SomeVarName(Generic[N]):
    def __init__(self, name: N):
        self._name = name

    def __get__(self, object: None, owner: Any) -> N:
        return self._name

class KlsA:
    SOME_VAR: ClassVar = SomeVarName("")

class KlsB(KlsA):
    SOME_VAR: ClassVar = SomeVarName("bar")

Perhaps @mikeshardmind has other suggestions here.

ITProKyle commented 1 year ago

hmm... interesting.

My problem is that the base class is coming from a package (that i published privately) that is used in a large number of projects across the company that I work for. I maintain most of these but some are maintained by teams with less python experience so I'd like to find something that isn't intrusive - preferably something on the base class so none of the other projects need to be changed.

I'll try a few things to see what works best. Thanks for the info.

mikeshardmind commented 1 year ago

@ITProKyle I believe the following should work, and if it doesn't, there might need to be a discussion about the behavior of Literals in the type system, given that these are typing by value and not typing by type CC: @erictraut

For direct comparison, the original:

from typing import ClassVar, Literal

class KlsA:
    SOME_VAR: ClassVar[str]

class KlsB(KlsA):  # error: "SOME_VAR" overrides symbol of same name in class "KlsA"
    SOME_VAR: ClassVar[Literal["bar"]] = "bar"

and what I believe should work even if it doesn't currently

from typing import ClassVar, Literal, LiteralString, TypeVar

T = TypeVar("T", bound=LiteralString)

class KlsA(Generic[T]):
    SOME_VAR: ClassVar[T]

class KlsB(KlsA):
    SOME_VAR: ClassVar[Literal["bar"]] = "bar"

Edit: I meant to make this generic, this is what I get responding on mobile, fixed-ish, but see below

Note, no change to subclasses if this works. I believe this is fully inline with the accepted definition of LiteralString, and that having this be generic in this manner also resolves the otherwise present variance issue

Any string literal is compatible with LiteralString, as is another LiteralString. However, an object typed as just str is not. A string created by composing LiteralString-typed objects is also acceptable as a LiteralString.

erictraut commented 1 year ago

@mikeshardmind, your use of a TypeVar in this location isn't allowed by the Python type system. A TypeVar must be scoped to a function (a def statement), a class, or a type alias. In your code sample, it isn't scoped to any of the above. You could fix this by changing KlsA to be parameterized by T, as in class KlsA(Generic[T]): ..., however, the would still be illegal because type variables are not allowed in ClassVar type annotations. This is because such usage would be unsound from a type perspective since all concrete instances of the class share the same class variable.

ITProKyle commented 1 year ago

The following looks to be working (what @mikeshardmind recommended but without the TypeVar):

from typing import ClassVar, Literal, LiteralString

class KlsA:
    SOME_VAR: ClassVar[LiteralString]

class KlsB(KlsA):
    SOME_VAR: ClassVar[Literal["bar"]] = "bar"
erictraut commented 1 year ago

Hmm, that looks like a bug. It shouldn't work because Literal["bar"] is a subtype of LiteralString. If we enforce invariance, this should be disallowed.

mikeshardmind commented 1 year ago

This is because such usage would be unsound from a type perspective since all concrete instances of the class share the same class variable.

I'm not sure I agree on the unsoundness here, but if and only if the base class never assigns a value, only prescribed that one must exist of a specific type. This is a rough edge case though since as you pointed out recently, type checkers make no distinction to if a variable is ever assigned or not.

ITProKyle commented 1 year ago

Hmm, that looks like a bug.

Good to know. I'll hold off on merging that for now.

mikeshardmind commented 1 year ago

I also double-checked to make sure I wasn't forgetting anything, but there's no help provided from abc either, as there's no way to define an abstract classvar provided.

erictraut commented 1 year ago
from typing import ClassVar, Generic, Literal, LiteralString, TypeVar

T = TypeVar("T", bound=LiteralString)

class KlsA(Generic[T]):
    SOME_VAR: ClassVar[T]

class KlsB(KlsA):
    SOME_VAR: ClassVar[Literal["bar"]] = "bar"

KlsA[Literal["x"]].SOME_VAR = "x"
print(KlsA[Literal["y"]].SOME_VAR) # "x"
reveal_type(KlsA[Literal["y"]].SOME_VAR)  # Literal["y"] !!!!
erictraut commented 1 year ago

I can confirm that there's a bug in the invariance enforcement for LiteralString. This special form requires special-case logic, and I missed the invariance enforcement check. This will be fixed in the next release.

mikeshardmind commented 1 year ago

Thinking about this a bit more, how do you use this? Would it be acceptable to remove it from the base class entirely (leaving subclasses unchanged, still defining it) and type functions that accept this slightly differently to ensure that classvar exists (such as via a protocol) ?

I'm trying to see if there's a way to only touch your internal library to fix this for all of your users first, absent a mechanism to specify an abstract class variable that must be defined in subclasses, I think this is the last remaining one.

ITProKyle commented 1 year ago

how do you use this? Would it be acceptable to remove it from the base class entirely?

That's possible. The only issue there that I can think of would be losing sphinx auto documentation for the ClassVar on the base class.

Booplicate commented 6 months ago

Just found out that change broke our config system:

class Config:
    pass

class Foo:
    prop: Config

class ExtendedConfig(Config):
    pass

class Bar(Foo):
    prop: ExtendedConfig

which was totally safe thanks to typehints we had... If you had a function that accepts Foo, it could use prop as Config which is safe. If the function accepts Bar, then it could use prop as ExtendedConfig which is again safe. Now we have to type: ignore it and lose the safety we had? Cool.

I've read the linked issues and still fail to see what this tries to achieve. Maybe it was breaking some rules, but convenience is more important than formal rules, which is why typing is optional in python and we have Any. There's a thin line which I think python community crosses further with every year. If you need a strict language why even pick python in the first place.

debonte commented 6 months ago

which was totally safe thanks to typehints we had...

@Booplicate, one issue with that code is that functions accepting a Foo can set Bar.prop to Config, which I'm guessing is why the diagnostic text calls out the mutability of prop -- "Variable is mutable so its type is invariant"

class Config:
    pass

class Foo:
    prop: Config = Config()

class ExtendedConfig(Config):
    pass

class Bar(Foo):
    prop: ExtendedConfig = ExtendedConfig()

def func(foo: Foo):
    foo.prop = Config()

bar = Bar()
print(bar.prop.__class__) # <class '__main__.ExtendedConfig'>
func(bar)
print(bar.prop.__class__) # <class '__main__.Config'>
Booplicate commented 6 months ago

@debonte I believe that case could be solved with a generic for the function so you can specify covariance/contravariance. This looks like rather an odd example - overriding an attribute outside of the constructor while you're expecting there to be inheritance. I couldn't even think of that, it's unsound. However, fair, you want to catch that for safety.

Probably I could solve my problem with generics too (?), but I'd have to specify 6 of them on a single class. I'd even willing to accept that cost myself (within my library), but there's other developers which use that code in dozens of projects. I can't even suppress the warning because the flag is used for multiple issues as far as I can tell, and I'd like to catch the other one since it makes sense to me.

erictraut commented 6 months ago

@Booplicate, if you're interested in debating and influencing how the Python type system should work, the Python typing forum is the right place to do so.

Pyright is a standards-based type checker that strives to comply with the typing spec. There are aspects of the typing spec that are under-specified, but we are working to clarify these areas. Participation from the larger Python community is welcome as we discuss these issues.