From f827be92fcdb225738a73051f75cb5e1243fe615 Mon Sep 17 00:00:00 2001 From: est31 Date: Sun, 13 Feb 2022 03:11:06 +0100 Subject: [PATCH 01/11] Add lint to tell about let else pattern --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/manual_let_else.rs | 160 ++++++++++++++++++++++++++ clippy_lints/src/utils/conf.rs | 2 +- clippy_utils/src/msrvs.rs | 1 + src/docs/manual_let_else.txt | 20 ++++ tests/ui/manual_let_else.rs | 167 ++++++++++++++++++++++++++++ tests/ui/manual_let_else.stderr | 104 +++++++++++++++++ 9 files changed, 457 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/manual_let_else.rs create mode 100644 src/docs/manual_let_else.txt create mode 100644 tests/ui/manual_let_else.rs create mode 100644 tests/ui/manual_let_else.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index cb5a4d4a5778..b5a1d194794c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3997,6 +3997,7 @@ Released 2018-09-13 [`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map [`manual_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten [`manual_instant_elapsed`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_instant_elapsed +[`manual_let_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_let_else [`manual_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_map [`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index c6ae0bddc5a9..2bb8dfee1525 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -251,6 +251,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::manual_bits::MANUAL_BITS_INFO, crate::manual_clamp::MANUAL_CLAMP_INFO, crate::manual_instant_elapsed::MANUAL_INSTANT_ELAPSED_INFO, + crate::manual_let_else::MANUAL_LET_ELSE_INFO, crate::manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE_INFO, crate::manual_rem_euclid::MANUAL_REM_EUCLID_INFO, crate::manual_retain::MANUAL_RETAIN_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7837e04bca1f..b261beab7936 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -170,6 +170,7 @@ mod manual_async_fn; mod manual_bits; mod manual_clamp; mod manual_instant_elapsed; +mod manual_let_else; mod manual_non_exhaustive; mod manual_rem_euclid; mod manual_retain; @@ -603,6 +604,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: )) }); store.register_late_pass(move |_| Box::new(matches::Matches::new(msrv))); + store.register_late_pass(move |_| Box::new(manual_let_else::ManualLetElse::new(msrv))); store.register_early_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustiveStruct::new(msrv))); store.register_late_pass(move |_| Box::new(manual_non_exhaustive::ManualNonExhaustiveEnum::new(msrv))); store.register_late_pass(move |_| Box::new(manual_strip::ManualStrip::new(msrv))); diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs new file mode 100644 index 000000000000..8983cd7ae1cf --- /dev/null +++ b/clippy_lints/src/manual_let_else.rs @@ -0,0 +1,160 @@ +use clippy_utils::diagnostics::span_lint; +use clippy_utils::visitors::{for_each_expr, Descend}; +use clippy_utils::{higher, meets_msrv, msrvs, peel_blocks}; +use if_chain::if_chain; +use rustc_hir::{Expr, ExprKind, Pat, QPath, Stmt, StmtKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_semver::RustcVersion; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use std::ops::ControlFlow; + +declare_clippy_lint! { + /// ### What it does + /// + /// Warn of cases where `let...else` could be used + /// + /// ### Why is this bad? + /// + /// `let...else` provides a standard construct for this pattern + /// that people can easily recognize. It's also more compact. + /// + /// ### Example + /// + /// ```rust + /// # let w = Some(0); + /// let v = if let Some(v) = w { v } else { return }; + /// ``` + /// + /// Could be written: + /// + /// ```rust + /// # #![feature(let_else)] + /// # fn main () { + /// # let w = Some(0); + /// let Some(v) = w else { return }; + /// # } + /// ``` + #[clippy::version = "1.67.0"] + pub MANUAL_LET_ELSE, + pedantic, + "manual implementation of a let...else statement" +} + +pub struct ManualLetElse { + msrv: Option, +} + +impl ManualLetElse { + #[must_use] + pub fn new(msrv: Option) -> Self { + Self { msrv } + } +} + +impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]); + +impl<'tcx> LateLintPass<'tcx> for ManualLetElse { + fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &'tcx Stmt<'tcx>) { + if !meets_msrv(self.msrv, msrvs::LET_ELSE) { + return; + } + + if in_external_macro(cx.sess(), stmt.span) { + return; + } + + if_chain! { + if let StmtKind::Local(local) = stmt.kind; + if let Some(init) = local.init; + if let Some(higher::IfLet { let_pat, let_expr: _, if_then, if_else }) = higher::IfLet::hir(cx, init); + if if_then_simple_identity(let_pat, if_then); + if let Some(if_else) = if_else; + if expr_diverges(cx, if_else); + then { + span_lint( + cx, + MANUAL_LET_ELSE, + stmt.span, + "this could be rewritten as `let else`", + ); + } + } + } + + extract_msrv_attr!(LateContext); +} + +fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool { + fn is_never(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool { + if let Some(ty) = cx.typeck_results().expr_ty_opt(expr) { + return ty.is_never(); + } + false + } + // We can't just call is_never on expr and be done, because the type system + // sometimes coerces the ! type to something different before we can get + // our hands on it. So instead, we do a manual search. We do fall back to + // is_never in some places when there is no better alternative. + for_each_expr(expr, |ex| { + match ex.kind { + ExprKind::Continue(_) | ExprKind::Break(_, _) | ExprKind::Ret(_) => ControlFlow::Break(()), + ExprKind::Call(call, _) => { + if is_never(cx, ex) || is_never(cx, call) { + return ControlFlow::Break(()); + } + ControlFlow::Continue(Descend::Yes) + }, + ExprKind::MethodCall(..) => { + if is_never(cx, ex) { + return ControlFlow::Break(()); + } + ControlFlow::Continue(Descend::Yes) + }, + ExprKind::If(if_expr, if_then, if_else) => { + let else_diverges = if_else.map_or(false, |ex| expr_diverges(cx, ex)); + let diverges = expr_diverges(cx, if_expr) || (else_diverges && expr_diverges(cx, if_then)); + if diverges { + return ControlFlow::Break(()); + } + ControlFlow::Continue(Descend::No) + }, + ExprKind::Match(match_expr, match_arms, _) => { + let diverges = + expr_diverges(cx, match_expr) || match_arms.iter().all(|arm| expr_diverges(cx, arm.body)); + if diverges { + return ControlFlow::Break(()); + } + ControlFlow::Continue(Descend::No) + }, + + // Don't continue into loops or labeled blocks, as they are breakable, + // and we'd have to start checking labels. + ExprKind::Block(_, Some(_)) | ExprKind::Loop(..) => ControlFlow::Continue(Descend::No), + + // Default: descend + _ => ControlFlow::Continue(Descend::Yes), + } + }) + .is_some() +} + +/// Checks if the passed `if_then` is a simple identity +fn if_then_simple_identity(let_pat: &'_ Pat<'_>, if_then: &'_ Expr<'_>) -> bool { + // TODO support patterns with multiple bindings and tuples, like: + // let (foo, bar) = if let (Some(foo), bar) = g() { (foo, bar) } else { ... } + if_chain! { + if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(if_then).kind; + if let [path_seg] = path.segments; + then { + let mut pat_bindings = Vec::new(); + let_pat.each_binding(|_ann, _hir_id, _sp, ident| { + pat_bindings.push(ident); + }); + if let [binding] = &pat_bindings[..] { + return path_seg.ident == *binding; + } + } + } + false +} diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index a10de31556c8..1aa86efd38f8 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -213,7 +213,7 @@ define_Conf! { /// /// Suppress lints whenever the suggested change would cause breakage for other crates. (avoid_breaking_exported_api: bool = true), - /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP. + /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE. /// /// The minimum rust version that the project supports (msrv: Option = None), diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 8b843732a236..9780794fa99c 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -12,6 +12,7 @@ macro_rules! msrv_aliases { // names may refer to stabilized feature flags or library items msrv_aliases! { + 1,65,0 { LET_ELSE } 1,62,0 { BOOL_THEN_SOME } 1,58,0 { FORMAT_ARGS_CAPTURE } 1,53,0 { OR_PATTERNS, MANUAL_BITS, BTREE_MAP_RETAIN, BTREE_SET_RETAIN, ARRAY_INTO_ITERATOR } diff --git a/src/docs/manual_let_else.txt b/src/docs/manual_let_else.txt new file mode 100644 index 000000000000..14166709f7fd --- /dev/null +++ b/src/docs/manual_let_else.txt @@ -0,0 +1,20 @@ +### What it does + +Warn of cases where `let...else` could be used + +### Why is this bad? + +`let...else` provides a standard construct for this pattern +that people can easily recognize. It's also more compact. + +### Example + +``` +let v = if let Some(v) = w { v } else { return }; +``` + +Could be written: + +``` +let Some(v) = w else { return }; +``` \ No newline at end of file diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs new file mode 100644 index 000000000000..f2827e225002 --- /dev/null +++ b/tests/ui/manual_let_else.rs @@ -0,0 +1,167 @@ +#![allow(unused_braces, unused_variables, dead_code)] +#![allow( + clippy::collapsible_else_if, + clippy::unused_unit, + clippy::never_loop, + clippy::let_unit_value +)] +#![warn(clippy::manual_let_else)] + +fn g() -> Option<()> { + None +} + +fn main() {} + +fn fire() { + let v = if let Some(v_some) = g() { v_some } else { return }; + let v = if let Some(v_some) = g() { + v_some + } else { + return; + }; + + let v = if let Some(v) = g() { + // Blocks around the identity should have no impact + { + { v } + } + } else { + // Some computation should still make it fire + g(); + return; + }; + + // continue and break diverge + loop { + let v = if let Some(v_some) = g() { v_some } else { continue }; + let v = if let Some(v_some) = g() { v_some } else { break }; + } + + // panic also diverges + let v = if let Some(v_some) = g() { v_some } else { panic!() }; + + // abort also diverges + let v = if let Some(v_some) = g() { + v_some + } else { + std::process::abort() + }; + + // If whose two branches diverge also diverges + let v = if let Some(v_some) = g() { + v_some + } else { + if true { return } else { panic!() } + }; + + // Top level else if + let v = if let Some(v_some) = g() { + v_some + } else if true { + return; + } else { + panic!("diverge"); + }; + + // All match arms diverge + let v = if let Some(v_some) = g() { + v_some + } else { + match (g(), g()) { + (Some(_), None) => return, + (None, Some(_)) => { + if true { + return; + } else { + panic!(); + } + }, + _ => return, + } + }; + + // Tuples supported for the declared variables + let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) { + v_some + } else { + return; + }; +} + +fn not_fire() { + let v = if let Some(v_some) = g() { + // Nothing returned. Should not fire. + } else { + return; + }; + + let w = 0; + let v = if let Some(v_some) = g() { + // Different variable than v_some. Should not fire. + w + } else { + return; + }; + + let v = if let Some(v_some) = g() { + // Computation in then clause. Should not fire. + g(); + v_some + } else { + return; + }; + + let v = if let Some(v_some) = g() { + v_some + } else { + if false { + return; + } + // This doesn't diverge. Should not fire. + () + }; + + let v = if let Some(v_some) = g() { + v_some + } else { + // There is one match arm that doesn't diverge. Should not fire. + match (g(), g()) { + (Some(_), None) => return, + (None, Some(_)) => return, + (Some(_), Some(_)) => (), + _ => return, + } + }; + + let v = if let Some(v_some) = g() { + v_some + } else { + // loop with a break statement inside does not diverge. + loop { + break; + } + }; + + + let v = if let Some(v_some) = g() { + v_some + } else { + enum Uninhabited {} + fn un() -> Uninhabited { + panic!() + } + // Don't lint if the type is uninhabited but not ! + un() + }; + + fn question_mark() -> Option<()> { + let v = if let Some(v) = g() { + v + } else { + // Question mark does not diverge + g()? + }; + Some(v) + } +} diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr new file mode 100644 index 000000000000..461de79f6b91 --- /dev/null +++ b/tests/ui/manual_let_else.stderr @@ -0,0 +1,104 @@ +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:17:5 + | +LL | let v = if let Some(v_some) = g() { v_some } else { return }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::manual-let-else` implied by `-D warnings` + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:18:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +LL | | } else { +LL | | return; +LL | | }; + | |______^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:24:5 + | +LL | / let v = if let Some(v) = g() { +LL | | // Blocks around the identity should have no impact +LL | | { +LL | | { v } +... | +LL | | return; +LL | | }; + | |______^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:37:9 + | +LL | let v = if let Some(v_some) = g() { v_some } else { continue }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:38:9 + | +LL | let v = if let Some(v_some) = g() { v_some } else { break }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:42:5 + | +LL | let v = if let Some(v_some) = g() { v_some } else { panic!() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:45:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +LL | | } else { +LL | | std::process::abort() +LL | | }; + | |______^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:52:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +LL | | } else { +LL | | if true { return } else { panic!() } +LL | | }; + | |______^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:59:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +LL | | } else if true { +LL | | return; +LL | | } else { +LL | | panic!("diverge"); +LL | | }; + | |______^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:68:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +LL | | } else { +LL | | match (g(), g()) { +... | +LL | | } +LL | | }; + | |______^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:85:5 + | +LL | / let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) { +LL | | v_some +LL | | } else { +LL | | return; +LL | | }; + | |______^ + +error: aborting due to 11 previous errors + From 2e01e6b4c2d5a84723738575df788823c608dce7 Mon Sep 17 00:00:00 2001 From: est31 Date: Wed, 16 Feb 2022 02:10:42 +0100 Subject: [PATCH 02/11] Also support linting for match --- clippy_lints/src/manual_let_else.rs | 112 ++++++++++++++++++++------ tests/ui/manual_let_else.rs | 28 ++++++- tests/ui/manual_let_else.stderr | 13 ++- tests/ui/manual_let_else_match.rs | 93 +++++++++++++++++++++ tests/ui/manual_let_else_match.stderr | 40 +++++++++ 5 files changed, 257 insertions(+), 29 deletions(-) create mode 100644 tests/ui/manual_let_else_match.rs create mode 100644 tests/ui/manual_let_else_match.stderr diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 8983cd7ae1cf..98510ee9acdf 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -1,12 +1,14 @@ use clippy_utils::diagnostics::span_lint; +use clippy_utils::higher::IfLetOrMatch; use clippy_utils::visitors::{for_each_expr, Descend}; -use clippy_utils::{higher, meets_msrv, msrvs, peel_blocks}; +use clippy_utils::{meets_msrv, msrvs, peel_blocks}; use if_chain::if_chain; -use rustc_hir::{Expr, ExprKind, Pat, QPath, Stmt, StmtKind}; +use rustc_hir::{Expr, ExprKind, MatchSource, Pat, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::Span; use std::ops::ControlFlow; declare_clippy_lint! { @@ -56,29 +58,64 @@ impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]); impl<'tcx> LateLintPass<'tcx> for ManualLetElse { fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &'tcx Stmt<'tcx>) { - if !meets_msrv(self.msrv, msrvs::LET_ELSE) { - return; - } - - if in_external_macro(cx.sess(), stmt.span) { - return; - } - - if_chain! { + let if_let_or_match = if_chain! { + if meets_msrv(self.msrv, msrvs::LET_ELSE); + if !in_external_macro(cx.sess(), stmt.span); if let StmtKind::Local(local) = stmt.kind; if let Some(init) = local.init; - if let Some(higher::IfLet { let_pat, let_expr: _, if_then, if_else }) = higher::IfLet::hir(cx, init); - if if_then_simple_identity(let_pat, if_then); - if let Some(if_else) = if_else; - if expr_diverges(cx, if_else); + if !from_different_macros(init.span, stmt.span); + if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init); then { - span_lint( - cx, - MANUAL_LET_ELSE, - stmt.span, - "this could be rewritten as `let else`", - ); + if_let_or_match + } else { + return; } + }; + + match if_let_or_match { + IfLetOrMatch::IfLet(_let_expr, let_pat, if_then, if_else) => if_chain! { + if expr_is_simple_identity(let_pat, if_then); + if let Some(if_else) = if_else; + if expr_diverges(cx, if_else); + then { + span_lint( + cx, + MANUAL_LET_ELSE, + stmt.span, + "this could be rewritten as `let else`", + ); + } + }, + IfLetOrMatch::Match(_match_expr, arms, source) => { + if source != MatchSource::Normal { + return; + } + // Any other number than two arms doesn't (neccessarily) + // have a trivial mapping to let else. + if arms.len() != 2 { + return; + } + // We iterate over both arms, trying to find one that is an identity, + // one that diverges. Our check needs to work regardless of the order + // of both arms. + let mut found_identity_arm = false; + let mut found_diverging_arm = false; + for arm in arms { + // Guards don't give us an easy mapping to let else + if arm.guard.is_some() { + return; + } + if expr_is_simple_identity(arm.pat, arm.body) { + found_identity_arm = true; + } else if expr_diverges(cx, arm.body) && pat_has_no_bindings(arm.pat) { + found_diverging_arm = true; + } + } + if !(found_identity_arm && found_diverging_arm) { + return; + } + span_lint(cx, MANUAL_LET_ELSE, stmt.span, "this could be rewritten as `let else`"); + }, } } @@ -139,16 +176,39 @@ fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool { .is_some() } -/// Checks if the passed `if_then` is a simple identity -fn if_then_simple_identity(let_pat: &'_ Pat<'_>, if_then: &'_ Expr<'_>) -> bool { +/// Returns true if the two spans come from different macro sites, +/// or one comes from an invocation and the other is not from a macro at all. +fn from_different_macros(span_a: Span, span_b: Span) -> bool { + // This pre-check is a speed up so that we don't build outer_expn_data unless needed. + match (span_a.from_expansion(), span_b.from_expansion()) { + (false, false) => return false, + (true, false) | (false, true) => return true, + // We need to determine if both are from the same macro + (true, true) => (), + } + let data_for_comparison = |sp: Span| { + let expn_data = sp.ctxt().outer_expn_data(); + (expn_data.kind, expn_data.call_site) + }; + data_for_comparison(span_a) != data_for_comparison(span_b) +} + +fn pat_has_no_bindings(pat: &'_ Pat<'_>) -> bool { + let mut has_no_bindings = true; + pat.each_binding_or_first(&mut |_, _, _, _| has_no_bindings = false); + has_no_bindings +} + +/// Checks if the passed block is a simple identity referring to bindings created by the pattern +fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool { // TODO support patterns with multiple bindings and tuples, like: - // let (foo, bar) = if let (Some(foo), bar) = g() { (foo, bar) } else { ... } + // let ... = if let (Some(foo), bar) = g() { (foo, bar) } else { ... } if_chain! { - if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(if_then).kind; + if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(expr).kind; if let [path_seg] = path.segments; then { let mut pat_bindings = Vec::new(); - let_pat.each_binding(|_ann, _hir_id, _sp, ident| { + pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| { pat_bindings.push(ident); }); if let [binding] = &pat_bindings[..] { diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs index f2827e225002..782f9f394fed 100644 --- a/tests/ui/manual_let_else.rs +++ b/tests/ui/manual_let_else.rs @@ -2,8 +2,8 @@ #![allow( clippy::collapsible_else_if, clippy::unused_unit, - clippy::never_loop, - clippy::let_unit_value + clippy::let_unit_value, + clippy::never_loop )] #![warn(clippy::manual_let_else)] @@ -87,6 +87,14 @@ fn fire() { } else { return; }; + + // entirely inside macro lints + macro_rules! create_binding_if_some { + ($n:ident, $e:expr) => { + let $n = if let Some(v) = $e { v } else { return }; + }; + } + create_binding_if_some!(w, g()); } fn not_fire() { @@ -164,4 +172,20 @@ fn not_fire() { }; Some(v) } + + // Macro boundary inside let + macro_rules! some_or_return { + ($e:expr) => { + if let Some(v) = $e { v } else { return } + }; + } + let v = some_or_return!(g()); + + // Also macro boundary inside let, but inside a macro + macro_rules! create_binding_if_some_nf { + ($n:ident, $e:expr) => { + let $n = some_or_return!($e); + }; + } + create_binding_if_some_nf!(v, g()); } diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr index 461de79f6b91..9a2c548c5aa2 100644 --- a/tests/ui/manual_let_else.stderr +++ b/tests/ui/manual_let_else.stderr @@ -100,5 +100,16 @@ LL | | return; LL | | }; | |______^ -error: aborting due to 11 previous errors +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:94:13 + | +LL | let $n = if let Some(v) = $e { v } else { return }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | create_binding_if_some!(w, g()); + | ------------------------------- in this macro invocation + | + = note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 12 previous errors diff --git a/tests/ui/manual_let_else_match.rs b/tests/ui/manual_let_else_match.rs new file mode 100644 index 000000000000..56da1db49f21 --- /dev/null +++ b/tests/ui/manual_let_else_match.rs @@ -0,0 +1,93 @@ +#![allow(unused_braces, unused_variables, dead_code)] +#![allow(clippy::collapsible_else_if, clippy::let_unit_value)] +#![warn(clippy::manual_let_else)] +// Ensure that we don't conflict with match -> if let lints +#![warn(clippy::single_match_else, clippy::single_match)] + +enum Variant { + Foo, + Bar(u32), + Baz(u32), +} + +fn f() -> Result { + Ok(0) +} + +fn g() -> Option<()> { + None +} + +fn h() -> Variant { + Variant::Foo +} + +fn main() {} + +fn fire() { + let v = match g() { + Some(v_some) => v_some, + None => return, + }; + + let v = match g() { + Some(v_some) => v_some, + _ => return, + }; + + loop { + // More complex pattern for the identity arm + let v = match h() { + Variant::Foo => continue, + Variant::Bar(v) | Variant::Baz(v) => v, + }; + } + + // There is a _ in the diverging arm + // TODO also support unused bindings aka _v + let v = match f() { + Ok(v) => v, + Err(_) => return, + }; +} + +fn not_fire() { + // Multiple diverging arms + let v = match h() { + Variant::Foo => panic!(), + Variant::Bar(_v) => return, + Variant::Baz(v) => v, + }; + + // Multiple identity arms + let v = match h() { + Variant::Foo => panic!(), + Variant::Bar(v) => v, + Variant::Baz(v) => v, + }; + + // No diverging arm at all, only identity arms. + // This is no case for let else, but destructuring assignment. + let v = match f() { + Ok(v) => v, + Err(e) => e, + }; + + // The identity arm has a guard + let v = match h() { + Variant::Bar(v) if g().is_none() => v, + _ => return, + }; + + // The diverging arm has a guard + let v = match f() { + Err(v) if v > 0 => panic!(), + Ok(v) | Err(v) => v, + }; + + // The diverging arm creates a binding + let v = match f() { + Ok(v) => v, + Err(e) => panic!("error: {e}"), + }; +} diff --git a/tests/ui/manual_let_else_match.stderr b/tests/ui/manual_let_else_match.stderr new file mode 100644 index 000000000000..2dae1d15ad42 --- /dev/null +++ b/tests/ui/manual_let_else_match.stderr @@ -0,0 +1,40 @@ +error: this could be rewritten as `let else` + --> $DIR/manual_let_else_match.rs:28:5 + | +LL | / let v = match g() { +LL | | Some(v_some) => v_some, +LL | | None => return, +LL | | }; + | |______^ + | + = note: `-D clippy::manual-let-else` implied by `-D warnings` + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else_match.rs:33:5 + | +LL | / let v = match g() { +LL | | Some(v_some) => v_some, +LL | | _ => return, +LL | | }; + | |______^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else_match.rs:40:9 + | +LL | / let v = match h() { +LL | | Variant::Foo => continue, +LL | | Variant::Bar(v) | Variant::Baz(v) => v, +LL | | }; + | |__________^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else_match.rs:48:5 + | +LL | / let v = match f() { +LL | | Ok(v) => v, +LL | | Err(_) => return, +LL | | }; + | |______^ + +error: aborting due to 4 previous errors + From c5a7696231ef60e77fdeb5068c729d63c0393547 Mon Sep 17 00:00:00 2001 From: est31 Date: Fri, 18 Feb 2022 05:43:48 +0100 Subject: [PATCH 03/11] Support tuples --- clippy_lints/src/manual_let_else.rs | 38 ++++++++++++++++++++--------- tests/ui/manual_let_else.rs | 7 ++++++ tests/ui/manual_let_else.stderr | 14 +++++++++-- 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 98510ee9acdf..5e46abee42d1 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -3,6 +3,7 @@ use clippy_utils::higher::IfLetOrMatch; use clippy_utils::visitors::{for_each_expr, Descend}; use clippy_utils::{meets_msrv, msrvs, peel_blocks}; use if_chain::if_chain; +use rustc_data_structures::fx::FxHashSet; use rustc_hir::{Expr, ExprKind, MatchSource, Pat, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; @@ -201,20 +202,33 @@ fn pat_has_no_bindings(pat: &'_ Pat<'_>) -> bool { /// Checks if the passed block is a simple identity referring to bindings created by the pattern fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool { - // TODO support patterns with multiple bindings and tuples, like: + // We support patterns with multiple bindings and tuples, like: // let ... = if let (Some(foo), bar) = g() { (foo, bar) } else { ... } - if_chain! { - if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(expr).kind; - if let [path_seg] = path.segments; - then { - let mut pat_bindings = Vec::new(); - pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| { - pat_bindings.push(ident); - }); - if let [binding] = &pat_bindings[..] { - return path_seg.ident == *binding; + let peeled = peel_blocks(expr); + let paths = match peeled.kind { + ExprKind::Tup(exprs) | ExprKind::Array(exprs) => exprs, + ExprKind::Path(_) => std::slice::from_ref(peeled), + _ => return false, + }; + let mut pat_bindings = FxHashSet::default(); + pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| { + pat_bindings.insert(ident); + }); + if pat_bindings.len() < paths.len() { + return false; + } + for path in paths { + if_chain! { + if let ExprKind::Path(QPath::Resolved(_ty, path)) = path.kind; + if let [path_seg] = path.segments; + then { + if !pat_bindings.remove(&path_seg.ident) { + return false; + } + } else { + return false; } } } - false + true } diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs index 782f9f394fed..d69e580ff8f4 100644 --- a/tests/ui/manual_let_else.rs +++ b/tests/ui/manual_let_else.rs @@ -88,6 +88,13 @@ fn fire() { return; }; + // Tuples supported for the identity block and pattern + let v = if let (Some(v_some), w_some) = (g(), 0) { + (w_some, v_some) + } else { + return; + }; + // entirely inside macro lints macro_rules! create_binding_if_some { ($n:ident, $e:expr) => { diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr index 9a2c548c5aa2..97e6420d705c 100644 --- a/tests/ui/manual_let_else.stderr +++ b/tests/ui/manual_let_else.stderr @@ -101,7 +101,17 @@ LL | | }; | |______^ error: this could be rewritten as `let else` - --> $DIR/manual_let_else.rs:94:13 + --> $DIR/manual_let_else.rs:92:5 + | +LL | / let v = if let (Some(v_some), w_some) = (g(), 0) { +LL | | (w_some, v_some) +LL | | } else { +LL | | return; +LL | | }; + | |______^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else.rs:101:13 | LL | let $n = if let Some(v) = $e { v } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -111,5 +121,5 @@ LL | create_binding_if_some!(w, g()); | = note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info) -error: aborting due to 12 previous errors +error: aborting due to 13 previous errors From 5da7a176b778656d2e6f736f5a23ce6fdb73379d Mon Sep 17 00:00:00 2001 From: est31 Date: Sat, 19 Feb 2022 07:15:20 +0100 Subject: [PATCH 04/11] Don't suggest let else in match if the else arm explicitly mentions non obvious paths --- clippy_lints/src/manual_let_else.rs | 43 ++++++++++++++++--- tests/ui/manual_let_else_match.rs | 62 +++++++++++++++++++-------- tests/ui/manual_let_else_match.stderr | 30 ++++++++++--- 3 files changed, 106 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 5e46abee42d1..87bac8aabdc5 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -1,14 +1,16 @@ use clippy_utils::diagnostics::span_lint; use clippy_utils::higher::IfLetOrMatch; +use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::{for_each_expr, Descend}; use clippy_utils::{meets_msrv, msrvs, peel_blocks}; use if_chain::if_chain; use rustc_data_structures::fx::FxHashSet; -use rustc_hir::{Expr, ExprKind, MatchSource, Pat, QPath, Stmt, StmtKind}; +use rustc_hir::{Expr, ExprKind, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::symbol::sym; use rustc_span::Span; use std::ops::ControlFlow; @@ -108,7 +110,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { } if expr_is_simple_identity(arm.pat, arm.body) { found_identity_arm = true; - } else if expr_diverges(cx, arm.body) && pat_has_no_bindings(arm.pat) { + } else if expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat) { found_diverging_arm = true; } } @@ -194,10 +196,39 @@ fn from_different_macros(span_a: Span, span_b: Span) -> bool { data_for_comparison(span_a) != data_for_comparison(span_b) } -fn pat_has_no_bindings(pat: &'_ Pat<'_>) -> bool { - let mut has_no_bindings = true; - pat.each_binding_or_first(&mut |_, _, _, _| has_no_bindings = false); - has_no_bindings +fn pat_allowed_for_else(cx: &LateContext<'_>, pat: &'_ Pat<'_>) -> bool { + // Check whether the pattern contains any bindings, as the + // binding might potentially be used in the body. + // TODO: only look for *used* bindings. + let mut has_bindings = false; + pat.each_binding_or_first(&mut |_, _, _, _| has_bindings = true); + if has_bindings { + return false; + } + + // Check whether any possibly "unknown" patterns are included, + // because users might not know which values some enum has. + // Well-known enums are excepted, as we assume people know them. + // We do a deep check, to be able to disallow Err(En::Foo(_)) + // for usage of the En::Foo variant, as we disallow En::Foo(_), + // but we allow Err(_). + let typeck_results = cx.typeck_results(); + let mut has_disallowed = false; + pat.walk_always(|pat| { + // Only do the check if the type is "spelled out" in the pattern + if !matches!( + pat.kind, + PatKind::Struct(..) | PatKind::TupleStruct(..) | PatKind::Path(..) + ) { + return; + }; + let ty = typeck_results.pat_ty(pat); + // Option and Result are allowed, everything else isn't. + if !(is_type_diagnostic_item(cx, ty, sym::Option) || is_type_diagnostic_item(cx, ty, sym::Result)) { + has_disallowed = true; + } + }); + !has_disallowed } /// Checks if the passed block is a simple identity referring to bindings created by the pattern diff --git a/tests/ui/manual_let_else_match.rs b/tests/ui/manual_let_else_match.rs index 56da1db49f21..93c86ca24fea 100644 --- a/tests/ui/manual_let_else_match.rs +++ b/tests/ui/manual_let_else_match.rs @@ -4,12 +4,6 @@ // Ensure that we don't conflict with match -> if let lints #![warn(clippy::single_match_else, clippy::single_match)] -enum Variant { - Foo, - Bar(u32), - Baz(u32), -} - fn f() -> Result { Ok(0) } @@ -18,7 +12,17 @@ fn g() -> Option<()> { None } -fn h() -> Variant { +fn h() -> (Option<()>, Option<()>) { + (None, None) +} + +enum Variant { + Foo, + Bar(u32), + Baz(u32), +} + +fn build_enum() -> Variant { Variant::Foo } @@ -36,9 +40,14 @@ fn fire() { }; loop { - // More complex pattern for the identity arm + // More complex pattern for the identity arm and diverging arm let v = match h() { - Variant::Foo => continue, + (Some(_), Some(_)) | (None, None) => continue, + (Some(v), None) | (None, Some(v)) => v, + }; + // Custom enums are supported as long as the "else" arm is a simple _ + let v = match build_enum() { + _ => continue, Variant::Bar(v) | Variant::Baz(v) => v, }; } @@ -49,21 +58,27 @@ fn fire() { Ok(v) => v, Err(_) => return, }; + + // Err(()) is an allowed pattern + let v = match f().map_err(|_| ()) { + Ok(v) => v, + Err(()) => return, + }; } fn not_fire() { // Multiple diverging arms let v = match h() { - Variant::Foo => panic!(), - Variant::Bar(_v) => return, - Variant::Baz(v) => v, + _ => panic!(), + (None, Some(_v)) => return, + (Some(v), None) => v, }; // Multiple identity arms let v = match h() { - Variant::Foo => panic!(), - Variant::Bar(v) => v, - Variant::Baz(v) => v, + _ => panic!(), + (None, Some(v)) => v, + (Some(v), None) => v, }; // No diverging arm at all, only identity arms. @@ -74,8 +89,8 @@ fn not_fire() { }; // The identity arm has a guard - let v = match h() { - Variant::Bar(v) if g().is_none() => v, + let v = match g() { + Some(v) if g().is_none() => v, _ => return, }; @@ -90,4 +105,17 @@ fn not_fire() { Ok(v) => v, Err(e) => panic!("error: {e}"), }; + + // Custom enum where the diverging arm + // explicitly mentions the variant + let v = match build_enum() { + Variant::Foo => return, + Variant::Bar(v) | Variant::Baz(v) => v, + }; + + // The custom enum is surrounded by an Err() + let v = match Err(build_enum()) { + Ok(v) | Err(Variant::Bar(v) | Variant::Baz(v)) => v, + Err(Variant::Foo) => return, + }; } diff --git a/tests/ui/manual_let_else_match.stderr b/tests/ui/manual_let_else_match.stderr index 2dae1d15ad42..a79afc6401d9 100644 --- a/tests/ui/manual_let_else_match.stderr +++ b/tests/ui/manual_let_else_match.stderr @@ -1,5 +1,5 @@ error: this could be rewritten as `let else` - --> $DIR/manual_let_else_match.rs:28:5 + --> $DIR/manual_let_else_match.rs:32:5 | LL | / let v = match g() { LL | | Some(v_some) => v_some, @@ -10,7 +10,7 @@ LL | | }; = note: `-D clippy::manual-let-else` implied by `-D warnings` error: this could be rewritten as `let else` - --> $DIR/manual_let_else_match.rs:33:5 + --> $DIR/manual_let_else_match.rs:37:5 | LL | / let v = match g() { LL | | Some(v_some) => v_some, @@ -19,16 +19,25 @@ LL | | }; | |______^ error: this could be rewritten as `let else` - --> $DIR/manual_let_else_match.rs:40:9 + --> $DIR/manual_let_else_match.rs:44:9 | LL | / let v = match h() { -LL | | Variant::Foo => continue, +LL | | (Some(_), Some(_)) | (None, None) => continue, +LL | | (Some(v), None) | (None, Some(v)) => v, +LL | | }; + | |__________^ + +error: this could be rewritten as `let else` + --> $DIR/manual_let_else_match.rs:49:9 + | +LL | / let v = match build_enum() { +LL | | _ => continue, LL | | Variant::Bar(v) | Variant::Baz(v) => v, LL | | }; | |__________^ error: this could be rewritten as `let else` - --> $DIR/manual_let_else_match.rs:48:5 + --> $DIR/manual_let_else_match.rs:57:5 | LL | / let v = match f() { LL | | Ok(v) => v, @@ -36,5 +45,14 @@ LL | | Err(_) => return, LL | | }; | |______^ -error: aborting due to 4 previous errors +error: this could be rewritten as `let else` + --> $DIR/manual_let_else_match.rs:63:5 + | +LL | / let v = match f().map_err(|_| ()) { +LL | | Ok(v) => v, +LL | | Err(()) => return, +LL | | }; + | |______^ + +error: aborting due to 6 previous errors From 173a8e04807dbdcc714d7dec0da331ee86538131 Mon Sep 17 00:00:00 2001 From: est31 Date: Thu, 6 Oct 2022 23:07:58 +0200 Subject: [PATCH 05/11] Replace from_different_macros with equivalent and simpler check --- clippy_lints/src/manual_let_else.rs | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 87bac8aabdc5..e366d6db7749 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -11,7 +11,6 @@ use rustc_middle::lint::in_external_macro; use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::symbol::sym; -use rustc_span::Span; use std::ops::ControlFlow; declare_clippy_lint! { @@ -66,7 +65,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { if !in_external_macro(cx.sess(), stmt.span); if let StmtKind::Local(local) = stmt.kind; if let Some(init) = local.init; - if !from_different_macros(init.span, stmt.span); + if init.span.ctxt() == stmt.span.ctxt(); if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init); then { if_let_or_match @@ -179,23 +178,6 @@ fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool { .is_some() } -/// Returns true if the two spans come from different macro sites, -/// or one comes from an invocation and the other is not from a macro at all. -fn from_different_macros(span_a: Span, span_b: Span) -> bool { - // This pre-check is a speed up so that we don't build outer_expn_data unless needed. - match (span_a.from_expansion(), span_b.from_expansion()) { - (false, false) => return false, - (true, false) | (false, true) => return true, - // We need to determine if both are from the same macro - (true, true) => (), - } - let data_for_comparison = |sp: Span| { - let expn_data = sp.ctxt().outer_expn_data(); - (expn_data.kind, expn_data.call_site) - }; - data_for_comparison(span_a) != data_for_comparison(span_b) -} - fn pat_allowed_for_else(cx: &LateContext<'_>, pat: &'_ Pat<'_>) -> bool { // Check whether the pattern contains any bindings, as the // binding might potentially be used in the body. From 9bd70dbb8855580ca75d37211fce74adc8fda734 Mon Sep 17 00:00:00 2001 From: est31 Date: Sat, 8 Oct 2022 02:50:30 +0200 Subject: [PATCH 06/11] Make the match checking configurable --- clippy_lints/src/lib.rs | 3 +- clippy_lints/src/manual_let_else.rs | 29 ++++++++++++++++--- clippy_lints/src/utils/conf.rs | 6 ++++ .../toml_unknown_key/conf_unknown_key.stderr | 1 + 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b261beab7936..5716ef71641c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -604,7 +604,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: )) }); store.register_late_pass(move |_| Box::new(matches::Matches::new(msrv))); - store.register_late_pass(move |_| Box::new(manual_let_else::ManualLetElse::new(msrv))); + let matches_for_let_else = conf.matches_for_let_else; + store.register_late_pass(move |_| Box::new(manual_let_else::ManualLetElse::new(msrv, matches_for_let_else))); store.register_early_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustiveStruct::new(msrv))); store.register_late_pass(move |_| Box::new(manual_non_exhaustive::ManualNonExhaustiveEnum::new(msrv))); store.register_late_pass(move |_| Box::new(manual_strip::ManualStrip::new(msrv))); diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index e366d6db7749..13a734818a7e 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -11,6 +11,7 @@ use rustc_middle::lint::in_external_macro; use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::symbol::sym; +use serde::Deserialize; use std::ops::ControlFlow; declare_clippy_lint! { @@ -47,12 +48,16 @@ declare_clippy_lint! { pub struct ManualLetElse { msrv: Option, + matches_behaviour: MatchLintBehaviour, } impl ManualLetElse { #[must_use] - pub fn new(msrv: Option) -> Self { - Self { msrv } + pub fn new(msrv: Option, matches_behaviour: MatchLintBehaviour) -> Self { + Self { + msrv, + matches_behaviour, + } } } @@ -89,6 +94,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { } }, IfLetOrMatch::Match(_match_expr, arms, source) => { + if self.matches_behaviour == MatchLintBehaviour::Never { + return; + } if source != MatchSource::Normal { return; } @@ -97,6 +105,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { if arms.len() != 2 { return; } + let check_types = self.matches_behaviour == MatchLintBehaviour::WellKnownTypes; // We iterate over both arms, trying to find one that is an identity, // one that diverges. Our check needs to work regardless of the order // of both arms. @@ -109,7 +118,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { } if expr_is_simple_identity(arm.pat, arm.body) { found_identity_arm = true; - } else if expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat) { + } else if expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat, check_types) { found_diverging_arm = true; } } @@ -178,7 +187,7 @@ fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool { .is_some() } -fn pat_allowed_for_else(cx: &LateContext<'_>, pat: &'_ Pat<'_>) -> bool { +fn pat_allowed_for_else(cx: &LateContext<'_>, pat: &'_ Pat<'_>, check_types: bool) -> bool { // Check whether the pattern contains any bindings, as the // binding might potentially be used in the body. // TODO: only look for *used* bindings. @@ -188,6 +197,11 @@ fn pat_allowed_for_else(cx: &LateContext<'_>, pat: &'_ Pat<'_>) -> bool { return false; } + // If we shouldn't check the types, exit early. + if !check_types { + return true; + } + // Check whether any possibly "unknown" patterns are included, // because users might not know which values some enum has. // Well-known enums are excepted, as we assume people know them. @@ -245,3 +259,10 @@ fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool { } true } + +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Deserialize)] +pub enum MatchLintBehaviour { + AllTypes, + WellKnownTypes, + Never, +} diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 1aa86efd38f8..ef6de7d333d3 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -335,6 +335,12 @@ define_Conf! { /// /// Enables verbose mode. Triggers if there is more than one uppercase char next to each other (upper_case_acronyms_aggressive: bool = false), + /// Lint: MANUAL_LET_ELSE. + /// + /// Whether the matches should be considered by the lint, and whether there should + /// be filtering for common types. + (matches_for_let_else: crate::manual_let_else::MatchLintBehaviour = + crate::manual_let_else::MatchLintBehaviour::WellKnownTypes), /// Lint: _CARGO_COMMON_METADATA. /// /// For internal testing only, ignores the current `publish` settings in the Cargo manifest. diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 82ee80541321..7db2e11225bd 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -22,6 +22,7 @@ error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown fie enum-variant-size-threshold large-error-threshold literal-representation-threshold + matches-for-let-else max-fn-params-bools max-include-file-size max-struct-bools From a1db9311dc0ddb0dea68d2661442df2c6f4b382c Mon Sep 17 00:00:00 2001 From: est31 Date: Mon, 10 Oct 2022 07:17:04 +0200 Subject: [PATCH 07/11] Make an attempt of creating suggestions They aren't perfect but better than nothing --- clippy_lints/src/manual_let_else.rs | 78 ++++++++++++------- tests/ui/manual_let_else.rs | 11 ++- tests/ui/manual_let_else.stderr | 106 +++++++++++++++++++++----- tests/ui/manual_let_else_match.stderr | 24 +++--- 4 files changed, 156 insertions(+), 63 deletions(-) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 13a734818a7e..8d743c86d6df 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -1,16 +1,19 @@ -use clippy_utils::diagnostics::span_lint; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::IfLetOrMatch; +use clippy_utils::source::snippet_opt; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::{for_each_expr, Descend}; use clippy_utils::{meets_msrv, msrvs, peel_blocks}; use if_chain::if_chain; use rustc_data_structures::fx::FxHashSet; +use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::symbol::sym; +use rustc_span::Span; use serde::Deserialize; use std::ops::ControlFlow; @@ -80,20 +83,15 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { }; match if_let_or_match { - IfLetOrMatch::IfLet(_let_expr, let_pat, if_then, if_else) => if_chain! { + IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else) => if_chain! { if expr_is_simple_identity(let_pat, if_then); if let Some(if_else) = if_else; if expr_diverges(cx, if_else); then { - span_lint( - cx, - MANUAL_LET_ELSE, - stmt.span, - "this could be rewritten as `let else`", - ); + emit_manual_let_else(cx, stmt.span, if_let_expr, let_pat, if_else); } }, - IfLetOrMatch::Match(_match_expr, arms, source) => { + IfLetOrMatch::Match(match_expr, arms, source) => { if self.matches_behaviour == MatchLintBehaviour::Never { return; } @@ -105,27 +103,22 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { if arms.len() != 2 { return; } - let check_types = self.matches_behaviour == MatchLintBehaviour::WellKnownTypes; - // We iterate over both arms, trying to find one that is an identity, - // one that diverges. Our check needs to work regardless of the order - // of both arms. - let mut found_identity_arm = false; - let mut found_diverging_arm = false; - for arm in arms { - // Guards don't give us an easy mapping to let else - if arm.guard.is_some() { - return; - } - if expr_is_simple_identity(arm.pat, arm.body) { - found_identity_arm = true; - } else if expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat, check_types) { - found_diverging_arm = true; - } + // Guards don't give us an easy mapping either + if arms.iter().any(|arm| arm.guard.is_some()) { + return; } - if !(found_identity_arm && found_diverging_arm) { + let check_types = self.matches_behaviour == MatchLintBehaviour::WellKnownTypes; + let diverging_arm_opt = arms + .iter() + .enumerate() + .find(|(_, arm)| expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat, check_types)); + let Some((idx, diverging_arm)) = diverging_arm_opt else { return; }; + let pat_arm = &arms[1 - idx]; + if !expr_is_simple_identity(pat_arm.pat, pat_arm.body) { return; } - span_lint(cx, MANUAL_LET_ELSE, stmt.span, "this could be rewritten as `let else`"); + + emit_manual_let_else(cx, stmt.span, match_expr, pat_arm.pat, diverging_arm.body); }, } } @@ -133,6 +126,37 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { extract_msrv_attr!(LateContext); } +fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat: &Pat<'_>, else_body: &Expr<'_>) { + span_lint_and_then( + cx, + MANUAL_LET_ELSE, + span, + "this could be rewritten as `let...else`", + |diag| { + // This is far from perfect, for example there needs to be: + // * mut additions for the bindings + // * renamings of the bindings + // * unused binding collision detection with existing ones + // * putting patterns with at the top level | inside () + // for this to be machine applicable. + let app = Applicability::HasPlaceholders; + + if let Some(sn_pat) = snippet_opt(cx, pat.span) && + let Some(sn_expr) = snippet_opt(cx, expr.span) && + let Some(sn_else) = snippet_opt(cx, else_body.span) + { + let else_bl = if matches!(else_body.kind, ExprKind::Block(..)) { + sn_else + } else { + format!("{{ {sn_else} }}") + }; + let sugg = format!("let {sn_pat} = {sn_expr} else {else_bl};"); + diag.span_suggestion(span, "consider writing", sugg, app); + } + }, + ); +} + fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool { fn is_never(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool { if let Some(ty) = cx.typeck_results().expr_ty_opt(expr) { diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs index d69e580ff8f4..bd0ac69e46e8 100644 --- a/tests/ui/manual_let_else.rs +++ b/tests/ui/manual_let_else.rs @@ -158,14 +158,13 @@ fn not_fire() { } }; - - let v = if let Some(v_some) = g() { + enum Uninhabited {} + fn un() -> Uninhabited { + panic!() + } + let v = if let Some(v_some) = None { v_some } else { - enum Uninhabited {} - fn un() -> Uninhabited { - panic!() - } // Don't lint if the type is uninhabited but not ! un() }; diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr index 97e6420d705c..e073276d20cd 100644 --- a/tests/ui/manual_let_else.stderr +++ b/tests/ui/manual_let_else.stderr @@ -1,12 +1,12 @@ -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:17:5 | LL | let v = if let Some(v_some) = g() { v_some } else { return }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { return };` | = note: `-D clippy::manual-let-else` implied by `-D warnings` -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:18:5 | LL | / let v = if let Some(v_some) = g() { @@ -15,8 +15,15 @@ LL | | } else { LL | | return; LL | | }; | |______^ + | +help: consider writing + | +LL ~ let Some(v_some) = g() else { +LL + return; +LL + }; + | -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:24:5 | LL | / let v = if let Some(v) = g() { @@ -27,26 +34,35 @@ LL | | { v } LL | | return; LL | | }; | |______^ + | +help: consider writing + | +LL ~ let Some(v) = g() else { +LL + // Some computation should still make it fire +LL + g(); +LL + return; +LL + }; + | -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:37:9 | LL | let v = if let Some(v_some) = g() { v_some } else { continue }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { continue };` -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:38:9 | LL | let v = if let Some(v_some) = g() { v_some } else { break }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { break };` -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:42:5 | LL | let v = if let Some(v_some) = g() { v_some } else { panic!() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { panic!() };` -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:45:5 | LL | / let v = if let Some(v_some) = g() { @@ -55,8 +71,15 @@ LL | | } else { LL | | std::process::abort() LL | | }; | |______^ + | +help: consider writing + | +LL ~ let Some(v_some) = g() else { +LL + std::process::abort() +LL + }; + | -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:52:5 | LL | / let v = if let Some(v_some) = g() { @@ -65,8 +88,15 @@ LL | | } else { LL | | if true { return } else { panic!() } LL | | }; | |______^ + | +help: consider writing + | +LL ~ let Some(v_some) = g() else { +LL + if true { return } else { panic!() } +LL + }; + | -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:59:5 | LL | / let v = if let Some(v_some) = g() { @@ -77,8 +107,17 @@ LL | | } else { LL | | panic!("diverge"); LL | | }; | |______^ + | +help: consider writing + | +LL ~ let Some(v_some) = g() else { if true { +LL + return; +LL + } else { +LL + panic!("diverge"); +LL + } }; + | -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:68:5 | LL | / let v = if let Some(v_some) = g() { @@ -89,8 +128,25 @@ LL | | match (g(), g()) { LL | | } LL | | }; | |______^ + | +help: consider writing + | +LL ~ let Some(v_some) = g() else { +LL + match (g(), g()) { +LL + (Some(_), None) => return, +LL + (None, Some(_)) => { +LL + if true { +LL + return; +LL + } else { +LL + panic!(); +LL + } +LL + }, +LL + _ => return, +LL + } +LL + }; + | -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:85:5 | LL | / let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) { @@ -99,8 +155,15 @@ LL | | } else { LL | | return; LL | | }; | |______^ + | +help: consider writing + | +LL ~ let Some(v_some) = g().map(|v| (v, 42)) else { +LL + return; +LL + }; + | -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:92:5 | LL | / let v = if let (Some(v_some), w_some) = (g(), 0) { @@ -109,12 +172,19 @@ LL | | } else { LL | | return; LL | | }; | |______^ + | +help: consider writing + | +LL ~ let (Some(v_some), w_some) = (g(), 0) else { +LL + return; +LL + }; + | -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else.rs:101:13 | LL | let $n = if let Some(v) = $e { v } else { return }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };` ... LL | create_binding_if_some!(w, g()); | ------------------------------- in this macro invocation diff --git a/tests/ui/manual_let_else_match.stderr b/tests/ui/manual_let_else_match.stderr index a79afc6401d9..38be5ac54547 100644 --- a/tests/ui/manual_let_else_match.stderr +++ b/tests/ui/manual_let_else_match.stderr @@ -1,58 +1,58 @@ -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else_match.rs:32:5 | LL | / let v = match g() { LL | | Some(v_some) => v_some, LL | | None => return, LL | | }; - | |______^ + | |______^ help: consider writing: `let Some(v_some) = g() else { return };` | = note: `-D clippy::manual-let-else` implied by `-D warnings` -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else_match.rs:37:5 | LL | / let v = match g() { LL | | Some(v_some) => v_some, LL | | _ => return, LL | | }; - | |______^ + | |______^ help: consider writing: `let Some(v_some) = g() else { return };` -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else_match.rs:44:9 | LL | / let v = match h() { LL | | (Some(_), Some(_)) | (None, None) => continue, LL | | (Some(v), None) | (None, Some(v)) => v, LL | | }; - | |__________^ + | |__________^ help: consider writing: `let (Some(v), None) | (None, Some(v)) = h() else { continue };` -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else_match.rs:49:9 | LL | / let v = match build_enum() { LL | | _ => continue, LL | | Variant::Bar(v) | Variant::Baz(v) => v, LL | | }; - | |__________^ + | |__________^ help: consider writing: `let Variant::Bar(v) | Variant::Baz(v) = build_enum() else { continue };` -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else_match.rs:57:5 | LL | / let v = match f() { LL | | Ok(v) => v, LL | | Err(_) => return, LL | | }; - | |______^ + | |______^ help: consider writing: `let Ok(v) = f() else { return };` -error: this could be rewritten as `let else` +error: this could be rewritten as `let...else` --> $DIR/manual_let_else_match.rs:63:5 | LL | / let v = match f().map_err(|_| ()) { LL | | Ok(v) => v, LL | | Err(()) => return, LL | | }; - | |______^ + | |______^ help: consider writing: `let Ok(v) = f().map_err(|_| ()) else { return };` error: aborting due to 6 previous errors From 01e651f2feea84ce0b4551a71101f7b29bcc65cb Mon Sep 17 00:00:00 2001 From: oxalica Date: Mon, 10 Oct 2022 16:00:33 +0200 Subject: [PATCH 08/11] Don't lint if the let is already a let-else We cannot apply the lint for 3-branches like in the added example. --- clippy_lints/src/manual_let_else.rs | 1 + tests/ui/manual_let_else.rs | 3 +++ 2 files changed, 4 insertions(+) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 8d743c86d6df..8a915127c3c6 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -73,6 +73,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { if !in_external_macro(cx.sess(), stmt.span); if let StmtKind::Local(local) = stmt.kind; if let Some(init) = local.init; + if local.els.is_none(); if init.span.ctxt() == stmt.span.ctxt(); if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init); then { diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs index bd0ac69e46e8..9e5df65b74d3 100644 --- a/tests/ui/manual_let_else.rs +++ b/tests/ui/manual_let_else.rs @@ -194,4 +194,7 @@ fn not_fire() { }; } create_binding_if_some_nf!(v, g()); + + // Already a let-else + let Some(a) = (if let Some(b) = Some(Some(())) { b } else { return }) else { panic!() }; } From 748169deaa3c57e15e3a84bb90e033dddaa64e03 Mon Sep 17 00:00:00 2001 From: est31 Date: Mon, 10 Oct 2022 21:51:24 +0200 Subject: [PATCH 09/11] Don't fire the lint if there is a type annotation Sometimes type annotations are needed for type inferrence to work, or because of coercions. We don't know this, and we also don't want users to possibly repeat the entire pattern. --- clippy_lints/src/manual_let_else.rs | 1 + tests/ui/manual_let_else.rs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 8a915127c3c6..521d02db80fb 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -74,6 +74,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { if let StmtKind::Local(local) = stmt.kind; if let Some(init) = local.init; if local.els.is_none(); + if local.ty.is_none(); if init.span.ctxt() == stmt.span.ctxt(); if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init); then { diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs index 9e5df65b74d3..9046c0affb59 100644 --- a/tests/ui/manual_let_else.rs +++ b/tests/ui/manual_let_else.rs @@ -197,4 +197,8 @@ fn not_fire() { // Already a let-else let Some(a) = (if let Some(b) = Some(Some(())) { b } else { return }) else { panic!() }; + + // If a type annotation is present, don't lint as + // expressing the type might be too hard + let v: () = if let Some(v_some) = g() { v_some } else { panic!() }; } From 96ea5b2cd6c8a1c258d26ef8b39697689785478f Mon Sep 17 00:00:00 2001 From: est31 Date: Wed, 12 Oct 2022 00:04:09 +0200 Subject: [PATCH 10/11] Fix dogfooding --- clippy_lints/src/dereference.rs | 4 +--- clippy_utils/src/consts.rs | 10 +++------- clippy_utils/src/hir_utils.rs | 7 ++----- 3 files changed, 6 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index a95d9f5390de..7b5c10db4e1b 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -274,9 +274,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { } let typeck = cx.typeck_results(); - let (kind, sub_expr) = if let Some(x) = try_parse_ref_op(cx.tcx, typeck, expr) { - x - } else { + let Some((kind, sub_expr)) = try_parse_ref_op(cx.tcx, typeck, expr) else { // The whole chain of reference operations has been seen if let Some((state, data)) = self.state.take() { report(cx, expr, state, data); diff --git a/clippy_utils/src/consts.rs b/clippy_utils/src/consts.rs index 07e4ef6a2fef..f01afda72b2c 100644 --- a/clippy_utils/src/consts.rs +++ b/clippy_utils/src/consts.rs @@ -147,9 +147,8 @@ impl Constant { _ => None, }, (&Self::Vec(ref l), &Self::Vec(ref r)) => { - let cmp_type = match *cmp_type.kind() { - ty::Array(ty, _) | ty::Slice(ty) => ty, - _ => return None, + let (ty::Array(cmp_type, _) | ty::Slice(cmp_type)) = *cmp_type.kind() else { + return None }; iter::zip(l, r) .map(|(li, ri)| Self::partial_cmp(tcx, cmp_type, li, ri)) @@ -401,10 +400,7 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> { use self::Constant::{Int, F32, F64}; match *o { Int(value) => { - let ity = match *ty.kind() { - ty::Int(ity) => ity, - _ => return None, - }; + let ty::Int(ity) = *ty.kind() else { return None }; // sign extend let value = sext(self.lcx.tcx, value, ity); let value = value.checked_neg()?; diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index cf24ec8b67b9..02b973e5b278 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -131,13 +131,10 @@ impl HirEqInterExpr<'_, '_, '_> { ([], None, [], None) => { // For empty blocks, check to see if the tokens are equal. This will catch the case where a macro // expanded to nothing, or the cfg attribute was used. - let (left, right) = match ( + let (Some(left), Some(right)) = ( snippet_opt(self.inner.cx, left.span), snippet_opt(self.inner.cx, right.span), - ) { - (Some(left), Some(right)) => (left, right), - _ => return true, - }; + ) else { return true }; let mut left_pos = 0; let left = tokenize(&left) .map(|t| { From dcde480a667f24923c9895e3eb6e9a030d271299 Mon Sep 17 00:00:00 2001 From: est31 Date: Sun, 16 Oct 2022 03:56:40 +0200 Subject: [PATCH 11/11] Also consider match guards for divergence check Plus, add some tests for the divergence check. --- clippy_lints/src/manual_let_else.rs | 7 ++- tests/ui/manual_let_else.rs | 33 ++++++++++ tests/ui/manual_let_else.stderr | 96 ++++++++++++++++++++++++----- 3 files changed, 120 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 521d02db80fb..1846596fa4c8 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -194,8 +194,11 @@ fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool { ControlFlow::Continue(Descend::No) }, ExprKind::Match(match_expr, match_arms, _) => { - let diverges = - expr_diverges(cx, match_expr) || match_arms.iter().all(|arm| expr_diverges(cx, arm.body)); + let diverges = expr_diverges(cx, match_expr) + || match_arms.iter().all(|arm| { + let guard_diverges = arm.guard.as_ref().map_or(false, |g| expr_diverges(cx, g.body())); + guard_diverges || expr_diverges(cx, arm.body) + }); if diverges { return ControlFlow::Break(()); } diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs index 9046c0affb59..2ef40e5911af 100644 --- a/tests/ui/manual_let_else.rs +++ b/tests/ui/manual_let_else.rs @@ -3,6 +3,7 @@ clippy::collapsible_else_if, clippy::unused_unit, clippy::let_unit_value, + clippy::match_single_binding, clippy::never_loop )] #![warn(clippy::manual_let_else)] @@ -55,6 +56,38 @@ fn fire() { if true { return } else { panic!() } }; + // Diverging after an if still makes the block diverge: + let v = if let Some(v_some) = g() { + v_some + } else { + if true {} + panic!(); + }; + + // A match diverges if all branches diverge: + // Note: the corresponding let-else requires a ; at the end of the match + // as otherwise the type checker does not turn it into a ! type. + let v = if let Some(v_some) = g() { + v_some + } else { + match () { + _ if panic!() => {}, + _ => panic!(), + } + }; + + // An if's expression can cause divergence: + let v = if let Some(v_some) = g() { v_some } else { if panic!() {} }; + + // An expression of a match can cause divergence: + let v = if let Some(v_some) = g() { + v_some + } else { + match panic!() { + _ => {}, + } + }; + // Top level else if let v = if let Some(v_some) = g() { v_some diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr index e073276d20cd..453b68b8bd00 100644 --- a/tests/ui/manual_let_else.stderr +++ b/tests/ui/manual_let_else.stderr @@ -1,5 +1,5 @@ error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:17:5 + --> $DIR/manual_let_else.rs:18:5 | LL | let v = if let Some(v_some) = g() { v_some } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { return };` @@ -7,7 +7,7 @@ LL | let v = if let Some(v_some) = g() { v_some } else { return }; = note: `-D clippy::manual-let-else` implied by `-D warnings` error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:18:5 + --> $DIR/manual_let_else.rs:19:5 | LL | / let v = if let Some(v_some) = g() { LL | | v_some @@ -24,7 +24,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:24:5 + --> $DIR/manual_let_else.rs:25:5 | LL | / let v = if let Some(v) = g() { LL | | // Blocks around the identity should have no impact @@ -45,25 +45,25 @@ LL + }; | error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:37:9 + --> $DIR/manual_let_else.rs:38:9 | LL | let v = if let Some(v_some) = g() { v_some } else { continue }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { continue };` error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:38:9 + --> $DIR/manual_let_else.rs:39:9 | LL | let v = if let Some(v_some) = g() { v_some } else { break }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { break };` error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:42:5 + --> $DIR/manual_let_else.rs:43:5 | LL | let v = if let Some(v_some) = g() { v_some } else { panic!() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { panic!() };` error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:45:5 + --> $DIR/manual_let_else.rs:46:5 | LL | / let v = if let Some(v_some) = g() { LL | | v_some @@ -80,7 +80,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:52:5 + --> $DIR/manual_let_else.rs:53:5 | LL | / let v = if let Some(v_some) = g() { LL | | v_some @@ -97,7 +97,75 @@ LL + }; | error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:59:5 + --> $DIR/manual_let_else.rs:60:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +LL | | } else { +LL | | if true {} +LL | | panic!(); +LL | | }; + | |______^ + | +help: consider writing + | +LL ~ let Some(v_some) = g() else { +LL + if true {} +LL + panic!(); +LL + }; + | + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else.rs:70:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +LL | | } else { +LL | | match () { +... | +LL | | } +LL | | }; + | |______^ + | +help: consider writing + | +LL ~ let Some(v_some) = g() else { +LL + match () { +LL + _ if panic!() => {}, +LL + _ => panic!(), +LL + } +LL + }; + | + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else.rs:80:5 + | +LL | let v = if let Some(v_some) = g() { v_some } else { if panic!() {} }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { if panic!() {} };` + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else.rs:83:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +LL | | } else { +LL | | match panic!() { +LL | | _ => {}, +LL | | } +LL | | }; + | |______^ + | +help: consider writing + | +LL ~ let Some(v_some) = g() else { +LL + match panic!() { +LL + _ => {}, +LL + } +LL + }; + | + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else.rs:92:5 | LL | / let v = if let Some(v_some) = g() { LL | | v_some @@ -118,7 +186,7 @@ LL + } }; | error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:68:5 + --> $DIR/manual_let_else.rs:101:5 | LL | / let v = if let Some(v_some) = g() { LL | | v_some @@ -147,7 +215,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:85:5 + --> $DIR/manual_let_else.rs:118:5 | LL | / let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) { LL | | v_some @@ -164,7 +232,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:92:5 + --> $DIR/manual_let_else.rs:125:5 | LL | / let v = if let (Some(v_some), w_some) = (g(), 0) { LL | | (w_some, v_some) @@ -181,7 +249,7 @@ LL + }; | error: this could be rewritten as `let...else` - --> $DIR/manual_let_else.rs:101:13 + --> $DIR/manual_let_else.rs:134:13 | LL | let $n = if let Some(v) = $e { v } else { return }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };` @@ -191,5 +259,5 @@ LL | create_binding_if_some!(w, g()); | = note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info) -error: aborting due to 13 previous errors +error: aborting due to 17 previous errors