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

stubtest: error for pos-only differences for dunder methods, too #12184

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Feb 15, 2022

Description

stubtest currently does not complain if a dunder method in a stub file has an argument that should be positional-only, but is marked in the stub as being positional-or-keyword. This PR proposes to remove this exception.

Removing the exception will increase our confidence in the accuracy of typeshed stubs with regard to positional-only arguments.

The rationale for the exception was the very large number of complaints stubtest had when checking typeshed. However, I have been using this patch over the past few days to file a series of PRs to typeshed, in order to minimise the impact of this PR:

As a result, this PR means that only the following new hits are generated when running stubtest on typeshed:

+ error: sqlite3.Connection.__exit__ is inconsistent, stub argument "t" should be positional-only (rename with a leading double underscore, i.e. "__type")
+ error: sqlite3.Connection.__exit__ is inconsistent, stub argument "exc" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.Connection.__exit__ is inconsistent, stub argument "tb" should be positional-only (rename with a leading double underscore, i.e. "__traceback")
+ error: sqlite3.Row.__eq__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.Row.__ge__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.Row.__getitem__ is inconsistent, stub argument "index" should be positional-only (rename with a leading double underscore, i.e. "__key")
+ error: sqlite3.Row.__gt__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.Row.__le__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.Row.__lt__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.Row.__ne__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.dbapi2.Connection.__exit__ is inconsistent, stub argument "t" should be positional-only (rename with a leading double underscore, i.e. "__type")
+ error: sqlite3.dbapi2.Connection.__exit__ is inconsistent, stub argument "exc" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.dbapi2.Connection.__exit__ is inconsistent, stub argument "tb" should be positional-only (rename with a leading double underscore, i.e. "__traceback")
+ error: sqlite3.dbapi2.Row.__eq__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.dbapi2.Row.__ge__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.dbapi2.Row.__getitem__ is inconsistent, stub argument "index" should be positional-only (rename with a leading double underscore, i.e. "__key")
+ error: sqlite3.dbapi2.Row.__gt__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.dbapi2.Row.__le__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.dbapi2.Row.__lt__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")
+ error: sqlite3.dbapi2.Row.__ne__ is inconsistent, stub argument "other" should be positional-only (rename with a leading double underscore, i.e. "__value")

All of these remaining hits will be resolved by one final outstanding PR to typeshed (EDIT: now merged -- thanks @JelleZijlstra!):

The full command I ran to obtain this diff was the following (with ../typeshed being the path to my clone of typeshed):

> python -m mypy.stubtest --check-typeshed --custom-typeshed-dir ../typeshed --allowlist ../typeshed/tests/stubtest_allowlists/py3_common.txt --allowlist ../typeshed/tests/stubtest_allowlists/py310.txt --allowlist ../typeshed/tests/stubtest_allowlists/win32.txt --allowlist ../typeshed/tests/stubtest_allowlists/win32-py310.txt > pos_only_report.txt

I ran this command without this patch applied, and with this patch applied. The command was run on Windows, with Python 3.10.

Test Plan

The existing exception does not appear to be tested; this PR does not appear to break any existing tests. I'm happy to add some tests for this change, if they'd be appreciated, however :)

Design question

We could add a new command-line option to turn this exception back on. Thoughts on that?

@AlexWaygood
Copy link
Member Author

I think that's the longest rationale I've ever written for deleting a single line of code.

@AlexWaygood
Copy link
Member Author

cc. @hauntsaninja

@JelleZijlstra
Copy link
Member

Thanks for reminding me to merge that PR, and thanks for all the fixes!

I don't think this needs a new config option; we should just remove the special case. But I'll let @hauntsaninja weigh in.

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 making all of these fixes, looks good!

If you're interested in pulling on this, there are a couple other places where we special case dunders (grep for is_dunder and SPECIAL_DUNDERS). In particular, I think the SPECIAL_DUNDERS check in verify_typeinfo could be productively removed.

On the design question: I don't think we need a flag to turn these specifically back off. stubtest already has a --ignore-positional-only for people who don't find this class of error useful. More generally, stubtest operates in a somewhat murky area and so is forced to have several opinions. If we get feedback that those opinions are bad, we should change, but I don't want to accrete complexity preemptively.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Feb 15, 2022

If you're interested in pulling on this, there are a couple other places where we special case dunders (grep for is_dunder and SPECIAL_DUNDERS). In particular, I think the SPECIAL_DUNDERS check in verify_typeinfo could be productively removed.

I already have a second patch waiting in the wings, I just figured I'd do the smaller one first ;)

@github-actions
Copy link
Contributor

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

@AlexWaygood
Copy link
Member Author

What's the easiest way to run the tests in teststubtest.py from the command line btw, for future reference?

@hauntsaninja
Copy link
Collaborator

pytest ./mypy/test/teststubtest.py should work :-)

@AlexWaygood
Copy link
Member Author

pytest ./mypy/test/teststubtest.py should work :-)

Thanks! I'm still relatively new to pytest 🤦‍♂️

@hauntsaninja hauntsaninja merged commit 777885f into python:master Feb 15, 2022
@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Feb 15, 2022

mypy's test setup is also very custom!

@AlexWaygood AlexWaygood deleted the stubtest-pos-only branch February 15, 2022 21:25
hauntsaninja pushed a commit to hauntsaninja/mypy that referenced this pull request Mar 5, 2022
hauntsaninja added a commit that referenced this pull request Mar 5, 2022
This reverts commit 777885f.

To help with python/typeshed#7441 and reduce risk of stubtest issues interfering with a mypy release. The extra type correctness here is really minimal.

Co-authored-by: hauntsaninja <>
hauntsaninja added a commit to hauntsaninja/typeshed that referenced this pull request May 18, 2024
It was decided we cared about this in python/mypy#12184
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.

3 participants