Auto merge of rust-lang#8179 - nmathewson:unused_async_io_amount, r=xFrednet

Extend unused_io_amount to cover async io.

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

Author: bors

Cargo.toml

@@ -47,7 +47,9 @@ itertools = "0.10"
 quote = "1.0"
 serde = { version = "1.0", features = ["derive"] }
 syn = { version = "1.0", features = ["full"] }
+futures = "0.3"
 parking_lot = "0.11.2"
+tokio = { version = "1", features = ["io-util"] }
 
 [build-dependencies]
 rustc_tools_util = { version = "0.2", path = "rustc_tools_util" }

clippy_lints/src/unused_io_amount.rs

@@ -1,4 +1,4 @@
-use clippy_utils::diagnostics::span_lint;
+use clippy_utils::diagnostics::{span_lint, span_lint_and_help};
 use clippy_utils::{is_try, match_trait_method, paths};
 use rustc_hir as hir;
 use rustc_lint::{LateContext, LateLintPass};
@@ -17,10 +17,17 @@ declare_clippy_lint! {
     /// partial-write/read, use
     /// `write_all`/`read_exact` instead.
     ///
+    /// When working with asynchronous code (either with the `futures`
+    /// crate or with `tokio`), a similar issue exists for
+    /// `AsyncWriteExt::write()` and `AsyncReadExt::read()` : these
+    /// functions are also not guaranteed to process the entire
+    /// buffer.  Your code should either handle partial-writes/reads, or
+    /// call the `write_all`/`read_exact` methods on those traits instead.
+    ///
     /// ### Known problems
     /// Detects only common patterns.
     ///
-    /// ### Example
+    /// ### Examples
     /// ```rust,ignore
     /// use std::io;
     /// fn foo<W: io::Write>(w: &mut W) -> io::Result<()> {
@@ -68,6 +75,23 @@ impl<'tcx> LateLintPass<'tcx> for UnusedIoAmount {
     }
 }
 
+/// If `expr` is an (e).await, return the inner expression "e" that's being
+/// waited on.  Otherwise return None.
+fn try_remove_await<'a>(expr: &'a hir::Expr<'a>) -> Option<&hir::Expr<'a>> {
+    if let hir::ExprKind::Match(expr, _, hir::MatchSource::AwaitDesugar) = expr.kind {
+        if let hir::ExprKind::Call(func, [ref arg_0, ..]) = expr.kind {
+            if matches!(
+                func.kind,
+                hir::ExprKind::Path(hir::QPath::LangItem(hir::LangItem::IntoFutureIntoFuture, ..))
+            ) {
+                return Some(arg_0);
+            }
+        }
+    }
+
+    None
+}
+
 fn check_map_error(cx: &LateContext<'_>, call: &hir::Expr<'_>, expr: &hir::Expr<'_>) {
     let mut call = call;
     while let hir::ExprKind::MethodCall(path, _, args, _) = call.kind {
@@ -77,30 +101,69 @@ fn check_map_error(cx: &LateContext<'_>, call: &hir::Expr<'_>, expr: &hir::Expr<
             break;
         }
     }
-    check_method_call(cx, call, expr);
+
+    if let Some(call) = try_remove_await(call) {
+        check_method_call(cx, call, expr, true);
+    } else {
+        check_method_call(cx, call, expr, false);
+    }
 }
 
-fn check_method_call(cx: &LateContext<'_>, call: &hir::Expr<'_>, expr: &hir::Expr<'_>) {
+fn check_method_call(cx: &LateContext<'_>, call: &hir::Expr<'_>, expr: &hir::Expr<'_>, is_await: bool) {
     if let hir::ExprKind::MethodCall(path, _, _, _) = call.kind {
         let symbol = path.ident.as_str();
-        let read_trait = match_trait_method(cx, call, &paths::IO_READ);
-        let write_trait = match_trait_method(cx, call, &paths::IO_WRITE);
+        let read_trait = if is_await {
+            match_trait_method(cx, call, &paths::FUTURES_IO_ASYNCREADEXT)
+                || match_trait_method(cx, call, &paths::TOKIO_IO_ASYNCREADEXT)
+        } else {
+            match_trait_method(cx, call, &paths::IO_READ)
+        };
+        let write_trait = if is_await {
+            match_trait_method(cx, call, &paths::FUTURES_IO_ASYNCWRITEEXT)
+                || match_trait_method(cx, call, &paths::TOKIO_IO_ASYNCWRITEEXT)
+        } else {
+            match_trait_method(cx, call, &paths::IO_WRITE)
+        };
 
-        match (read_trait, write_trait, symbol) {
-            (true, _, "read") => span_lint(
+        match (read_trait, write_trait, symbol, is_await) {
+            (true, _, "read", false) => span_lint_and_help(
+                cx,
+                UNUSED_IO_AMOUNT,
+                expr.span,
+                "read amount is not handled",
+                None,
+                "use `Read::read_exact` instead, or handle partial reads",
+            ),
+            (true, _, "read", true) => span_lint_and_help(
                 cx,
                 UNUSED_IO_AMOUNT,
                 expr.span,
-                "read amount is not handled. Use `Read::read_exact` instead",
+                "read amount is not handled",
+                None,
+                "use `AsyncReadExt::read_exact` instead, or handle partial reads",
             ),
-            (true, _, "read_vectored") => span_lint(cx, UNUSED_IO_AMOUNT, expr.span, "read amount is not handled"),
-            (_, true, "write") => span_lint(
+            (true, _, "read_vectored", _) => {
+                span_lint(cx, UNUSED_IO_AMOUNT, expr.span, "read amount is not handled");
+            },
+            (_, true, "write", false) => span_lint_and_help(
                 cx,
                 UNUSED_IO_AMOUNT,
                 expr.span,
-                "written amount is not handled. Use `Write::write_all` instead",
+                "written amount is not handled",
+                None,
+                "use `Write::write_all` instead, or handle partial writes",
             ),
-            (_, true, "write_vectored") => span_lint(cx, UNUSED_IO_AMOUNT, expr.span, "written amount is not handled"),
+            (_, true, "write", true) => span_lint_and_help(
+                cx,
+                UNUSED_IO_AMOUNT,
+                expr.span,
+                "written amount is not handled",
+                None,
+                "use `AsyncWriteExt::write_all` instead, or handle partial writes",
+            ),
+            (_, true, "write_vectored", _) => {
+                span_lint(cx, UNUSED_IO_AMOUNT, expr.span, "written amount is not handled");
+            },
             _ => (),
         }
     }

clippy_utils/src/paths.rs

@@ -64,6 +64,10 @@ pub const FROM_ITERATOR: [&str; 5] = ["core", "iter", "traits", "collect", "From
 pub const FROM_ITERATOR_METHOD: [&str; 6] = ["core", "iter", "traits", "collect", "FromIterator", "from_iter"];
 pub const FROM_STR_METHOD: [&str; 5] = ["core", "str", "traits", "FromStr", "from_str"];
 pub const FUTURE_FROM_GENERATOR: [&str; 3] = ["core", "future", "from_generator"];
+#[allow(clippy::invalid_paths)] // internal lints do not know about all external crates
+pub const FUTURES_IO_ASYNCREADEXT: [&str; 3] = ["futures_util", "io", "AsyncReadExt"];
+#[allow(clippy::invalid_paths)] // internal lints do not know about all external crates
+pub const FUTURES_IO_ASYNCWRITEEXT: [&str; 3] = ["futures_util", "io", "AsyncWriteExt"];
 pub const HASH: [&str; 3] = ["core", "hash", "Hash"];
 pub const HASHMAP_CONTAINS_KEY: [&str; 6] = ["std", "collections", "hash", "map", "HashMap", "contains_key"];
 pub const HASHMAP_ENTRY: [&str; 5] = ["std", "collections", "hash", "map", "Entry"];
@@ -194,6 +198,10 @@ pub const SYM_MODULE: [&str; 3] = ["rustc_span", "symbol", "sym"];
 pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"];
 pub const TO_OWNED_METHOD: [&str; 4] = ["alloc", "borrow", "ToOwned", "to_owned"];
 pub const TO_STRING_METHOD: [&str; 4] = ["alloc", "string", "ToString", "to_string"];
+#[allow(clippy::invalid_paths)] // internal lints do not know about all external crates
+pub const TOKIO_IO_ASYNCREADEXT: [&str; 5] = ["tokio", "io", "util", "async_read_ext", "AsyncReadExt"];
+#[allow(clippy::invalid_paths)] // internal lints do not know about all external crates
+pub const TOKIO_IO_ASYNCWRITEEXT: [&str; 5] = ["tokio", "io", "util", "async_write_ext", "AsyncWriteExt"];
 pub const TRY_FROM: [&str; 4] = ["core", "convert", "TryFrom", "try_from"];
 pub const VEC_AS_MUT_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_mut_slice"];
 pub const VEC_AS_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_slice"];

tests/compile-test.rs

@@ -21,13 +21,15 @@ const RUN_INTERNAL_TESTS: bool = cfg!(feature = "internal-lints");
 static TEST_DEPENDENCIES: &[&str] = &[
     "clippy_utils",
     "derive_new",
+    "futures",
     "if_chain",
     "itertools",
     "quote",
     "regex",
     "serde",
     "serde_derive",
     "syn",
+    "tokio",
     "parking_lot",
 ];
 
@@ -38,6 +40,8 @@ extern crate clippy_utils;
 #[allow(unused_extern_crates)]
 extern crate derive_new;
 #[allow(unused_extern_crates)]
+extern crate futures;
+#[allow(unused_extern_crates)]
 extern crate if_chain;
 #[allow(unused_extern_crates)]
 extern crate itertools;
@@ -47,6 +51,8 @@ extern crate parking_lot;
 extern crate quote;
 #[allow(unused_extern_crates)]
 extern crate syn;
+#[allow(unused_extern_crates)]
+extern crate tokio;
 
 /// Produces a string with an `--extern` flag for all UI test crate
 /// dependencies.

tests/ui/unused_io_amount.rs

@@ -1,6 +1,8 @@
 #![allow(dead_code)]
 #![warn(clippy::unused_io_amount)]
 
+extern crate futures;
+use futures::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt};
 use std::io::{self, Read};
 
 fn question_mark<T: io::Read + io::Write>(s: &mut T) -> io::Result<()> {
@@ -61,4 +63,55 @@ fn combine_or(file: &str) -> Result<(), Error> {
     Ok(())
 }
 
+async fn bad_async_write<W: AsyncWrite + Unpin>(w: &mut W) {
+    w.write(b"hello world").await.unwrap();
+}
+
+async fn bad_async_read<R: AsyncRead + Unpin>(r: &mut R) {
+    let mut buf = [0u8; 0];
+    r.read(&mut buf[..]).await.unwrap();
+}
+
+async fn io_not_ignored_async_write<W: AsyncWrite + Unpin>(mut w: W) {
+    // Here we're forgetting to await the future, so we should get a
+    // warning about _that_ (or we would, if it were enabled), but we
+    // won't get one about ignoring the return value.
+    w.write(b"hello world");
+}
+
+fn bad_async_write_closure<W: AsyncWrite + Unpin + 'static>(w: W) -> impl futures::Future<Output = io::Result<()>> {
+    let mut w = w;
+    async move {
+        w.write(b"hello world").await?;
+        Ok(())
+    }
+}
+
+async fn async_read_nested_or<R: AsyncRead + Unpin>(r: &mut R, do_it: bool) -> Result<[u8; 1], Error> {
+    let mut buf = [0u8; 1];
+    if do_it {
+        r.read(&mut buf[..]).await.or(Err(Error::Kind))?;
+    }
+    Ok(buf)
+}
+
+use tokio::io::{AsyncRead as TokioAsyncRead, AsyncReadExt as _, AsyncWrite as TokioAsyncWrite, AsyncWriteExt as _};
+
+async fn bad_async_write_tokio<W: TokioAsyncWrite + Unpin>(w: &mut W) {
+    w.write(b"hello world").await.unwrap();
+}
+
+async fn bad_async_read_tokio<R: TokioAsyncRead + Unpin>(r: &mut R) {
+    let mut buf = [0u8; 0];
+    r.read(&mut buf[..]).await.unwrap();
+}
+
+async fn undetected_bad_async_write<W: AsyncWrite + Unpin>(w: &mut W) {
+    // It would be good to detect this case some day, but the current lint
+    // doesn't handle it. (The documentation says that this lint "detects
+    // only common patterns".)
+    let future = w.write(b"Hello world");
+    future.await.unwrap();
+}
+
 fn main() {}