Skip to content

Commit

Permalink
feat: add manual_is_variant_and lint
Browse files Browse the repository at this point in the history
  • Loading branch information
yuxqiu committed Dec 26, 2023
1 parent f9b5def commit 0c95e6c
Show file tree
Hide file tree
Showing 11 changed files with 290 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5234,6 +5234,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
Expand Down
2 changes: 1 addition & 1 deletion clippy_config/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
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,
Expand Down
3 changes: 1 addition & 2 deletions clippy_lints/src/lines_filter_map_ok.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
59 changes: 59 additions & 0 deletions clippy_lints/src/methods/manual_is_variant_and.rs
Original file line number Diff line number Diff line change
@@ -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<bool>` nor `Result<bool, _>`
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(<f>).unwrap_or_default()` on an `Option` value"
} else {
"called `map(<f>).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,
);
}
36 changes: 35 additions & 1 deletion clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ mod iter_skip_zero;
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;
Expand Down Expand Up @@ -3776,6 +3777,32 @@ declare_clippy_lint! {
"filtering `Result` for `Ok` then force-unwrapping, which can be one type-safe operation"
}

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<usize, ()> = 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<usize, ()> = 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,
Expand Down Expand Up @@ -3928,6 +3955,7 @@ impl_lint_pass!(Methods => [
JOIN_ABSOLUTE_PATHS,
OPTION_MAP_OR_ERR_OK,
RESULT_FILTER_MAP,
MANUAL_IS_VARIANT_AND,
]);

/// Extracts a method call name, args, and `Span` of the method name.
Expand Down Expand Up @@ -4592,7 +4620,13 @@ impl Methods {
}
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]) => {
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/methods/option_map_unwrap_or.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)));

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/methods/path_ends_with_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}")"#);
Expand Down
51 changes: 51 additions & 0 deletions tests/ui/manual_is_variant_and.fixed
Original file line number Diff line number Diff line change
@@ -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<i32, ()> = 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<char, ()> = 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();
}
57 changes: 57 additions & 0 deletions tests/ui/manual_is_variant_and.rs
Original file line number Diff line number Diff line change
@@ -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<i32, ()> = 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<char, ()> = 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();
}
82 changes: 82 additions & 0 deletions tests/ui/manual_is_variant_and.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
error: called `map(<f>).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(<f>).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(<f>).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(<f>).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(<f>).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(<f>).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(<f>).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(<f>).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

0 comments on commit 0c95e6c

Please sign in to comment.