Allow only implementing Read::read_buf rust-lang/rust#106643

View all comments

This PR allows users to only implement Read::read_buf, without the need for implementing Read::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 implement Read::read, since you can't implement Read::read_buf. Thus, we don't expose rustc_must_implement_one_of to stable.

r? @thomcc

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

For a summary:

This extends std::io::Read by allowing users to implement Read by only providing a Read::read_buf impl, and not a Read::read impl. It does this using the unstable attribute rustc_must_implement_one_of¹.

#[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 rustc_must_implement_must_of user-visible, since read_buf is unstable (altho IMO it could easily be forgotten when stabilizing).

Whether or not we want to start using the #[rustc_must_implement_one_of(...)] attribute on the stdlib is a question that's split between lang (if they think the language feature has a path to stabilization) and libs-api (if they are okay with using it in that case). I'm on neither of these teams, and don't feel that strongly (but weakly, I think it probably needs a t-libs-api meeting to discuss after asking t-lang if it's okay start experimenting with)

r? @joshtriplett (who is on both relevant teams)

I'm nominating this for T-lang to consider that question: are we OK with people using rustc_must_implement_one_of in stable standard-library APIs?

Separately, I'm nominating this for libs-api. Are we OK with (on nightly) allowing people to implement read_buf and not read?

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 rustc_must_implement_one_of at least documented in the Rust reference. And once it's documented, we may well be up for just stabilizing it as must_implement_one_of.

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 read_buf that the attribute be either stabilized or at least documented in the reference or similar.

📌 Commit d7dac91 has been approved by joshtriplett

It is now in the queue for this repository.

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:

• I only know how to build rustc in dev mode. When it's built in stable mode, is it still going to tell the user to consider implementing the unstable read_buf method, which they cannot do on a stable toolchain? That would be undesirable and it would be good to have a UI test covering this case.

• Notice that even aside from "missing read" vs "missing one of read or read_buf", the new error is less helpful because it does not give you the signature you need to put, so there isn't a signature you can copy straight from the error into your code. Would it make sense to give 2 "help:", one with each signature?

• How does this impact rust-analyzer? When you write a trait impl and tell it to autofill the required methods, is it going to put both? neither? just the alphanumerically first one or the one on the lower line number in libstd? My sense is a lot of people rely on this autofill when writing trait impls in an IDE.

@bors r-

When it's built in stable mode, is it still going to tell the user to consider implementing the unstable read_buf method, which they cannot do on a stable toolchain? That would be undesirable and it would be good to have a UI test covering this case.

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?

Notice that even aside from "missing read" vs "missing one of read or read_buf", the new error is less helpful because it does not give you the signature you need to put, so there isn't a signature you can copy straight from the error into your code. Would it make sense to give 2 "help:", one with each signature?

Fair. That seems like a reasonable solution for the common case.

How does this impact rust-analyzer? When you write a trait impl and tell it to autofill the required methods, is it going to put both? neither? just the alphanumerically first one or the one on the lower line number in libstd? My sense is a lot of people rely on this autofill when writing trait impls in an IDE.

@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.

WaffleLapkin, would that be feasible, and can you add a test verifying that behavior as well?

@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.

This should be feasible. I'll make a PR that removes mentions of unstable methods from this diagnostic and shows the signatures soon

@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 read and read_buf having incompatible semantics, although I don't recall where exactly (maybe some tracking issue?), so I'm not sure if this can be supported at all.

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

The job aarch64-gnu-llvm-21-1 failed! Check out the build log: (web) (plain enhanced) (plain)

##[endgroup]
Executing "/scripts/stage_2_test_set1.sh"
+ /scripts/stage_2_test_set1.sh
+ '[' 1 == 1 ']'
+ echo 'PR_CI_JOB set; skipping tidy'
+ SKIP_TIDY='--skip tidy'
+ ../x.py --stage 2 test --skip tidy --skip compiler --skip src
PR_CI_JOB set; skipping tidy
##[group]Building bootstrap
    Finished `dev` profile [unoptimized] target(s) in 0.04s
##[endgroup]
---

---- [rustdoc-html] tests/rustdoc-html/jump-to-def/non-local-method.rs stdout ----
------python3 stdout------------------------------

------python3 stderr------------------------------
19: has check failed
 `XPATH PATTERN` did not match
     //@ has - '//a[@href="{{channel}}/std/io/trait.Read.html#tymethod.read"]' 'read'

Encountered 1 errors

------------------------------------------

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/aarch64-unknown-linux-gnu/test/rustdoc-html/jump-to-def/non-local-method" "/checkout/tests/rustdoc-html/jump-to-def/non-local-method.rs"
stdout: none
--- stderr -------------------------------
19: has check failed
 `XPATH PATTERN` did not match
     //@ has - '//a[@href="{{channel}}/std/io/trait.Read.html#tymethod.read"]' 'read'

Encountered 1 errors
------------------------------------------

---- [rustdoc-html] tests/rustdoc-html/jump-to-def/non-local-method.rs stdout end ----