Improve types for subprocess module #1059

jboning · 2017-03-21T19:06:05Z

This adds union types in the constructor to account for parameters that
might be either byte strings or unicode strings. At the same time, this
removes specific types from the user-accessible fields, to avoid the
need for users to at every use site specify which types their particular
instance was instantiated with.

Fixes #1004

This adds union types in the constructor to account for parameters that might be either byte strings or unicode strings. At the same time, this *removes* specific types from the user-accessible fields, to avoid the need for users to at every use site specify which types their particular instance was instantiated with.

JelleZijlstra · 2017-03-21T19:11:37Z

stdlib/2/subprocess.pyi

    returncode = 0
-    cmd = ...  # type: str
-    output = ...  # type: str  # May be None
+    # morally: Union[str, bytes, List[str], List[bytes]]


str and bytes are the same thing in Python 2. You should use str and unicode, or bytes and Text if you want the same code in 2 and 3. (Maybe we can merge these stubs in the future?)

Oops! I guess I got mixed up because of the python 3 stub syntax.

JelleZijlstra · 2017-03-22T05:28:20Z

stdlib/2/subprocess.pyi

-    def __init__(self, returncode: int, cmd: str, output: Optional[str] = ...) -> None: ...
+    def __init__(self,
+                 returncode: int,
+                 cmd: Union[bytes, Text, List[bytes], List[Text]],


Instead of List, can't it be any Sequence? That's what check_output is annotated as.

Ah indeed! Should I modify check_output (and family) to support bytes, too?

Sure, please do. Also, I think the argument should be Sequence[Union[bytes, Text]], since mixing bytes and str seems to work fine. Using a Union as the type argument is OK because Sequence is covariant.

JelleZijlstra · 2017-03-22T05:28:37Z

stdlib/2/subprocess.pyi

    returncode = 0
-    cmd = ...  # type: str
-    output = ...  # type: str  # May be None
+    cmd = ...  # type: Any


I think the "moral" comments were actually helpful here.

OK, I'll add them back! I thought they might be redundant with the actual types in the constructor.

JelleZijlstra · 2017-03-22T05:29:45Z

Thanks! Sorry for not noticing the thing I commented on earlier.

This defines _TXT and _CMD aliases, and uses them everywhere applicable. Also brings the _FILE alias to python3.

jboning · 2017-03-23T22:23:57Z

stdlib/3/subprocess.pyi

+from typing import Sequence, Any, AnyStr, Mapping, Callable, Tuple, IO, Optional, Union, List, Type, Text
 from types import TracebackType

+_FILE = Union[int, IO[Any]]


I brought this _FILE thing over from the python 2 stubs, though I'm actually not sure it's correct.

You can actually pass file descriptors (i.e., ints) to the stdout and stderr arguments in both Python 2 and 3. Didn't know about this either, but it seems to work.

jboning · 2017-03-23T22:29:02Z

I've standardized on Union[bytes, Text] everywhere that strings are used in the subprocess module, since it seems happily to accept either, and Any everywhere that a return type might be either bytes or Text.

There's still a disorganized mixture of Optional and non-Optional arguments. Maybe I should also standardize this to have the explicit Optional everywhere? (Or maybe I should stop offering to incorporate more changes into this PR so that it can eventually get merged :P)

JelleZijlstra · 2017-03-23T22:38:01Z

Feel free to add more changes; I'll review it tonight and merge if I don't see any other issues.

JelleZijlstra · 2017-03-24T05:09:45Z

stdlib/2/subprocess.pyi

                 startupinfo: Any = ...,
-                 creationflags: int = ...) -> str: ...
+                 creationflags: int = ...
+                 ) -> Any: ...  # morally: _TXT


Can it really return unicode in py2? It returns str for me even with universal_newlines on. (In Python 3 it's bytes without universal_newlines and str with it.)

Indeed; fascinating! I didn't compare the documentation for the different versions closely enough. It looks like in python 2, the difference is that os.fdopen() is called with "U" in the mode to; but file objects with universal newlines turned still just return strings. I think that means I should convert all of the py2 output/stderr types to be bytes.

JelleZijlstra · 2017-03-24T05:11:36Z

stdlib/2/subprocess.pyi

    def wait(self) -> int: ...
-    def communicate(self, input: Optional[AnyStr] = ...) -> Tuple[Optional[bytes], Optional[bytes]]: ...
+    # morally: -> Tuple[_TXT, _TXT]
+    def communicate(self, input: Optional[_TXT] = ...) -> Tuple[Any, Any]: ...


I think the original return annotation was also correct here.

I think Any is preferable in the same way it is for the other sort of output annotations; with Optional, a user who knows they passed in a PIPE must explicitly check for None (or cast). I'm updating the descriptive comment to say the right thing though (the original annotation).

JelleZijlstra · 2017-03-24T05:13:50Z

stdlib/3/subprocess.pyi

+from typing import Sequence, Any, AnyStr, Mapping, Callable, Tuple, IO, Optional, Union, List, Type, Text
 from types import TracebackType

+_FILE = Union[int, IO[Any]]


You can actually pass file descriptors (i.e., ints) to the stdout and stderr arguments in both Python 2 and 3. Didn't know about this either, but it seems to work.

JelleZijlstra · 2017-03-24T05:16:25Z

stdlib/3/subprocess.pyi

+    def __init__(self,
+                 returncode: int,
+                 cmd: _CMD,
+                 output: Optional[_TXT] = ...,


Odd that this argument isn't called "stdout", but confirmed in the source that it's true.

yeah; I think that's because it predates the addition of "stderr", and the "stdout" alias was added at the same time for consistency.

…nyStr

JelleZijlstra · 2017-03-24T15:23:58Z

Merged! Thanks for all the fixes.

jboning · 2017-03-24T17:40:26Z

Thanks for the several rounds of review!

gvanrossum · 2017-03-24T21:42:18Z

Sadly now the mypy tests fail, like this:

PARALLEL 2
SUMMARY  139 tasks selected
passed 22, failed 1, pending 115; running 1                                     

FAILURE  #23 check file runtests.py

mypy/waiter.py:36: error: Argument 3 to "Popen" has incompatible type Dict[str, str]; expected Mapping[Union[bytes, str], Union[bytes, str]]
mypy/test/testpythoneval.py:123: error: Argument 2 to "Popen" has incompatible type Dict[str, str]; expected Mapping[Union[bytes, str], Union[bytes, str]]

passed 26, failed 2, pending 110; running 1                                     

FAILURE  #27 check package mypy

mypy/waiter.py:36: error: Argument 3 to "Popen" has incompatible type Dict[str, str]; expected Mapping[Union[bytes, str], Union[bytes, str]]
mypy/test/testpythoneval.py:123: error: Argument 2 to "Popen" has incompatible type Dict[str, str]; expected Mapping[Union[bytes, str], Union[bytes, str]]

passed 118, failed 3, pending 17; running 1                                     

FAILURE  #121 check stdlibsamples (3.2)

subprocess.py:1586: error: Need type annotation for variable
test/test_subprocess.py:1294: error: Need type annotation for variable

SUMMARY  3/139 tasks and 3/3981 tests failed
 FAILURE check file runtests.py
 FAILURE check package mypy
 FAILURE check stdlibsamples (3.2)
SUMMARY  3/139 tasks and 3/3981 tests failed
*** FAILURE ***

JelleZijlstra · 2017-03-24T21:58:28Z

Oh no :(. I guess it's because Mapping is covariant in the value type but not the key type (not sure why). If that's true, we should be able to fix by reverting the key type to str. I can confirm whether that works and submit a PR tonight.

jboning · 2017-03-24T22:03:48Z

The stdlibsamples test failures mystified me for a moment; but I think those are actually a result of #1080 now that there are more, and more specific, annotations for select().

JelleZijlstra · 2017-03-24T22:15:33Z

I just posted a PR in #1090 to add a tox.ini that includes a way to test mypy's test suite against typeshed. I'll send a followup PR later to include it in Travis. (I'd prefer to catch these issues before we merge.)

gvanrossum · 2017-03-24T22:33:09Z

FWIW all you need to do to repro this is mypy runtests.py.

jboning · 2017-03-24T22:39:18Z

Indeed, switching the key type back to str works. Union[Mapping[str, _TXT], Mapping[bytes, _TXT]] also makes the mypy tests pass. It seems like that might be better since it's a little more flexible; what do you think?

gvanrossum · 2017-03-24T22:44:01Z

I'll send a PR.

See #1059 (comment)

gvanrossum · 2017-03-24T22:46:14Z

#1091. (It does not fix the issue introduced by #1080, see discussion there.)

See #1059 (comment)

JelleZijlstra reviewed Mar 21, 2017

View reviewed changes

jboning added 2 commits March 21, 2017 12:22

remove 'moral' comments; add List import; change 'str' to 'Text'

3df2b13

import Text

07bf55d

JelleZijlstra requested changes Mar 22, 2017

View reviewed changes

jboning added 2 commits March 22, 2017 11:24

List -> Sequence; reinsert 'moral' comments

bd196b3

Regularize string types everywhere

09c99df

This defines _TXT and _CMD aliases, and uses them everywhere applicable. Also brings the _FILE alias to python3.

jboning commented Mar 23, 2017

View reviewed changes

jboning changed the title ~~Improve types for CalledProcessError~~ Improve types for subprocess module Mar 23, 2017

jboning added 2 commits March 23, 2017 15:32

fix typo; possibly fix indentation

7001078

remove trailing comma, which caused problems in python 2 tests

7ee363d

JelleZijlstra requested changes Mar 24, 2017

View reviewed changes

fix py2 outputs to be bytes; tweak descriptive comments; remove one A…

dddede9

…nyStr

JelleZijlstra approved these changes Mar 24, 2017

View reviewed changes

JelleZijlstra merged commit 62f57e4 into python:master Mar 24, 2017

jboning deleted the called-process-error branch March 24, 2017 17:40

gvanrossum mentioned this pull request Mar 24, 2017

[Reverted] Improved annotations for select.select() #1080

Merged

gvanrossum pushed a commit that referenced this pull request Mar 24, 2017

Make type of 'env' more flexible

b2ef811

See #1059 (comment)

gvanrossum mentioned this pull request Mar 24, 2017

Make type of 'env' more flexible #1091

Merged

JelleZijlstra pushed a commit that referenced this pull request Mar 24, 2017

Make type of 'env' more flexible (#1091)

fd77dd2

See #1059 (comment)

hswong3i pushed a commit to alvistack/python-typeshed that referenced this pull request May 25, 2025

ParamSpec: backport bpo-46676 (python#1059)

5be289a

Uh oh!

Improve types for subprocess module #1059

Improve types for subprocess module #1059

Uh oh!

Conversation

jboning commented Mar 21, 2017

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!

JelleZijlstra commented Mar 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jboning commented Mar 23, 2017

Uh oh!

JelleZijlstra commented Mar 23, 2017

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!

jboning Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

JelleZijlstra commented Mar 24, 2017

Uh oh!

jboning commented Mar 24, 2017

Uh oh!

gvanrossum commented Mar 24, 2017

Uh oh!

JelleZijlstra commented Mar 24, 2017

Uh oh!

jboning commented Mar 24, 2017

Uh oh!

JelleZijlstra commented Mar 24, 2017

Uh oh!

gvanrossum commented Mar 24, 2017

Uh oh!

jboning commented Mar 24, 2017

Uh oh!

gvanrossum commented Mar 24, 2017

Uh oh!

gvanrossum commented Mar 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jboning Mar 24, 2017 •

edited

Loading