From 5da7a176b778656d2e6f736f5a23ce6fdb73379d Mon Sep 17 00:00:00 2001 From: est31 Date: Sat, 19 Feb 2022 07:15:20 +0100 Subject: [PATCH] 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 5e46abee4..87bac8aab 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 56da1db49..93c86ca24 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 2dae1d15a..a79afc640 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