From ed183ee9acbc8b0c13a7a1a4859db9b4e7ca7d37 Mon Sep 17 00:00:00 2001 From: kraktus Date: Sat, 29 Oct 2022 21:45:40 +0200 Subject: [PATCH] Fix [`unnecessary_lazy_eval`] when type has significant drop --- clippy_utils/src/eager_or_lazy.rs | 26 ++++++--- tests/ui/unnecessary_lazy_eval.fixed | 23 +++++--- tests/ui/unnecessary_lazy_eval.rs | 23 +++++--- tests/ui/unnecessary_lazy_eval.stderr | 84 +++++++++++++-------------- 4 files changed, 92 insertions(+), 64 deletions(-) diff --git a/clippy_utils/src/eager_or_lazy.rs b/clippy_utils/src/eager_or_lazy.rs index 95b3e651e2b..107405609fd 100644 --- a/clippy_utils/src/eager_or_lazy.rs +++ b/clippy_utils/src/eager_or_lazy.rs @@ -91,6 +91,16 @@ fn fn_eagerness(cx: &LateContext<'_>, fn_id: DefId, name: Symbol, have_one_arg: } } +fn res_has_significant_drop(res: Res, cx: &LateContext<'_>, e: &Expr<'_>) -> bool { + if let Res::Def(DefKind::Ctor(..) | DefKind::Variant, _) | Res::SelfCtor(_) = res { + cx.typeck_results() + .expr_ty(e) + .has_significant_drop(cx.tcx, cx.param_env) + } else { + false + } +} + #[expect(clippy::too_many_lines)] fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessSuggestion { struct V<'cx, 'tcx> { @@ -113,13 +123,8 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS }, args, ) => match self.cx.qpath_res(path, hir_id) { - Res::Def(DefKind::Ctor(..) | DefKind::Variant, _) | Res::SelfCtor(_) => { - if self - .cx - .typeck_results() - .expr_ty(e) - .has_significant_drop(self.cx.tcx, self.cx.param_env) - { + res @ (Res::Def(DefKind::Ctor(..) | DefKind::Variant, _) | Res::SelfCtor(_)) => { + if res_has_significant_drop(res, self.cx, e) { self.eagerness = ForceNoChange; return; } @@ -147,6 +152,12 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS self.eagerness |= NoChange; return; }, + ExprKind::Path(ref path) => { + if res_has_significant_drop(self.cx.qpath_res(path, e.hir_id), self.cx, e) { + self.eagerness = ForceNoChange; + return; + } + }, ExprKind::MethodCall(name, ..) => { self.eagerness |= self .cx @@ -206,7 +217,6 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS | ExprKind::Match(..) | ExprKind::Closure { .. } | ExprKind::Field(..) - | ExprKind::Path(_) | ExprKind::AddrOf(..) | ExprKind::Struct(..) | ExprKind::Repeat(..) diff --git a/tests/ui/unnecessary_lazy_eval.fixed b/tests/ui/unnecessary_lazy_eval.fixed index ce4a82e0217..22e9bd8bdc5 100644 --- a/tests/ui/unnecessary_lazy_eval.fixed +++ b/tests/ui/unnecessary_lazy_eval.fixed @@ -33,6 +33,14 @@ impl Drop for Issue9427 { } } +struct Issue9427FollowUp; + +impl Drop for Issue9427FollowUp { + fn drop(&mut self) { + panic!("side effect drop"); + } +} + fn main() { let astronomers_pi = 10; let ext_arr: [usize; 1] = [2]; @@ -87,6 +95,7 @@ fn main() { // Should not lint - bool let _ = (0 == 1).then(|| Issue9427(0)); // Issue9427 has a significant drop + let _ = false.then(|| Issue9427FollowUp); // Issue9427FollowUp has a significant drop // should not lint, bind_instead_of_map takes priority let _ = Some(10).and_then(|idx| Some(ext_arr[idx])); @@ -133,13 +142,13 @@ fn main() { let _: Result = res.or(Ok(astronomers_pi)); let _: Result = res.or(Ok(ext_str.some_field)); let _: Result = res. - // some lines - // some lines - // some lines - // some lines - // some lines - // some lines - or(Ok(ext_str.some_field)); + // some lines + // some lines + // some lines + // some lines + // some lines + // some lines + or(Ok(ext_str.some_field)); // neither bind_instead_of_map nor unnecessary_lazy_eval applies here let _: Result = res.and_then(|x| Err(x)); diff --git a/tests/ui/unnecessary_lazy_eval.rs b/tests/ui/unnecessary_lazy_eval.rs index 59cdf662854..8726d84a23f 100644 --- a/tests/ui/unnecessary_lazy_eval.rs +++ b/tests/ui/unnecessary_lazy_eval.rs @@ -33,6 +33,14 @@ impl Drop for Issue9427 { } } +struct Issue9427FollowUp; + +impl Drop for Issue9427FollowUp { + fn drop(&mut self) { + panic!("side effect drop"); + } +} + fn main() { let astronomers_pi = 10; let ext_arr: [usize; 1] = [2]; @@ -87,6 +95,7 @@ fn main() { // Should not lint - bool let _ = (0 == 1).then(|| Issue9427(0)); // Issue9427 has a significant drop + let _ = false.then(|| Issue9427FollowUp); // Issue9427FollowUp has a significant drop // should not lint, bind_instead_of_map takes priority let _ = Some(10).and_then(|idx| Some(ext_arr[idx])); @@ -133,13 +142,13 @@ fn main() { let _: Result = res.or_else(|_| Ok(astronomers_pi)); let _: Result = res.or_else(|_| Ok(ext_str.some_field)); let _: Result = res. - // some lines - // some lines - // some lines - // some lines - // some lines - // some lines - or_else(|_| Ok(ext_str.some_field)); + // some lines + // some lines + // some lines + // some lines + // some lines + // some lines + or_else(|_| Ok(ext_str.some_field)); // neither bind_instead_of_map nor unnecessary_lazy_eval applies here let _: Result = res.and_then(|x| Err(x)); diff --git a/tests/ui/unnecessary_lazy_eval.stderr b/tests/ui/unnecessary_lazy_eval.stderr index 8a9ece4aa7e..0339755442c 100644 --- a/tests/ui/unnecessary_lazy_eval.stderr +++ b/tests/ui/unnecessary_lazy_eval.stderr @@ -1,5 +1,5 @@ error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:48:13 + --> $DIR/unnecessary_lazy_eval.rs:56:13 | LL | let _ = opt.unwrap_or_else(|| 2); | ^^^^-------------------- @@ -9,7 +9,7 @@ LL | let _ = opt.unwrap_or_else(|| 2); = note: `-D clippy::unnecessary-lazy-evaluations` implied by `-D warnings` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:49:13 + --> $DIR/unnecessary_lazy_eval.rs:57:13 | LL | let _ = opt.unwrap_or_else(|| astronomers_pi); | ^^^^--------------------------------- @@ -17,7 +17,7 @@ LL | let _ = opt.unwrap_or_else(|| astronomers_pi); | help: use `unwrap_or(..)` instead: `unwrap_or(astronomers_pi)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:50:13 + --> $DIR/unnecessary_lazy_eval.rs:58:13 | LL | let _ = opt.unwrap_or_else(|| ext_str.some_field); | ^^^^------------------------------------- @@ -25,7 +25,7 @@ LL | let _ = opt.unwrap_or_else(|| ext_str.some_field); | help: use `unwrap_or(..)` instead: `unwrap_or(ext_str.some_field)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:52:13 + --> $DIR/unnecessary_lazy_eval.rs:60:13 | LL | let _ = opt.and_then(|_| ext_opt); | ^^^^--------------------- @@ -33,7 +33,7 @@ LL | let _ = opt.and_then(|_| ext_opt); | help: use `and(..)` instead: `and(ext_opt)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:53:13 + --> $DIR/unnecessary_lazy_eval.rs:61:13 | LL | let _ = opt.or_else(|| ext_opt); | ^^^^------------------- @@ -41,7 +41,7 @@ LL | let _ = opt.or_else(|| ext_opt); | help: use `or(..)` instead: `or(ext_opt)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:54:13 + --> $DIR/unnecessary_lazy_eval.rs:62:13 | LL | let _ = opt.or_else(|| None); | ^^^^---------------- @@ -49,7 +49,7 @@ LL | let _ = opt.or_else(|| None); | help: use `or(..)` instead: `or(None)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:55:13 + --> $DIR/unnecessary_lazy_eval.rs:63:13 | LL | let _ = opt.get_or_insert_with(|| 2); | ^^^^------------------------ @@ -57,7 +57,7 @@ LL | let _ = opt.get_or_insert_with(|| 2); | help: use `get_or_insert(..)` instead: `get_or_insert(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:56:13 + --> $DIR/unnecessary_lazy_eval.rs:64:13 | LL | let _ = opt.ok_or_else(|| 2); | ^^^^---------------- @@ -65,7 +65,7 @@ LL | let _ = opt.ok_or_else(|| 2); | help: use `ok_or(..)` instead: `ok_or(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:57:13 + --> $DIR/unnecessary_lazy_eval.rs:65:13 | LL | let _ = nested_tuple_opt.unwrap_or_else(|| Some((1, 2))); | ^^^^^^^^^^^^^^^^^------------------------------- @@ -73,7 +73,7 @@ LL | let _ = nested_tuple_opt.unwrap_or_else(|| Some((1, 2))); | help: use `unwrap_or(..)` instead: `unwrap_or(Some((1, 2)))` error: unnecessary closure used with `bool::then` - --> $DIR/unnecessary_lazy_eval.rs:58:13 + --> $DIR/unnecessary_lazy_eval.rs:66:13 | LL | let _ = cond.then(|| astronomers_pi); | ^^^^^----------------------- @@ -81,7 +81,7 @@ LL | let _ = cond.then(|| astronomers_pi); | help: use `then_some(..)` instead: `then_some(astronomers_pi)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:61:13 + --> $DIR/unnecessary_lazy_eval.rs:69:13 | LL | let _ = Some(10).unwrap_or_else(|| 2); | ^^^^^^^^^-------------------- @@ -89,7 +89,7 @@ LL | let _ = Some(10).unwrap_or_else(|| 2); | help: use `unwrap_or(..)` instead: `unwrap_or(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:62:13 + --> $DIR/unnecessary_lazy_eval.rs:70:13 | LL | let _ = Some(10).and_then(|_| ext_opt); | ^^^^^^^^^--------------------- @@ -97,7 +97,7 @@ LL | let _ = Some(10).and_then(|_| ext_opt); | help: use `and(..)` instead: `and(ext_opt)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:63:28 + --> $DIR/unnecessary_lazy_eval.rs:71:28 | LL | let _: Option = None.or_else(|| ext_opt); | ^^^^^------------------- @@ -105,7 +105,7 @@ LL | let _: Option = None.or_else(|| ext_opt); | help: use `or(..)` instead: `or(ext_opt)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:64:13 + --> $DIR/unnecessary_lazy_eval.rs:72:13 | LL | let _ = None.get_or_insert_with(|| 2); | ^^^^^------------------------ @@ -113,7 +113,7 @@ LL | let _ = None.get_or_insert_with(|| 2); | help: use `get_or_insert(..)` instead: `get_or_insert(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:65:35 + --> $DIR/unnecessary_lazy_eval.rs:73:35 | LL | let _: Result = None.ok_or_else(|| 2); | ^^^^^---------------- @@ -121,7 +121,7 @@ LL | let _: Result = None.ok_or_else(|| 2); | help: use `ok_or(..)` instead: `ok_or(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:66:28 + --> $DIR/unnecessary_lazy_eval.rs:74:28 | LL | let _: Option = None.or_else(|| None); | ^^^^^---------------- @@ -129,7 +129,7 @@ LL | let _: Option = None.or_else(|| None); | help: use `or(..)` instead: `or(None)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:69:13 + --> $DIR/unnecessary_lazy_eval.rs:77:13 | LL | let _ = deep.0.unwrap_or_else(|| 2); | ^^^^^^^-------------------- @@ -137,7 +137,7 @@ LL | let _ = deep.0.unwrap_or_else(|| 2); | help: use `unwrap_or(..)` instead: `unwrap_or(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:70:13 + --> $DIR/unnecessary_lazy_eval.rs:78:13 | LL | let _ = deep.0.and_then(|_| ext_opt); | ^^^^^^^--------------------- @@ -145,7 +145,7 @@ LL | let _ = deep.0.and_then(|_| ext_opt); | help: use `and(..)` instead: `and(ext_opt)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:71:13 + --> $DIR/unnecessary_lazy_eval.rs:79:13 | LL | let _ = deep.0.or_else(|| None); | ^^^^^^^---------------- @@ -153,7 +153,7 @@ LL | let _ = deep.0.or_else(|| None); | help: use `or(..)` instead: `or(None)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:72:13 + --> $DIR/unnecessary_lazy_eval.rs:80:13 | LL | let _ = deep.0.get_or_insert_with(|| 2); | ^^^^^^^------------------------ @@ -161,7 +161,7 @@ LL | let _ = deep.0.get_or_insert_with(|| 2); | help: use `get_or_insert(..)` instead: `get_or_insert(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:73:13 + --> $DIR/unnecessary_lazy_eval.rs:81:13 | LL | let _ = deep.0.ok_or_else(|| 2); | ^^^^^^^---------------- @@ -169,7 +169,7 @@ LL | let _ = deep.0.ok_or_else(|| 2); | help: use `ok_or(..)` instead: `ok_or(2)` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:96:28 + --> $DIR/unnecessary_lazy_eval.rs:105:28 | LL | let _: Option = None.or_else(|| Some(3)); | ^^^^^------------------- @@ -177,7 +177,7 @@ LL | let _: Option = None.or_else(|| Some(3)); | help: use `or(..)` instead: `or(Some(3))` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:97:13 + --> $DIR/unnecessary_lazy_eval.rs:106:13 | LL | let _ = deep.0.or_else(|| Some(3)); | ^^^^^^^------------------- @@ -185,7 +185,7 @@ LL | let _ = deep.0.or_else(|| Some(3)); | help: use `or(..)` instead: `or(Some(3))` error: unnecessary closure used to substitute value for `Option::None` - --> $DIR/unnecessary_lazy_eval.rs:98:13 + --> $DIR/unnecessary_lazy_eval.rs:107:13 | LL | let _ = opt.or_else(|| Some(3)); | ^^^^------------------- @@ -193,7 +193,7 @@ LL | let _ = opt.or_else(|| Some(3)); | help: use `or(..)` instead: `or(Some(3))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:104:13 + --> $DIR/unnecessary_lazy_eval.rs:113:13 | LL | let _ = res2.unwrap_or_else(|_| 2); | ^^^^^--------------------- @@ -201,7 +201,7 @@ LL | let _ = res2.unwrap_or_else(|_| 2); | help: use `unwrap_or(..)` instead: `unwrap_or(2)` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:105:13 + --> $DIR/unnecessary_lazy_eval.rs:114:13 | LL | let _ = res2.unwrap_or_else(|_| astronomers_pi); | ^^^^^---------------------------------- @@ -209,7 +209,7 @@ LL | let _ = res2.unwrap_or_else(|_| astronomers_pi); | help: use `unwrap_or(..)` instead: `unwrap_or(astronomers_pi)` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:106:13 + --> $DIR/unnecessary_lazy_eval.rs:115:13 | LL | let _ = res2.unwrap_or_else(|_| ext_str.some_field); | ^^^^^-------------------------------------- @@ -217,7 +217,7 @@ LL | let _ = res2.unwrap_or_else(|_| ext_str.some_field); | help: use `unwrap_or(..)` instead: `unwrap_or(ext_str.some_field)` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:128:35 + --> $DIR/unnecessary_lazy_eval.rs:137:35 | LL | let _: Result = res.and_then(|_| Err(2)); | ^^^^-------------------- @@ -225,7 +225,7 @@ LL | let _: Result = res.and_then(|_| Err(2)); | help: use `and(..)` instead: `and(Err(2))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:129:35 + --> $DIR/unnecessary_lazy_eval.rs:138:35 | LL | let _: Result = res.and_then(|_| Err(astronomers_pi)); | ^^^^--------------------------------- @@ -233,7 +233,7 @@ LL | let _: Result = res.and_then(|_| Err(astronomers_pi)); | help: use `and(..)` instead: `and(Err(astronomers_pi))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:130:35 + --> $DIR/unnecessary_lazy_eval.rs:139:35 | LL | let _: Result = res.and_then(|_| Err(ext_str.some_field)); | ^^^^------------------------------------- @@ -241,7 +241,7 @@ LL | let _: Result = res.and_then(|_| Err(ext_str.some_field)) | help: use `and(..)` instead: `and(Err(ext_str.some_field))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:132:35 + --> $DIR/unnecessary_lazy_eval.rs:141:35 | LL | let _: Result = res.or_else(|_| Ok(2)); | ^^^^------------------ @@ -249,7 +249,7 @@ LL | let _: Result = res.or_else(|_| Ok(2)); | help: use `or(..)` instead: `or(Ok(2))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:133:35 + --> $DIR/unnecessary_lazy_eval.rs:142:35 | LL | let _: Result = res.or_else(|_| Ok(astronomers_pi)); | ^^^^------------------------------- @@ -257,7 +257,7 @@ LL | let _: Result = res.or_else(|_| Ok(astronomers_pi)); | help: use `or(..)` instead: `or(Ok(astronomers_pi))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:134:35 + --> $DIR/unnecessary_lazy_eval.rs:143:35 | LL | let _: Result = res.or_else(|_| Ok(ext_str.some_field)); | ^^^^----------------------------------- @@ -265,19 +265,19 @@ LL | let _: Result = res.or_else(|_| Ok(ext_str.some_field)); | help: use `or(..)` instead: `or(Ok(ext_str.some_field))` error: unnecessary closure used to substitute value for `Result::Err` - --> $DIR/unnecessary_lazy_eval.rs:135:35 + --> $DIR/unnecessary_lazy_eval.rs:144:35 | LL | let _: Result = res. | ___________________________________^ -LL | | // some lines -LL | | // some lines -LL | | // some lines +LL | | // some lines +LL | | // some lines +LL | | // some lines ... | -LL | | // some lines -LL | | or_else(|_| Ok(ext_str.some_field)); - | |_________----------------------------------^ - | | - | help: use `or(..)` instead: `or(Ok(ext_str.some_field))` +LL | | // some lines +LL | | or_else(|_| Ok(ext_str.some_field)); + | |_____----------------------------------^ + | | + | help: use `or(..)` instead: `or(Ok(ext_str.some_field))` error: aborting due to 34 previous errors