Skip to content

Conversation

@abhijeetbodas2001
Copy link
Contributor

@abhijeetbodas2001 abhijeetbodas2001 commented May 4, 2025

Summary

Fixes #17541

Before this change, in the case of overloaded functions, @dataclass_transform was detected only when applied to the implementation, not the overloads.
However, the spec also allows this decorator to be applied to any of the overloads as well.
With this PR, we start handling @dataclass_transforms applied to overloads.

Test Plan

Fixed existing TODOs in the test suite.

@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2025

mypy_primer results

Changes were detected when running on open source projects
strawberry (https://github.com/strawberry-graphql/strawberry)
- error[lint:unknown-argument] strawberry/relay/types.py:878:25: Argument `start_cursor` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:879:25: Argument `end_cursor` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:880:25: Argument `has_previous_page` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:881:25: Argument `has_next_page` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:906:21: Argument `start_cursor` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:907:21: Argument `end_cursor` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:908:21: Argument `has_previous_page` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:909:21: Argument `has_next_page` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:942:17: Argument `start_cursor` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:943:17: Argument `end_cursor` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:944:17: Argument `has_previous_page` does not match any known parameter of bound method `__init__`
- error[lint:unknown-argument] strawberry/relay/types.py:945:17: Argument `has_next_page` does not match any known parameter of bound method `__init__`
+ error[lint:invalid-return-type] strawberry/tools/create_type.py:67:12: Return type does not match returned value: Expected `type`, found `<decorator produced by dataclass-like function>`
+ error[lint:invalid-return-type] strawberry/tools/merge_types.py:35:12: Return type does not match returned value: Expected `type`, found `<decorator produced by dataclass-like function>`
- Found 462 diagnostics
+ Found 452 diagnostics

@abhijeetbodas2001
Copy link
Contributor Author

This is as of now incomplete, in that it only handles the cases where the function which is supposed to act like a dataclass decorator is being used in a "call" context, like so:

@versioned_class(version=2)
class D2:
    x: str

This PR does not yet fix cases where versioned_class is used in a "name load" context:

@versioned_class
class D1:
    x: str

This is because, in the later case, logic to determine if the FunctionLiteral acts like a dataclass decorator is here:

if let Type::FunctionLiteral(f) = decorator_ty {
if let Some(params) = f.dataclass_transformer_params(self.db()) {
dataclass_params = Some(params.into());
continue;
}
}

For the FunctionLiteral case of versioned_class, my understanding is that the only live binding of versioned_class at the use is the implementation. If the implementation does not have dataclass_transform applied to it (and instead a prior overload has it), we will not find any dataclass_params in the above code, and will not deem versioned_class to be dataclass-like.

@dhruvmanila I would appreciate any thoughts on how to handle this case. We will need to probably add in some custom behavior to "carry over" the @dataclass_transform decorator between bindings. I am not sure how to do this. I am also not sure whether this has to be done more generally for decorators, instead of just for dataclass_transform.

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label May 4, 2025
@abhijeetbodas2001 abhijeetbodas2001 force-pushed the dataclass branch 2 times, most recently from 0f7d49f to 35f9adb Compare May 5, 2025 18:12
@abhijeetbodas2001
Copy link
Contributor Author

I (ab)used to_overloaded to fix the above issue also.
Marking this as not-draft to get some reviews.

@abhijeetbodas2001 abhijeetbodas2001 marked this pull request as ready for review May 5, 2025 18:14
@AlexWaygood AlexWaygood removed their request for review May 5, 2025 18:21
@dhruvmanila dhruvmanila self-requested a review May 6, 2025 05:20
@sharkdp
Copy link
Contributor

sharkdp commented May 6, 2025

Thank you very much for working on this! I am planning to do a full review soon. One thing that I hoped would be happening with this PR is that we understand strawberry.type. I would imagine that this would cause some ecosystem changes once we do.

But it does not seem to work yet, on your branch:

strawberry_test.py:

import strawberry

@strawberry.type
class User:
    name: str
    age: int

User(name="Patrick", age=100)
▶ uv run --no-project --with strawberry-graphql cargo run --bin ty -- check strawberry_test.py
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.13s
     Running `/home/shark/.cargo-target/debug/ty check strawberry_test.py`
error: lint:unknown-argument: Argument `name` does not match any known parameter of bound method `__init__`
  --> strawberry_test.py:10:6
   |
10 | User(name="foo", age=8)
   |      ^^^^^^^^^^
   |

error: lint:unknown-argument: Argument `age` does not match any known parameter of bound method `__init__`
  --> strawberry_test.py:10:18
   |
10 | User(name="foo", age=8)
   |                  ^^^^^
   |

Found 2 diagnostics

It might be interesting to understand why this does not work yet (not implying that you need to do this, I can also look into it when I get around to it).

@abhijeetbodas2001
Copy link
Contributor Author

abhijeetbodas2001 commented May 6, 2025

Interesting. I would certainly be interested in investigating this, but it does not give any errors for me though:

[~/code/ruff] $ git reset --hard origin/dataclass
HEAD is now at 518ef57c2 fix typo in comment
[~/code/ruff] $ git lol -3
518ef57c2 (HEAD -> dataclass, origin/dataclass) fix typo in comment
837ad29b4 fix name load usage also
828248142 [ty] Detect overloads decorated with `@dataclass_transform`
[~/code/ruff] $ uv run --no-project --with strawberry-graphql cargo run --bin ty -- check /tmp/strawberry_test.py
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.51s
     Running `target/debug/ty check /tmp/strawberry_test.py`
All checks passed!

Just to confirm, you're on the latest version of this branch right? I had force pushed a couple of times here.

@sharkdp
Copy link
Contributor

sharkdp commented May 6, 2025

Just to confirm, you're on the latest version of this branch right? I had force pushed a couple of times here.

Oh, indeed. I must have done something wrong when switching branches. It works as expected — nice!

I expected more ecosystem changes in strawberry, but it looks like in mypy_primer, we're only checking the "strawberry" subfolder, and not strawberry's tests, which would contain many examples. The occurrences of @strawberry.type inside the "strawberry" source folder are mostly in doc comments.

Sorry for the distraction.

@abhijeetbodas2001
Copy link
Contributor Author

Hmmph, I don't understand how mypy_primer works (or even what it does, actually!).
Let me see if I can figure that out and include their tests in the analysis.

@sharkdp
Copy link
Contributor

sharkdp commented May 7, 2025

Hmmph, I don't understand how mypy_primer works (or even what it does, actually!).
Let me see if I can figure that out and include their tests in the analysis.

Sorry, I didn't mean to imply that we should change that. I think it's fine as is. It is configured here: https://github.com/hauntsaninja/mypy_primer/blob/a4e8faf6832df707a36a4225f88ec4dc04733e96/mypy_primer/projects.py#L1517

We use mypy_primer to run ty across a set of open source projects (the ones you see in that linked file). Once for the version on main, and once for the feature branch. It then computes a diff of ty's diagnostics output and posts it as a PR comment

Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

This looks great — thank you very much!

@sharkdp sharkdp merged commit 3dedd70 into astral-sh:main May 7, 2025
35 checks passed
@abhijeetbodas2001 abhijeetbodas2001 deleted the dataclass branch May 7, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[red-knot] Detect overloads decorated with @dataclass_transform

4 participants