concurrent.futures: Bad signature for Executor.submit() under Python <= 3.8

[dalcinl]

Reproducer

python3.8 -m venv /tmp/py38
source /tmp/py38/bin/activate
pip -q install mypy
stubtest concurrent.futures

Tentative fix

diff --git a/stdlib/concurrent/futures/_base.pyi b/stdlib/concurrent/futures/_base.pyi
index 5b756d87..fc6a342c 100644
--- a/stdlib/concurrent/futures/_base.pyi
+++ b/stdlib/concurrent/futures/_base.pyi
@@ -60,7 +60,7 @@ class Future(Generic[_T]):
         def __class_getitem__(cls, item: Any) -> GenericAlias: ...
 
 class Executor:
-    if sys.version_info >= (3, 9):
+    if sys.version_info >= (3, 8):
         def submit(self, __fn: Callable[_P, _T], *args: _P.args, **kwargs: _P.kwargs) -> Future[_T]: ...
     else:
         def submit(self, fn: Callable[_P, _T], *args: _P.args, **kwargs: _P.kwargs) -> Future[_T]: ...

[srittau]

Looking at the source in 3.8, Python still seems to accept fn as a positional argument, but warns about it. So I'd argue that the current stub is correct:

https://github.com/python/cpython/blob/d35af52caae844cb4ea0aff06fa3fc5328708af1/Lib/concurrent/futures/_base.py#L558-L581

[dalcinl]

Python still seems to accept fn as a positional argument

I believe you meant "... as a KEYWORD argument".

Did you notice the last line?
https://github.com/python/cpython/blob/d35af52caae844cb4ea0aff06fa3fc5328708af1/Lib/concurrent/futures/_base.py#L581

Whether the current stub is correct or not is a bit open to debate. From the point of view of intention, looks like it is incorrect. However, the really pressing issue for me is to get mypy happy, otherwise I cannot typecheck my own code using concurrent.futures.

[JelleZijlstra]

It's hard to say what the right answer is because the method is abstract. However, the process pool executor does still accept fn as a kwarg: https://github.com/python/cpython/blob/d35af52caae844cb4ea0aff06fa3fc5328708af1/Lib/concurrent/futures/process.py#L617.

Can you clarify why your code breaks? Are you creating your own executor?

[dalcinl]

@JelleZijlstra Yes, I am subclassing the Executor class. But the root of the issue is not my own code. Please copy and paste the following reproducer in a terminal:

rm -rf /tmp/py38
python3.8 -m venv /tmp/py38
source /tmp/py38/bin/activate
pip -q install mypy
stubtest concurrent.futures

So mypy's stubtest tool fails to check typeshed's stdlib stubs. These issues percolate to my own code.

[dalcinl]

Note That in Python 3.8 we have the following signature override in the submit() method:
https://github.com/python/cpython/blob/v3.8.0/Lib/concurrent/futures/_base.py#L573

[JelleZijlstra]

stubtest is a great tool, but it can have false positives. The implementation of Executor.submit does accept fn as a kwarg in 3.8 even if stubtest doesn't see it. I'm still not clear on the concrete problem being solved here.

[dalcinl]

stubtest is a great tool, but it can have false positives

I don't think this is a false positive. IMHO, it is actually spotting an bug/inconsistency in typeshed stubs.

I'm still not clear on the concrete problem being solved here.

Python 3.8 effectively replaces the signature of Executor.submit(), as done in this line:
https://github.com/python/cpython/blob/v3.8.0/Lib/concurrent/futures/_base.py#L573
This mean that in the "official" signature for Py3.8, the fn argument is positional-only, thus the typeshed stub is incorrect (for this particular Python version).
Setting __text_signature__ in Python code is a blessed feature, please see python/cpython#80723 for further details.
Of course, the Py3.8 stdlib code stills support fn as a keyword argument for backward compatibility reasons.

[srittau]

typeshed generally favors the implementation over documentation and the implementation in 3.8 still allows a keyword argument.

But I also wonder why stubtest even complains about this, since the implementation uses *args and **kwargs and why the stubtest in our CI doesn't complain.

[hauntsaninja]

stubtest complains because CPython sets __text_signature__, which affects what inspect.signature sees. stubtest in CI doesn't complain because we don't check positional-only correctness for old Python versions (#3693).

I don't feel strongly, but I'd be okay changing this. At this point, it's probably valuable for users to know their code won't work on 3.9 and it looks like the associated CPython change was pushed through quickly and without too much pain (deprecated in 3.8, removed in 3.9).

[dalcinl]

@hauntsaninja I tried using typing.overload to special case Py3.8, but stubtests would still complain. Perhaps the proper way to go is to add the method overloads here in typeshed, and somehow fix stubtests to handle that even if __text_signature__ does not match one of the overloads. I can contribute the needed changes to typeshed, but I have no idea about the mypy/stubtests part.

[hauntsaninja]

I'm a little confused about what exactly your use case is; e.g. I'm not sure what overload you're trying to add, or why it would need changes to mypy / stubtest.

To be concrete, this is the change I thought we were discussing:

diff --git a/stdlib/concurrent/futures/_base.pyi b/stdlib/concurrent/futures/_base.pyi
index 5b756d87..303383a3 100644
--- a/stdlib/concurrent/futures/_base.pyi
+++ b/stdlib/concurrent/futures/_base.pyi
@@ -60,11 +60,7 @@ class Future(Generic[_T]):
         def __class_getitem__(cls, item: Any) -> GenericAlias: ...
 
 class Executor:
-    if sys.version_info >= (3, 9):
-        def submit(self, __fn: Callable[_P, _T], *args: _P.args, **kwargs: _P.kwargs) -> Future[_T]: ...
-    else:
-        def submit(self, fn: Callable[_P, _T], *args: _P.args, **kwargs: _P.kwargs) -> Future[_T]: ...
-
+    def submit(self, __fn: Callable[_P, _T], *args: _P.args, **kwargs: _P.kwargs) -> Future[_T]: ...
     def map(
         self, fn: Callable[..., _T], *iterables: Iterable[Any], timeout: float | None = ..., chunksize: int = ...
     ) -> Iterator[_T]: ...

[dalcinl]

To be concrete, this is the change I thought we were discussing:

Your change is not correct for Python 3.7 (and 3.6, although I know it is EOL). Look at my original patch in the description, I believe that's the proper fix in order to support all the old Pythons mypy supports.

Regarding overloads, I tried to special case 3.8 (using a elif sys.version_info >= (3, 8) branch), and use typing.overload to define two different submit() methods: One with fn positional-only, and another with fn as keyword-only, but I failed, stubtest would still complain. Anyway, such an aproach is probably overkill.

[hauntsaninja]

Okay, I see. Yeah, I don't think overloads are a good solution here (subclasses would still break LSP, stubtest would still complain, both of those are probably desirable). Like I said in a previous message, I'd be fine with changing the version check.

Also I can confirm that mypy will do the right thing with # type: ignore[override] in the mpi4py PR you link :-)

[dalcinl]

Also I can confirm that mypy will do the right thing with # type: ignore[override] in the mpi4py PR you link :-)

Where should I put the # type: ignore[override] comment?

Please note I'm already using that comment in a if py3.8 branch:
https://github.com/mpi4py/mpi4py/pull/198/files#diff-b2bc441b3e198ea6edbc835befb855e081c681ba6783db8142eecb6b06acbb9cR44

But stubtests is still issuing a warning and erroring, I'm silencing it via allowlist:
https://github.com/mpi4py/mpi4py/pull/198/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR173

The warning I'm getting is not from my own code, but from the abstract Executor class from concurrent.modules, as I argued above and you acknowledged. Looks like stubtests is checking not only the things that are defined within a module, but also external things that are imported and made public (via from Bar import Foo as Foo).

I cannot add # type: ignore[override] in typeshed, so here I am, submitting an issue and advocating for a one-line fix :-).