Extend unused_io_amount to cover async io.

[nmathewson]

Clippy helpfully warns about code like this, telling you that you
probably meant "write_all":

fn say_hi<W:Write>(w: &mut W) {
   w.write(b"hello").unwrap();
}

This patch attempts to extend the lint so it also covers this
case:

async fn say_hi<W:AsyncWrite>(w: &mut W) {
   w.write(b"hello").await.unwrap();
}

(I've run into this second case several times in my own programming,
and so have my coworkers, so unless we're especially accident-prone
in this area, it's probably worth addressing?)

Since this is my first attempt at a clippy patch, I've probably
made all kinds of mistakes: please help me fix them? I'd like
to learn more here.

Open questions I have:

• Should this be a separate lint from unused_io_amount? Maybe
  unused_async_io_amount? If so, how should I structure their
  shared code?
• Should this cover tokio's AsyncWrite too?
• Is it okay to write lints for stuff that isn't part of
  the standard library? I see that "regex" also has lints,
  and I figure that "futures" is probably okay too, since it's
  an official rust-lang repository.
• What other tests are needed?
• How should I improve the code?

Thanks for your time!

---

changelog: [unused_io_amount] now supports async read and write traits

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information.

[xFrednet]

Welcome to Clippy @nmathewson,

I've skimmed through your changes. The direction appears to be correct, so don't worry about that. I first want to address your questions before doing a full review. 🙃

Should this be a separate lint from unused_io_amount? Maybe unused_async_io_amount? If so, how should I structure their shared code?

• In Clippy, we usually split lints into two if it makes sense to allow one and deny the other or if a specific test case produces many false positives. In this case, I don't really see a reason to do so, as calling write without handling the read/write amount always seems wrong. An advantage would be that we would have different lint documentations.

  If you wanted to split them up, then you would track what triggered the lint and then pass the corresponding lint to the span_lint calls in check_method_call. The matched trait could be used to determine if it was WriteAsync or not.

Is it okay to write lints for stuff that isn't part of the standard library? I see that "regex" also has lints, and I figure that "futures" is probably okay too, since it's an official rust-lang repository.

• Usually, we avoid adding lints for creates beside the standard library, as they are harder to maintain and affect fewer people. The regex lints were added before this was discussed, from what I know. Since futures-rs is an official rust-lang crate and is commonly used, I think it could be reasonable to add a lint for it. I'll ask on Zulip what the others think.

Should this cover tokio's AsyncWrite too?

• I'm guessing that tokio's AsyncWrite would be too specific, but I'll ask.

  Some additional context in case you're interested: We are hoping that there will be a stable linting API in the future so that crates like tokio can create their on lints. Currently, most of us are sadly limited on time, which results in little to no progress in that regard at the moment.

What other tests are needed?

• Since the lint doesn't create a suggestion, this should be fairly straightforward. Could you add one more example with an async block and async closure? Maybe also one where the call is nested like this:

  async fn bad_async_write<W: AsyncWrite + Unpin>(w: &mut W) {
      // Dooing a bunch of stuff
  
      println!("Dear Programmer...")
      println!("How are you today?")
      let listen = false;
      if !listen {
          w.write(b"I ignored your answer").await.unwrap();
      } else {
          w.write(b"That's good to hear").await.unwrap();
      }
  }

How should I improve the code?

• I'll take a closer look at your implementation once we have some answers from the others. Could you tell me what the correct usage would be in the given example? (I haven't worked with async too much). The normal Write trait has a write and write_all function, which the lint also suggests to use. However, there wasn't a write_all function for futures::io::AsyncWrite and tokio::io::AsyncWrite. The correct usage should also be displayed in the help message of span_lint.

I hope this answers some of your questions. Let me know if I can help you in the meantime 🙃

Zulip link

[nmathewson]

Thanks, @xFrednet ! I'll work on the tests, have a look at the Zulip discussion, and see where it goes?

Could you tell me what the correct usage would be in the given example? (I haven't worked with async too much).

The correct usage would indeed: be write_all in place of write:

In the futures crate, there is a base trait called AsyncWrite that has the methods needed to implement a writer. But since those methods are based on Poll, they're inconvenient to use, so futures defines an AsyncWriteExt extension trait that exposes a number convenience methods, including write() and write_all().

The story is similar for AsyncRead and AsyncReadExt.

[bors]

☔ The latest upstream changes (presumably #8196) made this pull request unmergeable. Please resolve the merge conflicts.

[nmathewson]

I've just pushed a rebased version of the branch, to:

• resolve merge conflicts
• cover tokio as well as async-rs
• test more examples
• clean up the code a bit
• improve the help message from the span_lint to mention the right traits.

This resolves the issues I'm aware of, but please let me know if there are more. :)

[xFrednet]

Thanks, @xFrednet ! I'll work on the tests, have a look at the Zulip discussion, and see where it goes?

Based on the discussion on Zulip it seems like I misunderstood how Clippy deals with lints for external crates, meaning that this is reasonable and okay to add to Clippy 👍

Could you tell me what the correct usage would be in the given example? (I haven't worked with async too much).

The correct usage would indeed: be write_all in place of write:

In the futures crate, there is a base trait called AsyncWrite that has the methods needed to implement a writer. But since those methods are based on Poll, they're inconvenient to use, so futures defines an AsyncWriteExt extension trait that exposes a number convenience methods, including write() and write_all().

The story is similar for AsyncRead and AsyncReadExt.

Thank you for the explanation, then it should be fine. I'll take a look at the code now 🙃

[xFrednet]

This version looks good to me, I've just added two small comments 🙃. I think it would also be good to reference the async traits in the lint description, maybe by adding the following to the "Why is this bad?" section:

When working with tokio or futures-rs the AsyncWriteExt and AsyncReadExt traits can be used to call write_all, read_exact respectively.

---

The PR is still marked as WIP based on the title, are you planning to add more to this @nmathewson? Otherwise, I think we can merge this soon 🙃.

One more thing, Clippy creates its changelogs based on the lint description could you include a changelog line like this:

changelog: [unused_io_amount] now supports async read and write traits

[nmathewson]

Thank you! I've made the requested changes and marked this PR as non-WIP. I'll keep an eye on CI to see if I break anything.

[xFrednet]

Looks good to me, and the CI is also happy! 🎉 Thank you very much for the pull request, I hope you also had fun working on it. 🙃

btw: I edited the PR to include the changelog, now everything should merge without a problem 👍

@bors r+

[bors]

📌 Commit b6bcf0c has been approved by xFrednet

[bors]

⌛ Testing commit b6bcf0c with merge 490566b...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 490566b to master...