Skip to content

BinaryIO does not have peek method #3951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vegarsti opened this issue Apr 29, 2020 · 7 comments · Fixed by #4146
Closed

BinaryIO does not have peek method #3951

vegarsti opened this issue Apr 29, 2020 · 7 comments · Fixed by #4146
Labels
topic: io I/O related issues

Comments

@vegarsti
Copy link
Contributor

vegarsti commented Apr 29, 2020

When opening a file with binary mode, typing.BinaryIO is inferred.
This type should have the https://docs.python.org/3/library/io.html#io.BufferedReader.peek method.
It looks like this is known, judging by the TODO on this line in the typing.pyi stub:

# TODO peek?

(There are also other TODOs there for BinaryIO - if those should be added, I could of course do those in the same change if that's not a problem.)

A reproducible example:

with open("some-file", 'rb') as f:
    f.peek()

Running mypy on this results in error: "BinaryIO" has no attribute "peek".

This issue has also been reported earlier in mypy.

I'd be glad to submit a PR for this. Is it as simple as adding peek to BinaryIO in typing.pyi? Or should there also be tests for these changes?

@JelleZijlstra
Copy link
Member

This is probably fine to add. The trouble with the IO protocols is that they are fairly ill-defined, in that there are lots of variations in exactly what methods different IO classes have, so it's tricky to say exactly which methods should be on each IO protocol. However, given that this method exists on the return type of open(), I agree we should add it.

No tests are necessary.

@vegarsti
Copy link
Contributor Author

I see. Thanks for the quick reply!

@vegarsti
Copy link
Contributor Author

@JelleZijlstra FYI, I opened a PR (sorry if you already got a notification from the mention there)

@srittau
Copy link
Collaborator

srittau commented Apr 30, 2020

I didn't see this discussion here, so I answered in the PR. But quickly: I don't think we should add BinaryIO.peek() as the "implementation" in typing.BinaryIO does not have it. Also, open() in the stubs returns an IO[Any] for Python 3. Maybe it's time we added a custom sub-type to be returned by open() (deriving from IO/BinaryIO)?

@JelleZijlstra
Copy link
Member

I feel like the right solution is to make the open() stubs reflect the types that open() actually returns, which are various classes in io:

In [13]: type(open('/dev/null', 'w'))                                                                                                                                                
Out[13]: _io.TextIOWrapper

In [14]: type(open('/dev/null', 'wb'))                                                                                                                                               
Out[14]: _io.BufferedWriter

In [15]: type(open('/dev/null', 'r+'))                                                                                                                                               
Out[15]: _io.TextIOWrapper

In [16]: type(open('/dev/null', 'rb+'))                                                                                                                                              
Out[16]: _io.BufferedRandom

In [17]: type(open('/dev/null', 'w+'))                                                                                                                                               
Out[17]: _io.TextIOWrapper

This would require making these classes inherit from TextIO or BinaryIO, which they currently don't do.

I can look into this after #3371 and #4082 (which touch some of the same code) are merged.

@vegarsti
Copy link
Contributor Author

Agreed, that sounds like a much better solution! Let me know if I can be of help!

@srittau srittau added the topic: io I/O related issues label May 28, 2020
@JelleZijlstra
Copy link
Member

JelleZijlstra commented May 30, 2020

Here's what open() returns:

  • If text mode, always TextIOWrapper
  • If unbuffered, FileIO. This appears to map directly to the buffering= keyword argument to open() but the logic is fairly complicated.
  • Else BufferedReader, BufferedWriter or BufferedRandom depending on mode.

The code is at https://github.com/python/cpython/blob/master/Modules/_io/_iomodule.c#L231.

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this issue May 30, 2020
This makes these classes usable if type annotations are given as "IO"
or "TextIO". In the future, we'll then be able to move open() to
return a concrete class instead (python#3951).
JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this issue May 30, 2020
This makes these classes usable if type annotations are given as "IO"
or "TextIO". In the future, we'll then be able to move open() to
return a concrete class instead (python#3951).
JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this issue May 30, 2020
Fixes python#3951.

We use the values of the "mode" and "buffering" arguments to figure out
the concrete type open() will return at runtime. (Compare the CPython
code in https://github.com/python/cpython/blob/master/Modules/_io/_iomodule.c#L231.)

I tested by writing a script that generated every mode/buffering combination, then
running mypy with the open plugin disabled and comparing the results.

This PR depends on python#4145.
JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this issue May 30, 2020
Fixes python#3951.

We use the values of the "mode" and "buffering" arguments to figure out
the concrete type open() will return at runtime. (Compare the CPython
code in https://github.com/python/cpython/blob/master/Modules/_io/_iomodule.c#L231.)

I tested by writing a script that generated every mode/buffering combination, then
running mypy with the open plugin disabled and comparing the results.

This PR depends on python#4145.
JelleZijlstra added a commit that referenced this issue May 30, 2020
This makes these classes usable if type annotations are given as "IO"
or "TextIO". In the future, we'll then be able to move open() to
return a concrete class instead (#3951).
srittau pushed a commit that referenced this issue May 31, 2020
* make io classes inherit from typing IO classes

This makes these classes usable if type annotations are given as "IO"
or "TextIO". In the future, we'll then be able to move open() to
return a concrete class instead (#3951).

* open: introduce concrete return types

Fixes #3951.

We use the values of the "mode" and "buffering" arguments to figure out
the concrete type open() will return at runtime. (Compare the CPython
code in https://github.com/python/cpython/blob/master/Modules/_io/_iomodule.c#L231.)
vishalkuo pushed a commit to vishalkuo/typeshed that referenced this issue Jun 26, 2020
This makes these classes usable if type annotations are given as "IO"
or "TextIO". In the future, we'll then be able to move open() to
return a concrete class instead (python#3951).
vishalkuo pushed a commit to vishalkuo/typeshed that referenced this issue Jun 26, 2020
* make io classes inherit from typing IO classes

This makes these classes usable if type annotations are given as "IO"
or "TextIO". In the future, we'll then be able to move open() to
return a concrete class instead (python#3951).

* open: introduce concrete return types

Fixes python#3951.

We use the values of the "mode" and "buffering" arguments to figure out
the concrete type open() will return at runtime. (Compare the CPython
code in https://github.com/python/cpython/blob/master/Modules/_io/_iomodule.c#L231.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: io I/O related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants