Skip to content

Support positional and keyword-only arguments #18762

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 14, 2025
Merged

Support positional and keyword-only arguments #18762

merged 3 commits into from
Mar 14, 2025

Conversation

pganssle-google
Copy link
Contributor

Currently the signature parsing logic fails when confronted with a / or a *, rather than recognizing them as demarcating positional-only and keyword-only arguments.

This patch supports parsing signatures with these features, but doesn't pass this information along to the ArgSig or FunctionSig classes, since the information would not be used anyway.

@pganssle-google
Support positional and keyword-only arguments
80fb6d0 
Currently the signature parsing logic fails when confronted with a `/`
or a `*`, rather than recognizing them as demarcating positional-only
and keyword-only arguments.

This patch supports parsing signatures with these features, but doesn't
pass this information along to the `ArgSig` or `FunctionSig` classes,
since the information would not be used anyway.
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pganssle-google
Copy link
Contributor Author

This PR was triggered because I noticed that a very recent version of pybind11 which adds / to the type signatures of some properties/methods was losing type information in the stubgen. I am not sure which version will start triggering it, but I suspect that running this test against the pybind11 master branch would fail because name() would lose its type information.

pganssle-google
pganssle-google commented Mar 6, 2025
View reviewed changes
mypy/test/teststubgen.py
@@ -399,6 +399,101 @@ def test_infer_sig_from_docstring_bad_indentation(self) -> None:
None,
)

def test_infer_sig_from_docstring_args_kwargs(self) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context: This behavior is not actually changing, but an intermediate version of the code would have failed these tests, and I only noticed this because the pybind11 stubgen tests started failing, so I figured I would add tests for this proactively.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
hauntsaninja
hauntsaninja reviewed Mar 6, 2025
View reviewed changes
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.

Not familiar with this code, but is the keyword_only int handled correctly if there's a *args arg as opposed to just *, kw, ...)?

@pganssle-google
Copy link
Contributor Author

pganssle-google commented Mar 7, 2025 •
edited
Loading

Not familiar with this code, but is the keyword_only int handled correctly if there's a *args arg as opposed to just *, kw, ...)?

I guess not, it should be the following entry. I've fixed that and added some more test cases for this and other args/kwargs related thing. It seems like some of the rules around *args and **kwargs signature validation were never working, and I didn't want the scope of this PR to expand too much (plus it might not be a good idea to make the parser stricter for things that are currently parsing — albeit incorrectly).

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 7, 2025

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Mar 10, 2025
@tensorflower-gardener
Improve mypy stubs
86ffb3e 
These stubs are generated by cherry-picking in python/mypy#18762 to mypy 1.13.0

PiperOrigin-RevId: 734567518
@copybara-service copybara-service bot mentioned this pull request Mar 10, 2025
Merged
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Mar 10, 2025
@tensorflower-gardener
Improve mypy stubs
3fe9999 
These stubs are generated by cherry-picking in python/mypy#18762 to mypy 1.13.0

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#23553 from olupton:remove-unused-include 970f55f476a9b4bf2dcb829152d4104f21864436
PiperOrigin-RevId: 734567518
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Mar 10, 2025
@tensorflower-gardener
Improve mypy stubs
310951a 
These stubs are generated by cherry-picking in python/mypy#18762 to mypy 1.13.0

PiperOrigin-RevId: 735413074
hauntsaninja
hauntsaninja approved these changes Mar 14, 2025
View reviewed changes
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.

Thanks for the improvement!

@hauntsaninja hauntsaninja merged commit bbd7a6c into python:master Mar 14, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hauntsaninja hauntsaninja hauntsaninja approved these changes

Assignees
No one assigned
Labels
topic-stubgen
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pganssle-google @hauntsaninja @sterliakov