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

stubgen: generate valid dataclass stubs #15625

Merged
merged 4 commits into from
Sep 15, 2023

Conversation

hamdanal
Copy link
Collaborator

@hamdanal hamdanal commented Jul 8, 2023

Fixes #12441

Edit:
Also fixes #9986
Edit 2:
Fixes #15966

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Please, add test cases for:

  1. InitVar annotation
  2. dataclass() with other keywords, not only init=False
  3. Fields defined with = field() (it is special enough to be tested)
  4. _: dataclasses.KW_ONLY mark

[out]
from dataclasses import dataclass

@dataclass
Copy link
Member

Choose a reason for hiding this comment

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

I think that init=False is assumed here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

According to the dataclass documentation,

init: If true (the default), a __init__() method will be generated.
If the class already defines __init__(), this parameter is ignored.

So it is ignored in this case. This was not the goal of the test anyway, it was to test that user defined methods are always included in the stub.

We cannot safely remove `__init__` and depend on the plugin
because its signature depends on dataclass field assignments to
`dataclasses.field` and these assignments are not included in the stub
@hamdanal
Copy link
Collaborator Author

Thanks for the review, I added more tests.

  1. dataclass() with other keywords, not only init=False

Added more keywords in the tests but the goal of the test is make sure keywords are not stripped out and call expressions are recognized as dataclasses, it is not to test each keyword individually as they are all handled the same way.

  1. Fields defined with = field() (it is special enough to be tested)

Thanks. Since a field() assignment could affect the signature of __init__ and since stubs don't have default values yet (I opened #15355 to preserve simple defaults in function signatures in stubs but we still don't have neither "call" defaults like field() nor attribute defaults, so the assignment to field() gets erased), it is not safe to ignore the generated __init__ method, so now I keep it.

  1. _: dataclasses.KW_ONLY mark

It was indeed special because it was considered private name, added a test.

@hamdanal
Copy link
Collaborator Author

Hmm, how can I restrict a stubgen test case to a certain python version?

@hamdanal
Copy link
Collaborator Author

Sorry to ping you @sobolevn. Could you please take another look? This came up again in a new project of mine. Thank you

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thank you!

@hauntsaninja hauntsaninja merged commit 2bbc42f into python:master Sep 15, 2023
13 checks passed
@hamdanal hamdanal deleted the stubgen-dataclasses branch September 16, 2023 07:04
i: InitVar[str]
j: InitVar = ...
non_field = ...
def __init__(self, a, b, f, g, h, i, j) -> None: ...
Copy link

Choose a reason for hiding this comment

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

Sorry to comment on an old, merged PR, but I wanted to ask if it's expected that the arguments to __init__() are missing type annotations in the generated stub.

If I install this stub and then try to typecheck

X(a="foo", ...)

I would expect mypy to error with

error: Argument "a" to "X" has incompatible type "str"; expected "int"  [arg-type]

but instead it passes (I think the type annotation for a is implicitly Any when it's not annotated in the signature, which is incorrect I think - I think the type of a should be int, not Any).

Let me know if this is actually better positioned as a new issue (with reproducible example etc.), but I just wanted to see if I was missing something obvious before opening a new one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Stubgen doesn't use inferred types of parameters in generated stubs unless it sees a parameter with a primitive default value. You'll always need to tweak the generated stubs by hand if you want better experience with the type checker.

Also note that the __init__ method can be safely deleted from the stub most of the time. It is included because a field declaration like some_field: int = field(init=False) that affects the signature of __init__ cannot be currently represented in the stub.

With that being said, I agree that perhaps we can do better but I am not a mypy maintainer. So yes please open a new issue asking for this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants