python / cpython

The Python programming language
https://www.python.org
Other
63.5k stars 30.41k forks source link

io.RawIOBase and typing.IO signatures differ #108411

Open kosta opened 1 year ago

kosta commented 1 year ago

Bug report

Checklist

CPython versions tested on:

3.11, CPython main branch

Operating systems tested on:

Linux, macOS, Windows

Output from running 'python -VV' on the command line:

Python 3.11.4 (main, Jun 20 2023, 17:23:00) [Clang 14.0.3 (clang-1403.0.22.14.1)]

A clear and concise description of the bug:

The method parameters in python are part of the public interface, due to kwargs.

From typing.IO: undocumented / source

    def read(self, n: int = -1) -> AnyStr:
        pass

From io.RawIOBase: docs / source

    def read(self, size=-1):
        ...

If I implement both IO[bytes] and RawIOBase, should name the variable n or size?

Additionally, RawIOBase.read checks for null, so should the type hint be Optional[int]?

sobolevn commented 1 year ago

Well, that's unfortunate :(

I don't think that we can change either of these two. Because they both are a part of the contract.

kosta commented 1 year ago

But isn't RawIOBase supposed to be IO[bytes]? At least that's how I understood it.

I think a breaking change could be fine if it resolves a self-contradiction. In my opinion, the typing signature should be adjusted. But I am sure there are people with more knowledge on this subject than me ;)

erlend-aasland commented 1 year ago

FTR, the size parameter of _io.RawIOBase.read is positional-only.

JelleZijlstra commented 1 year ago

In typeshed we also mark this param positional-only: https://github.com/python/typeshed/blob/a094aa09c2aa47721664d3fdef91eda4fac24ebb/stdlib/typing.pyi#L720 (the double underscore means positional-only).

Maybe we should also make the runtime parameter positional-only. It's technically a compatibility break, but it's unlikely to affect a lot of people, and the parameter is positional-only on the most common practical IO types.

JelleZijlstra commented 1 year ago

Additionally, RawIOBase.read checks for null, so should the type hint be Optional[int]?

Child classes can have a broader parameter type than their parent class, so if you implement both io.RawIOBase and typing.IO, it's OK to use int | None as the argument type even if one of the base classes only allows int. Maybe we should have used int | None in typing.IO, but changing this now is more likely to break existing subclasses.

LoveEatCandy commented 4 months ago

FTR, the size parameter of _io.RawIOBase.read is positional-only.

Since method's parameters of io.RawIOBase like _io.RawIOBase.read is positional-only, can we say that changing the parameters of io.RawIOBase to be the same as typing.IO would not cause compatibility break?