diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 8983cd7ae..98510ee9a 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 f2827e225..782f9f394 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 461de79f6..9a2c548c5 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 000000000..56da1db49 --- /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 000000000..2dae1d15a --- /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 +