gh-155419: honor the file object's current position in hashlib.file_digest - #155422

Duck typing this is just checking "does it have a getbuffer" method. I could see getbuffer implementations which only return the data which will show up in read vs. what BytesIO does (the whole allocated data). I don't think there is a formalized "what does getbuffer do" with this case covered.

For new feature 3.16+ I think this approach is reasonable / if people have other getbuffer implementations they'll let us know. As a bugfix backport it seems like a subtle breaking change to performance sensitive code. To backport I think should explicitly check isinstance(fileobj, io.BytesIO).

How can it be a breaking change? it was already like that for the past years

And fileobj.getbuffer() returns a view on the buffer and buf[fileobj.tell():] returns a view as well, so 0-copy is still ensure, or am I missing something?

FTR, we document file_digest as follows:

fileobj must be a file-like object opened for reading in binary mode. It accepts file objects from builtin open(), BytesIO instances, SocketIO objects from socket.socket.makefile(), and similar. fileobj must be opened in blocking mode, otherwise a BlockingIOError may be raised.

So if people are doing something else, they're on their own.

context: the file_digest code was shipped in 3.11 and has had few changes since: https://github.com/python/cpython/pull/31930/changes.

While the comment asserts "this is a BytesIO" custom I/O stacks for Python exist. Searching there are quite a few custom getbuffer implementations outside of io.BytesIO (Many for display buffers, but some I/O buffers). Those pass this code and the typeshed stub protocol (https://github.com/python/typeshed/blob/6875aaf17c8322b00499f79276340fec4cc87451/stdlib/hashlib.pyi#L97-L109) which just assert "getbuffer method required". Custom optimized I/O stacks would likely implement a getbuffer which produces the result the codebase cares about. That might pay attention to the file offset, dropping already read bytes or might contain all bytes ever written.

The change here:

1. Makes it so in addition to getbuffer() the object must support a tell() method unconditionally
2. Currently implements that the file offset / tell result is required to go from "all data" to the "data not yet read".

Neither of those are "required" to me from the typeshed protocol or duck typing. The added tell requirement would break code which runs fine today and so is a breaking change.

If we "narrow" this to just BytesIO explicitly then requiring tell isn't too big of a hurdle and to me it does fix what feels like unintended behavior. Adding a requirement for tell is changing the API meaningfully hence 3.16+.

Ah I see what you mean now. Ok this makes sense. I will use an isinstance check for the fast path in all versions and let the slow path orherwise. Custom implementations without tell() would fall back to the slow path.