mypy has implemented pep-681 incorrectly and does not intepret descriptors correctly

[zzzeek]

Hi -

Now that mypy has pep-681 support, they are making a mistake that we spent a lot of time with the pyright people working out.

This SQLAlchemy mapping and usage is valid:

from __future__ import annotations

from sqlalchemy.orm import DeclarativeBase
from sqlalchemy.orm import Mapped
from sqlalchemy.orm import mapped_column
from sqlalchemy.orm import MappedAsDataclass


class Base(MappedAsDataclass, DeclarativeBase):
    pass


class A(Base):
    __tablename__ = "a"

    id: Mapped[int] = mapped_column(primary_key=True, init=False)
    data: Mapped[str]


a1 = A(data='some data')

however Mypy reports:

test3.py:20: error: Argument "data" to "A" has incompatible type "str"; expected "Mapped[str]"  [arg-type]
Found 1 error in 1 file (checked 1 source file)

this is not correct. Mapped[] is a descriptor, so the appropriate value that should be set is a string, not a descriptor object.

Here's a complete example using only typing as an import

from __future__ import annotations

from typing import Any
from typing import Generic
from typing import Optional
from typing import overload
from typing import TYPE_CHECKING
from typing import TypeVar
from typing import Union

from typing_extensions import dataclass_transform

_T = TypeVar("_T")


def model_field(
    *,
    default: Optional[Any] = None,
    init: bool = True,
) -> Any:
    raise NotImplementedError()


@dataclass_transform(
    eq_default=True, order_default=True, field_specifiers=(model_field,)
)
class ModelBase:
    def __init_subclass__(
        cls,
        *,
        init: bool = True,
        frozen: bool = False,
        eq: bool = True,
        order: bool = True,
    ):
        ...


class Mapped(Generic[_T]):
    if TYPE_CHECKING:

        @overload
        def __get__(self, instance: None, owner: Any) -> Mapped[_T]:
            ...

        @overload
        def __get__(self, instance: object, owner: Any) -> _T:
            ...

        def __get__(
            self, instance: Optional[object], owner: Any
        ) -> Union[Mapped[_T], _T]:
            ...

        def __set__(self, instance: Any, value: _T) -> None:
            ...

        def __delete__(self, instance: Any) -> None:
            ...


class Customer(ModelBase):
    a: int


c1 = Customer(a=5)


class AlsoCustomer(ModelBase):
    a: Mapped[int]


# this is correct under pyright
c2 = AlsoCustomer(a=5)


# this is an error under pyright
c3 = AlsoCustomer(a="some string")

We had very involved discussions about this here: microsoft/pyright#2958 where the authors of pep-681 decided only to add a note about this here: https://peps.python.org/pep-0681/#descriptor-typed-field-support

While I was disappointed they would not add this language to the spec explicitly, they state it in that paragraph:

When enabled, the type of each parameter on the synthesized init method corresponding to a descriptor-typed field would be the type of the value parameter to the descriptor’s set method rather than the descriptor type itself. Similarly, when setting the field, the set value type would be expected. And when getting the value of the field, its type would be expected to match the return type of get.

This idea was based on the belief that dataclass did not properly support descriptor-typed fields. In fact it does, but type checkers (at least mypy and pyright) did not reflect the runtime behavior which led to our misunderstanding. For more details, see the Pyright bug.

That is, Python dataclasses when a descriptor is used as the left hand type, the type of the actual attribute passed to init must be the contained datatype, not the descriptor type.

We at SQLAlchemy spent weeks back and forth with the pep-681 authors on this and I am a little bit terrified that because this language wasn't included, Mypy did it exactly the wrong way.

I am very hopeful that the Mypy authors can correct this behavior else we will have to get our users to disable pep-681 support for mypy.

[zzzeek]

I've updated the description to include a typing-only example, illustrating how this spec is meant to work per the authors of pep-681

[zzzeek]

it looks like mypy has this issue already at #13856. why was mypy 1.1.1 released with this feature knowing it was broken? Is there a way I can tell my users to disable mypy's pep-681 support until this is fixed?

[JukkaL]

cc @wesleywright

[Dreamsorcerer]

Just a thought, but does the sqlalchemy repo have any examples that are type checked, which might have been caught by mypy_primer (or whichever step it is) in a PR? As more projects migrate to sqlalchemy 2, these breakages should be caught earlier, but if sqlalchemy has some typed examples, that could probably help ensure they are checked.

I'll try and make sure some of the aio-libs examples use MappedAsDataclass in future (though I'm not sure which of our repos are included in mypy_primer).

[zzzeek]

we have a whole directory of files that are type checked as part of CI, we didnt add mappedasdataclass becuase mypy has not supported it: https://github.com/sqlalchemy/sqlalchemy/tree/main/test/ext/mypy/plain_files

[JukkaL]

@zzzeek It would be great if you can try out #15006, which will hopefully fix the issue. I'm planning to include it in the 1.2 release (it should go out in the next day or two).

[zzzeek]

that's exciting, will check it out in a few

[zzzeek]

looks good, will have a patch ready for this when 1.2 is released, thanks!