diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/manual_let_else.rs | 160 | ||||
| -rw-r--r-- | clippy_lints/src/utils/conf.rs | 2 | ||||
| -rw-r--r-- | clippy_utils/src/msrvs.rs | 1 | ||||
| -rw-r--r-- | src/docs/manual_let_else.txt | 20 | ||||
| -rw-r--r-- | tests/ui/manual_let_else.rs | 167 | ||||
| -rw-r--r-- | tests/ui/manual_let_else.stderr | 104 |
9 files changed, 457 insertions, 1 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index cb5a4d4a577..b5a1d194794 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 c6ae0bddc5a..2bb8dfee152 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 7837e04bca1..b261beab793 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 00000000000..8983cd7ae1c --- /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<RustcVersion>, +} + +impl ManualLetElse { + #[must_use] + pub fn new(msrv: Option<RustcVersion>) -> 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 a10de31556c..1aa86efd38f 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<String> = None), diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 8b843732a23..9780794fa99 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 00000000000..14166709f7f --- /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 00000000000..f2827e22500 --- /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 00000000000..461de79f6b9 --- /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 + |