Auto merge of #12005 - PartiallyTyped:11713, r=blyxyas

`unused_io_amount` captures `Ok(_)`s

Partial rewrite of `unused_io_amount` to lint over `Ok(_)` and `Ok(..)`.

Moved the check to `check_block` to simplify context checking for expressions and allow us to check only some expressions.

For match (expr, arms) we emit a lint for io ops used on `expr` when an arm is `Ok(_)|Ok(..)`. Also considers the cases when there are guards in the arms and `if let Ok(_) = ...` cases.

For `Ok(_)` and `Ok(..)` it emits a note indicating where the value is ignored.

changelog: False Negatives [`unused_io_amount`]: Extended `unused_io_amount` to catch `Ok(_)`s in `If let` and match exprs.

Closes #11713

r? `@giraffate`

Author: bors

clippy_lints/src/unused_io_amount.rs

@@ -1,9 +1,10 @@
-use clippy_utils::diagnostics::{span_lint, span_lint_and_help};
-use clippy_utils::{is_trait_method, is_try, match_trait_method, paths};
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::{is_res_lang_ctor, is_trait_method, match_trait_method, paths};
+use hir::{ExprKind, PatKind};
 use rustc_hir as hir;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
-use rustc_span::sym;
+use rustc_span::{sym, Span};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -45,126 +46,219 @@ declare_clippy_lint! {
 
 declare_lint_pass!(UnusedIoAmount => [UNUSED_IO_AMOUNT]);
 
+#[derive(Copy, Clone)]
+enum IoOp {
+    AsyncWrite(bool),
+    AsyncRead(bool),
+    SyncRead(bool),
+    SyncWrite(bool),
+}
+
 impl<'tcx> LateLintPass<'tcx> for UnusedIoAmount {
-    fn check_stmt(&mut self, cx: &LateContext<'_>, s: &hir::Stmt<'_>) {
-        let (hir::StmtKind::Semi(expr) | hir::StmtKind::Expr(expr)) = s.kind else {
-            return;
-        };
+    /// We perform the check on the block level.
+    /// If we want to catch match and if expressions that act as returns of the block
+    ///   we need to check them at `check_expr` or `check_block` as they are not stmts
+    ///   but we can't check them at `check_expr` because we need the broader context
+    ///   because we should do this only for the final expression of the block, and not for
+    ///   `StmtKind::Local` which binds values => the io amount is used.
+    ///
+    /// To check for unused io amount in stmts, we only consider `StmtKind::Semi`.
+    /// `StmtKind::Local` is not considered because it binds values => the io amount is used.
+    /// `StmtKind::Expr` is not considered because requires unit type => the io amount is used.
+    /// `StmtKind::Item` is not considered because it's not an expression.
+    ///
+    /// We then check the individual expressions via `check_expr`. We use the same logic for
+    /// semi expressions and the final expression as we need to check match and if expressions
+    /// for binding of the io amount to `Ok(_)`.
+    ///
+    /// We explicitly check for the match source to be Normal as it needs special logic
+    /// to consider the arms, and we want to avoid breaking the logic for situations where things
+    /// get desugared to match.
+    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'tcx>) {
+        for stmt in block.stmts {
+            if let hir::StmtKind::Semi(exp) = stmt.kind {
+                check_expr(cx, exp);
+            }
+        }
 
-        match expr.kind {
-            hir::ExprKind::Match(res, _, _) if is_try(cx, expr).is_some() => {
-                if let hir::ExprKind::Call(func, [ref arg_0, ..]) = res.kind {
-                    if matches!(
-                        func.kind,
-                        hir::ExprKind::Path(hir::QPath::LangItem(hir::LangItem::TryTraitBranch, ..))
-                    ) {
-                        check_map_error(cx, arg_0, expr);
+        if let Some(exp) = block.expr
+            && matches!(exp.kind, hir::ExprKind::If(_, _, _) | hir::ExprKind::Match(_, _, _))
+        {
+            check_expr(cx, exp);
+        }
+    }
+}
+
+fn check_expr<'a>(cx: &LateContext<'a>, expr: &'a hir::Expr<'a>) {
+    match expr.kind {
+        hir::ExprKind::If(cond, _, _)
+            if let ExprKind::Let(hir::Let { pat, init, .. }) = cond.kind
+                && pattern_is_ignored_ok(cx, pat)
+                && let Some(op) = should_lint(cx, init) =>
+        {
+            emit_lint(cx, cond.span, op, &[pat.span]);
+        },
+        hir::ExprKind::Match(expr, arms, hir::MatchSource::Normal) if let Some(op) = should_lint(cx, expr) => {
+            let found_arms: Vec<_> = arms
+                .iter()
+                .filter_map(|arm| {
+                    if pattern_is_ignored_ok(cx, arm.pat) {
+                        Some(arm.span)
+                    } else {
+                        None
                     }
-                } else {
-                    check_map_error(cx, res, expr);
-                }
-            },
-            hir::ExprKind::MethodCall(path, arg_0, ..) => match path.ident.as_str() {
-                "expect" | "unwrap" | "unwrap_or" | "unwrap_or_else" | "is_ok" | "is_err" => {
-                    check_map_error(cx, arg_0, expr);
-                },
-                _ => (),
-            },
-            _ => (),
+                })
+                .collect();
+            if !found_arms.is_empty() {
+                emit_lint(cx, expr.span, op, found_arms.as_slice());
+            }
+        },
+        _ if let Some(op) = should_lint(cx, expr) => {
+            emit_lint(cx, expr.span, op, &[]);
+        },
+        _ => {},
+    };
+}
+
+fn should_lint<'a>(cx: &LateContext<'a>, mut inner: &'a hir::Expr<'a>) -> Option<IoOp> {
+    inner = unpack_match(inner);
+    inner = unpack_try(inner);
+    inner = unpack_call_chain(inner);
+    inner = unpack_await(inner);
+    // we type-check it to get whether it's a read/write or their vectorized forms
+    // and keep only the ones that are produce io amount
+    check_io_mode(cx, inner)
+}
+
+fn pattern_is_ignored_ok(cx: &LateContext<'_>, pat: &hir::Pat<'_>) -> bool {
+    // the if checks whether we are in a result Ok( ) pattern
+    // and the return checks whether it is unhandled
+
+    if let PatKind::TupleStruct(ref path, inner_pat, ddp) = pat.kind
+        // we check against Result::Ok to avoid linting on Err(_) or something else.
+        && is_res_lang_ctor(cx, cx.qpath_res(path, pat.hir_id), hir::LangItem::ResultOk)
+    {
+        return match (inner_pat, ddp.as_opt_usize()) {
+            // Ok(_) pattern
+            ([inner_pat], None) if matches!(inner_pat.kind, PatKind::Wild) => true,
+            // Ok(..) pattern
+            ([], Some(0)) => true,
+            _ => false,
+        };
+    }
+    false
+}
+
+fn unpack_call_chain<'a>(mut expr: &'a hir::Expr<'a>) -> &'a hir::Expr<'a> {
+    while let hir::ExprKind::MethodCall(path, receiver, ..) = expr.kind {
+        if matches!(
+            path.ident.as_str(),
+            "unwrap" | "expect" | "unwrap_or" | "unwrap_or_else" | "ok" | "is_ok" | "is_err" | "or_else" | "or"
+        ) {
+            expr = receiver;
+        } else {
+            break;
         }
     }
+    expr
+}
+
+fn unpack_try<'a>(mut expr: &'a hir::Expr<'a>) -> &'a hir::Expr<'a> {
+    while let hir::ExprKind::Call(func, [ref arg_0, ..]) = expr.kind
+        && matches!(
+            func.kind,
+            hir::ExprKind::Path(hir::QPath::LangItem(hir::LangItem::TryTraitBranch, ..))
+        )
+    {
+        expr = arg_0;
+    }
+    expr
+}
+
+fn unpack_match<'a>(mut expr: &'a hir::Expr<'a>) -> &'a hir::Expr<'a> {
+    while let hir::ExprKind::Match(res, _, _) = expr.kind {
+        expr = res;
+    }
+    expr
 }
 
 /// 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>> {
+fn unpack_await<'a>(expr: &'a hir::Expr<'a>) -> &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);
+                return arg_0;
             }
         }
     }
-
-    None
+    expr
 }
 
-fn check_map_error(cx: &LateContext<'_>, call: &hir::Expr<'_>, expr: &hir::Expr<'_>) {
-    let mut call = call;
-    while let hir::ExprKind::MethodCall(path, receiver, ..) = call.kind {
-        if matches!(path.ident.as_str(), "or" | "or_else" | "ok") {
-            call = receiver;
-        } else {
-            break;
-        }
-    }
+/// Check whether the current expr is a function call for an IO operation
+fn check_io_mode(cx: &LateContext<'_>, call: &hir::Expr<'_>) -> Option<IoOp> {
+    let hir::ExprKind::MethodCall(path, ..) = call.kind else {
+        return None;
+    };
 
-    if let Some(call) = try_remove_await(call) {
-        check_method_call(cx, call, expr, true);
-    } else {
-        check_method_call(cx, call, expr, false);
+    let vectorized = match path.ident.as_str() {
+        "write_vectored" | "read_vectored" => true,
+        "write" | "read" => false,
+        _ => {
+            return None;
+        },
+    };
+
+    match (
+        is_trait_method(cx, call, sym::IoRead),
+        is_trait_method(cx, call, sym::IoWrite),
+        match_trait_method(cx, call, &paths::FUTURES_IO_ASYNCREADEXT)
+            || match_trait_method(cx, call, &paths::TOKIO_IO_ASYNCREADEXT),
+        match_trait_method(cx, call, &paths::TOKIO_IO_ASYNCWRITEEXT)
+            || match_trait_method(cx, call, &paths::FUTURES_IO_ASYNCWRITEEXT),
+    ) {
+        (true, _, _, _) => Some(IoOp::SyncRead(vectorized)),
+        (_, true, _, _) => Some(IoOp::SyncWrite(vectorized)),
+        (_, _, true, _) => Some(IoOp::AsyncRead(vectorized)),
+        (_, _, _, true) => Some(IoOp::AsyncWrite(vectorized)),
+        _ => None,
     }
 }
 
-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 = if is_await {
-            match_trait_method(cx, call, &paths::FUTURES_IO_ASYNCREADEXT)
-                || match_trait_method(cx, call, &paths::TOKIO_IO_ASYNCREADEXT)
-        } else {
-            is_trait_method(cx, call, sym::IoRead)
-        };
-        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 {
-            is_trait_method(cx, call, sym::IoWrite)
-        };
+fn emit_lint(cx: &LateContext<'_>, span: Span, op: IoOp, wild_cards: &[Span]) {
+    let (msg, help) = match op {
+        IoOp::AsyncRead(false) => (
+            "read amount is not handled",
+            Some("use `AsyncReadExt::read_exact` instead, or handle partial reads"),
+        ),
+        IoOp::SyncRead(false) => (
+            "read amount is not handled",
+            Some("use `Read::read_exact` instead, or handle partial reads"),
+        ),
+        IoOp::SyncWrite(false) => (
+            "written amount is not handled",
+            Some("use `Write::write_all` instead, or handle partial writes"),
+        ),
+        IoOp::AsyncWrite(false) => (
+            "written amount is not handled",
+            Some("use `AsyncWriteExt::write_all` instead, or handle partial writes"),
+        ),
+        IoOp::SyncRead(true) | IoOp::AsyncRead(true) => ("read amount is not handled", None),
+        IoOp::SyncWrite(true) | IoOp::AsyncWrite(true) => ("written amount is not handled", None),
+    };
 
-        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",
-                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", false) => span_lint_and_help(
-                cx,
-                UNUSED_IO_AMOUNT,
-                expr.span,
-                "written amount is not handled",
-                None,
-                "use `Write::write_all` instead, or handle partial writes",
-            ),
-            (_, 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");
-            },
-            _ => (),
+    span_lint_and_then(cx, UNUSED_IO_AMOUNT, span, msg, |diag| {
+        if let Some(help_str) = help {
+            diag.help(help_str);
         }
-    }
+        for span in wild_cards {
+            diag.span_note(
+                *span,
+                "the result is consumed here, but the amount of I/O bytes remains unhandled",
+            );
+        }
+    });
 }