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

Stricter settings in mypy_test #9294

Merged
merged 5 commits into from
Dec 2, 2022

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Nov 28, 2022

I've noticed some blank # type: ignore comments. For the sake of being consistent, explicit and strict, I've added their missing error codes and added "--enable-error-code ignore-without-code" & --warn-unused-ignores.

In the same vein, mypy_test.py can be made stricter, I've used --strict in favor of:

--warn-unused-ignores
--no-implicit-optional
--disallow-untyped-decorators
--disallow-any-generics
--strict-equality

(source: https://mypy.readthedocs.io/en/stable/existing_code.html#introduce-stricter-options)

Once #5768 is completed, we should be able to drop --allow-subclassing-any as well.

@AlexWaygood
Copy link
Member

Ah, removing mypy error codes from the test cases was a deliberate policy decision we made a few months ago; see the justification laid out in

I suppose we could document this, but the README for the test_cases directory is already a bit too long for my liking :/

@Avasam
Copy link
Collaborator Author

Avasam commented Nov 28, 2022

No problem! Thanks for the source. I had a feeling about it. Maybe a simple comment in the mypy config (well, in the args list, it's not really a config file) would be sufficient.

Thoughts on stricter mypy_test?

@AlexWaygood AlexWaygood changed the title Stricter type: ignore comments and mypy_test Stricter settings in mypy_test Nov 28, 2022
@AlexWaygood
Copy link
Member

This might not make our tests that much stricter. The issue is that mypy does some pretty cursèd special-casing for files that it detects are in a "typeshed directory". A side effect of that special-casing is, unfortunately, that a bunch of the stricter mypy settings just don't work on files in a "typeshed directory" (and it's obviously pretty hard to escape being in a typeshed directory when you're working on files in this repo). For example, if I apply this diff:

diff --git a/stdlib/builtins.pyi b/stdlib/builtins.pyi
index df74a00a4..62dd12145 100644
--- a/stdlib/builtins.pyi
+++ b/stdlib/builtins.pyi
@@ -67,7 +67,7 @@ _KT = TypeVar("_KT")
 _VT = TypeVar("_VT")
 _S = TypeVar("_S")
 _T1 = TypeVar("_T1")
-_T2 = TypeVar("_T2")
+_T2 = TypeVar("_T2")  # type: ignore
 _T3 = TypeVar("_T3")
 _T4 = TypeVar("_T4")
 _T5 = TypeVar("_T5")
diff --git a/tests/mypy_test.py b/tests/mypy_test.py
index a0069ae69..986ac8f82 100644
--- a/tests/mypy_test.py
+++ b/tests/mypy_test.py
@@ -259,6 +259,7 @@ def get_mypy_flags(args: TestConfig, temp_name: str, *, testing_stdlib: bool) ->
         "ignore-without-code",
         "--config-file",
         temp_name,
+        "--warn-unused-ignores",
     ]
     if not testing_stdlib:
         flags.append("--explicit-package-bases")

Then, sadly, mypy_test.py still passes just fine:

(.venv) C:\Users\alexw\coding\typeshed>python tests/mypy_test.py -p3.11 stdlib
*** Testing Python 3.11 on win32
Testing stdlib (489 files)...
Running mypy --python-version 3.11 --show-traceback --warn-incomplete-stub --show-error-codes --no-error-summary --platform win32 --no-site-packages --custom-typeshed-dir C:\Users\alexw\coding\typeshed --no-implicit-optional --disallow-untyped-decorators --disallow-any-generics --strict-equality --enable-error-code ignore-without-code --config-file /tmp/... --warn-unused-ignores
success

--- success, 489 files checked ---

Having said that, I'm not sure there's any reason not to do this. It might mean that mypy_test.py suddenly starts failing with a future mypy upgrade, if mypy improves the way it special-cases "typeshed directories" at some point in the future. But we can probably cross that bridge when we come to it.

@Avasam
Copy link
Collaborator Author

Avasam commented Nov 30, 2022

I didn't know about that special casing!
In any case this change is basically just the same thing as #9293, but for mypy.

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 30, 2022

The "typeshed_dir" special-casing is why we have to copy the whole test_cases directory into a tempdir in regr_test.py before running mypy on the test cases — --warn-unused-ignores is crucial for that test, and there's no other way I could think of to get it to work in this repo 🙃

@AlexWaygood AlexWaygood merged commit c6b9b4c into python:main Dec 2, 2022
@Avasam Avasam deleted the stricter-ignore-comments branch February 29, 2024 00:46
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