Fix sys.stdout overriding in mypy.api (#6125)

[elkhadiy]

Overriding sys.stdout and sys.stderr in mypy.api is not threadsafe.
This causes problems sometimes when using the api in pyls for example.

I am struggling a little with getting the right type for f, was expecting something like Union[Callable[[TextIO, TextIO], None], Callable[[], None]] to work.

[msullivan]

Thanks for this! I left some comments, and additionally:

There are some things this misses:

• There are a handful of error message prints in build.py
• In errors.py, there are a bunch of prints in report_internal_error for displaying a traceback. (I think that a case /could/ be made for having those bypass the API capture, though we'd probably also want to arrange for the failure to bypass that as well? By using a MypyInternalError exception that we turn into sys.exit(2) in __main__.py but let propagate in the api?)
• The argparse.ArgumentParser argument processing will print to sys.stderr. To avoid this I think we need to create a subclass that overrides the error method.
• Possibly others?

This is all pretty doable, but it makes me nervous about missing things.

It makes me wonder if the stable API ought to just use subprocess and invoke mypy in a fresh interpreter? It would be slower, but easier to make correct.
(Of course, I added mypy.api.run_dmypy specifically to avoid the performance penalty associated with interpreter startup, but we could make that an unstable API and document its thread safety limitations.)

[msullivan]

We actually do need to worry about mypy.dmypy, since it does have this API (which has a Dropbox internal client).

If you don't want to fix mypy.dmypy now, it is fine to leave run_dmypy API thread-safety as a TODO, but in that case we need to continue hijacking sys.stdout and sys.stderr in run_dmypy.

[msullivan]

We shouldn't use sys.stdout as a default argument value, because it is evaluated at module load time, not when the function is called. This means that if something else hijacks stdout, we will ignore that and write to the original one.

In cases where it needs to have a default value, have it be an Optional[TextIO] defaulting to None, and do something along the lines of stdout = stdout or sys.stdout. I think a bunch of functions can just be changed to have it not have a default value.

[msullivan]

I'd drop the stdout argument to this function and all the functions it call that don't need it.

[elkhadiy]

• There are a handful of error message prints in build.py

Thanks I'll check that and try and look around to see if we missed anything else.

• In errors.py, there are a bunch of prints in report_internal_error for displaying a traceback. (I think that a case /could/ be made for having those bypass the API capture, though we'd probably also want to arrange for the failure to bypass that as well? By using a MypyInternalError exception that we turn into sys.exit(2) in __main__.py but let propagate in the api?)

Propagating exceptions through the api sounds like a wise decision. For example in mypy/api.py:1036:

def fail(msg: str, stderr: TextIO) -> None:
    stderr.write('%s\n' % msg)
    sys.exit(1)

Would it be better to raise an exception instead of sys.exit(1)?

• The argparse.ArgumentParser argument processing will print to sys.stderr. To avoid this I think we need to create a subclass that overrides the error method.

Ah yes this little guy https://docs.python.org/dev/library/argparse.html#argparse.ArgumentParser.error needs to change.

It makes me wonder if the stable API ought to just use subprocess and invoke mypy in a fresh interpreter? It would be slower, but easier to make correct.

This is actually what we were first planning to do in https://github.com/tomv564/pyls-mypy/blob/master/pyls_mypy/plugin.py#L56, with something like:

res = subprocess.run(
    ["python", "-m", "mypy", *args],
    stdout=subprocess.PIPE, stderr=subprocess.PIPE
    )
report = res.stdout
errors = res.stderr

(Of course, I added mypy.api.run_dmypy specifically to avoid the performance penalty associated with interpreter startup, but we could make that an unstable API and document its thread safety limitations.)

This could also be a solution, I like it! Have the normal mypy.api.run be threadsafe and potentially slower and propose mypy.api.run_dmypy as an unstable alternative were we continue hijacking stdout and stderr while potentially ironing out missing things.

Let me first work with your comments and we can go from there.

[gvanrossum]

If you’re just going to use subprocess, why bother with mypy.api at all?

[mr-c]

Should this be closed in light of #6750 being merged?

[gvanrossum]

Superseded by #6750.