microsoft / pyright

Static Type Checker for Python
Other
13.37k stars 1.46k forks source link

pydantic field overrides from optional to required marked as errors as of 1.1.359 #8766

Closed devanubis closed 2 months ago

devanubis commented 2 months ago

Describe the bug

pyright>=1.1.359 flags a pydantic subclass overriding a field from optional to required as invalid.

This was valid in 1.1.158, and for non-pydantic classes and data-classes is still also valid.

Code or Screenshots

requirements:

pydantic==2.8.2
pyright==1.1.139
import pydantic

class Foo:
    foo: int
    bar: int | None = None

class Bar(Foo):
    bar: int  # valid

class Fiz(pydantic.BaseModel):
    fiz: int
    buz: int | None = None

class Buz(Fiz):
    buz: int  # invalid in pyright >= 1.1.359

The override of buz: int generates a pyright error:

error: "buz" overrides a field of the same name but is missing a default value (reportGeneralTypeIssues)

As a work-around, I can override buz with:

class Buz(Fiz):
    buz: int = pydantic.Field(default=...)

pyright accepts that as valid, and pydantic still treats the field as required at runtime. But it's a bit annoying to have to explicitly trick or # type: ignore these field overrides.

LeeeeT commented 2 months ago

This is not a bug. Both Bar and Buz override the parent's attribute in an incompatible way. Attributes of a class are mutable by default, and therefore invariant (meaning you cannot change their type nor remove the default value). Allowing this would hide the bugs caused by violating the Liskov substitution principle.

I ran pyright on your code example and it correctly emits an error.

erictraut commented 2 months ago

Pyright reports two errors in your code.

First, you are attempting to override a mutable field with an incompatible type. These classes are not frozen, so their fields are mutable. That means the fields' types are invariant, not covariant. That means a subclass that overrides the base class cannot change the type of a field without violating the Liskov Substitution Principle.

Here's a code sample that shows why this is unsafe.

from typing import reveal_type
import pydantic

class Fiz(pydantic.BaseModel):
    fiz: int
    buz: int | None = None

class Buz(Fiz):
    buz: int

def clear_buz(f: Fiz):
    f.buz = None

b = Buz(fiz=1, buz=1)

clear_buz(b)

reveal_type(b.buz)
print(b.buz)

This is a real type error, and pyright is correct for reporting it, so I don't consider this a bug.

Let's fix that issue in the code by marking both Fiz and Buz as frozen.

class Fiz(pydantic.BaseModel, frozen=True):
    fiz: int
    buz: int | None = None

class Buz(Fiz, frozen=True):
    buz: int

That eliminates one of the two errors, but it leaves the other: "buz" overrides a field of the same name bug is missing a default value.

This error comes from a check that was introduced in response to this issue. It is in more of a gray area in terms of the typing spec. As noted in that bug report, the fact that pyright previously didn't report an error here could lead to false negatives (runtime errors that are not caught statically).

For example, the following program crashes:

import pydantic

class Fiz(pydantic.BaseModel, frozen=True):
    fiz: int
    buz: int | None = None

class Buz(Fiz, frozen=True):
    buz: int

Buz(fiz=1)

So, I think pyright is justified in generating a type error for this second condition.

devanubis commented 2 months ago

Apologies @LeeeeT and @erictraut , I hadn't realised the project I was working on (and testing the minimal example) was using basic rather than strict checking. You're right, the base-classes also raise an error.

I don't particularly want to separate our structures out into additional superclasses to share common fields and to separate the optional and required variants, but it may be the better option long-term.

Thanks for explaining.

erictraut commented 2 months ago

You may want to use standard type checking rather than either basic or strict. The standard type checking mode includes all of the checks mandated by the Python typing spec.