Run mypy on `Lib/test/libregrtest` in CI #109382

AlexWaygood · 2023-09-13T16:41:40Z

@vstinner: this is essentially the minimum set of changes that are required to get mypy to pass when run on libregrtest. Let me know what you think!

I think it might be good to post on Discourse and check that everybody's okay with this idea, before we go through with it. This would be the first time we'd be running mypy on a project inside the Lib/ directory; all existing mypy-checked projects live inside Tools/.

vstinner

Most libregrtest/ changes LGTM, but I'm not sure about coding style changes. Can you please create a PR just for this change, and then update this PR to just add the infra to run mypy?

Lib/test/test_audit.py

AlexWaygood · 2023-09-13T17:07:38Z

Most libregrtest/ changes LGTM, but I'm not sure about coding style changes.

Which coding style changes do you dislike? I tried hard not to make any changes in this PR unless they were strictly required to make the mypy check pass (which is why I think it's good to introduce mypy to CI in the same PR: it validates that the changes in this PR are correct, at least from mypy's perspective). If you highlight the changes you're not sure about, hopefully I can explain why they were necessary to make mypy happy ;-)

vstinner · 2023-09-13T17:32:57Z

I don't get how replacing self.workers with workers is needed by mypy for example.

AlexWaygood · 2023-09-13T17:39:58Z

I don't get how replacing self.workers with workers is needed by mypy for example.

Good point, those changes aren't needed anymore. I think I needed them in an earlier version of this PR, but mypy now seems happy without them :-)

I've reverted those, thanks!

vstinner

Sorry, this change is too big to be reviewed. Please extract non-controversial changes: remove .github/workflows/mypy.yml changes and the lines where I added comments :-) I mean, please create a new PR with the most obvious non-controversial type hint changes.

vstinner · 2023-09-14T07:53:27Z

Lib/test/libregrtest/results.py

@@ -1,4 +1,5 @@
 import sys
+import xml.etree.ElementTree as ET


Please don't do that. I want to minimize imports at startup, to minimize how many modules are imported when a test is run. Maybe even add a comment to explain why the import is not done at top level, that it's a deliberate choice.

Okay. The reason why I did this was to accurately add type hints for the TestResults.testsuite_xml attribute on line 35. The current type hints for that attribute are incorrect, and caused several mypy errors.

A possible workaround is to do the top-level import inside an if TYPE_CHECKING block.

A possible workaround is to do the top-level import inside an if TYPE_CHECKING block.

I made that change in 581ef0d; let me know what you think!

Lib/test/libregrtest/setup.py

Lib/test/libregrtest/single.py

vstinner · 2023-09-14T08:01:18Z

Lib/test/libregrtest/single.py

@@ -184,6 +187,11 @@ def _runtest_env_changed_exc(result: TestResult, runtests: RunTests,
        result.state = State.PASSED


+class PrintWarningsType(Protocol):
+    orig_stderr: io.TextIOWrapper
+    def __call__(self, msg: str) -> None: ...


If possible, I would prefer avoiding to have to add glue on top of code just to please mypy. If print_warning() API is too weird for mypy, maybe rewrite it with such class directly in test.support, no?

If print_warning() API is too weird for mypy, maybe rewrite it with such class directly in test.support, no?

Unfortunately not. Mypy doesn't know that the test.support module exists: when it comes to the stdlib, it only knows about the modules we include stubs for in typeshed. Historically, we haven't included stubs for any modules in the Lib/test/ directory, as these aren't available on all builds of Python, and are generally only meant to be used by CPython core devs like us. As a result, everything that is imported from test.support is inferred by mypy as being of type Any.

However, that also means that we don't need this protocol here: I only added it to give mypy a little more information, and make the code slightly more type safe. Since you don't like it, I'll remove it!

Mypy doesn't know that the test.support module exists

Can't we make mypy aware of test.support, instead of having to put ducktape to workaround these limitations?

Can't we make mypy aware of test.support

This is somewhat tricky due to the fact that regrtest is in the Lib/test/ directory. Mypy hates being run on stuff inside the Lib/ directory, as it thinks that everything inside the Lib/ directory is "shadowing the stdlib". The only way we're managing to get mypy happy with being run on libregrtest is by cd-ing into Lib/test/, and then running mypy with very specific options so that it doesn't look anywhere else in the Lib/ directory, it only looks in Lib/test/libregrtest.

There would be some ways of making mypy aware of test.support, but I'm not sure I like any of them:

We could write a custom script that copies libregrtest and test.support into a temporary directory, then runs mypy on them together. Why I don't like this: it's easier for users if they can run mypy just by invoking mypy on the command line. Having to run mypy via a wrapper script is a bad user experience, and it's more code for us to maintain as well.

We could add stubs for test.support in typeshed, so that mypy sees test.support as "just another part of the stdlib". Why I don't like this: lots of Python users can't access test.support on their installed versions of Python, but if we add stubs for test.support in typeshed, mypy will never complain if users import test.support in their own code. Also, it will be a lot of work for us typeshed maintainers to maintain stubs for test.support, since the test.support module tends to change its API a lot more frequently than other things in the Lib/ directory.

We could add stubs for test.support inside the libregrtest directory itself. Why I don't like this: this just feels like it would be really ugly, and really hard to maintain. The stubs would go out of date really quickly, if we weren't careful.

We could write a custom script that copies libregrtest and test.support into a temporary directory

That sounds like a good idea to me.

I would prefer that mypy users need workaround, than libregrtest have to write complicated workarounds in the working code just to please mypy.

vstinner · 2023-09-14T08:02:41Z

Lib/test/libregrtest/single.py

@@ -195,7 +203,7 @@ def _runtest(result: TestResult, runtests: RunTests) -> None:
        timeout is not None and threading_helper.can_start_thread
    )
    if use_timeout:
-        faulthandler.dump_traceback_later(timeout, exit=True)
+        faulthandler.dump_traceback_later(cast(float, timeout), exit=True)


I don't understand this cast. timeout is a float is use_timeout is True. Why is a cast needed?

Yes, this code is type-safe. The issue is that mypy only understands a limited set of type narrowing operations, and the way timeout has been narrowed to be a float here isn't one of them.

Mypy understands this kind of logic:

x: float | None if x is not None: # OK, mypy understands `f` has to be a float here

But this kind of thing is too complex for mypy:

x: float | None x_is_None = x is None if not x_is_None: # mypy will not understand that x has been safely narrowed to `None` here

Another way we could make mypy happy would be to add an assert, like this -- but it feels redundant to me, since the type of timeout has already been narrowed to be a float in the lines immediately preceding this:

Suggested change

faulthandler.dump_traceback_later(cast(float, timeout), exit=True)

assert timeout is not None

faulthandler.dump_traceback_later(timeout, exit=True)

vstinner · 2023-09-14T08:04:09Z

Lib/test/libregrtest/worker.py

@@ -67,7 +68,8 @@ def worker_process(worker_json: StrJSON) -> NoReturn:
    runtests = RunTests.from_json(worker_json)
    test_name = runtests.tests[0]
    match_tests: FilterTuple | None = runtests.match_tests
-    json_file: JsonFile = runtests.json_file
+    json_file = runtests.json_file
+    assert isinstance(json_file, JsonFile)


I dislike assert, it's removed by python -O. Why is this assertion needed? The attribute is declared as json_file: JsonFile | None. Is it to remove the None case?

If mypy doesn't understand the logic, maybe a developer can have the same doubt. Maybe add a regular if json_file is None: raise ....

Or maybe, RunTests can have a sub-type which adds the json_file attribute: the class used by create_worker_process() / worker_process(). Would it please mypy?

Why is this assertion needed? The attribute is declared as json_file: JsonFile | None. Is it to remove the None case?

Yes, it's to remove the None case.

If mypy doesn't understand the logic, maybe a developer can have the same doubt. Maybe add a regular if json_file is None: raise ....

Sure. Should I do the same for all asserts being proposed in this PR?

Or maybe, RunTests can have a sub-type which adds the json_file attribute: the class used by create_worker_process() / worker_process(). Would it please mypy?

Yes, that might also make mypy happier. Hard to tell without seeing a PR and running mypy on it, though :-)

AlexWaygood · 2023-09-14T13:33:37Z

please create a new PR with the most obvious non-controversial type hint changes.

Okay, I've had a go at doing that in #109405

vstinner · 2023-09-15T13:15:19Z

Can you rebase your PR on the main branch?

AlexWaygood · 2023-09-15T13:27:12Z

I'll close this for now, and continue to break it up into smaller PRs so that it's obvious what I'm doing at each stage 👍

vstinner · 2023-09-15T14:36:44Z

Nice! I like this idea :-)

Run mypy on Lib/test/libregrtest in CI

24db8b9

AlexWaygood requested a review from vstinner September 13, 2023 16:41

AlexWaygood added the skip news label Sep 13, 2023

vstinner reviewed Sep 13, 2023

View reviewed changes

Lib/test/test_audit.py Outdated Show resolved Hide resolved

AlexWaygood added 4 commits September 13, 2023 17:52

fixes

c217d64

improve workflow file

73ac976

nit

ad42b09

minimize diff

a637bce

AlexWaygood added 2 commits September 13, 2023 18:21

Add instructions to the mypy.ini file on how to run mypy

e1b0e4a

Improve workflow file

267bc83

Revert some unnecessary changes

d24bcf9

vstinner reviewed Sep 14, 2023

View reviewed changes

AlexWaygood added 5 commits September 14, 2023 13:25

PEP 585

87c3e8e

Add comment to type: ignore comment on case TestResults()

800d080

Remove PrintWarningsType

98d3000

results.py: Don't import xml.etree.ElementTree in the global scope

581ef0d

minimize diff

89e4a77

AlexWaygood mentioned this pull request Sep 14, 2023

gh-109413: libregrtest: Add and improve type annotations #109405

Merged

AlexWaygood closed this Sep 15, 2023

AlexWaygood deleted the regrtest-mypy-2 branch September 15, 2023 13:27

AlexWaygood restored the regrtest-mypy-2 branch September 15, 2023 13:27

AlexWaygood mentioned this pull request Sep 15, 2023

gh-109413: Add a custom script for running mypy on libregrtest #109464

Closed

AlexWaygood deleted the regrtest-mypy-2 branch March 7, 2024 14:36

	faulthandler.dump_traceback_later(cast(float, timeout), exit=True)
	assert timeout is not None
	faulthandler.dump_traceback_later(timeout, exit=True)

Uh oh!

Run mypy on Lib/test/libregrtest in CI #109382

Run mypy on Lib/test/libregrtest in CI #109382

Uh oh!

Conversation

AlexWaygood commented Sep 13, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexWaygood commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Sep 13, 2023

Uh oh!

AlexWaygood commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Sep 14, 2023

Uh oh!

vstinner commented Sep 15, 2023

Uh oh!

AlexWaygood commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Sep 15, 2023

Uh oh!

Uh oh!

Run mypy on `Lib/test/libregrtest` in CI #109382

Run mypy on `Lib/test/libregrtest` in CI #109382

AlexWaygood commented Sep 13, 2023 •

edited

Loading

AlexWaygood commented Sep 13, 2023 •

edited

Loading

AlexWaygood commented Sep 15, 2023 •

edited

Loading