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

[dataclass_transform] support subclass/metaclass-based transforms #14657

Conversation

wesleywright
Copy link
Collaborator

Support dataclass_transforms that use inheritance or metaclasses rather than decorators. This only needs plumbing changes so that we can get the correct metadata for a given class and trigger the dataclasses transform plugin; logic should otherwise remain the same.

The code changes here are a little invasive because of how the dataclasses plugin handles it's "reason" (ie, the AST node that triggered the plugin). Currently it takes a ClassDefContext where reason: Expression, but in the case of inheritance/metaclass-based transforms, it makes more sense for the class definition itself to be the reason (since the parent class and keyword args are supplied in the class definition itself). To accommodate for this, I refactored the DataclassTransformer class to take a reason: Expression | Statement while leaving the plugin API itself alone. This mostly involved updating the identifiers used throughout the class.

@github-actions

This comment has been minimized.

@wesleywright
Copy link
Collaborator Author

wesleywright commented Feb 9, 2023

I haven't checked every line, but the few errors I looked at it in the mypy_primer comment above seem to be valid issues that this change surfaces (particularly by enabling typechecking for Pydantic models).

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach you take here looks good! I didn't look at the mypy_primer output yet, but it makes sense that this can trigger some errors in code that is already using dataclass transforms. Left a bunch of minor comments.

if value is not None:
return value
else:
self._api.fail(f'"{name}" argument must be True or False.', expression)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: Leave out the period at the end of the message, for consistency.

Copy link
Collaborator Author

@wesleywright wesleywright Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was aiming to match the existing error wording from mypy/plugins/common.py; will cut a separate diff to consolidate and improve all of these (which will also be useful for #14667)

metaclass_type = node.metaclass_type
if (
metaclass_type is not None
and metaclass_type.type.defn.dataclass_transform_spec is not None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we search through the MRO of the metaclass as well?

Copy link
Collaborator Author

@wesleywright wesleywright Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of odd; from the PEP:

If dataclass_transform is applied to a class, dataclass-like semantics will be assumed for any class that directly or indirectly derives from the decorated class or uses the decorated class as a metaclass. Attributes on the decorated class and its base classes are not considered to be fields.

Anything that inherits from a class with dataclass_transform_spec must have dataclass-like semantics. I don't think that precludes it from being used a metaclass, but that would seem kind of unnatural? The PEP also uses the wording "uses the decorated class as a metaclass", and I'm not sure if that's semantically equivalent to "uses a class inheriting from the decorated class as a metaclass" (I would assume it only means the directly-specified metaclass).

That said, Pyright does seem to support it, and the PEP names Pyright as the reference implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was misunderstanding Pyright's behavior earlier; it didn't report an error for this example:

from typing import dataclass_transform

@dataclass_transform()
class BaseMeta(type): ...

class SubMeta(BaseMeta): ...

class Foo(metaclass=SubMeta):
    foo: int
Foo(foo=0)

Pyright doesn't detect any errors here, but that's because it's abandoning analysis due to an arbitrary metaclass rather than due to detecting that it's a proper dataclass transform. For example, it also accepts invalid expressions such as Foo(foo=1, bar=2). If BaseMeta is used as the metaclass instead, it detects these errors correctly.

This seems to match my interpretation above, and leads me to believe that we should only support the immediate metaclass and not any of its parent classes.


class Person(Dataclass, kw_only=True):
name: str
age: int
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test what happens if we subclass Person.


class Person(metaclass=Dataclass, kw_only=True):
name: str
age: int
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test what happens if we subclass Person.

from typing import dataclass_transform

@dataclass_transform(frozen_default=True)
class Dataclass(type): ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test what happens if we subclass Dataclass and use it as a metaclass.

mypy/nodes.py Outdated
@@ -1125,6 +1126,7 @@ def __init__(
# Used for error reporting (to keep backwad compatibility with pre-3.8)
self.deco_line: int | None = None
self.removed_statements = []
self.dataclass_transform_spec: DataclassTransformSpec | None = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add this to TypeInfo instead? We've tried to keep ClassDef pretty simple and move most meta-information to TypeInfo.

person.name = "John Smith" # E: Property "name" defined in "Person" is read-only

[typing fixtures/typing-full.pyi]
[builtins fixtures/dataclasses.pyi]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semi-related comment: Can you add some incremental mode test cases as well to test serialization (no need to do it in this PR)? Also other dataclass features could be covered. Similarly, daemon test cases would be nice, but again it can happen later. I can help if you are not sure how these work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do.

Also other dataclass features could be covered.

Could you clarify this a bit? I was trying to avoid covering all features since that would duplicate the a lot of test cases, but I'm happy to expand the test file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to test serialization of all the new attributes in incremental mode test cases.

To test mypy daemon properly, something like this could a good starting point:

  • Test propagating changes after changing flags (e.g. frozen).
  • Test switching between regular class and dataclass transform class.
  • If a base class or meta class is turned into a dataclass transform, the change should be propagated to other classes that use them.
  • Test modifying the signature of an attribute. This should propagate changes to subclasses in different modules that inherit that class.

The daemon test cases may need code changes, so it may be best to add them in a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be best to first add one or two daemon test cases, and once we've established a good way to write them, adding the remaining ones should be easy.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

artigraph (https://github.com/artigraph/artigraph)
+ src/arti/fingerprints/__init__.py:23: error: Argument "key" to "Fingerprint" has incompatible type "int"; expected "Optional[int64]"  [arg-type]
+ src/arti/types/__init__.py:328: error: Missing named argument "description" for "Type"  [call-arg]
+ src/arti/types/python.py:47: error: Missing named argument "description"  [call-arg]
+ src/arti/types/python.py:54: error: Unexpected keyword argument "element" for "Type"  [call-arg]
+ src/arti/types/python.py:142: error: Missing named argument "description"  [call-arg]
+ src/arti/types/python.py:142: error: Argument "items" has incompatible type "Tuple[Any, ...]"; expected "FrozenSet[Any]"  [arg-type]
+ src/arti/types/python.py:168: error: Missing named argument "description"  [call-arg]
+ src/arti/types/python.py:223: error: Missing named argument "description" for "Struct"  [call-arg]
+ src/arti/types/python.py:225: error: Argument "fields" to "Struct" has incompatible type "Dict[str, Type]"; expected "frozendict[str, Type]"  [arg-type]
+ src/arti/storage/__init__.py:138: error: Missing named argument "value" for "StringLiteral"  [call-arg]
+ tests/arti/dummies.py:29: error: Missing named argument "description" for "Int64"  [call-arg]
+ tests/arti/dummies.py:92: error: Missing named argument "description" for "Int32"  [call-arg]
+ tests/arti/dummies.py:98: error: Missing named argument "description" for "Struct"  [call-arg]
+ tests/arti/dummies.py:98: error: Argument "fields" to "Struct" has incompatible type "Dict[str, Int32]"; expected "frozendict[str, Type]"  [arg-type]
+ tests/arti/dummies.py:98: error: Missing named argument "description" for "Int32"  [call-arg]
+ tests/arti/dummies.py:104: error: Missing named argument "description" for "Struct"  [call-arg]
+ tests/arti/dummies.py:104: error: Argument "fields" to "Struct" has incompatible type "Dict[str, Int32]"; expected "frozendict[str, Type]"  [arg-type]
+ tests/arti/dummies.py:104: error: Missing named argument "description" for "Int32"  [call-arg]
+ tests/arti/dummies.py:110: error: Missing named argument "description" for "Struct"  [call-arg]
+ tests/arti/dummies.py:110: error: Argument "fields" to "Struct" has incompatible type "Dict[str, Int32]"; expected "frozendict[str, Type]"  [arg-type]
+ tests/arti/dummies.py:110: error: Missing named argument "description" for "Int32"  [call-arg]
+ tests/arti/dummies.py:116: error: Missing named argument "description" for "Struct"  [call-arg]
+ tests/arti/dummies.py:116: error: Argument "fields" to "Struct" has incompatible type "Dict[str, Int32]"; expected "frozendict[str, Type]"  [arg-type]
+ tests/arti/dummies.py:116: error: Missing named argument "description" for "Int32"  [call-arg]
+ src/arti/types/pydantic.py:47: error: Missing named argument "description" for "Struct"  [call-arg]
+ src/arti/types/pydantic.py:49: error: Argument "fields" to "Struct" has incompatible type "Dict[str, Type]"; expected "frozendict[str, Type]"  [arg-type]
+ src/arti/types/pyarrow.py:32: error: Missing named argument "description" for "Type"  [call-arg]
+ src/arti/types/pyarrow.py:90: error: Missing named argument "description"  [call-arg]
+ src/arti/types/pyarrow.py:91: error: Missing named argument "description"  [call-arg]
+ src/arti/types/pyarrow.py:91: error: Missing named argument "byte_size"  [call-arg]
+ src/arti/types/pyarrow.py:144: error: Missing named argument "description"  [call-arg]
+ src/arti/types/pyarrow.py:167: error: Missing named argument "description"  [call-arg]
+ src/arti/types/pyarrow.py:203: error: Missing named argument "description"  [call-arg]
+ src/arti/types/pyarrow.py:204: error: Argument "fields" has incompatible type "Dict[Any, Type]"; expected "frozendict[str, Type]"  [arg-type]
+ src/arti/types/pyarrow.py:300: error: Unexpected keyword argument "precision" for "Type"  [call-arg]
+ src/arti/types/numpy.py:79: error: Missing named argument "description"  [call-arg]
+ tests/arti/storage/test_gcs_storage.py:23: error: Missing named argument "description" for "Collection"  [call-arg]
+ tests/arti/storage/test_gcs_storage.py:23: error: Missing named argument "description" for "Struct"  [call-arg]
+ tests/arti/storage/test_gcs_storage.py:23: error: Argument "fields" to "Struct" has incompatible type "Dict[str, Int32]"; expected "frozendict[str, Type]"  [arg-type]
+ tests/arti/storage/test_gcs_storage.py:23: error: Missing named argument "description" for "Int32"  [call-arg]
+ src/arti/types/pandas.py:37: error: Missing named argument "description" for "List"  [call-arg]
+ src/arti/types/pandas.py:37: error: Missing named argument "description" for "String"  [call-arg]
+ src/arti/types/pandas.py:42: error: Missing named argument "description" for "List"  [call-arg]
+ src/arti/types/pandas.py:82: error: Missing named argument "description" for "List"  [call-arg]
+ src/arti/types/pandas.py:83: error: Missing named argument "description" for "Struct"  [call-arg]
+ src/arti/types/pandas.py:84: error: Argument "fields" to "Struct" has incompatible type "Dict[Any, Type]"; expected "frozendict[str, Type]"  [arg-type]
+ src/arti/types/pandas.py:101: error: Missing named argument "description" for "List"  [call-arg]
+ docs/examples/spend/demo.py:23: error: Missing named argument "description" for "Collection"  [call-arg]
+ docs/examples/spend/demo.py:24: error: Missing named argument "description" for "Struct"  [call-arg]
+ docs/examples/spend/demo.py:24: error: Argument "fields" to "Struct" has incompatible type "Dict[str, Type]"; expected "frozendict[str, Type]"  [arg-type]
+ docs/examples/spend/demo.py:24: error: Missing named argument "description" for "Int64"  [call-arg]
+ docs/examples/spend/demo.py:24: error: Missing named argument "description" for "Date"  [call-arg]
+ docs/examples/spend/demo.py:24: error: Missing named argument "description" for "Float64"  [call-arg]
+ docs/examples/spend/demo.py:32: error: Missing named argument "description" for "Float64"  [call-arg]
+ docs/examples/spend/demo.py:45: error: Missing named argument "type" for "Transactions"  [call-arg]
+ docs/examples/spend/demo.py:46: error: Argument "annotations" to "Transactions" has incompatible type "List[Vendor]"; expected "Tuple[Annotation, ...]"  [arg-type]
+ docs/examples/spend/demo.py:50: error: Unexpected keyword argument "transactions" for "Producer"  [call-arg]
+ tests/arti/views/test_views.py:22: error: Missing named argument "mode" for "View"  [call-arg]
+ tests/arti/views/test_views.py:22: error: Missing named argument "type" for "View"  [call-arg]
+ tests/arti/views/test_views.py:57: error: Missing named argument "description" for "List"  [call-arg]
+ tests/arti/views/test_views.py:57: error: Missing named argument "description" for "Int64"  [call-arg]
+ tests/arti/views/test_views.py:67: error: Missing named argument "description" for "Float64"  [call-arg]
+ tests/arti/types/test_types.py:49: error: Missing named argument "description" for "Type"  [call-arg]
+ tests/arti/types/test_types.py:84: error: Missing named argument "description" for "Type"  [call-arg]
+ tests/arti/types/test_types.py:94: error: Missing named argument "description" for "Float32"  [call-arg]
+ tests/arti/types/test_types.py:95: error: Missing named argument "description" for "Enum"  [call-arg]
+ tests/arti/types/test_types.py:100: error: Missing named argument "description" for "Enum"  [call-arg]
+ tests/arti/types/test_types.py:103: error: Missing named argument "description" for "Enum"  [call-arg]
+ tests/arti/types/test_types.py:103: error: Argument "items" to "Enum" has incompatible type "Collection[float]"; expected "FrozenSet[Any]"  [arg-type]
+ tests/arti/types/test_types.py:109: error: Missing named argument "description" for "Float32"  [call-arg]
+ tests/arti/types/test_types.py:112: error: Missing named argument "description" for "Enum"  [call-arg]
+ tests/arti/types/test_types.py:112: error: Argument "items" to "Enum" has incompatible type "List[<nothing>]"; expected "FrozenSet[Any]"  [arg-type]
+ tests/arti/types/test_types.py:115: error: Missing named argument "description" for "Enum"  [call-arg]
+ tests/arti/types/test_types.py:115: error: Argument "items" to "Enum" has incompatible type "Dict[float, float]"; expected "FrozenSet[Any]"  [arg-type]
+ tests/arti/types/test_types.py:119: error: Missing named argument "description" for "Enum"  [call-arg]
+ tests/arti/types/test_types.py:119: error: Argument "items" to "Enum" has incompatible type "Tuple[int, int, int]"; expected "FrozenSet[Any]"  [arg-type]
+ tests/arti/types/test_types.py:121: error: Missing named argument "description" for "Enum"  [call-arg]
+ tests/arti/types/test_types.py:121: error: Argument "items" to "Enum" has incompatible type "Tuple[float, float, int]"; expected "FrozenSet[Any]"  [arg-type]
+ tests/arti/types/test_types.py:125: error: Missing named argument "description" for "Enum"  [call-arg]
+ tests/arti/types/test_types.py:125: error: Argument "type" to "Enum" has incompatible type "Type[float]"; expected "Type"  [arg-type]
+ tests/arti/types/test_types.py:129: error: Missing named argument "description" for "List"  [call-arg]
+ tests/arti/types/test_types.py:129: error: Missing named argument "description" for "Int32"  [call-arg]
+ tests/arti/types/test_types.py:130: error: Missing named argument "description" for "Int32"  [call-arg]
+ tests/arti/types/test_types.py:135: error: Missing named argument "description" for "Collection"  [call-arg]
+ tests/arti/types/test_types.py:135: error: Missing named argument "description" for "Int32"  [call-arg]
+ tests/arti/types/test_types.py:138: error: Missing named argument "description" for "Int32"  [call-arg]
+ tests/arti/types/test_types.py:148: error: Missing named argument "description" for "Collection"  [call-arg]
+ tests/arti/types/test_types.py:148: error: Missing named argument "description" for "Struct"  [call-arg]

... (truncated 375 lines) ...

steam.py (https://github.com/Gobot1234/steam.py)
+ steam/gateway.py:416: error: Argument "platform_type" to "BeginAuthSessionViaCredentialsRequest" has incompatible type "int"; expected "EAuthTokenPlatformType"  [arg-type]
+ steam/gateway.py:417: error: Argument "persistence" to "BeginAuthSessionViaCredentialsRequest" has incompatible type "int"; expected "ESessionPersistence"  [arg-type]
+ steam/state.py:718: error: MsgT? has no attribute "view_id"  [attr-defined]
+ steam/state.py:719: error: MsgT? has no attribute "view"  [attr-defined]
+ steam/state.py:2414: error: ProtoMsgsT? has no attribute "message_sequence"  [attr-defined]

pydantic (https://github.com/samuelcolvin/pydantic)
+ pydantic/main.py:48: error: "field_specifiers" support is currently unimplemented  [misc]

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

@JukkaL JukkaL merged commit 99b04ca into python:master Feb 10, 2023
JukkaL pushed a commit that referenced this pull request Feb 14, 2023
Follow up on some of the recurring feedback from #14580 and #14657.
There are many error messages similar to `X must be True or False.` in
MyPy. This commit updates them all to:

- remove the dangling period for consistency with other error messages
- clarify that we need a `True` or `False` literal
- use the `literal-required` error code for consistency with other
literal errors

This should have no impact outside of error message formatting.
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