Repository navigation
mypy has implemented pep-681 incorrectly and does not intepret descriptors correctly #14868
Description
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.
Activity
zzzeek commentedon Mar 10, 2023
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 commentedon Mar 10, 2023
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 commentedon Mar 14, 2023
cc @wesleywright
Dreamsorcerer commentedon Mar 14, 2023
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
MappedAsDataclassin future (though I'm not sure which of our repos are included in mypy_primer).zzzeek commentedon Mar 14, 2023
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
feat: avoid use of Ref, make use of descriptor to add reactive behavior
feat: avoid use of Ref, make use of descriptor to add reactive behavior
JukkaL commentedon Apr 4, 2023
@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 commentedon Apr 4, 2023
that's exciting, will check it out in a few
zzzeek commentedon Apr 4, 2023
looks good, will have a patch ready for this when 1.2 is released, thanks!
Support descriptors in dataclass transform (#15006)
Support descriptors in dataclass transform (#15006)
mypy 1.2 has fixed dataclass descriptor support