Read::read_exact docs unnecessarily scare off users

[daboross]

In a recent URLO post, someone saw the "no guarantees" paragraph in read_exact, and this convinced them not to use it (and to instead use read).

The post: (link)

Okay. That does means I should replace all read with read_exact? Since it said no guarantee I was hesitant to use it.

The paragraph:

No guarantees are provided about the contents of buf when this function is called, implementations cannot rely on any property of the contents of buf being true. It is recommended that implementations only write data to buf instead of reading its contents.

While the caution is (or at least was) legitimate, I think for today's rust it's a bit negative. And if this is a user's first real read method they use, then I can 100% understand getting thrown off by this language.

I'd like to think we can reduce the caution necessary when using it, while still keeping the caution for implementors. Thoughts on replacing it with a more positive message, like the following?

This function may be called with any buf, and makes no requirements on the contents of buf or any property about the contents of buf being true. It is recommended that implementations only write data to buf instead of reading its contents.

The italicized "implementations" is taken from the documentation on read, which reads:

No guarantees are provided about the contents of buf when this function is called, implementations cannot rely on any property of the contents of buf being true. It is recommended that implementations only write data to buf instead of reading its contents.

I don't think it's necessary to modify the paragraph on read, as it has much more overall content, as it also contains this following paragraph:

Correspondingly, however, callers of this method may not assume any guarantees about how the implementation uses buf. The trait is safe to implement, so it is possible that the code that's supposed to write to the buffer might also read from it. It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit<T>) is not safe, and can lead to undefined behavior.

In addition, the read documentation is big enough that users can skip over the "no guarantees" paragraph. Case in point, the user from the URLO thread above chose to use read, while avoiding read_exact, even though read contains the exact same paragraph.

I'll end by saying I don't know the full history, but I think this might have had something to do with passing uninitialized byte buffers to read methods. I believe that it's been decided that passing in an uninitialized buffer is UB, though, so it shouldn't be as big of a concern. The current discussion of reading into uninitialized buffers is at 42788.

This issue has been assigned to @poliorcetics via this comment.

[RalfJung]

I think I authored or at least reviewed the changes that made the read docs more clear. The docs are prepared for a future where the passing of an uninitialized buffer itself is not UB, but of course if you call arbitrary safe code (via Read::read) and give it an uninitialized buffer that is still unsound and can cause UB depending on what the code does. I was not aware read_exact contains the exact same issue!

Would it make sense to copy the longer explanatory paragraph to read_exact, and/or link to the read docs to avoid duplication?

[poliorcetics]

@rustbot claim

[ilyvion]

I'm not sure I understand the confusion. This paragraph:

No guarantees are provided about the contents of buf when this function is called, implementations cannot rely on any property of the contents of buf being true. It is recommended that implementations only write data to buf instead of reading its contents.

applies to implementations of Read, consumers don't need to pay attention to that bit at all. As for the bit that applies to consumers;

Correspondingly, however, callers of this method may not assume any guarantees about how the implementation uses buf. The trait is safe to implement, so it is possible that the code that's supposed to write to the buffer might also read from it. It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit<T>) is not safe, and can lead to undefined behavior.

it just says to pass a regularly allocated buf and to not try being clever by using an uninitialized buffer.

[daboross]

I don't think the text was ever incorrect, it's just that someone skimming the docs can read things incorrectly.

As I understand it, in the case I opened this issue for, the user certainly didn't read and understand the entire paragraph. They just saw "no guarantees", and decided this function wasn't the right thing to use.

I'm OK with needing people to read the entire doc, but I think this goes beyond that? Particularly, if someone misreads or doesn't understand the word "implementations" in the first sentence of the paragraph, it reads as literally not guaranteeing anything about the output. I thought this was overly harsh for something the majority of users shouldn't have to care about, especially when it's harsher than another function, read, which should be used much less often by end users.

Plus, I think plenty of users who will encounter this before they understand proper terminology surrounding traits - "implementations" refers to someone implementing the trait, but that's not exactly obvious until someone's worked with rust for longer. And reading from a file is often a simple thing one might try to do before exploring more complicated aspects of the language? I guess I think it's totally understandable to misread this - even if they pay attention and try to understand the entire docs.

Maybe something like renaming read, like suggested in https://internals.rust-lang.org/t/poor-naming-of-read-write-i-o-methods/12685/13, would be a better long-term solution for getting people to actually use read_exact rather than read. But I think making the language less, well, scary on Read::read_exact could mitigate this?

I appreciate the changes in #74486, and I apologize for not being active on this issue. But I'd also argue that if this issue was ever valid, it still is - and this should be reopened.

[mbxt]

I know this thread has been closed for a while, but maybe I can offer my two cents as someone who misread the documentation and recently encountered the same confusion:

The context switch from providing documentation to callers to providing documentation to implementers is only stated after the warning, after alarm bells had already gone off in my head, so in a sense, it's my mistake for getting distracted by those warnings and not reading more carefully, but on the other hand,

I think reordering from

No guarantees are provided about the contents of buf when this function is called, so implementations cannot rely on any property of the contents of buf being true. It is recommended that implementations only write data to buf instead of reading its contents. The documentation on read has a more detailed explanation on this subject.

to

It is recommended that implementations only write data to buf instead of reading its contents; no guarantees are provided about the contents of buf when this function is called, so implementations cannot rely on any property of the contents of buf being true. The documentation on read has a more detailed explanation on this subject.

could provide a simple win. Going one step further to

It is recommended that trait implementations...

would have been even more clear, but I could see how from a certain perspective this might seem unnecessarily wordy, or might not be consistent with other documentation.

In hindsight, the documentation for read_exact now makes perfect sense, and it wasn't a matter of learning anything I didn't already know about traits, it was merely a matter of noticing the context switch and not getting tripped up by the warning.