Skip to content

Properly track positional-only arguments for unannotated functions #10802

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

Merged
merged 5 commits into from
Jul 15, 2021

Conversation

msullivan
Copy link
Collaborator

I was originally planning to address this by adding an ARG_POS_ONLY
kind (as a concrete win for what I hoped would be a generally positive
refactor), but it was actually really easy to fix it on its own, and
then we can address the refactor purely on its own merits.

@msullivan msullivan requested a review from JukkaL July 13, 2021 19:51
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 PR! I wonder if instead you could set the relevant argument names None for lambdas, and you might not need most of the changes. Not sure if this is sufficient, though.

@msullivan
Copy link
Collaborator Author

OK, I updated this to drop num_pos_only. This required augmenting Argument with a pos_only field to allow arg_names to be calculated from it. (The other option was to explicitly pass in an arg_names from the parser separate from arguments, which seemed to be a lateral move at best.)

I was originally planning to address this by adding an ARG_POS_ONLY
kind (as a concrete win for what I hoped would be a generally positive
refactor), but it was actually really easy to fix it on its own, and
then we can address the refactor purely on its own merits.
@msullivan msullivan force-pushed the pos-only-unannotated branch from 357a004 to 831651c Compare July 14, 2021 19:55
@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.

Thanks, this looks cleaner now! LGTM. Left a few minor comments -- feel free too address them before merging if you agree with them.

for name in arg_names]
if special_function_elide_names(n.name):
arg_names = [None] * len(arg_names)
arg_names = [None if arg.pos_only else arg.variable.name for arg in args]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is arg_names actually used anywhere? Can it be deleted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It populates the CallableType, if there is one

@msullivan msullivan merged commit d180456 into master Jul 15, 2021
@msullivan msullivan deleted the pos-only-unannotated branch July 15, 2021 17:57
@github-actions
Copy link
Contributor

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

werkzeug (https://github.com/pallets/werkzeug.git)
- tests/test_wsgi.py:474: error: Function is missing a return type annotation
+ tests/test_wsgi.py:474: error: Function is missing a type annotation
- tests/test_wrappers.py:285: error: Function is missing a return type annotation
+ tests/test_wrappers.py:285: error: Function is missing a type annotation

ibis (https://github.com/ibis-project/ibis.git)
- version: 0.920+dev.ed09f8d831aba43af7b7078ceb1d0706e1c124f6
+ version: 0.920+dev.3cd0e07f50cd113c01b681d4aa73063024c778de
-   File "/tmp/mypy_primer/old_mypy/venv/bin/mypy", line 8, in <module>
+   File "/tmp/mypy_primer/new_mypy/venv/bin/mypy", line 8, in <module>
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/__main__.py", line 11, in console_entry
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/__main__.py", line 11, in console_entry
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/main.py", line 87, in main
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/main.py", line 87, in main
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/main.py", line 165, in run_build
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/main.py", line 165, in run_build
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 179, in build
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 179, in build
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 254, in _build
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 254, in _build
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 2698, in dispatch
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 2698, in dispatch
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 3022, in process_graph
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 3022, in process_graph
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 3120, in process_stale_scc
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 3120, in process_stale_scc
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 2166, in type_check_first_pass
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/build.py", line 2166, in type_check_first_pass
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/checker.py", line 295, in check_first_pass
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/checker.py", line 295, in check_first_pass
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/checker.py", line 402, in accept
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/checker.py", line 402, in accept
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/nodes.py", line 783, in accept
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/nodes.py", line 785, in accept
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/checker.py", line 3640, in visit_decorator
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/checker.py", line 3640, in visit_decorator
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/checkexpr.py", line 951, in check_call
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/checkexpr.py", line 951, in check_call
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/checkexpr.py", line 932, in check_call
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/checkexpr.py", line 932, in check_call
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/checkexpr.py", line 1044, in check_callable_call
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/checkexpr.py", line 1044, in check_callable_call
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/checkexpr.py", line 752, in apply_function_plugin
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/checkexpr.py", line 752, in apply_function_plugin
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/plugins/singledispatch.py", line 192, in call_singledispatch_function_after_register_argument
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/plugins/singledispatch.py", line 192, in call_singledispatch_function_after_register_argument
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/plugins/singledispatch.py", line 159, in register_function
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/plugins/singledispatch.py", line 159, in register_function
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.8/site-packages/mypy/plugins/singledispatch.py", line 33, in get_singledispatch_info
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.8/site-packages/mypy/plugins/singledispatch.py", line 33, in get_singledispatch_info

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