Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suggests is_some_and over map().unwrap #11030

Merged
merged 8 commits into from
Jun 29, 2023
4 changes: 3 additions & 1 deletion clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ declare_clippy_lint! {
/// # let result: Result<usize, ()> = Ok(1);
/// # fn some_function(foo: ()) -> usize { 1 }
/// option.map(|a| a + 1).unwrap_or(0);
/// option.map(|a| a > 10).unwrap_or(false);
/// result.map(|a| a + 1).unwrap_or_else(some_function);
/// ```
///
Expand All @@ -522,6 +523,7 @@ declare_clippy_lint! {
/// # let result: Result<usize, ()> = Ok(1);
/// # fn some_function(foo: ()) -> usize { 1 }
/// option.map_or(0, |a| a + 1);
/// option.is_some_and(|a| a > 10);
/// result.map_or_else(some_function, |a| a + 1);
/// ```
#[clippy::version = "1.45.0"]
Expand Down Expand Up @@ -3904,7 +3906,7 @@ impl Methods {
manual_saturating_arithmetic::check(cx, expr, lhs, rhs, u_arg, &arith["checked_".len()..]);
},
Some(("map", m_recv, [m_arg], span, _)) => {
option_map_unwrap_or::check(cx, expr, m_recv, m_arg, recv, u_arg, span);
option_map_unwrap_or::check(cx, expr, m_recv, m_arg, recv, u_arg, span, &self.msrv);
},
Some(("then_some", t_recv, [t_arg], _, _)) => {
obfuscated_if_else::check(cx, expr, t_recv, t_arg, u_arg);
Expand Down
28 changes: 25 additions & 3 deletions clippy_lints/src/methods/option_map_unwrap_or.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::ty::is_copy;
use clippy_utils::ty::is_type_diagnostic_item;
Expand All @@ -19,6 +20,7 @@ use rustc_span::sym;
use super::MAP_UNWRAP_OR;

/// lint use of `map().unwrap_or()` for `Option`s
#[expect(clippy::too_many_arguments)]
pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
expr: &rustc_hir::Expr<'_>,
Expand All @@ -27,6 +29,7 @@ pub(super) fn check<'tcx>(
unwrap_recv: &rustc_hir::Expr<'_>,
unwrap_arg: &'tcx rustc_hir::Expr<'_>,
map_span: Span,
msrv: &Msrv,
darklyspaced marked this conversation as resolved.
Show resolved Hide resolved
) {
// lint if the caller of `map()` is an `Option`
if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Option) {
Expand Down Expand Up @@ -74,16 +77,29 @@ pub(super) fn check<'tcx>(
return;
}

// 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)
&& matches!(&unwrap_arg.kind, ExprKind::Lit(lit)
if matches!(lit.node, rustc_ast::LitKind::Bool(false)));

let mut applicability = Applicability::MachineApplicable;
// get snippet for unwrap_or()
let unwrap_snippet = snippet_with_applicability(cx, unwrap_arg.span, "..", &mut applicability);
// lint message
// comparing the snippet from source to raw text ("None") below is safe
// because we already have checked the type.
let arg = if unwrap_snippet == "None" { "None" } else { "<a>" };
let arg = if unwrap_snippet == "None" {
"None"
} else if suggest_is_some_and {
"false"
} else {
"<a>"
};
let unwrap_snippet_none = unwrap_snippet == "None";
let suggest = if unwrap_snippet_none {
"and_then(<f>)"
} else if suggest_is_some_and {
"is_some_and(<f>)"
} else {
"map_or(<a>, <f>)"
};
Expand All @@ -98,12 +114,18 @@ pub(super) fn check<'tcx>(
let mut suggestion = vec![
(
map_span,
String::from(if unwrap_snippet_none { "and_then" } else { "map_or" }),
String::from(if unwrap_snippet_none {
"and_then"
} else if suggest_is_some_and {
"is_some_and"
} else {
"map_or"
}),
),
(expr.span.with_lo(unwrap_recv.span.hi()), String::new()),
];

if !unwrap_snippet_none {
if !unwrap_snippet_none && !suggest_is_some_and {
suggestion.push((map_arg_span.with_hi(map_arg_span.lo()), format!("{unwrap_snippet}, ")));
}

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,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, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS.
/// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, OPTION_MAP_UNWRAP_OR, 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, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS.
///
/// The minimum rust version that the project supports
(msrv: Option<String> = None),
Expand Down
1 change: 1 addition & 0 deletions clippy_utils/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ macro_rules! msrv_aliases {

// names may refer to stabilized feature flags or library items
msrv_aliases! {
1,70,0 { OPTION_IS_SOME_AND }
1,68,0 { PATH_MAIN_SEPARATOR_STR }
1,65,0 { LET_ELSE, POINTER_CAST_CONSTNESS }
1,62,0 { BOOL_THEN_SOME, DEFAULT_ENUM_ATTRIBUTE }
Expand Down
18 changes: 18 additions & 0 deletions tests/ui/map_unwrap_or.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ fn option_methods() {
.unwrap_or_else(||
0
);

// Check for `map(f).unwrap_or(false)` use.
let _ = opt.map(|x| x > 5).unwrap_or(false);

}

#[rustfmt::skip]
Expand Down Expand Up @@ -95,6 +99,20 @@ fn msrv_1_41() {
let _ = res.map(|x| x + 1).unwrap_or_else(|_e| 0);
}

#[clippy::msrv = "1.69"]
fn msrv_1_69() {
let opt: Option<i32> = Some(1);

let _ = opt.map(|x| x > 5).unwrap_or(false);
}

#[clippy::msrv = "1.70"]
fn msrv_1_70() {
let opt: Option<i32> = Some(1);

let _ = opt.map(|x| x > 5).unwrap_or(false);
}

mod issue_10579 {
// Different variations of the same issue.
fn v1() {
Expand Down
44 changes: 40 additions & 4 deletions tests/ui/map_unwrap_or.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,20 @@ LL | | 0
LL | | );
| |_________^

error: called `map(<f>).unwrap_or(false)` on an `Option` value. This can be done more directly by calling `is_some_and(<f>)` instead
--> $DIR/map_unwrap_or.rs:61:13
|
LL | let _ = opt.map(|x| x > 5).unwrap_or(false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use `is_some_and(<f>)` instead
|
LL - let _ = opt.map(|x| x > 5).unwrap_or(false);
LL + let _ = opt.is_some_and(|x| x > 5);
|

error: called `map(<f>).unwrap_or_else(<g>)` on a `Result` value. This can be done more directly by calling `.map_or_else(<g>, <f>)` instead
--> $DIR/map_unwrap_or.rs:67:13
--> $DIR/map_unwrap_or.rs:71:13
|
LL | let _ = res.map(|x| {
| _____________^
Expand All @@ -137,7 +149,7 @@ LL | | ).unwrap_or_else(|_e| 0);
| |____________________________^

error: called `map(<f>).unwrap_or_else(<g>)` on a `Result` value. This can be done more directly by calling `.map_or_else(<g>, <f>)` instead
--> $DIR/map_unwrap_or.rs:71:13
--> $DIR/map_unwrap_or.rs:75:13
|
LL | let _ = res.map(|x| x + 1)
| _____________^
Expand All @@ -147,10 +159,34 @@ LL | | });
| |__________^

error: called `map(<f>).unwrap_or_else(<g>)` on a `Result` value. This can be done more directly by calling `.map_or_else(<g>, <f>)` instead
--> $DIR/map_unwrap_or.rs:95:13
--> $DIR/map_unwrap_or.rs:99:13
|
LL | let _ = res.map(|x| x + 1).unwrap_or_else(|_e| 0);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `res.map_or_else(|_e| 0, |x| x + 1)`

error: aborting due to 12 previous errors
error: called `map(<f>).unwrap_or(<a>)` on an `Option` value. This can be done more directly by calling `map_or(<a>, <f>)` instead
--> $DIR/map_unwrap_or.rs:106:13
|
LL | let _ = opt.map(|x| x > 5).unwrap_or(false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use `map_or(<a>, <f>)` instead
|
LL - let _ = opt.map(|x| x > 5).unwrap_or(false);
LL + let _ = opt.map_or(false, |x| x > 5);
|

error: called `map(<f>).unwrap_or(false)` on an `Option` value. This can be done more directly by calling `is_some_and(<f>)` instead
--> $DIR/map_unwrap_or.rs:113:13
|
LL | let _ = opt.map(|x| x > 5).unwrap_or(false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use `is_some_and(<f>)` instead
|
LL - let _ = opt.map(|x| x > 5).unwrap_or(false);
LL + let _ = opt.is_some_and(|x| x > 5);
|

error: aborting due to 15 previous errors