Skip to content

Commit

Permalink
Auto merge of rust-lang#12005 - PartiallyTyped:11713, r=blyxyas
Browse files Browse the repository at this point in the history 
`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 rust-lang#11713

r? `@giraffate`
  • Loading branch information
@bors
bors committed Jan 21, 2024
2 parents 7386856 + f73879e commit 64d08a8
Show file tree
Hide file tree
Showing 3 changed files with 400 additions and 120 deletions.
292 changes: 193 additions & 99 deletions clippy_lints/src/unused_io_amount.rs
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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",
);
}
});
}
Loading

0 comments on commit 64d08a8

Please sign in to comment.