From ea77fcce86947447e03baadcb4ab28cd5676f053 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Fri, 24 Oct 2025 20:29:29 +0200 Subject: [PATCH 1/3] clean-up --- clippy_lints/src/unwrap.rs | 93 ++++++++------- .../ui/checked_unwrap/complex_conditionals.rs | 28 +---- .../complex_conditionals.stderr | 56 ++++----- .../complex_conditionals_nested.rs | 11 +- .../complex_conditionals_nested.stderr | 19 +-- tests/ui/checked_unwrap/if_let_chains.rs | 2 +- tests/ui/checked_unwrap/if_let_chains.stderr | 7 +- .../ui/checked_unwrap/simple_conditionals.rs | 38 +----- .../checked_unwrap/simple_conditionals.stderr | 109 +++++++----------- 9 files changed, 140 insertions(+), 223 deletions(-) diff --git a/clippy_lints/src/unwrap.rs b/clippy_lints/src/unwrap.rs index 99201a1ca215..0a96ee092089 100644 --- a/clippy_lints/src/unwrap.rs +++ b/clippy_lints/src/unwrap.rs @@ -156,39 +156,52 @@ fn collect_unwrap_info<'tcx>( } } - match expr.kind { - ExprKind::Binary(op, left, right) - if matches!( - (invert, op.node), - (false, BinOpKind::And | BinOpKind::BitAnd) | (true, BinOpKind::Or | BinOpKind::BitOr) - ) => - { - let mut unwrap_info = collect_unwrap_info(cx, if_expr, left, branch, invert, false); - unwrap_info.extend(collect_unwrap_info(cx, if_expr, right, branch, invert, false)); - unwrap_info - }, - ExprKind::Unary(UnOp::Not, expr) => collect_unwrap_info(cx, if_expr, expr, branch, !invert, false), - ExprKind::MethodCall(method_name, receiver, [], _) - if let Some(local_id) = receiver.res_local_id() - && let ty = cx.typeck_results().expr_ty(receiver) - && let name = method_name.ident.name - && let Some((kind, unwrappable)) = option_or_result_call(cx, ty, name) => - { - let safe_to_unwrap = unwrappable != invert; - - vec![UnwrapInfo { - local_id, - if_expr, - check: expr, - check_name: name, - branch, - safe_to_unwrap, - kind, - is_entire_condition, - }] - }, - _ => vec![], + fn inner<'tcx>( + cx: &LateContext<'tcx>, + if_expr: &'tcx Expr<'_>, + expr: &'tcx Expr<'_>, + branch: &'tcx Expr<'_>, + invert: bool, + is_entire_condition: bool, + out: &mut Vec>, + ) { + match expr.kind { + ExprKind::Binary(op, left, right) + if matches!( + (invert, op.node), + (false, BinOpKind::And | BinOpKind::BitAnd) | (true, BinOpKind::Or | BinOpKind::BitOr) + ) => + { + inner(cx, if_expr, left, branch, invert, false, out); + inner(cx, if_expr, right, branch, invert, false, out); + }, + ExprKind::Unary(UnOp::Not, expr) => inner(cx, if_expr, expr, branch, !invert, false, out), + ExprKind::MethodCall(method_name, receiver, [], _) + if let Some(local_id) = receiver.res_local_id() + && let ty = cx.typeck_results().expr_ty(receiver) + && let name = method_name.ident.name + && let Some((kind, unwrappable)) = option_or_result_call(cx, ty, name) => + { + let safe_to_unwrap = unwrappable != invert; + + out.push(UnwrapInfo { + local_id, + if_expr, + check: expr, + check_name: name, + branch, + safe_to_unwrap, + kind, + is_entire_condition, + }); + }, + _ => {}, + } } + + let mut out = vec![]; + inner(cx, if_expr, expr, branch, invert, is_entire_condition, &mut out); + out } /// A HIR visitor delegate that checks if a local variable of type `Option` or `Result` is mutated, @@ -321,21 +334,14 @@ impl<'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'_, 'tcx> { && let Some(id) = self_arg.res_local_id() && matches!(method_name.ident.name, sym::unwrap | sym::expect | sym::unwrap_err) && let call_to_unwrap = matches!(method_name.ident.name, sym::unwrap | sym::expect) - && let Some(unwrappable) = self.unwrappables.iter() - .find(|u| u.local_id == id) + && let Some(unwrappable) = self.unwrappables.iter().find(|u| u.local_id == id) // Span contexts should not differ with the conditional branch && let span_ctxt = expr.span.ctxt() && unwrappable.branch.span.ctxt() == span_ctxt && unwrappable.check.span.ctxt() == span_ctxt { if call_to_unwrap == unwrappable.safe_to_unwrap { - let is_entire_condition = unwrappable.is_entire_condition; let unwrappable_variable_name = self.cx.tcx.hir_name(unwrappable.local_id); - let suggested_pattern = if call_to_unwrap { - unwrappable.kind.success_variant_pattern() - } else { - unwrappable.kind.error_variant_pattern() - }; span_lint_hir_and_then( self.cx, @@ -347,12 +353,17 @@ impl<'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'_, 'tcx> { method_name.ident.name, unwrappable.check_name, ), |diag| { - if is_entire_condition { + if unwrappable.is_entire_condition { diag.span_suggestion( unwrappable.check.span.with_lo(unwrappable.if_expr.span.lo()), "try", format!( "if let {suggested_pattern} = {borrow_prefix}{unwrappable_variable_name}", + suggested_pattern = if call_to_unwrap { + unwrappable.kind.success_variant_pattern() + } else { + unwrappable.kind.error_variant_pattern() + }, borrow_prefix = match as_ref_kind { Some(AsRefKind::AsRef) => "&", Some(AsRefKind::AsMut) => "&mut ", diff --git a/tests/ui/checked_unwrap/complex_conditionals.rs b/tests/ui/checked_unwrap/complex_conditionals.rs index 7d0bcc547a42..d1db2e67e269 100644 --- a/tests/ui/checked_unwrap/complex_conditionals.rs +++ b/tests/ui/checked_unwrap/complex_conditionals.rs @@ -1,27 +1,19 @@ -#![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] -#![allow( - clippy::if_same_then_else, - clippy::branches_sharing_code, - clippy::unnecessary_literal_unwrap -)] +#![warn(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] +#![expect(clippy::branches_sharing_code, clippy::unnecessary_literal_unwrap)] fn test_complex_conditions() { let x: Result<(), ()> = Ok(()); let y: Result<(), ()> = Ok(()); if x.is_ok() && y.is_err() { - // unnecessary x.unwrap(); //~^ unnecessary_unwrap - // will panic x.unwrap_err(); //~^ panicking_unwrap - // will panic y.unwrap(); //~^ panicking_unwrap - // unnecessary y.unwrap_err(); //~^ unnecessary_unwrap } else { @@ -37,45 +29,35 @@ fn test_complex_conditions() { x.unwrap(); y.unwrap(); } else { - // will panic x.unwrap(); //~^ panicking_unwrap - // unnecessary x.unwrap_err(); //~^ unnecessary_unwrap - // will panic y.unwrap(); //~^ panicking_unwrap - // unnecessary y.unwrap_err(); //~^ unnecessary_unwrap } let z: Result<(), ()> = Ok(()); if x.is_ok() && !(y.is_ok() || z.is_err()) { - // unnecessary x.unwrap(); //~^ unnecessary_unwrap - // will panic x.unwrap_err(); //~^ panicking_unwrap - // will panic y.unwrap(); //~^ panicking_unwrap - // unnecessary y.unwrap_err(); //~^ unnecessary_unwrap - // unnecessary z.unwrap(); //~^ unnecessary_unwrap - // will panic z.unwrap_err(); //~^ panicking_unwrap } @@ -85,27 +67,21 @@ fn test_complex_conditions() { y.unwrap(); z.unwrap(); } else { - // will panic x.unwrap(); //~^ panicking_unwrap - // unnecessary x.unwrap_err(); //~^ unnecessary_unwrap - // unnecessary y.unwrap(); //~^ unnecessary_unwrap - // will panic y.unwrap_err(); //~^ panicking_unwrap - // will panic z.unwrap(); //~^ panicking_unwrap - // unnecessary z.unwrap_err(); //~^ unnecessary_unwrap } diff --git a/tests/ui/checked_unwrap/complex_conditionals.stderr b/tests/ui/checked_unwrap/complex_conditionals.stderr index d3905850c970..e154e3c35dc9 100644 --- a/tests/ui/checked_unwrap/complex_conditionals.stderr +++ b/tests/ui/checked_unwrap/complex_conditionals.stderr @@ -1,21 +1,17 @@ error: called `unwrap` on `x` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/complex_conditionals.rs:13:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:8:9 | LL | if x.is_ok() && y.is_err() { | --------- the check is happening here -LL | // unnecessary LL | x.unwrap(); | ^^^^^^^^^^ | = help: try using `if let` or `match` -note: the lint level is defined here - --> tests/ui/checked_unwrap/complex_conditionals.rs:1:35 - | -LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::unnecessary-unwrap` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_unwrap)]` error: this call to `unwrap_err()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals.rs:17:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:11:9 | LL | if x.is_ok() && y.is_err() { | --------- because of this check @@ -23,14 +19,11 @@ LL | if x.is_ok() && y.is_err() { LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ | -note: the lint level is defined here - --> tests/ui/checked_unwrap/complex_conditionals.rs:1:9 - | -LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] - | ^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::panicking-unwrap` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::panicking_unwrap)]` error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals.rs:21:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:14:9 | LL | if x.is_ok() && y.is_err() { | ---------- because of this check @@ -39,7 +32,7 @@ LL | y.unwrap(); | ^^^^^^^^^^ error: called `unwrap_err` on `y` after checking its variant with `is_err` - --> tests/ui/checked_unwrap/complex_conditionals.rs:25:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:17:9 | LL | if x.is_ok() && y.is_err() { | ---------- the check is happening here @@ -50,7 +43,7 @@ LL | y.unwrap_err(); = help: try using `if let` or `match` error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals.rs:41:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:32:9 | LL | if x.is_ok() || y.is_ok() { | --------- because of this check @@ -59,7 +52,7 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: called `unwrap_err` on `x` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/complex_conditionals.rs:45:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:35:9 | LL | if x.is_ok() || y.is_ok() { | --------- the check is happening here @@ -70,7 +63,7 @@ LL | x.unwrap_err(); = help: try using `if let` or `match` error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals.rs:49:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:38:9 | LL | if x.is_ok() || y.is_ok() { | --------- because of this check @@ -79,7 +72,7 @@ LL | y.unwrap(); | ^^^^^^^^^^ error: called `unwrap_err` on `y` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/complex_conditionals.rs:53:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:41:9 | LL | if x.is_ok() || y.is_ok() { | --------- the check is happening here @@ -90,18 +83,17 @@ LL | y.unwrap_err(); = help: try using `if let` or `match` error: called `unwrap` on `x` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/complex_conditionals.rs:59:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:46:9 | LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { | --------- the check is happening here -LL | // unnecessary LL | x.unwrap(); | ^^^^^^^^^^ | = help: try using `if let` or `match` error: this call to `unwrap_err()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals.rs:63:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:49:9 | LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { | --------- because of this check @@ -110,7 +102,7 @@ LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals.rs:67:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:52:9 | LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { | --------- because of this check @@ -119,7 +111,7 @@ LL | y.unwrap(); | ^^^^^^^^^^ error: called `unwrap_err` on `y` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/complex_conditionals.rs:71:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:55:9 | LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { | --------- the check is happening here @@ -130,7 +122,7 @@ LL | y.unwrap_err(); = help: try using `if let` or `match` error: called `unwrap` on `z` after checking its variant with `is_err` - --> tests/ui/checked_unwrap/complex_conditionals.rs:75:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:58:9 | LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { | ---------- the check is happening here @@ -141,7 +133,7 @@ LL | z.unwrap(); = help: try using `if let` or `match` error: this call to `unwrap_err()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals.rs:79:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:61:9 | LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { | ---------- because of this check @@ -150,7 +142,7 @@ LL | z.unwrap_err(); | ^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals.rs:89:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:70:9 | LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { | --------- because of this check @@ -159,7 +151,7 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: called `unwrap_err` on `x` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/complex_conditionals.rs:93:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:73:9 | LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { | --------- the check is happening here @@ -170,7 +162,7 @@ LL | x.unwrap_err(); = help: try using `if let` or `match` error: called `unwrap` on `y` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/complex_conditionals.rs:97:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:76:9 | LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { | --------- the check is happening here @@ -181,7 +173,7 @@ LL | y.unwrap(); = help: try using `if let` or `match` error: this call to `unwrap_err()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals.rs:101:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:79:9 | LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { | --------- because of this check @@ -190,7 +182,7 @@ LL | y.unwrap_err(); | ^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals.rs:105:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:82:9 | LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { | ---------- because of this check @@ -199,7 +191,7 @@ LL | z.unwrap(); | ^^^^^^^^^^ error: called `unwrap_err` on `z` after checking its variant with `is_err` - --> tests/ui/checked_unwrap/complex_conditionals.rs:109:9 + --> tests/ui/checked_unwrap/complex_conditionals.rs:85:9 | LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { | ---------- the check is happening here diff --git a/tests/ui/checked_unwrap/complex_conditionals_nested.rs b/tests/ui/checked_unwrap/complex_conditionals_nested.rs index 7635f848cb34..6789e7c262b3 100644 --- a/tests/ui/checked_unwrap/complex_conditionals_nested.rs +++ b/tests/ui/checked_unwrap/complex_conditionals_nested.rs @@ -1,19 +1,14 @@ -#![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] -#![allow( - clippy::if_same_then_else, - clippy::branches_sharing_code, - clippy::unnecessary_literal_unwrap -)] //@no-rustfix: has placeholders +#![warn(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] +#![expect(clippy::branches_sharing_code, clippy::unnecessary_literal_unwrap)] + fn test_nested() { fn nested() { let x = Some(()); if x.is_some() { - // unnecessary x.unwrap(); //~^ unnecessary_unwrap } else { - // will panic x.unwrap(); //~^ panicking_unwrap } diff --git a/tests/ui/checked_unwrap/complex_conditionals_nested.stderr b/tests/ui/checked_unwrap/complex_conditionals_nested.stderr index 329be4d36621..7e4ef049f4a5 100644 --- a/tests/ui/checked_unwrap/complex_conditionals_nested.stderr +++ b/tests/ui/checked_unwrap/complex_conditionals_nested.stderr @@ -1,20 +1,16 @@ error: called `unwrap` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/complex_conditionals_nested.rs:13:13 + --> tests/ui/checked_unwrap/complex_conditionals_nested.rs:9:13 | LL | if x.is_some() { | -------------- help: try: `if let Some() = x` -LL | // unnecessary LL | x.unwrap(); | ^^^^^^^^^^ | -note: the lint level is defined here - --> tests/ui/checked_unwrap/complex_conditionals_nested.rs:1:35 - | -LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::unnecessary-unwrap` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_unwrap)]` error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/complex_conditionals_nested.rs:17:13 + --> tests/ui/checked_unwrap/complex_conditionals_nested.rs:12:13 | LL | if x.is_some() { | ----------- because of this check @@ -22,11 +18,8 @@ LL | if x.is_some() { LL | x.unwrap(); | ^^^^^^^^^^ | -note: the lint level is defined here - --> tests/ui/checked_unwrap/complex_conditionals_nested.rs:1:9 - | -LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] - | ^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::panicking-unwrap` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::panicking_unwrap)]` error: aborting due to 2 previous errors diff --git a/tests/ui/checked_unwrap/if_let_chains.rs b/tests/ui/checked_unwrap/if_let_chains.rs index cfa7715965cd..5c20ebb80024 100644 --- a/tests/ui/checked_unwrap/if_let_chains.rs +++ b/tests/ui/checked_unwrap/if_let_chains.rs @@ -1,5 +1,5 @@ //@require-annotations-for-level: ERROR -#![deny(clippy::unnecessary_unwrap)] +#![warn(clippy::unnecessary_unwrap)] #[clippy::msrv = "1.85"] fn if_let_chains_unsupported(a: Option, b: Option) { diff --git a/tests/ui/checked_unwrap/if_let_chains.stderr b/tests/ui/checked_unwrap/if_let_chains.stderr index 8a4137de37a3..801b074fc277 100644 --- a/tests/ui/checked_unwrap/if_let_chains.stderr +++ b/tests/ui/checked_unwrap/if_let_chains.stderr @@ -8,11 +8,8 @@ LL | println!("the value of a is {}", a.unwrap()); | ^^^^^^^^^^ | = help: try using `match` -note: the lint level is defined here - --> tests/ui/checked_unwrap/if_let_chains.rs:2:9 - | -LL | #![deny(clippy::unnecessary_unwrap)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::unnecessary-unwrap` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_unwrap)]` error: called `unwrap` on `a` after checking its variant with `is_none` --> tests/ui/checked_unwrap/if_let_chains.rs:20:42 diff --git a/tests/ui/checked_unwrap/simple_conditionals.rs b/tests/ui/checked_unwrap/simple_conditionals.rs index bba264080b40..b76269307159 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.rs +++ b/tests/ui/checked_unwrap/simple_conditionals.rs @@ -1,6 +1,6 @@ //@no-rustfix: has placeholders -#![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] -#![allow( +#![warn(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] +#![expect( clippy::if_same_then_else, clippy::branches_sharing_code, clippy::unnecessary_literal_unwrap @@ -9,7 +9,6 @@ macro_rules! m { ($a:expr) => { if $a.is_some() { - // unnecessary $a.unwrap(); //~^ unnecessary_unwrap } @@ -43,90 +42,71 @@ macro_rules! checks_some { fn main() { let x = Some(()); if x.is_some() { - // unnecessary x.unwrap(); //~^ unnecessary_unwrap - // unnecessary x.expect("an error message"); //~^ unnecessary_unwrap } else { - // will panic x.unwrap(); //~^ panicking_unwrap - // will panic x.expect("an error message"); //~^ panicking_unwrap } if x.is_none() { - // will panic x.unwrap(); //~^ panicking_unwrap } else { - // unnecessary x.unwrap(); //~^ unnecessary_unwrap } m!(x); - // ok checks_in_param!(x.is_some(), x.unwrap()); - // ok checks_unwrap!(x, x.unwrap()); - // ok checks_some!(x.is_some(), x); let mut x: Result<(), ()> = Ok(()); if x.is_ok() { - // unnecessary x.unwrap(); //~^ unnecessary_unwrap - // unnecessary x.expect("an error message"); //~^ unnecessary_unwrap - // will panic x.unwrap_err(); //~^ panicking_unwrap } else { - // will panic x.unwrap(); //~^ panicking_unwrap - // will panic x.expect("an error message"); //~^ panicking_unwrap - // unnecessary x.unwrap_err(); //~^ unnecessary_unwrap } if x.is_err() { - // will panic x.unwrap(); //~^ panicking_unwrap - // unnecessary x.unwrap_err(); //~^ unnecessary_unwrap } else { - // unnecessary x.unwrap(); //~^ unnecessary_unwrap - // will panic x.unwrap_err(); //~^ panicking_unwrap } if x.is_ok() { x = Err(()); - // not unnecessary because of mutation of x + // not unnecessary because of mutation of `x` // it will always panic but the lint is not smart enough to see this (it only // checks if conditions). x.unwrap(); } else { x = Ok(()); - // not unnecessary because of mutation of x + // not unnecessary because of mutation of `x` // it will always panic but the lint is not smart enough to see this (it // only checks if conditions). x.unwrap_err(); @@ -175,13 +155,11 @@ fn issue11371() { //~^ panicking_unwrap } - // This should not lint. Statics are, at the time of writing, not linted on anyway, - // but if at some point they are supported by this lint, it should correctly see that - // `X` is being mutated and not suggest `if let Some(..) = X {}` + // This should not lint and suggest `if let Some(..) = X {}`, as `X` is being mutated static mut X: Option = Some(123); unsafe { + #[expect(static_mut_refs)] if X.is_some() { - //~^ ERROR: creating a shared reference X = None; X.unwrap(); } @@ -299,17 +277,13 @@ fn check_expect() { let x = Some(()); if x.is_some() { #[expect(clippy::unnecessary_unwrap)] - // unnecessary x.unwrap(); #[expect(clippy::unnecessary_unwrap)] - // unnecessary x.expect("an error message"); } else { #[expect(clippy::panicking_unwrap)] - // will panic x.unwrap(); #[expect(clippy::panicking_unwrap)] - // will panic x.expect("an error message"); } } diff --git a/tests/ui/checked_unwrap/simple_conditionals.stderr b/tests/ui/checked_unwrap/simple_conditionals.stderr index 2007a8595413..ad234ee1d96a 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.stderr +++ b/tests/ui/checked_unwrap/simple_conditionals.stderr @@ -1,20 +1,16 @@ error: called `unwrap` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:47:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:45:9 | LL | if x.is_some() { | -------------- help: try: `if let Some() = x` -LL | // unnecessary LL | x.unwrap(); | ^^^^^^^^^^ | -note: the lint level is defined here - --> tests/ui/checked_unwrap/simple_conditionals.rs:2:35 - | -LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::unnecessary-unwrap` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_unwrap)]` error: called `expect` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:51:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:48:9 | LL | if x.is_some() { | -------------- help: try: `if let Some() = x` @@ -23,7 +19,7 @@ LL | x.expect("an error message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:55:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:51:9 | LL | if x.is_some() { | ----------- because of this check @@ -31,14 +27,11 @@ LL | if x.is_some() { LL | x.unwrap(); | ^^^^^^^^^^ | -note: the lint level is defined here - --> tests/ui/checked_unwrap/simple_conditionals.rs:2:9 - | -LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] - | ^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::panicking-unwrap` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::panicking_unwrap)]` error: this call to `expect()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:59:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:54:9 | LL | if x.is_some() { | ----------- because of this check @@ -47,16 +40,15 @@ LL | x.expect("an error message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:64:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:58:9 | LL | if x.is_none() { | ----------- because of this check -LL | // will panic LL | x.unwrap(); | ^^^^^^^^^^ error: called `unwrap` on `x` after checking its variant with `is_none` - --> tests/ui/checked_unwrap/simple_conditionals.rs:68:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:61:9 | LL | if x.is_none() { | -------------- help: try: `if let Some() = x` @@ -65,11 +57,10 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: called `unwrap` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:13:13 + --> tests/ui/checked_unwrap/simple_conditionals.rs:12:13 | LL | if $a.is_some() { | --------------- help: try: `if let Some() = x` -LL | // unnecessary LL | $a.unwrap(); | ^^^^^^^^^^^ ... @@ -79,16 +70,15 @@ LL | m!(x); = note: this error originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info) error: called `unwrap` on `x` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:81:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:70:9 | LL | if x.is_ok() { | ------------ help: try: `if let Ok() = x` -LL | // unnecessary LL | x.unwrap(); | ^^^^^^^^^^ error: called `expect` on `x` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:85:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:73:9 | LL | if x.is_ok() { | ------------ help: try: `if let Ok() = x` @@ -97,7 +87,7 @@ LL | x.expect("an error message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap_err()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:89:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:76:9 | LL | if x.is_ok() { | --------- because of this check @@ -106,7 +96,7 @@ LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:93:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:79:9 | LL | if x.is_ok() { | --------- because of this check @@ -115,7 +105,7 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: this call to `expect()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:97:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:82:9 | LL | if x.is_ok() { | --------- because of this check @@ -124,7 +114,7 @@ LL | x.expect("an error message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap_err` on `x` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:101:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:85:9 | LL | if x.is_ok() { | ------------ help: try: `if let Err() = x` @@ -133,16 +123,15 @@ LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:106:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:89:9 | LL | if x.is_err() { | ---------- because of this check -LL | // will panic LL | x.unwrap(); | ^^^^^^^^^^ error: called `unwrap_err` on `x` after checking its variant with `is_err` - --> tests/ui/checked_unwrap/simple_conditionals.rs:110:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:92:9 | LL | if x.is_err() { | ------------- help: try: `if let Err() = x` @@ -151,7 +140,7 @@ LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ error: called `unwrap` on `x` after checking its variant with `is_err` - --> tests/ui/checked_unwrap/simple_conditionals.rs:114:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:95:9 | LL | if x.is_err() { | ------------- help: try: `if let Ok() = x` @@ -160,7 +149,7 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: this call to `unwrap_err()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:118:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:98:9 | LL | if x.is_err() { | ---------- because of this check @@ -169,7 +158,7 @@ LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ error: called `unwrap` on `option` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:143:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:123:9 | LL | if option.is_some() { | ------------------- help: try: `if let Some() = &option` @@ -177,7 +166,7 @@ LL | option.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:146:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:126:9 | LL | if option.is_some() { | ---------------- because of this check @@ -186,7 +175,7 @@ LL | option.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `result` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:153:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:133:9 | LL | if result.is_ok() { | ----------------- help: try: `if let Ok() = &result` @@ -194,7 +183,7 @@ LL | result.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:156:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:136:9 | LL | if result.is_ok() { | -------------- because of this check @@ -203,7 +192,7 @@ LL | result.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `option` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:162:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:142:9 | LL | if option.is_some() { | ------------------- help: try: `if let Some() = &mut option` @@ -211,7 +200,7 @@ LL | option.as_mut().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:165:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:145:9 | LL | if option.is_some() { | ---------------- because of this check @@ -220,7 +209,7 @@ LL | option.as_mut().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `result` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:171:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:151:9 | LL | if result.is_ok() { | ----------------- help: try: `if let Ok() = &mut result` @@ -228,7 +217,7 @@ LL | result.as_mut().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:174:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:154:9 | LL | if result.is_ok() { | -------------- because of this check @@ -237,7 +226,7 @@ LL | result.as_mut().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `option` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:205:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:183:17 | LL | if option.is_some() { | ------------------- help: try: `if let Some() = &option` @@ -245,7 +234,7 @@ LL | let _ = option.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:208:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:186:17 | LL | if option.is_some() { | ---------------- because of this check @@ -254,7 +243,7 @@ LL | let _ = option.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `result` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:216:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:194:9 | LL | if result.is_ok() { | ----------------- help: try: `if let Ok() = &result` @@ -263,7 +252,7 @@ LL | result.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:220:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:198:9 | LL | if result.is_ok() { | -------------- because of this check @@ -272,7 +261,7 @@ LL | result.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:246:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:224:17 | LL | if x.is_some() { | -------------- help: try: `if let Some() = x` @@ -280,7 +269,7 @@ LL | _ = x.unwrap(); | ^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:249:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:227:17 | LL | if x.is_some() { | ----------- because of this check @@ -289,7 +278,7 @@ LL | _ = x.unwrap(); | ^^^^^^^^^^ error: called `unwrap` on `r` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:255:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:233:17 | LL | if r.is_ok() { | ------------ help: try: `if let Ok() = &r` @@ -297,7 +286,7 @@ LL | _ = r.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:258:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:236:17 | LL | if r.is_ok() { | --------- because of this check @@ -306,7 +295,7 @@ LL | _ = r.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:267:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:245:17 | LL | if x.is_some() { | -------------- help: try: `if let Some() = x` @@ -314,7 +303,7 @@ LL | _ = x.unwrap(); | ^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:270:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:248:17 | LL | if x.is_some() { | ----------- because of this check @@ -323,7 +312,7 @@ LL | _ = x.unwrap(); | ^^^^^^^^^^ error: called `unwrap` on `option` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:280:26 + --> tests/ui/checked_unwrap/simple_conditionals.rs:258:26 | LL | if option.is_some() { | ------------------- help: try: `if let Some() = option` @@ -331,7 +320,7 @@ LL | println!("{:?}", option.unwrap()); | ^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:283:26 + --> tests/ui/checked_unwrap/simple_conditionals.rs:261:26 | LL | if option.is_some() { | ---------------- because of this check @@ -340,7 +329,7 @@ LL | println!("{:?}", option.unwrap()); | ^^^^^^^^^^^^^^^ error: called `unwrap` on `result` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:290:26 + --> tests/ui/checked_unwrap/simple_conditionals.rs:268:26 | LL | if result.is_ok() { | ----------------- help: try: `if let Ok() = result` @@ -348,7 +337,7 @@ LL | println!("{:?}", result.unwrap()); | ^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:293:26 + --> tests/ui/checked_unwrap/simple_conditionals.rs:271:26 | LL | if result.is_ok() { | -------------- because of this check @@ -356,15 +345,5 @@ LL | if result.is_ok() { LL | println!("{:?}", result.unwrap()); | ^^^^^^^^^^^^^^^ -error: creating a shared reference to mutable static - --> tests/ui/checked_unwrap/simple_conditionals.rs:183:12 - | -LL | if X.is_some() { - | ^^^^^^^^^^^ shared reference to mutable static - | - = note: for more information, see - = note: shared references to mutable statics are dangerous; it's undefined behavior if the static is mutated or if a mutable reference is created for it while the shared reference lives - = note: `#[deny(static_mut_refs)]` (part of `#[deny(rust_2024_compatibility)]`) on by default - -error: aborting due to 40 previous errors +error: aborting due to 39 previous errors From b39a73474ac341f42c6a88536447fb06db6256de Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Fri, 24 Oct 2025 21:30:18 +0200 Subject: [PATCH 2/3] feat({unnecessary,panicking}_unwrap): lint field accesses --- clippy_lints/src/unwrap.rs | 130 +++++++++++++-- .../ui/checked_unwrap/simple_conditionals.rs | 85 +++++++++- .../checked_unwrap/simple_conditionals.stderr | 150 +++++++++++++----- 3 files changed, 310 insertions(+), 55 deletions(-) diff --git a/clippy_lints/src/unwrap.rs b/clippy_lints/src/unwrap.rs index 0a96ee092089..a5ef7f8e5559 100644 --- a/clippy_lints/src/unwrap.rs +++ b/clippy_lints/src/unwrap.rs @@ -1,15 +1,20 @@ +use std::borrow::Cow; + use clippy_config::Conf; use clippy_utils::diagnostics::span_lint_hir_and_then; use clippy_utils::msrvs::Msrv; use clippy_utils::res::{MaybeDef, MaybeResPath}; +use clippy_utils::source::snippet; use clippy_utils::usage::is_potentially_local_place; use clippy_utils::{can_use_if_let_chains, higher, sym}; +use rustc_abi::FieldIdx; use rustc_errors::Applicability; use rustc_hir::intravisit::{FnKind, Visitor, walk_expr, walk_fn}; use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Node, UnOp}; -use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceWithHirId}; -use rustc_lint::{LateContext, LateLintPass}; +use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, Place, PlaceWithHirId}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::nested_filter; +use rustc_middle::hir::place::ProjectionKind; use rustc_middle::mir::FakeReadCause; use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_session::impl_lint_pass; @@ -114,11 +119,89 @@ impl UnwrappableKind { } } +#[derive(Copy, Clone, Debug, Eq)] +enum Local { + /// `x.opt` + WithFieldAccess { + local_id: HirId, + field_idx: FieldIdx, + /// The span of the whole expression + span: Span, + }, + /// `x` + Pure { local_id: HirId }, +} + +/// Identical to derived impl, but ignores `span` on [`Local::WithFieldAccess`] +impl PartialEq for Local { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + ( + Self::WithFieldAccess { + local_id: self_local_id, + field_idx: self_field_idx, + .. + }, + Self::WithFieldAccess { + local_id: other_local_id, + field_idx: other_field_idx, + .. + }, + ) => self_local_id == other_local_id && self_field_idx == other_field_idx, + ( + Self::Pure { + local_id: self_local_id, + }, + Self::Pure { + local_id: other_local_id, + }, + ) => self_local_id == other_local_id, + _ => false, + } + } +} + +impl Local { + fn snippet(self, cx: &LateContext<'_>) -> Cow<'static, str> { + match self { + Self::WithFieldAccess { span, .. } => snippet(cx.sess(), span, "_"), + Self::Pure { local_id } => cx.tcx.hir_name(local_id).to_string().into(), + } + } + + fn is_potentially_local_place(self, place: &Place<'_>) -> bool { + match self { + Self::WithFieldAccess { + local_id, field_idx, .. + } => { + if is_potentially_local_place(local_id, place) + // If there were projections other than the field projection, err on the side of caution and say + // that they _might_ have mutated the field + // + // The reason we use `<=` and not `==` is that, if there were no projections, then the whole local + // was mutated, which means that our field was mutated as well + && place.projections.len() <= 1 + && place.projections.last().is_none_or(|proj| match proj.kind { + ProjectionKind::Field(f_idx, _) => f_idx == field_idx, + // If this is a projection we don't expect, it _might_ be mutating something + _ => false, + }) + { + true + } else { + false + } + }, + Self::Pure { local_id } => is_potentially_local_place(local_id, place), + } + } +} + /// Contains information about whether a variable can be unwrapped. #[derive(Copy, Clone, Debug)] struct UnwrapInfo<'tcx> { /// The variable that is checked - local_id: HirId, + local: Local, /// The if itself if_expr: &'tcx Expr<'tcx>, /// The check, like `x.is_ok()` @@ -177,7 +260,7 @@ fn collect_unwrap_info<'tcx>( }, ExprKind::Unary(UnOp::Not, expr) => inner(cx, if_expr, expr, branch, !invert, false, out), ExprKind::MethodCall(method_name, receiver, [], _) - if let Some(local_id) = receiver.res_local_id() + if let Some(local) = extract_local(cx, receiver) && let ty = cx.typeck_results().expr_ty(receiver) && let name = method_name.ident.name && let Some((kind, unwrappable)) = option_or_result_call(cx, ty, name) => @@ -185,7 +268,7 @@ fn collect_unwrap_info<'tcx>( let safe_to_unwrap = unwrappable != invert; out.push(UnwrapInfo { - local_id, + local, if_expr, check: expr, check_name: name, @@ -204,6 +287,25 @@ fn collect_unwrap_info<'tcx>( out } +/// Extracts either a local used by itself ([`Local::Pure`]), or a field access to a local +/// ([`Local::WithFieldAccess`]) +fn extract_local(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { + if let Some(local_id) = expr.res_local_id() { + Some(Local::Pure { local_id }) + } else if let ExprKind::Field(recv, _) = expr.kind + && let Some(local_id) = recv.res_local_id() + && let Some(field_idx) = cx.typeck_results().opt_field_index(expr.hir_id) + { + Some(Local::WithFieldAccess { + local_id, + field_idx, + span: expr.span, + }) + } else { + None + } +} + /// A HIR visitor delegate that checks if a local variable of type `Option` or `Result` is mutated, /// *except* for if `.as_mut()` is called. /// The reason for why we allow that one specifically is that `.as_mut()` cannot change @@ -213,7 +315,7 @@ fn collect_unwrap_info<'tcx>( /// (And also `.as_mut()` is a somewhat common method that is still worth linting on.) struct MutationVisitor<'tcx> { is_mutated: bool, - local_id: HirId, + local: Local, tcx: TyCtxt<'tcx>, } @@ -237,7 +339,7 @@ fn is_as_mut_use(tcx: TyCtxt<'_>, expr_id: HirId) -> bool { impl<'tcx> Delegate<'tcx> for MutationVisitor<'tcx> { fn borrow(&mut self, cat: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind) { if let ty::BorrowKind::Mutable = bk - && is_potentially_local_place(self.local_id, &cat.place) + && self.local.is_potentially_local_place(&cat.place) && !is_as_mut_use(self.tcx, diag_expr_id) { self.is_mutated = true; @@ -245,7 +347,7 @@ impl<'tcx> Delegate<'tcx> for MutationVisitor<'tcx> { } fn mutate(&mut self, cat: &PlaceWithHirId<'tcx>, _: HirId) { - if is_potentially_local_place(self.local_id, &cat.place) { + if self.local.is_potentially_local_place(&cat.place) { self.is_mutated = true; } } @@ -269,7 +371,7 @@ impl<'tcx> UnwrappableVariablesVisitor<'_, 'tcx> { for unwrap_info in collect_unwrap_info(self.cx, if_expr, cond, branch, else_branch, true) { let mut delegate = MutationVisitor { is_mutated: false, - local_id: unwrap_info.local_id, + local: unwrap_info.local, tcx: self.cx.tcx, }; @@ -331,17 +433,17 @@ impl<'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'_, 'tcx> { // find `unwrap[_err]()` or `expect("...")` calls: if let ExprKind::MethodCall(method_name, self_arg, ..) = expr.kind && let (self_arg, as_ref_kind) = consume_option_as_ref(self_arg) - && let Some(id) = self_arg.res_local_id() + && let Some(local) = extract_local(self.cx, self_arg) && matches!(method_name.ident.name, sym::unwrap | sym::expect | sym::unwrap_err) && let call_to_unwrap = matches!(method_name.ident.name, sym::unwrap | sym::expect) - && let Some(unwrappable) = self.unwrappables.iter().find(|u| u.local_id == id) + && let Some(unwrappable) = self.unwrappables.iter().find(|u| u.local == local) // Span contexts should not differ with the conditional branch && let span_ctxt = expr.span.ctxt() && unwrappable.branch.span.ctxt() == span_ctxt && unwrappable.check.span.ctxt() == span_ctxt { if call_to_unwrap == unwrappable.safe_to_unwrap { - let unwrappable_variable_name = self.cx.tcx.hir_name(unwrappable.local_id); + let unwrappable_variable_str = unwrappable.local.snippet(self.cx); span_lint_hir_and_then( self.cx, @@ -349,7 +451,7 @@ impl<'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'_, 'tcx> { expr.hir_id, expr.span, format!( - "called `{}` on `{unwrappable_variable_name}` after checking its variant with `{}`", + "called `{}` on `{unwrappable_variable_str}` after checking its variant with `{}`", method_name.ident.name, unwrappable.check_name, ), |diag| { @@ -358,7 +460,7 @@ impl<'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'_, 'tcx> { unwrappable.check.span.with_lo(unwrappable.if_expr.span.lo()), "try", format!( - "if let {suggested_pattern} = {borrow_prefix}{unwrappable_variable_name}", + "if let {suggested_pattern} = {borrow_prefix}{unwrappable_variable_str}", suggested_pattern = if call_to_unwrap { unwrappable.kind.success_variant_pattern() } else { diff --git a/tests/ui/checked_unwrap/simple_conditionals.rs b/tests/ui/checked_unwrap/simple_conditionals.rs index b76269307159..f5b11f4813f8 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.rs +++ b/tests/ui/checked_unwrap/simple_conditionals.rs @@ -3,7 +3,8 @@ #![expect( clippy::if_same_then_else, clippy::branches_sharing_code, - clippy::unnecessary_literal_unwrap + clippy::unnecessary_literal_unwrap, + clippy::self_assignment )] macro_rules! m { @@ -287,3 +288,85 @@ fn check_expect() { x.expect("an error message"); } } + +fn issue15321() { + struct Soption { + option: Option, + other: bool, + } + let mut sopt = Soption { + option: Some(true), + other: true, + }; + // Lint: nothing was mutated + let _res = if sopt.option.is_some() { + sopt.option.unwrap() + //~^ unnecessary_unwrap + } else { + sopt.option.unwrap() + //~^ panicking_unwrap + }; + // Lint: an unrelated field was mutated + let _res = if sopt.option.is_some() { + sopt.other = false; + sopt.option.unwrap() + //~^ unnecessary_unwrap + } else { + sopt.other = false; + sopt.option.unwrap() + //~^ panicking_unwrap + }; + // No lint: the whole local was mutated + let _res = if sopt.option.is_some() { + sopt = sopt; + sopt.option.unwrap() + } else { + sopt.option = None; + sopt.option.unwrap() + }; + // No lint: the field we're looking at was mutated + let _res = if sopt.option.is_some() { + sopt = sopt; + sopt.option.unwrap() + } else { + sopt.option = None; + sopt.option.unwrap() + }; + + struct Toption(Option, bool); + let mut topt = Toption(Some(true), true); + // Lint: nothing was mutated + let _res = if topt.0.is_some() { + topt.0.unwrap() + //~^ unnecessary_unwrap + } else { + topt.0.unwrap() + //~^ panicking_unwrap + }; + // Lint: an unrelated field was mutated + let _res = if topt.0.is_some() { + topt.1 = false; + topt.0.unwrap() + //~^ unnecessary_unwrap + } else { + topt.1 = false; + topt.0.unwrap() + //~^ panicking_unwrap + }; + // No lint: the whole local was mutated + let _res = if topt.0.is_some() { + topt = topt; + topt.0.unwrap() + } else { + topt = topt; + topt.0.unwrap() + }; + // No lint: the field we're looking at was mutated + let _res = if topt.0.is_some() { + topt.0 = None; + topt.0.unwrap() + } else { + topt.0 = None; + topt.0.unwrap() + }; +} diff --git a/tests/ui/checked_unwrap/simple_conditionals.stderr b/tests/ui/checked_unwrap/simple_conditionals.stderr index ad234ee1d96a..ac9dbd8ac57a 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.stderr +++ b/tests/ui/checked_unwrap/simple_conditionals.stderr @@ -1,5 +1,5 @@ error: called `unwrap` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:45:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:46:9 | LL | if x.is_some() { | -------------- help: try: `if let Some() = x` @@ -10,7 +10,7 @@ LL | x.unwrap(); = help: to override `-D warnings` add `#[allow(clippy::unnecessary_unwrap)]` error: called `expect` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:48:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:49:9 | LL | if x.is_some() { | -------------- help: try: `if let Some() = x` @@ -19,7 +19,7 @@ LL | x.expect("an error message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:51:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:52:9 | LL | if x.is_some() { | ----------- because of this check @@ -31,7 +31,7 @@ LL | x.unwrap(); = help: to override `-D warnings` add `#[allow(clippy::panicking_unwrap)]` error: this call to `expect()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:54:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:55:9 | LL | if x.is_some() { | ----------- because of this check @@ -40,7 +40,7 @@ LL | x.expect("an error message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:58:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:59:9 | LL | if x.is_none() { | ----------- because of this check @@ -48,7 +48,7 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: called `unwrap` on `x` after checking its variant with `is_none` - --> tests/ui/checked_unwrap/simple_conditionals.rs:61:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:62:9 | LL | if x.is_none() { | -------------- help: try: `if let Some() = x` @@ -57,7 +57,7 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: called `unwrap` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:12:13 + --> tests/ui/checked_unwrap/simple_conditionals.rs:13:13 | LL | if $a.is_some() { | --------------- help: try: `if let Some() = x` @@ -70,7 +70,7 @@ LL | m!(x); = note: this error originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info) error: called `unwrap` on `x` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:70:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:71:9 | LL | if x.is_ok() { | ------------ help: try: `if let Ok() = x` @@ -78,7 +78,7 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: called `expect` on `x` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:73:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:74:9 | LL | if x.is_ok() { | ------------ help: try: `if let Ok() = x` @@ -87,7 +87,7 @@ LL | x.expect("an error message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap_err()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:76:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:77:9 | LL | if x.is_ok() { | --------- because of this check @@ -96,7 +96,7 @@ LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:79:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:80:9 | LL | if x.is_ok() { | --------- because of this check @@ -105,7 +105,7 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: this call to `expect()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:82:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:83:9 | LL | if x.is_ok() { | --------- because of this check @@ -114,7 +114,7 @@ LL | x.expect("an error message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap_err` on `x` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:85:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:86:9 | LL | if x.is_ok() { | ------------ help: try: `if let Err() = x` @@ -123,7 +123,7 @@ LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:89:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:90:9 | LL | if x.is_err() { | ---------- because of this check @@ -131,7 +131,7 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: called `unwrap_err` on `x` after checking its variant with `is_err` - --> tests/ui/checked_unwrap/simple_conditionals.rs:92:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:93:9 | LL | if x.is_err() { | ------------- help: try: `if let Err() = x` @@ -140,7 +140,7 @@ LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ error: called `unwrap` on `x` after checking its variant with `is_err` - --> tests/ui/checked_unwrap/simple_conditionals.rs:95:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:96:9 | LL | if x.is_err() { | ------------- help: try: `if let Ok() = x` @@ -149,7 +149,7 @@ LL | x.unwrap(); | ^^^^^^^^^^ error: this call to `unwrap_err()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:98:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:99:9 | LL | if x.is_err() { | ---------- because of this check @@ -158,7 +158,7 @@ LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ error: called `unwrap` on `option` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:123:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:124:9 | LL | if option.is_some() { | ------------------- help: try: `if let Some() = &option` @@ -166,7 +166,7 @@ LL | option.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:126:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:127:9 | LL | if option.is_some() { | ---------------- because of this check @@ -175,7 +175,7 @@ LL | option.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `result` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:133:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:134:9 | LL | if result.is_ok() { | ----------------- help: try: `if let Ok() = &result` @@ -183,7 +183,7 @@ LL | result.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:136:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:137:9 | LL | if result.is_ok() { | -------------- because of this check @@ -192,7 +192,7 @@ LL | result.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `option` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:142:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:143:9 | LL | if option.is_some() { | ------------------- help: try: `if let Some() = &mut option` @@ -200,7 +200,7 @@ LL | option.as_mut().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:145:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:146:9 | LL | if option.is_some() { | ---------------- because of this check @@ -209,7 +209,7 @@ LL | option.as_mut().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `result` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:151:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:152:9 | LL | if result.is_ok() { | ----------------- help: try: `if let Ok() = &mut result` @@ -217,7 +217,7 @@ LL | result.as_mut().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:154:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:155:9 | LL | if result.is_ok() { | -------------- because of this check @@ -226,7 +226,7 @@ LL | result.as_mut().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `option` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:183:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:184:17 | LL | if option.is_some() { | ------------------- help: try: `if let Some() = &option` @@ -234,7 +234,7 @@ LL | let _ = option.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:186:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:187:17 | LL | if option.is_some() { | ---------------- because of this check @@ -243,7 +243,7 @@ LL | let _ = option.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `result` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:194:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:195:9 | LL | if result.is_ok() { | ----------------- help: try: `if let Ok() = &result` @@ -252,7 +252,7 @@ LL | result.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:198:9 + --> tests/ui/checked_unwrap/simple_conditionals.rs:199:9 | LL | if result.is_ok() { | -------------- because of this check @@ -261,7 +261,7 @@ LL | result.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:224:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:225:17 | LL | if x.is_some() { | -------------- help: try: `if let Some() = x` @@ -269,7 +269,7 @@ LL | _ = x.unwrap(); | ^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:227:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:228:17 | LL | if x.is_some() { | ----------- because of this check @@ -278,7 +278,7 @@ LL | _ = x.unwrap(); | ^^^^^^^^^^ error: called `unwrap` on `r` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:233:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:234:17 | LL | if r.is_ok() { | ------------ help: try: `if let Ok() = &r` @@ -286,7 +286,7 @@ LL | _ = r.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:236:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:237:17 | LL | if r.is_ok() { | --------- because of this check @@ -295,7 +295,7 @@ LL | _ = r.as_ref().unwrap(); | ^^^^^^^^^^^^^^^^^^^ error: called `unwrap` on `x` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:245:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:246:17 | LL | if x.is_some() { | -------------- help: try: `if let Some() = x` @@ -303,7 +303,7 @@ LL | _ = x.unwrap(); | ^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:248:17 + --> tests/ui/checked_unwrap/simple_conditionals.rs:249:17 | LL | if x.is_some() { | ----------- because of this check @@ -312,7 +312,7 @@ LL | _ = x.unwrap(); | ^^^^^^^^^^ error: called `unwrap` on `option` after checking its variant with `is_some` - --> tests/ui/checked_unwrap/simple_conditionals.rs:258:26 + --> tests/ui/checked_unwrap/simple_conditionals.rs:259:26 | LL | if option.is_some() { | ------------------- help: try: `if let Some() = option` @@ -320,7 +320,7 @@ LL | println!("{:?}", option.unwrap()); | ^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:261:26 + --> tests/ui/checked_unwrap/simple_conditionals.rs:262:26 | LL | if option.is_some() { | ---------------- because of this check @@ -329,7 +329,7 @@ LL | println!("{:?}", option.unwrap()); | ^^^^^^^^^^^^^^^ error: called `unwrap` on `result` after checking its variant with `is_ok` - --> tests/ui/checked_unwrap/simple_conditionals.rs:268:26 + --> tests/ui/checked_unwrap/simple_conditionals.rs:269:26 | LL | if result.is_ok() { | ----------------- help: try: `if let Ok() = result` @@ -337,7 +337,7 @@ LL | println!("{:?}", result.unwrap()); | ^^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> tests/ui/checked_unwrap/simple_conditionals.rs:271:26 + --> tests/ui/checked_unwrap/simple_conditionals.rs:272:26 | LL | if result.is_ok() { | -------------- because of this check @@ -345,5 +345,75 @@ LL | if result.is_ok() { LL | println!("{:?}", result.unwrap()); | ^^^^^^^^^^^^^^^ -error: aborting due to 39 previous errors +error: called `unwrap` on `sopt.option` after checking its variant with `is_some` + --> tests/ui/checked_unwrap/simple_conditionals.rs:303:9 + | +LL | let _res = if sopt.option.is_some() { + | ------------------------ help: try: `if let Some() = sopt.option` +LL | sopt.option.unwrap() + | ^^^^^^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> tests/ui/checked_unwrap/simple_conditionals.rs:306:9 + | +LL | let _res = if sopt.option.is_some() { + | --------------------- because of this check +... +LL | sopt.option.unwrap() + | ^^^^^^^^^^^^^^^^^^^^ + +error: called `unwrap` on `sopt.option` after checking its variant with `is_some` + --> tests/ui/checked_unwrap/simple_conditionals.rs:312:9 + | +LL | let _res = if sopt.option.is_some() { + | ------------------------ help: try: `if let Some() = sopt.option` +LL | sopt.other = false; +LL | sopt.option.unwrap() + | ^^^^^^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> tests/ui/checked_unwrap/simple_conditionals.rs:316:9 + | +LL | let _res = if sopt.option.is_some() { + | --------------------- because of this check +... +LL | sopt.option.unwrap() + | ^^^^^^^^^^^^^^^^^^^^ + +error: called `unwrap` on `topt.0` after checking its variant with `is_some` + --> tests/ui/checked_unwrap/simple_conditionals.rs:340:9 + | +LL | let _res = if topt.0.is_some() { + | ------------------- help: try: `if let Some() = topt.0` +LL | topt.0.unwrap() + | ^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> tests/ui/checked_unwrap/simple_conditionals.rs:343:9 + | +LL | let _res = if topt.0.is_some() { + | ---------------- because of this check +... +LL | topt.0.unwrap() + | ^^^^^^^^^^^^^^^ + +error: called `unwrap` on `topt.0` after checking its variant with `is_some` + --> tests/ui/checked_unwrap/simple_conditionals.rs:349:9 + | +LL | let _res = if topt.0.is_some() { + | ------------------- help: try: `if let Some() = topt.0` +LL | topt.1 = false; +LL | topt.0.unwrap() + | ^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> tests/ui/checked_unwrap/simple_conditionals.rs:353:9 + | +LL | let _res = if topt.0.is_some() { + | ---------------- because of this check +... +LL | topt.0.unwrap() + | ^^^^^^^^^^^^^^^ + +error: aborting due to 47 previous errors From 23ee56d742897891747e1edcba9537e1e26b584a Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Sat, 25 Oct 2025 13:08:51 +0200 Subject: [PATCH 3/3] lint nested field accesses as well --- clippy_lints/src/unwrap.rs | 96 ++++++++++--------- .../ui/checked_unwrap/simple_conditionals.rs | 67 +++++++++++++ .../checked_unwrap/simple_conditionals.stderr | 55 ++++++++++- 3 files changed, 174 insertions(+), 44 deletions(-) diff --git a/clippy_lints/src/unwrap.rs b/clippy_lints/src/unwrap.rs index a5ef7f8e5559..37a1d33dfa37 100644 --- a/clippy_lints/src/unwrap.rs +++ b/clippy_lints/src/unwrap.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::iter; use clippy_config::Conf; use clippy_utils::diagnostics::span_lint_hir_and_then; @@ -119,12 +120,15 @@ impl UnwrappableKind { } } -#[derive(Copy, Clone, Debug, Eq)] +#[derive(Clone, Debug, Eq)] enum Local { - /// `x.opt` + /// `x.field1.field2.field3` WithFieldAccess { local_id: HirId, - field_idx: FieldIdx, + /// The indices of the field accessed. + /// + /// Stored last-to-first, i.e. for the example above: `[field3, field2, field1]` + field_indices: Vec, /// The span of the whole expression span: Span, }, @@ -139,15 +143,15 @@ impl PartialEq for Local { ( Self::WithFieldAccess { local_id: self_local_id, - field_idx: self_field_idx, + field_indices: self_field_indices, .. }, Self::WithFieldAccess { local_id: other_local_id, - field_idx: other_field_idx, + field_indices: other_field_indices, .. }, - ) => self_local_id == other_local_id && self_field_idx == other_field_idx, + ) => self_local_id == other_local_id && self_field_indices == other_field_indices, ( Self::Pure { local_id: self_local_id, @@ -162,43 +166,42 @@ impl PartialEq for Local { } impl Local { - fn snippet(self, cx: &LateContext<'_>) -> Cow<'static, str> { - match self { + fn snippet(&self, cx: &LateContext<'_>) -> Cow<'static, str> { + match *self { Self::WithFieldAccess { span, .. } => snippet(cx.sess(), span, "_"), Self::Pure { local_id } => cx.tcx.hir_name(local_id).to_string().into(), } } - fn is_potentially_local_place(self, place: &Place<'_>) -> bool { + fn is_potentially_local_place(&self, place: &Place<'_>) -> bool { match self { Self::WithFieldAccess { - local_id, field_idx, .. + local_id, + field_indices, + .. } => { - if is_potentially_local_place(local_id, place) - // If there were projections other than the field projection, err on the side of caution and say - // that they _might_ have mutated the field + is_potentially_local_place(*local_id, place) + // If there were projections other than field projections, err on the side of caution and say that they + // _might_ be mutating something. // - // The reason we use `<=` and not `==` is that, if there were no projections, then the whole local - // was mutated, which means that our field was mutated as well - && place.projections.len() <= 1 - && place.projections.last().is_none_or(|proj| match proj.kind { - ProjectionKind::Field(f_idx, _) => f_idx == field_idx, - // If this is a projection we don't expect, it _might_ be mutating something - _ => false, + // The reason we use `<=` and not `==` is that a mutation of `struct` or `struct.field1` should count as + // mutation of the child fields such as `struct.field1.field2` + && place.projections.len() <= field_indices.len() + && iter::zip(&place.projections, field_indices.iter().copied().rev()).all(|(proj, field_idx)| { + match proj.kind { + ProjectionKind::Field(f_idx, _) => f_idx == field_idx, + // If this is a projection we don't expect, it _might_ be mutating something + _ => false, + } }) - { - true - } else { - false - } }, - Self::Pure { local_id } => is_potentially_local_place(local_id, place), + Self::Pure { local_id } => is_potentially_local_place(*local_id, place), } } } /// Contains information about whether a variable can be unwrapped. -#[derive(Copy, Clone, Debug)] +#[derive(Clone, Debug)] struct UnwrapInfo<'tcx> { /// The variable that is checked local: Local, @@ -287,20 +290,27 @@ fn collect_unwrap_info<'tcx>( out } -/// Extracts either a local used by itself ([`Local::Pure`]), or a field access to a local -/// ([`Local::WithFieldAccess`]) -fn extract_local(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { - if let Some(local_id) = expr.res_local_id() { - Some(Local::Pure { local_id }) - } else if let ExprKind::Field(recv, _) = expr.kind - && let Some(local_id) = recv.res_local_id() +/// Extracts either a local used by itself ([`Local::Pure`]), or (one or more levels of) field +/// access to a local ([`Local::WithFieldAccess`]) +fn extract_local(cx: &LateContext<'_>, mut expr: &Expr<'_>) -> Option { + let span = expr.span; + let mut field_indices = vec![]; + while let ExprKind::Field(recv, _) = expr.kind && let Some(field_idx) = cx.typeck_results().opt_field_index(expr.hir_id) { - Some(Local::WithFieldAccess { - local_id, - field_idx, - span: expr.span, - }) + field_indices.push(field_idx); + expr = recv; + } + if let Some(local_id) = expr.res_local_id() { + if field_indices.is_empty() { + Some(Local::Pure { local_id }) + } else { + Some(Local::WithFieldAccess { + local_id, + field_indices, + span, + }) + } } else { None } @@ -313,9 +323,9 @@ fn extract_local(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { /// `is_some` + `unwrap` is equivalent to `if let Some(..) = ..`, which it would not be if /// the option is changed to None between `is_some` and `unwrap`, ditto for `Result`. /// (And also `.as_mut()` is a somewhat common method that is still worth linting on.) -struct MutationVisitor<'tcx> { +struct MutationVisitor<'tcx, 'lcl> { is_mutated: bool, - local: Local, + local: &'lcl Local, tcx: TyCtxt<'tcx>, } @@ -336,7 +346,7 @@ fn is_as_mut_use(tcx: TyCtxt<'_>, expr_id: HirId) -> bool { } } -impl<'tcx> Delegate<'tcx> for MutationVisitor<'tcx> { +impl<'tcx> Delegate<'tcx> for MutationVisitor<'tcx, '_> { fn borrow(&mut self, cat: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind) { if let ty::BorrowKind::Mutable = bk && self.local.is_potentially_local_place(&cat.place) @@ -371,7 +381,7 @@ impl<'tcx> UnwrappableVariablesVisitor<'_, 'tcx> { for unwrap_info in collect_unwrap_info(self.cx, if_expr, cond, branch, else_branch, true) { let mut delegate = MutationVisitor { is_mutated: false, - local: unwrap_info.local, + local: &unwrap_info.local, tcx: self.cx.tcx, }; diff --git a/tests/ui/checked_unwrap/simple_conditionals.rs b/tests/ui/checked_unwrap/simple_conditionals.rs index f5b11f4813f8..190f201c6227 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.rs +++ b/tests/ui/checked_unwrap/simple_conditionals.rs @@ -369,4 +369,71 @@ fn issue15321() { topt.0 = None; topt.0.unwrap() }; + + // Nested field accesses get linted as well + struct Soption2 { + other: bool, + option: Soption, + } + let mut sopt2 = Soption2 { + other: true, + option: Soption { + option: Some(true), + other: true, + }, + }; + // Lint: no fields were mutated + let _res = if sopt2.option.option.is_some() { + sopt2.option.option.unwrap() + //~^ unnecessary_unwrap + } else { + sopt2.option.option.unwrap() + //~^ panicking_unwrap + }; + // Lint: an unrelated outer field was mutated -- don't get confused by `Soption2.other` having the + // same `FieldIdx` of 1 as `Soption.option` + let _res = if sopt2.option.option.is_some() { + sopt2.other = false; + sopt2.option.option.unwrap() + //~^ unnecessary_unwrap + } else { + sopt2.other = false; + sopt2.option.option.unwrap() + //~^ panicking_unwrap + }; + // Lint: an unrelated inner field was mutated + let _res = if sopt2.option.option.is_some() { + sopt2.option.other = false; + sopt2.option.option.unwrap() + //~^ unnecessary_unwrap + } else { + sopt2.option.other = false; + sopt2.option.option.unwrap() + //~^ panicking_unwrap + }; + // Don't lint: the whole local was mutated + let _res = if sopt2.option.option.is_some() { + sopt2 = sopt2; + sopt2.option.option.unwrap() + } else { + sopt2 = sopt2; + sopt2.option.option.unwrap() + }; + // Don't lint: a parent field of the field we're looking at was mutated, and with that the + // field we're looking at + let _res = if sopt2.option.option.is_some() { + sopt2.option = sopt; + sopt2.option.option.unwrap() + } else { + sopt2.option = sopt; + sopt2.option.option.unwrap() + }; + // Don't lint: the field we're looking at was mutated directly + let _res = if sopt2.option.option.is_some() { + sopt2.option.option = None; + sopt2.option.option.unwrap() + } else { + sopt2.option.option = None; + sopt2.option.option.unwrap() + }; } diff --git a/tests/ui/checked_unwrap/simple_conditionals.stderr b/tests/ui/checked_unwrap/simple_conditionals.stderr index ac9dbd8ac57a..08f9bdc979b1 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.stderr +++ b/tests/ui/checked_unwrap/simple_conditionals.stderr @@ -415,5 +415,58 @@ LL | let _res = if topt.0.is_some() { LL | topt.0.unwrap() | ^^^^^^^^^^^^^^^ -error: aborting due to 47 previous errors +error: called `unwrap` on `sopt2.option.option` after checking its variant with `is_some` + --> tests/ui/checked_unwrap/simple_conditionals.rs:387:9 + | +LL | let _res = if sopt2.option.option.is_some() { + | -------------------------------- help: try: `if let Some() = sopt2.option.option` +LL | sopt2.option.option.unwrap() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> tests/ui/checked_unwrap/simple_conditionals.rs:390:9 + | +LL | let _res = if sopt2.option.option.is_some() { + | ----------------------------- because of this check +... +LL | sopt2.option.option.unwrap() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: called `unwrap` on `sopt2.option.option` after checking its variant with `is_some` + --> tests/ui/checked_unwrap/simple_conditionals.rs:397:9 + | +LL | let _res = if sopt2.option.option.is_some() { + | -------------------------------- help: try: `if let Some() = sopt2.option.option` +LL | sopt2.other = false; +LL | sopt2.option.option.unwrap() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> tests/ui/checked_unwrap/simple_conditionals.rs:401:9 + | +LL | let _res = if sopt2.option.option.is_some() { + | ----------------------------- because of this check +... +LL | sopt2.option.option.unwrap() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: called `unwrap` on `sopt2.option.option` after checking its variant with `is_some` + --> tests/ui/checked_unwrap/simple_conditionals.rs:407:9 + | +LL | let _res = if sopt2.option.option.is_some() { + | -------------------------------- help: try: `if let Some() = sopt2.option.option` +LL | sopt2.option.other = false; +LL | sopt2.option.option.unwrap() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> tests/ui/checked_unwrap/simple_conditionals.rs:411:9 + | +LL | let _res = if sopt2.option.option.is_some() { + | ----------------------------- because of this check +... +LL | sopt2.option.option.unwrap() + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 53 previous errors