Skip to content

Make BytesIO inherit from BufferedIOBase. #4082

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

Merged
merged 12 commits into from
May 29, 2020
Merged

Make BytesIO inherit from BufferedIOBase. #4082

merged 12 commits into from
May 29, 2020

Conversation

tarcisioe
Copy link
Contributor

An attempt at #4075.

@tarcisioe tarcisioe marked this pull request as draft May 26, 2020 01:50
@JelleZijlstra
Copy link
Member

The error messages from mypy selftest are quite concerning: test-data/stdlib-samples/3.2/test/test_base64.py:60: error: Argument 2 to "decode" has incompatible type "BytesIO"; expected "IO[bytes]". That means BytesIO is no longer compatible with IO[bytes], which seems likely to break a lot of real-world code using BytesIO. What happens if you make it inherit from IO[bytes] as well as BufferedIOBase?

@tarcisioe
Copy link
Contributor Author

tarcisioe commented May 26, 2020

I'll try that. I removed the explicit inheritance from BinaryIO because other classes there which should be compatible (afaik), such as BufferedReader don't have it.

I'm not really sure how that works, though. I'm unsure how mypy (or other type-checkers) handle that since AFAIK IO is not a Protocol.

@tarcisioe
Copy link
Contributor Author

stdlib/3/io.pyi:80: error: Definition of "__enter__" in base class "IOBase" is incompatible with definition in base class "IO"

I find that a bit weird, since IOBase's __enter__ is typed as

    def __enter__(self: _T) -> _T: ...

Where _T is _T = TypeVar('_T', bound=IOBase), thus for BytesIO it should return BytesIO, which by definition now is IO[bytes], which should therefore satisfy the definition from IO:

    @abstractmethod
    def __enter__(self) -> 'IO[AnyStr]':
        pass

I mean, at least in my mind, it should. Maybe I'm misinterpreting how mypy computes it.

@JelleZijlstra
Copy link
Member

Can you try putting def __enter__(self: _T) -> _T: ... explicitly in BytesIO?

@JelleZijlstra
Copy link
Member

Now there's just a minor discrepancy in the argument name of seek, which should be __pos instead of __offset.

The argument is positional-only though, so I'm not quite sure why stubtest complains about it. @hauntsaninja is that intentional?

@tarcisioe
Copy link
Contributor Author

tarcisioe commented May 26, 2020

Maybe that's because it should be positional-only in the stub and isn't? It seems it's only positional-only at runtime.

Also there are other issues on Python 3.5 and 3.6 apparently:

error: io.BytesIO.read1 is inconsistent, stub argument "__size" has a default value but runtime argument does not

Stub: at line 66

def (self: io.BufferedIOBase, builtins.int =) -> builtins.bytes

Runtime:

def (self, size, /)

error: io.BytesIO.readline is inconsistent, runtime argument "size" has a default value of type None, which is incompatible with stub argument type builtins.int

Stub: at line 47

def (self: io.IOBase, builtins.int =) -> builtins.bytes

Runtime:

def (self, size=None, /)

Maybe those should actually be Optional[builtins.int]?

@hauntsaninja
Copy link
Collaborator

Yeah, it's intentional that stubtest complains about positional-only argument names in this case. In general, it's more permissive for positional-only args, but will complain if the names are totally different. The exact logic is https://github.com/python/mypy/blob/10195b8d3e1fa7a94f9a9c28b9cd10770f1c449a/mypy/stubtest.py#L305 Agreed that it's annoying in this case and that we should whitelist by adding io.BytesIO.seek to tests/stubtest_whitelists/py3_common.txt. In the medium term, I want to make stubtest check inherited methods more permissively.

@tarcisioe The double underscore prefix in the stub is interpreted to mean the argument positional-only.

For the read1 error, I think you might have to override, conditional on sys.version_info.

For the readline error, at least based on _pyio, it could make sense to change IOBase.readline to __size: Optional[int] = ... (https://github.com/python/cpython/blob/b45af1a5691e83b86321fc52d173f66cf891ce5f/Lib/_pyio.py#L548)

@tarcisioe
Copy link
Contributor Author

I'll tackle those ASAP, @hauntsaninja! Thanks for the explanations.

@tarcisioe tarcisioe marked this pull request as ready for review May 28, 2020 01:59
@tarcisioe
Copy link
Contributor Author

@JelleZijlstra @hauntsaninja I had to make some other changes, but as far as I was able to check. they are consistent. Can you guys please take a look?

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly concerned about the inheritance from BinaryIO.

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request May 28, 2020
The mypy issue got fixed by the good people of mypy. I did have to add an
override for __enter__ similar to what we're doing in python#4082.
@srittau srittau added the topic: io I/O related issues label May 28, 2020
JelleZijlstra added a commit that referenced this pull request May 28, 2020
The mypy issue got fixed by the good people of mypy. I did have to add an
override for __enter__ similar to what we're doing in #4082.
@tarcisioe
Copy link
Contributor Author

I think all is done now.

stdlib/3/io.pyi Outdated
if sys.version_info >= (3, 7):
def read1(self, __size: Optional[int] = ...) -> bytes: ...
def read1(self, __size: int = ...) -> bytes: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this does accept None, so it should have stayed Optional. Just applied this change myself. If Travis is happy now I'll merge this.

@JelleZijlstra JelleZijlstra merged commit b58b8f3 into python:master May 29, 2020
@tarcisioe tarcisioe deleted the bytes-io-inherit-bufferediobase branch May 29, 2020 00:23
@hauntsaninja
Copy link
Collaborator

Thanks for getting this tricky change through, @tarcisioe !

vishalkuo pushed a commit to vishalkuo/typeshed that referenced this pull request Jun 26, 2020
The mypy issue got fixed by the good people of mypy. I did have to add an
override for __enter__ similar to what we're doing in python#4082.
vishalkuo pushed a commit to vishalkuo/typeshed that referenced this pull request Jun 26, 2020
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 this pull request may close these issues.

4 participants