-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Allow only implementing Read::read_buf
#106643
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: master
Are you sure you want to change the base?
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
For a summary: This extends #[rustc_must_implement_one_of(read, read_buf)]
pub trait std::io::Read {
// (new)
fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
let mut buf = BorrowedBuf::from(buf);
self.read_buf(buf.unfilled()).map(|()| buf.len())
}
// (existing)
fn read_buf(&mut self, buf: BorrowedCursor<'_>) -> Result<()> {
default_read_buf(|b| self.read(b), buf)
}
} This does not make Whether or not we want to start using the r? @joshtriplett (who is on both relevant teams) Footnotes
|
I'm nominating this for T-lang to consider that question: are we OK with people using |
Separately, I'm nominating this for libs-api. Are we OK with (on nightly) allowing people to implement I am in favor, and in the absence of objections I'd r+ this once we have T-lang confirmation. |
Discussed this asynchronously with some T-lang folks, and one item that came up: we'd really like to see |
I've created a tracking issue: #107460 |
Removing I-libs-api-nominated for now, until the attribute has documentation in the reference. |
We discussed this once more in @rust-lang/lang, and confirmed that we have no objection to adding this on nightly. @bors r+ We'd consider it a blocker for stabilization of |
I am concerned about this negatively affecting the experience of non-nightly users. For example with this code: struct MyRead;
impl std::io::Read for MyRead {} current stable gives you this error: error[E0046]: not all trait items implemented, missing: `read`
--> src/main.rs:3:1
|
3 | impl std::io::Read for MyRead {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `read` in implementation
|
= help: implement the missing item: `fn read(&mut self, _: &mut [u8]) -> Result<usize, std::io::Error> { todo!() }` whereas rustc built from this PR gives this error: error[E0046]: not all trait items implemented, missing one of: `read`, `read_buf`
--> src/main.rs:3:1
|
3 | impl std::io::Read for MyRead {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing one of `read`, `read_buf` in implementation
|
note: required because of this annotation
--> /git/rust/library/std/src/io/mod.rs:552:1
|
552 | #[rustc_must_implement_one_of(read, read_buf)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Some concerns:
@bors r- |
Valid; I agree that we should avoid suggesting implementing a method that you can't implement. @WaffleLapkin, would that be feasible, and can you add a test verifying that behavior as well?
Fair. That seems like a reasonable solution for the common case. |
@dtolnay thanks for rising these concerns! My guess is that r-a will not autofill any methods, as they all have default bodies. I'll look into how we can support this feature in r-a.
@joshtriplett This should be feasible. I'll make a PR that removes mentions of unstable methods from this diagnostic and shows the signatures soon™. For now, marking this PR as blocked. |
@WaffleLapkin has there been any progress on this? thanks |
@Dylan-DPC I made 0 progress on this so far. In my opinion r-a support is a bigger concern here (although I did not progress there either). I don't have much time to work on this recently :( Also note that there were some concerns raised about |
This PR allows users to only implement
Read::read_buf
, without the need for implementingRead::read
.rustc_must_implement_one_of
annotation ensures that at least one of the methods is implemented, so that the default impls don't create infinite recursion.Note that
Read::read_buf
is unstable, so this doesn't change anything on stable, there you still need to implementRead::read
, since you can't implementRead::read_buf
. Thus, we don't exposerustc_must_implement_one_of
to stable.r? @thomcc