More precise return types for open(), Path.open(), bz2.open(), etc.

[ilai-deutel]

This PR adds overloaded signatures for {builtin,pathlib.Path,bz2,gwip,lzma}.open() functions, using Literal modes to determine the return types.

mypy tests pass after updating the open and Path.open plugins and making a few changes to the expected outputs: python/mypy#7794
Fixes#2911

Related discussion: python/mypy#7643

[srittau]

Thank you, that is very useful! Without doing a full review yet, I think it would make sense to consolidate the _OPEN_TEXT_MODE and _OPEN_BINARY_MODE constants in one file, at least where they are equal.

[ilai-deutel]

Do you have any suggestion on where to put these constants? When consolidating the constants in builtins.pyi and importing them in pathlib.pyi, everything seems to be working except the pytype tests.

Ran pytype with 1655 pyis, got 4 errors.
stdlib/3/pathlib.pyi: pytype.load_pytd.BadDependencyError: No _OPEN_TEXT_MODE in module __builtin__, referenced from 'pathlib'
stdlib/3/zipapp.pyi: pytype.load_pytd.BadDependencyError: No _OPEN_TEXT_MODE in module __builtin__, referenced from 'pathlib'
third_party/2/pathlib2.pyi: pytype.load_pytd.BadDependencyError: No _OPEN_TEXT_MODE in module __builtin__, referenced from 'pathlib2'
third_party/2and3/toml.pyi: pytype.load_pytd.BadDependencyError: No _OPEN_TEXT_MODE in module __builtin__, referenced from 'pathlib'

I fixed that issue in 0335f3f by defining these constants in pathlib.pyi as well instead of importing them, it is less ideal than keeping them in one place.

[srittau]

I'd probably put them into os/__init__.pyi, since while open is a builtin, conceptually it matches many of the function there. I have no real preference, though.

[ilai-deutel]

@srittau Actually what about io, since io.open == open but os.open != open?

[srittau]

Good idea!

[ilai-deutel]

Moved to io

[JelleZijlstra]

There's some merge conflicts and a CI failure, could you take a look?

[ilai-deutel]

mypy_selftest fails because this PR changes the behavior of mypy.
For instance, reveal_type(open()) now produces error: All overload variants of "open" require at least one argument instead of Too few arguments for "open".

ilai-deutel/mypy@b5f8f01 fixes the mypy tests. I was waiting for this PR to be merged before opening one on the mypy repo, but I just went ahead and created it (python/mypy#7794) so that we can check that mypy tests pass on Travis CI.

[JelleZijlstra]

Thanks! We'll need to make simultaneous merges in both repos. I can do that in a few days.

[bluetech]

This is impressive work! I only checked the Python 3 builtins.open() stubs, they look correct to me.

I wonder if you considered making it return the actual types, instead of the IO protocol? Specifically, the open()documentation describes the return type as follows:

---

The type of file object returned by the open() function depends on the mode. When open() is used to open a file in a text mode ('w', 'r', 'wt', 'rt', etc.), it returns a subclass of io.TextIOBase (specifically io.TextIOWrapper). When used to open a file in a binary mode with buffering, the returned class is a subclass of io.BufferedIOBase. The exact class varies: in read binary mode, it returns an io.BufferedReader; in write binary and append binary modes, it returns an io.BufferedWriter, and in read/write mode, it returns an io.BufferedRandom. When buffering is disabled, the raw stream, a subclass of io.RawIOBase, io.FileIO, is returned.

---

By my count, encoding it fully would require 5(!) overloads, but for such an important function it might be worthwhile.

[ilai-deutel]

@bluetechio.BufferedReader, io.BufferedWriter, io.BufferedRandom and io.FileIO are not subclasses of BinaryIO, so I think that existing code that expects BinaryIO would fail.

For instance, mypy currently returns no error with the following code:

importiofromclick.testingimportEchoingStdinf=open('a', 'rb')
g=open('b', 'wb')
EchoingStdin(input=f, output=g)

However, if f is a BufferedReader and g a BufferedWriter, mypy will complain:

error: Argument "input" to "EchoingStdin" has incompatible type "BufferedReader"; expected "BinaryIO"
error: Argument "output" to "EchoingStdin" has incompatible type "BufferedWriter"; expected "BinaryIO"

See also #1229

Perhaps these concrete classes should inherit from BinaryIO, there might a reason why they don't.

[bluetech]

@ilai-deutel Right.

Perhaps these concrete classes should inherit from BinaryIO, there might a reason why they don't.

I tried it. There are several issues, fixable except one: FileIO wants to inherit from both RawIOBase and BinaryIO. But RawIOBase can return None from read and write (if it is not possible to read/write in non-blocking mode), while BinaryIO doesn't allow that. This is actually a problem with current stubs -- can get None when type says it can't. One solution might be to just not have FileIO implement BinaryIO.

---

Anyway, this is probably already a separate discussion from this PR. Trying to switch to concrete types can be done after this PR is merged.

[JelleZijlstra]

Sorry for letting this slide! There's a merge conflict now.

[JelleZijlstra]

I fixed the merge conflict locally (https://github.com/JelleZijlstra/typeshed/tree/ilai-deutel-overloaded-open) but it looks like I don't have permission to push to the PR branch. I'll probably just open a separate PR from this branch. I also submitted python/mypy#8908 to remove the failing test from mypy, since it doesn't seem useful.

[ilai-deutel]

Sorry for letting this slide!

No worries!

I fixed the merge conflict locally (https://github.com/JelleZijlstra/typeshed/tree/ilai-deutel-overloaded-open) but it looks like I don't have permission to push to the PR branch. I'll probably just open a separate PR from this branch. I also submitted python/mypy#8908 to remove the failing test from mypy, since it doesn't seem useful.

You should now have write access to my branch, but feel free to open a separate PR with your existing branch if it's easier

[JelleZijlstra]

To github.com:ilai-deutel/typeshed.git
! [remote rejected] ilai-deutel-overloaded-open -> overloaded-open (permission denied)
error: failed to push some refs to 'git@github.com:ilai-deutel/typeshed.git'

Didn't work unfortunately.

[ilai-deutel]

Looks like it's a GitHub issue, I get a javascript error (Uncaught QueryError) when I click the "Allow edits by maintainers" checkbox.

I pushed your merge commits to this branch

[srittau]

I think this is ready for the merge dance with mypy. I can't do it myself, unfortunately. A few remarks below, but those are not showstoppers.

[srittau]

Union return types are usually problematic, since they force the use to assert or cast the return value, even if the user knows the return type. In this particular case I actually like the returned Union, since the last overload is only used if mode is dynamic, so type checking at runtime makes sence.

[JelleZijlstra]

CI is green now after I removed the mypy test, so merging this won't require any complications.

[JelleZijlstra]

🎉