diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ac7297f8f1..e8eadd5db4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5294,6 +5294,7 @@ Released 2018-09-13 [`manual_is_ascii_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_ascii_check [`manual_is_finite`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_finite [`manual_is_infinite`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_infinite +[`manual_is_variant_and`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_variant_and [`manual_let_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_let_else [`manual_main_separator_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_main_separator_str [`manual_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_map diff --git a/clippy_config/src/msrvs.rs b/clippy_config/src/msrvs.rs index 471ad73e207..3507e106fab 100644 --- a/clippy_config/src/msrvs.rs +++ b/clippy_config/src/msrvs.rs @@ -17,7 +17,7 @@ macro_rules! msrv_aliases { // names may refer to stabilized feature flags or library items msrv_aliases! { 1,71,0 { TUPLE_ARRAY_CONVERSIONS, BUILD_HASHER_HASH_ONE } - 1,70,0 { OPTION_IS_SOME_AND, BINARY_HEAP_RETAIN } + 1,70,0 { OPTION_RESULT_IS_VARIANT_AND, BINARY_HEAP_RETAIN } 1,68,0 { PATH_MAIN_SEPARATOR_STR } 1,65,0 { LET_ELSE, POINTER_CAST_CONSTNESS } 1,62,0 { BOOL_THEN_SOME, DEFAULT_ENUM_ATTRIBUTE } diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 9dd22135d95..5963c4c77d0 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -385,6 +385,7 @@ crate::methods::JOIN_ABSOLUTE_PATHS_INFO, crate::methods::MANUAL_FILTER_MAP_INFO, crate::methods::MANUAL_FIND_MAP_INFO, + crate::methods::MANUAL_IS_VARIANT_AND_INFO, crate::methods::MANUAL_NEXT_BACK_INFO, crate::methods::MANUAL_OK_OR_INFO, crate::methods::MANUAL_SATURATING_ARITHMETIC_INFO, diff --git a/clippy_lints/src/lines_filter_map_ok.rs b/clippy_lints/src/lines_filter_map_ok.rs index 8a0955147bb..29957e423b0 100644 --- a/clippy_lints/src/lines_filter_map_ok.rs +++ b/clippy_lints/src/lines_filter_map_ok.rs @@ -96,8 +96,7 @@ fn should_lint(cx: &LateContext<'_>, args: &[Expr<'_>], method_str: &str) -> boo ExprKind::Path(qpath) => cx .qpath_res(qpath, fm_arg.hir_id) .opt_def_id() - .map(|did| match_def_path(cx, did, &paths::CORE_RESULT_OK_METHOD)) - .unwrap_or_default(), + .is_some_and(|did| match_def_path(cx, did, &paths::CORE_RESULT_OK_METHOD)), // Detect `|x| x.ok()` ExprKind::Closure(Closure { body, .. }) => { if let Body { diff --git a/clippy_lints/src/methods/manual_is_variant_and.rs b/clippy_lints/src/methods/manual_is_variant_and.rs new file mode 100644 index 00000000000..d29acd4622a --- /dev/null +++ b/clippy_lints/src/methods/manual_is_variant_and.rs @@ -0,0 +1,59 @@ +use clippy_config::msrvs::{Msrv, OPTION_RESULT_IS_VARIANT_AND}; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_errors::Applicability; +use rustc_lint::LateContext; +use rustc_span::{sym, Span}; + +use super::MANUAL_IS_VARIANT_AND; + +pub(super) fn check<'tcx>( + cx: &LateContext<'_>, + expr: &'tcx rustc_hir::Expr<'_>, + map_recv: &'tcx rustc_hir::Expr<'_>, + map_arg: &'tcx rustc_hir::Expr<'_>, + map_span: Span, + msrv: &Msrv, +) { + // Don't lint if: + + // 1. the `expr` is generated by a macro + if expr.span.from_expansion() { + return; + } + + // 2. the caller of `map()` is neither `Option` nor `Result` + let is_option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(map_recv), sym::Option); + let is_result = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(map_recv), sym::Result); + if !is_option && !is_result { + return; + } + + // 3. the caller of `unwrap_or_default` is neither `Option` nor `Result` + if !cx.typeck_results().expr_ty(expr).is_bool() { + return; + } + + // 4. msrv doesn't meet `OPTION_RESULT_IS_VARIANT_AND` + if !msrv.meets(OPTION_RESULT_IS_VARIANT_AND) { + return; + } + + let lint_msg = if is_option { + "called `map().unwrap_or_default()` on an `Option` value" + } else { + "called `map().unwrap_or_default()` on a `Result` value" + }; + let suggestion = if is_option { "is_some_and" } else { "is_ok_and" }; + + span_lint_and_sugg( + cx, + MANUAL_IS_VARIANT_AND, + expr.span.with_lo(map_span.lo()), + lint_msg, + "use", + format!("{}({})", suggestion, snippet(cx, map_arg.span, "..")), + Applicability::MachineApplicable, + ); +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 25b1ea526e2..0cde17ef5ad 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -51,6 +51,7 @@ mod iter_with_drain; mod iterator_step_by_zero; mod join_absolute_paths; +mod manual_is_variant_and; mod manual_next_back; mod manual_ok_or; mod manual_saturating_arithmetic; @@ -3829,6 +3830,32 @@ "filtering an iterator over `Result`s for `Ok` can be achieved with `flatten`" } +declare_clippy_lint! { + /// Checks for usage of `option.map(f).unwrap_or_default()` and `result.map(f).unwrap_or_default()` where f is a function or closure that returns the `bool` type. + /// + /// ### Why is this bad? + /// Readability. These can be written more concisely as `option.is_some_and(f)` and `result.is_ok_and(f)`. + /// + /// ### Example + /// ```no_run + /// # let option = Some(1); + /// # let result: Result = Ok(1); + /// option.map(|a| a > 10).unwrap_or_default(); + /// result.map(|a| a > 10).unwrap_or_default(); + /// ``` + /// Use instead: + /// ```no_run + /// # let option = Some(1); + /// # let result: Result = Ok(1); + /// option.is_some_and(|a| a > 10); + /// result.is_ok_and(|a| a > 10); + /// ``` + #[clippy::version = "1.76.0"] + pub MANUAL_IS_VARIANT_AND, + pedantic, + "using `.map(f).unwrap_or_default()`, which is more succinctly expressed as `is_some_and(f)` or `is_ok_and(f)`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3983,6 +4010,7 @@ pub fn new( RESULT_FILTER_MAP, ITER_FILTER_IS_SOME, ITER_FILTER_IS_OK, + MANUAL_IS_VARIANT_AND, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -4664,7 +4692,13 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { } unnecessary_literal_unwrap::check(cx, expr, recv, name, args); }, - ("unwrap_or_default" | "unwrap_unchecked" | "unwrap_err_unchecked", []) => { + ("unwrap_or_default", []) => { + if let Some(("map", m_recv, [arg], span, _)) = method_call(recv) { + manual_is_variant_and::check(cx, expr, m_recv, arg, span, &self.msrv); + } + unnecessary_literal_unwrap::check(cx, expr, recv, name, args); + }, + ("unwrap_unchecked" | "unwrap_err_unchecked", []) => { unnecessary_literal_unwrap::check(cx, expr, recv, name, args); }, ("unwrap_or_else", [u_arg]) => { diff --git a/clippy_lints/src/methods/option_map_unwrap_or.rs b/clippy_lints/src/methods/option_map_unwrap_or.rs index 63e64a5b35d..35deabe49b6 100644 --- a/clippy_lints/src/methods/option_map_unwrap_or.rs +++ b/clippy_lints/src/methods/option_map_unwrap_or.rs @@ -72,7 +72,7 @@ pub(super) fn check<'tcx>( } // is_some_and is stabilised && `unwrap_or` argument is false; suggest `is_some_and` instead - let suggest_is_some_and = msrv.meets(msrvs::OPTION_IS_SOME_AND) + let suggest_is_some_and = msrv.meets(msrvs::OPTION_RESULT_IS_VARIANT_AND) && matches!(&unwrap_arg.kind, ExprKind::Lit(lit) if matches!(lit.node, rustc_ast::LitKind::Bool(false))); diff --git a/clippy_lints/src/methods/path_ends_with_ext.rs b/clippy_lints/src/methods/path_ends_with_ext.rs index 094ead9f4ad..29f44ec2a4d 100644 --- a/clippy_lints/src/methods/path_ends_with_ext.rs +++ b/clippy_lints/src/methods/path_ends_with_ext.rs @@ -33,7 +33,7 @@ pub(super) fn check( && path.chars().all(char::is_alphanumeric) { let mut sugg = snippet(cx, recv.span, "..").into_owned(); - if msrv.meets(msrvs::OPTION_IS_SOME_AND) { + if msrv.meets(msrvs::OPTION_RESULT_IS_VARIANT_AND) { let _ = write!(sugg, r#".extension().is_some_and(|ext| ext == "{path}")"#); } else { let _ = write!(sugg, r#".extension().map_or(false, |ext| ext == "{path}")"#); diff --git a/tests/ui/manual_is_variant_and.fixed b/tests/ui/manual_is_variant_and.fixed new file mode 100644 index 00000000000..8c34b51103c --- /dev/null +++ b/tests/ui/manual_is_variant_and.fixed @@ -0,0 +1,51 @@ +//@aux-build:option_helpers.rs +#![warn(clippy::manual_is_variant_and)] + +#[macro_use] +extern crate option_helpers; + +#[rustfmt::skip] +fn option_methods() { + let opt = Some(1); + + // Check for `option.map(_).unwrap_or_default()` use. + // Single line case. + let _ = opt.is_some_and(|x| x > 1); + // Multi-line cases. + let _ = opt.is_some_and(|x| { + x > 1 + }); + let _ = opt.is_some_and(|x| x > 1); + let _ = opt + .is_some_and(|x| x > 1); + + // won't fix because the return type of the closure is not `bool` + let _ = opt.map(|x| x + 1).unwrap_or_default(); + + let opt2 = Some('a'); + let _ = opt2.is_some_and(char::is_alphanumeric); // should lint + let _ = opt_map!(opt2, |x| x == 'a').unwrap_or_default(); // should not lint +} + +#[rustfmt::skip] +fn result_methods() { + let res: Result = Ok(1); + + // multi line cases + let _ = res.is_ok_and(|x| { + x > 1 + }); + let _ = res.is_ok_and(|x| x > 1); + + // won't fix because the return type of the closure is not `bool` + let _ = res.map(|x| x + 1).unwrap_or_default(); + + let res2: Result = Ok('a'); + let _ = res2.is_ok_and(char::is_alphanumeric); // should lint + let _ = opt_map!(res2, |x| x == 'a').unwrap_or_default(); // should not lint +} + +fn main() { + option_methods(); + result_methods(); +} diff --git a/tests/ui/manual_is_variant_and.rs b/tests/ui/manual_is_variant_and.rs new file mode 100644 index 00000000000..25b2489d942 --- /dev/null +++ b/tests/ui/manual_is_variant_and.rs @@ -0,0 +1,57 @@ +//@aux-build:option_helpers.rs +#![warn(clippy::manual_is_variant_and)] + +#[macro_use] +extern crate option_helpers; + +#[rustfmt::skip] +fn option_methods() { + let opt = Some(1); + + // Check for `option.map(_).unwrap_or_default()` use. + // Single line case. + let _ = opt.map(|x| x > 1) + // Should lint even though this call is on a separate line. + .unwrap_or_default(); + // Multi-line cases. + let _ = opt.map(|x| { + x > 1 + } + ).unwrap_or_default(); + let _ = opt.map(|x| x > 1).unwrap_or_default(); + let _ = opt + .map(|x| x > 1) + .unwrap_or_default(); + + // won't fix because the return type of the closure is not `bool` + let _ = opt.map(|x| x + 1).unwrap_or_default(); + + let opt2 = Some('a'); + let _ = opt2.map(char::is_alphanumeric).unwrap_or_default(); // should lint + let _ = opt_map!(opt2, |x| x == 'a').unwrap_or_default(); // should not lint +} + +#[rustfmt::skip] +fn result_methods() { + let res: Result = Ok(1); + + // multi line cases + let _ = res.map(|x| { + x > 1 + } + ).unwrap_or_default(); + let _ = res.map(|x| x > 1) + .unwrap_or_default(); + + // won't fix because the return type of the closure is not `bool` + let _ = res.map(|x| x + 1).unwrap_or_default(); + + let res2: Result = Ok('a'); + let _ = res2.map(char::is_alphanumeric).unwrap_or_default(); // should lint + let _ = opt_map!(res2, |x| x == 'a').unwrap_or_default(); // should not lint +} + +fn main() { + option_methods(); + result_methods(); +} diff --git a/tests/ui/manual_is_variant_and.stderr b/tests/ui/manual_is_variant_and.stderr new file mode 100644 index 00000000000..c243de098dc --- /dev/null +++ b/tests/ui/manual_is_variant_and.stderr @@ -0,0 +1,82 @@ +error: called `map().unwrap_or_default()` on an `Option` value + --> $DIR/manual_is_variant_and.rs:13:17 + | +LL | let _ = opt.map(|x| x > 1) + | _________________^ +LL | | // Should lint even though this call is on a separate line. +LL | | .unwrap_or_default(); + | |____________________________^ help: use: `is_some_and(|x| x > 1)` + | + = note: `-D clippy::manual-is-variant-and` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_is_variant_and)]` + +error: called `map().unwrap_or_default()` on an `Option` value + --> $DIR/manual_is_variant_and.rs:17:17 + | +LL | let _ = opt.map(|x| { + | _________________^ +LL | | x > 1 +LL | | } +LL | | ).unwrap_or_default(); + | |_________________________^ + | +help: use + | +LL ~ let _ = opt.is_some_and(|x| { +LL + x > 1 +LL ~ }); + | + +error: called `map().unwrap_or_default()` on an `Option` value + --> $DIR/manual_is_variant_and.rs:21:17 + | +LL | let _ = opt.map(|x| x > 1).unwrap_or_default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `is_some_and(|x| x > 1)` + +error: called `map().unwrap_or_default()` on an `Option` value + --> $DIR/manual_is_variant_and.rs:23:10 + | +LL | .map(|x| x > 1) + | __________^ +LL | | .unwrap_or_default(); + | |____________________________^ help: use: `is_some_and(|x| x > 1)` + +error: called `map().unwrap_or_default()` on an `Option` value + --> $DIR/manual_is_variant_and.rs:30:18 + | +LL | let _ = opt2.map(char::is_alphanumeric).unwrap_or_default(); // should lint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `is_some_and(char::is_alphanumeric)` + +error: called `map().unwrap_or_default()` on a `Result` value + --> $DIR/manual_is_variant_and.rs:39:17 + | +LL | let _ = res.map(|x| { + | _________________^ +LL | | x > 1 +LL | | } +LL | | ).unwrap_or_default(); + | |_________________________^ + | +help: use + | +LL ~ let _ = res.is_ok_and(|x| { +LL + x > 1 +LL ~ }); + | + +error: called `map().unwrap_or_default()` on a `Result` value + --> $DIR/manual_is_variant_and.rs:43:17 + | +LL | let _ = res.map(|x| x > 1) + | _________________^ +LL | | .unwrap_or_default(); + | |____________________________^ help: use: `is_ok_and(|x| x > 1)` + +error: called `map().unwrap_or_default()` on a `Result` value + --> $DIR/manual_is_variant_and.rs:50:18 + | +LL | let _ = res2.map(char::is_alphanumeric).unwrap_or_default(); // should lint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `is_ok_and(char::is_alphanumeric)` + +error: aborting due to 8 previous errors +