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

feat: add manual_is_variant_and lint #11865

Merged
merged 1 commit into from
Dec 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5293,6 +5293,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 }
yuxqiu marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -385,6 +385,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 @@ -51,6 +51,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 @@ -3829,6 +3830,32 @@ declare_clippy_lint! {
"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<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 @@ -3983,6 +4010,7 @@ impl_lint_pass!(Methods => [
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.
Expand Down Expand Up @@ -4664,7 +4692,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