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] detect transform spec changes in incremental mode #14695

Merged
merged 6 commits into from
Feb 22, 2023

Conversation

wesleywright
Copy link
Collaborator

@wesleywright wesleywright commented Feb 13, 2023

Adds support for triggering rechecking of downstream classes when @dataclass_transform is added or removed from a function/class, as well as when parameters to dataclass_transform are updated. These changes aren't propagated normally since they don't change the type signature of the dataclass_transform decorator.

Also adds new a new find-grained-dataclass-transform.test file to test the new logic.

@@ -280,6 +283,7 @@ def snapshot_definition(node: SymbolNode | None, common: SymbolSnapshot) -> Symb
tuple(snapshot_type(tdef) for tdef in node.defn.type_vars),
[snapshot_type(base) for base in node.bases],
[snapshot_type(p) for p in node._promote],
node.dataclass_transform_spec or find_dataclass_transform_spec(node),
Copy link
Collaborator Author

@wesleywright wesleywright Feb 13, 2023

Choose a reason for hiding this comment

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

this may be somewhat expensive; an alternative may be to attach the relevant spec to each class in the hierarchy during semantic analysis rather than only attaching the spec to the transformer class

Copy link
Collaborator

Choose a reason for hiding this comment

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

Class hierarchies are almost always quite shallow, so this shouldn't be expensive. Similar to above, the snapshots must support equality. Using the serialized form should do the trick.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

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 main thing here that doesn't seem right is that snapshots should only contain objects that support value-based equality. You can also add some test cases to test-data/unit/diff.test which tests the astdiff module.

@@ -239,6 +241,7 @@ def snapshot_definition(node: SymbolNode | None, common: SymbolSnapshot) -> Symb
node.is_static,
signature,
is_trivial_body,
dataclass_transform_spec,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't support equality and can't be used in snapshots. You can probably use the serialized form instead.

@@ -280,6 +283,7 @@ def snapshot_definition(node: SymbolNode | None, common: SymbolSnapshot) -> Symb
tuple(snapshot_type(tdef) for tdef in node.defn.type_vars),
[snapshot_type(base) for base in node.bases],
[snapshot_type(p) for p in node._promote],
node.dataclass_transform_spec or find_dataclass_transform_spec(node),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Class hierarchies are almost always quite shallow, so this shouldn't be expensive. Similar to above, the snapshots must support equality. Using the serialized form should do the trick.

@dataclass_transform()
class Dataclass: ...

class A(Dataclass):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here the change in MRO will trigger dependencies, so this doesn't perfectly test how applying dataclass transform behaves. You can work around this by having a Dataclass base class in the original version, but omit the dataclass_transform decorator.

@wesleywright wesleywright changed the title [dataclass_transform] detect decorator changes in incremental mode [dataclass_transform] detect transform spec changes in incremental mode Feb 16, 2023
@wesleywright wesleywright marked this pull request as ready for review February 16, 2023 16:30
@github-actions
Copy link
Contributor

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

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, looks good now!

@JukkaL JukkaL merged commit 29bcc7f into python:master Feb 22, 2023
@cdce8p cdce8p mentioned this pull request Feb 22, 2023
cdce8p pushed a commit to cdce8p/mypy that referenced this pull request Feb 23, 2023
… incremental mode (python#14695)

Adds support for triggering rechecking of downstream classes when
`@dataclass_transform` is added or removed from a function/class, as
well as when parameters to `dataclass_transform` are updated. These
changes aren't propagated normally since they don't change the type
signature of the `dataclass_transform` decorator.

Also adds new a new `find-grained-dataclass-transform.test` file to test
the new logic.

(cherry picked from commit 29bcc7f)
hauntsaninja pushed a commit that referenced this pull request Feb 23, 2023
… incremental mode (#14695) (#14768)

Adds support for triggering rechecking of downstream classes when
`@dataclass_transform` is added or removed from a function/class, as
well as when parameters to `dataclass_transform` are updated. These
changes aren't propagated normally since they don't change the type
signature of the `dataclass_transform` decorator.

Also adds new a new `find-grained-dataclass-transform.test` file to test
the new logic.

(cherry picked from commit 29bcc7f)

Co-authored-by: Wesley Collin Wright <wesleyw@dropbox.com>
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.

None yet

2 participants