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/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_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7837e04bca1f..5716ef71641c 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,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: )) }); store.register_late_pass(move |_| Box::new(matches::Matches::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 new file mode 100644 index 000000000000..1846596fa4c8 --- /dev/null +++ b/clippy_lints/src/manual_let_else.rs @@ -0,0 +1,297 @@ +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; + +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, + matches_behaviour: MatchLintBehaviour, +} + +impl ManualLetElse { + #[must_use] + pub fn new(msrv: Option, matches_behaviour: MatchLintBehaviour) -> Self { + Self { + msrv, + matches_behaviour, + } + } +} + +impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]); + +impl<'tcx> LateLintPass<'tcx> for ManualLetElse { + fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &'tcx Stmt<'tcx>) { + 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 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 { + if_let_or_match + } else { + return; + } + }; + + match if_let_or_match { + 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 { + emit_manual_let_else(cx, stmt.span, if_let_expr, let_pat, if_else); + } + }, + IfLetOrMatch::Match(match_expr, arms, source) => { + if self.matches_behaviour == MatchLintBehaviour::Never { + return; + } + 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; + } + // Guards don't give us an easy mapping either + if arms.iter().any(|arm| arm.guard.is_some()) { + return; + } + 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; + } + + emit_manual_let_else(cx, stmt.span, match_expr, pat_arm.pat, diverging_arm.body); + }, + } + } + + 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) { + 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| { + 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(()); + } + 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() +} + +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. + let mut has_bindings = false; + pat.each_binding_or_first(&mut |_, _, _, _| has_bindings = true); + if has_bindings { + 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. + // 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 +fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool { + // We support patterns with multiple bindings and tuples, like: + // let ... = if let (Some(foo), bar) = g() { (foo, bar) } else { ... } + 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; + } + } + } + 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 a10de31556c8..ef6de7d333d3 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), @@ -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/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| { 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-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 diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs new file mode 100644 index 000000000000..2ef40e5911af --- /dev/null +++ b/tests/ui/manual_let_else.rs @@ -0,0 +1,237 @@ +#![allow(unused_braces, unused_variables, dead_code)] +#![allow( + clippy::collapsible_else_if, + clippy::unused_unit, + clippy::let_unit_value, + clippy::match_single_binding, + clippy::never_loop +)] +#![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!() } + }; + + // 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 + } 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; + }; + + // 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) => { + let $n = if let Some(v) = $e { v } else { return }; + }; + } + create_binding_if_some!(w, g()); +} + +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; + } + }; + + enum Uninhabited {} + fn un() -> Uninhabited { + panic!() + } + let v = if let Some(v_some) = None { + v_some + } else { + // 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) + } + + // 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()); + + // 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!() }; +} diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr new file mode 100644 index 000000000000..453b68b8bd00 --- /dev/null +++ b/tests/ui/manual_let_else.stderr @@ -0,0 +1,263 @@ +error: this could be rewritten as `let...else` + --> $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 };` + | + = note: `-D clippy::manual-let-else` implied by `-D warnings` + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else.rs:19:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +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` + --> $DIR/manual_let_else.rs:25:5 + | +LL | / let v = if let Some(v) = g() { +LL | | // Blocks around the identity should have no impact +LL | | { +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` + --> $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: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: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:46:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +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` + --> $DIR/manual_let_else.rs:53:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +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` + --> $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 +LL | | } else if true { +LL | | return; +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` + --> $DIR/manual_let_else.rs:101:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +LL | | } else { +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` + --> $DIR/manual_let_else.rs:118:5 + | +LL | / let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) { +LL | | v_some +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` + --> $DIR/manual_let_else.rs:125:5 + | +LL | / let v = if let (Some(v_some), w_some) = (g(), 0) { +LL | | (w_some, v_some) +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` + --> $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 };` +... +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 17 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..93c86ca24fea --- /dev/null +++ b/tests/ui/manual_let_else_match.rs @@ -0,0 +1,121 @@ +#![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)] + +fn f() -> Result { + Ok(0) +} + +fn g() -> Option<()> { + None +} + +fn h() -> (Option<()>, Option<()>) { + (None, None) +} + +enum Variant { + Foo, + Bar(u32), + Baz(u32), +} + +fn build_enum() -> 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 and diverging arm + let v = match h() { + (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, + }; + } + + // There is a _ in the diverging arm + // TODO also support unused bindings aka _v + let v = match f() { + 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() { + _ => panic!(), + (None, Some(_v)) => return, + (Some(v), None) => v, + }; + + // Multiple identity arms + let v = match h() { + _ => panic!(), + (None, Some(v)) => v, + (Some(v), None) => 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 g() { + Some(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}"), + }; + + // 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 new file mode 100644 index 000000000000..38be5ac54547 --- /dev/null +++ b/tests/ui/manual_let_else_match.stderr @@ -0,0 +1,58 @@ +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` + --> $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` + --> $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` + --> $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` + --> $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` + --> $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 +