Skip to content

Commit

Permalink
Fixed FP in unused_io_amount for Ok(lit), unrachable!
Browse files Browse the repository at this point in the history
We introduce the following rules for match exprs.
- `panic!` and `unreachable!` are treated as consumption.
- guard expressions in any arm imply consumption.

For match exprs:
- Lint only if exacrtly 2 non-consuming arms exist
- Lint only if one arm is an `Ok(_)` and the other is `Err(_)`

Added additional requirement that for a block return expression
that is a match, the source must be `Normal`.

changelog: FP [`unused_io_amount`] when matching Ok(literal)
  • Loading branch information
m-rph committed Feb 1, 2024
1 parent 99423e8 commit fe8c2e2
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 29 deletions.
101 changes: 76 additions & 25 deletions clippy_lints/src/unused_io_amount.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::{is_res_lang_ctor, is_trait_method, match_trait_method, paths};
use clippy_utils::macros::{is_panic, root_macro_call_first_node};
use clippy_utils::{is_res_lang_ctor, is_trait_method, match_trait_method, paths, peel_blocks};
use hir::{ExprKind, PatKind};
use rustc_hir as hir;
use rustc_lint::{LateContext, LateLintPass};
Expand Down Expand Up @@ -82,37 +83,72 @@ impl<'tcx> LateLintPass<'tcx> for UnusedIoAmount {
}

if let Some(exp) = block.expr
&& matches!(exp.kind, hir::ExprKind::If(_, _, _) | hir::ExprKind::Match(_, _, _))
&& matches!(
exp.kind,
hir::ExprKind::If(_, _, _) | hir::ExprKind::Match(_, _, hir::MatchSource::Normal)
)
{
check_expr(cx, exp);
}
}
}

fn non_consuming_err_arm<'a>(cx: &LateContext<'a>, arm: &hir::Arm<'a>) -> bool {
// if there is a guard, we consider the result to be consumed
if arm.guard.is_some() {
return false;
}
if is_unreachable_or_panic(cx, arm.body) {
// if the body is unreachable or there is a panic,
// we consider the result to be consumed
return false;
}

if let PatKind::TupleStruct(ref path, [inner_pat], _) = arm.pat.kind {
return is_res_lang_ctor(cx, cx.qpath_res(path, inner_pat.hir_id), hir::LangItem::ResultErr);
}

false
}

fn non_consuming_ok_arm<'a>(cx: &LateContext<'a>, arm: &hir::Arm<'a>) -> bool {
// if there is a guard, we consider the result to be consumed
if arm.guard.is_some() {
return false;
}
if is_unreachable_or_panic(cx, arm.body) {
// if the body is unreachable or there is a panic,
// we consider the result to be consumed
return false;
}

if is_ok_wild_or_dotdot_pattern(cx, arm.pat) {
return true;
}
false
}

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)
&& is_ok_wild_or_dotdot_pattern(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
}
})
.collect();
if !found_arms.is_empty() {
emit_lint(cx, expr.span, op, found_arms.as_slice());
// we will capture only the case where the match is Ok( ) or Err( )
// prefer to match the minimum possible, and expand later if needed
// to avoid false positives on something as used as this
hir::ExprKind::Match(expr, [arm1, arm2], hir::MatchSource::Normal) if let Some(op) = should_lint(cx, expr) => {
if non_consuming_ok_arm(cx, arm1) && non_consuming_err_arm(cx, arm2) {
emit_lint(cx, expr.span, op, &[arm1.pat.span]);
}
if non_consuming_ok_arm(cx, arm2) && non_consuming_err_arm(cx, arm1) {
emit_lint(cx, expr.span, op, &[arm2.pat.span]);
}
},
hir::ExprKind::Match(_, _, hir::MatchSource::Normal) => {},
_ if let Some(op) = should_lint(cx, expr) => {
emit_lint(cx, expr.span, op, &[]);
},
Expand All @@ -130,25 +166,40 @@ fn should_lint<'a>(cx: &LateContext<'a>, mut inner: &'a hir::Expr<'a>) -> Option
check_io_mode(cx, inner)
}

fn pattern_is_ignored_ok(cx: &LateContext<'_>, pat: &hir::Pat<'_>) -> bool {
fn is_ok_wild_or_dotdot_pattern<'a>(cx: &LateContext<'a>, pat: &hir::Pat<'a>) -> 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
if let PatKind::TupleStruct(ref path, inner_pat, _) = 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,
};
if matches!(inner_pat, []) {
return true;
}

if let [cons_pat] = inner_pat
&& matches!(cons_pat.kind, PatKind::Wild)
{
return true;
}
return false;
}
false
}

// this is partially taken from panic_unimplemented
fn is_unreachable_or_panic(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool {
let expr = peel_blocks(expr);
let Some(macro_call) = root_macro_call_first_node(cx, expr) else {
return false;
};
if is_panic(cx, macro_call.def_id) {
return !cx.tcx.hir().is_inside_const_context(expr.hir_id);
}
matches!(cx.tcx.item_name(macro_call.def_id).as_str(), "unreachable")
}

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!(
Expand Down
43 changes: 43 additions & 0 deletions tests/ui/unused_io_amount.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,4 +229,47 @@ fn on_return_should_not_raise<T: io::Read + io::Write>(s: &mut T) -> io::Result<
s.read(&mut buf)
}

pub fn unwrap_in_block(rdr: &mut dyn std::io::Read) -> std::io::Result<usize> {
let read = { rdr.read(&mut [0])? };
Ok(read)
}

pub fn consumed_example(rdr: &mut dyn std::io::Read) {
match rdr.read(&mut [0]) {
Ok(0) => println!("EOF"),
Ok(_) => println!("fully read"),
Err(_) => println!("fail"),
};
match rdr.read(&mut [0]) {
Ok(0) => println!("EOF"),
Ok(_) => println!("fully read"),
Err(_) => println!("fail"),
}
}

pub fn unreachable_or_panic(rdr: &mut dyn std::io::Read) {
{
match rdr.read(&mut [0]) {
Ok(_) => unreachable!(),
Err(_) => println!("expected"),
}
}

{
match rdr.read(&mut [0]) {
Ok(_) => panic!(),
Err(_) => println!("expected"),
}
}
}

pub fn wildcards(rdr: &mut dyn std::io::Read) {
{
match rdr.read(&mut [0]) {
Ok(1) => todo!(),
_ => todo!(),
}
}
}

fn main() {}
8 changes: 4 additions & 4 deletions tests/ui/unused_io_amount.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ note: the result is consumed here, but the amount of I/O bytes remains unhandled
--> $DIR/unused_io_amount.rs:149:9
|
LL | Ok(_) => todo!(),
| ^^^^^^^^^^^^^^^^
| ^^^^^

error: read amount is not handled
--> $DIR/unused_io_amount.rs:155:11
Expand All @@ -193,7 +193,7 @@ note: the result is consumed here, but the amount of I/O bytes remains unhandled
--> $DIR/unused_io_amount.rs:157:9
|
LL | Ok(_) => todo!(),
| ^^^^^^^^^^^^^^^^
| ^^^^^

error: read amount is not handled
--> $DIR/unused_io_amount.rs:164:11
Expand All @@ -206,7 +206,7 @@ note: the result is consumed here, but the amount of I/O bytes remains unhandled
--> $DIR/unused_io_amount.rs:166:9
|
LL | Ok(_) => todo!(),
| ^^^^^^^^^^^^^^^^
| ^^^^^

error: written amount is not handled
--> $DIR/unused_io_amount.rs:173:11
Expand All @@ -219,7 +219,7 @@ note: the result is consumed here, but the amount of I/O bytes remains unhandled
--> $DIR/unused_io_amount.rs:175:9
|
LL | Ok(_) => todo!(),
| ^^^^^^^^^^^^^^^^
| ^^^^^

error: read amount is not handled
--> $DIR/unused_io_amount.rs:186:8
Expand Down

0 comments on commit fe8c2e2

Please sign in to comment.