Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash with dataclass_transform cache #14734

Closed
wants to merge 1 commit into from

Conversation

cdce8p
Copy link
Collaborator

@cdce8p cdce8p commented Feb 19, 2023

When serializing a tuple[...] field to json, it's automatically converted to [...].
Thus when reading the cache a list would get assigned to a class field specified as tuple which causes a crash with the compiled version of mypy.

Issue was added in #14667 as it started to parse the field_specifiers argument.
/cc: @wesleywright

@cdce8p cdce8p mentioned this pull request Feb 19, 2023
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@wesleywright
Copy link
Collaborator

This should also be fixed by #14695 fwiw (I took a slightly different approach, but it’s more or less the same fix)

@wesleywright
Copy link
Collaborator

Out of curiosity, how did you trigger this? Using the master version of MyPy with a library that uses dataclass_transform?

@cdce8p
Copy link
Collaborator Author

cdce8p commented Feb 19, 2023

Out of curiosity, how did you trigger this? Using the master version of MyPy with a library that uses dataclass_transform?

It's only an issue in the compiled version of mypy. I regularly install the latest one from the mypy wheels repo. In this case
https://github.com/mypyc/mypy_mypyc-wheels/releases/tag/v1.1.0+dev.c03e979ca06c3bf082a4cd07458a1bc3205dc5e5

I noticed the error while debugging a issues with pydantic models and dataclasses. The recent dataclass_transform additions don't work particularly well with unfortunately. Especially #14667 caused a lot of issues.

Tested with pydantic 1.10.5.

from __future__ import annotations
import dataclasses

import pydantic.dataclasses
from pydantic import BaseModel, Field
from typing_extensions import reveal_type

class Event(BaseModel):
    id: int | None = Field(None, alias="id")


Event()  # Error
reveal_type(1)


@pydantic.dataclasses.dataclass
class Info:
    status: str


def func(info: Info) -> None:
    dataclasses.asdict(info)  # Error

func(Info(status="Hello"))
test.py:12: error: Missing named argument "id" for "Event"  [call-arg]
test.py:13: note: Revealed type is "Literal[1]?"
test.py:22: error: No overload variant of "asdict" matches argument type "Info"  [call-overload]
test.py:22: note: Possible overload variants:
test.py:22: note:     def asdict(obj: DataclassInstance) -> Dict[str, Any]
test.py:22: note:     def [_T] asdict(obj: DataclassInstance, *, dict_factory: Callable[[List[Tuple[str, Any]]], _T]) -> _T
# mypy.ini
[mypy]
python_version = 3.10
plugins = pydantic.mypy

[pydantic-mypy]
init_forbid_extra = true
init_typed = true
warn_required_dynamic_aliases = true
warn_untyped_fields = true
mypy --config-file mypy.ini test.py

@wesleywright
Copy link
Collaborator

wesleywright commented Feb 19, 2023

I noticed the error while debugging a issues with pydantic models and dataclasses. The recent dataclass_transform additions don't work particularly well with unfortunately. Especially #14667 caused a lot of issues.

Yeah, Pydantic will surface a lot of errors now that it didn't before the support for dataclass_transform was started. Most of the ones I looked at (other than incremental caching, which definitely needs at least #14695 to fix) seemed to be "expected" errors, at least as far as I could tell by interpreting PEP 681.

  • The Too many positional arguments error error is correct as far as PEP 681 since Pydantic seems to specify keyword-only arguments by default, but apparently Pydantic doesn't actually enforce that? (I also spot at least one other violation of PEP 681: Pydantic specifies dataclass_transform multiple times, but that's undefined behavior by the PEP.) FWIW, to fix this error for your snippet, I think you could just use @pydantic.dataclasses.dataclass(kw_only=False).
  • SImilarly, the lack of default for your Event.id field seems to be an issue on the Pydantic side. Pydantic accepts that argument as a positional, but the PEP states "[t]hese standardized parameters must be keyword-only."
  • For the asdict error, I think I misinterpreted the PEP. We're currently omitting __dataclass_fields__ for classes created with dataclass_transform, but I think it was intended that it be included (that's what Pyright does, and PEP 681 says that the classes should have dataclass semantics unless specified otherwise). I'll make a PR to fix this one. Thanks for the catch.

@cdce8p
Copy link
Collaborator Author

cdce8p commented Feb 19, 2023

Thanks for taking a look. The Too many positional arguments was my error, I already removed it from my snipped. Sorry about that.

As for the other two, pydantic has a custom mypy plugin that used to handle it just fine.
For pydantic.dataclasses.dataclass that delegated to the mypy dataclass transformer which previously added the __dataclass_fields__ attribute. However now with dataclass_transform this condition filters it

# Only add if the class is a dataclasses dataclass, and omit it for dataclass_transform
# classes.
# It would be nice if this condition were reified rather than using an `is` check.
# Only add if the class is a dataclasses dataclass, and omit it for dataclass_transform
# classes.
if self._spec is not _TRANSFORM_SPEC_FOR_DATACLASSES:
return

The Event.id issue is likely also a result of the dataclass_transform precedence. In particular this here

mypy/mypy/semanal_main.py

Lines 476 to 483 in c03e979

# Check if the class definition itself triggers a dataclass transform (via a parent class/
# metaclass)
spec = find_dataclass_transform_spec(info)
if spec is not None:
with self.file_context(file_node, options, info):
# We can't use the normal hook because reason = defn, and ClassDefContext only accepts
# an Expression for reason
ok = ok and dataclasses_plugin.DataclassTransformer(defn, defn, spec, self).transform()

Because of that, the pydantic plugin isn't used which would normally have handled the BaseModel / ModelMetaclass.
https://github.com/pydantic/pydantic/blob/v1.10.5/pydantic/mypy.py

@wesleywright
Copy link
Collaborator

This is very helpful info; thank you for sharing!

@cdce8p
Copy link
Collaborator Author

cdce8p commented Feb 19, 2023

I found a possible solution / workaround for the second issue (with Event.id). I can modify the pydantic plugin to reset the dataclass_transform_spec from ModelMetaclass. Basically pretend the metaclass isn't annotated with dataclass_transform. That would fix it.

I'm preparing a PR for it at the moment.

@cdce8p
Copy link
Collaborator Author

cdce8p commented Feb 19, 2023

Opened pydantic/pydantic#5077 to fix the pydantic plugin.

Remaining Todo's:

@wesleywright
Copy link
Collaborator

FYI #14752 should fix __dataclass_fields__

@wesleywright
Copy link
Collaborator

now that #14695 has landed, can you confirm whether the issue here is fixed for you?

@cdce8p
Copy link
Collaborator Author

cdce8p commented Feb 22, 2023

now that #14695 has landed, can you confirm whether the issue here is fixed for you?

Took a moment as I needed to wait until the wheels were build. As expected, #14695 does already resolve the issue. I'll go ahead and close this one then. Thanks @wesleywright 👍🏻

@cdce8p cdce8p closed this Feb 22, 2023
@cdce8p cdce8p deleted the crash-dataclass_transform-cache branch February 22, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants