mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

New Lint: Result_filter_map

Browse Source 
Added the `Result` mirror of `option_filter_map` to catch

```
   .into_iter().filter(Result::is_ok).map(Result::unwrap)
```

changelog: New Lint: [`result_filter_map`]
Co-authored-by: Alex Macleod <alex@macleod.io>
This commit is contained in:
Quinn Sinclair 2023-12-04 02:20:47 +01:00 committed by PartiallyTyped
parent 29bdc8b2bc
commit 8892420aa7
10 changed files with 217 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -5470,6 +5470,7 @@ Released 2018-09-13
[`reserve_after_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#reserve_after_initialization
[`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs
[`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used
[`result_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_filter_map
[`result_large_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
[`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option
[`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn

1
clippy_lints/src/declared_lints.rs
View File

@ -419,6 +419,7 @@
crate::methods::READ_LINE_WITHOUT_TRIM_INFO,
crate::methods::REDUNDANT_AS_STR_INFO,
crate::methods::REPEAT_ONCE_INFO,
crate::methods::RESULT_FILTER_MAP_INFO,
crate::methods::RESULT_MAP_OR_INTO_OPTION_INFO,
crate::methods::SEARCH_IS_SOME_INFO,
crate::methods::SEEK_FROM_CURRENT_INFO,

84
clippy_lints/src/methods/filter_map.rs
View File

@ -14,7 +14,7 @@
use rustc_span::Span;
use std::borrow::Cow;
use super::{MANUAL_FILTER_MAP, MANUAL_FIND_MAP, OPTION_FILTER_MAP};
use super::{MANUAL_FILTER_MAP, MANUAL_FIND_MAP, OPTION_FILTER_MAP, RESULT_FILTER_MAP};
fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) -> bool {
match &expr.kind {
@ -46,6 +46,9 @@ fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) ->
fn is_option_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) -> bool {
is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some))
}
fn is_ok_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) -> bool {
is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_ok))
}
#[derive(Debug, Copy, Clone)]
enum OffendingFilterExpr<'tcx> {
@ -273,6 +276,18 @@ fn is_filter_some_map_unwrap(
(iterator || option) && is_option_filter_map(cx, filter_arg, map_arg)
}
/// is `filter(|x| x.is_ok()).map(|x| x.unwrap())`
fn is_filter_ok_map_unwrap(
cx: &LateContext<'_>,
expr: &hir::Expr<'_>,
filter_arg: &hir::Expr<'_>,
map_arg: &hir::Expr<'_>,
) -> bool {
// result has no filter, so we only check for iterators
let iterator = is_trait_method(cx, expr, sym::Iterator);
iterator && is_ok_filter_map(cx, filter_arg, map_arg)
}
/// lint use of `filter().map()` or `find().map()` for `Iterators`
#[allow(clippy::too_many_arguments)]
pub(super) fn check(
@ -300,30 +315,21 @@ pub(super) fn check(
return;
}
if is_trait_method(cx, map_recv, sym::Iterator)
if is_filter_ok_map_unwrap(cx, expr, filter_arg, map_arg) {
span_lint_and_sugg(
cx,
RESULT_FILTER_MAP,
filter_span.with_hi(expr.span.hi()),
"`filter` for `Ok` followed by `unwrap`",
"consider using `flatten` instead",
reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, map_span)).into_owned(),
Applicability::MachineApplicable,
);
// filter(|x| ...is_some())...
&& let ExprKind::Closure(&Closure { body: filter_body_id, .. }) = filter_arg.kind
&& let filter_body = cx.tcx.hir().body(filter_body_id)
&& let [filter_param] = filter_body.params
// optional ref pattern: `filter(|&x| ..)`
&& let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind {
(ref_pat, true)
} else {
(filter_param.pat, false)
return;
}
&& let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind
&& let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id)
&& let ExprKind::Closure(&Closure { body: map_body_id, .. }) = map_arg.kind
&& let map_body = cx.tcx.hir().body(map_body_id)
&& let [map_param] = map_body.params
&& let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind
&& let Some(check_result) =
offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref)
{
if let Some((map_param_ident, check_result)) = is_find_or_filter(cx, map_recv, filter_arg, map_arg) {
let span = filter_span.with_hi(expr.span.hi());
let (filter_name, lint) = if is_find {
("find", MANUAL_FIND_MAP)
@ -395,6 +401,40 @@ pub(super) fn check(
}
}
fn is_find_or_filter<'a>(
cx: &LateContext<'a>,
map_recv: &hir::Expr<'_>,
filter_arg: &hir::Expr<'_>,
map_arg: &hir::Expr<'_>,
) -> Option<(Ident, CheckResult<'a>)> {
if is_trait_method(cx, map_recv, sym::Iterator)
// filter(|x| ...is_some())...
&& let ExprKind::Closure(&Closure { body: filter_body_id, .. }) = filter_arg.kind
&& let filter_body = cx.tcx.hir().body(filter_body_id)
&& let [filter_param] = filter_body.params
// optional ref pattern: `filter(|&x| ..)`
&& let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind {
(ref_pat, true)
} else {
(filter_param.pat, false)
}
&& let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind
&& let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id)
&& let ExprKind::Closure(&Closure { body: map_body_id, .. }) = map_arg.kind
&& let map_body = cx.tcx.hir().body(map_body_id)
&& let [map_param] = map_body.params
&& let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind
&& let Some(check_result) =
offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref)
{
return Some((map_param_ident, check_result));
}
None
}
fn acceptable_methods(method: &PathSegment<'_>) -> bool {
let methods: [Symbol; 8] = [
sym::clone,

27
clippy_lints/src/methods/mod.rs
View File

@ -1175,7 +1175,8 @@
declare_clippy_lint! {
/// ### What it does
/// Checks for indirect collection of populated `Option`
/// Checks for iterators of `Option`s using ``.filter(Option::is_some).map(Option::unwrap)` that may
/// be replaced with a `.flatten()` call.
///
/// ### Why is this bad?
/// `Option` is like a collection of 0-1 things, so `flatten`
@ -3752,6 +3753,29 @@
"using `Option.map_or(Err(_), Ok)`, which is more succinctly expressed as `Option.ok_or(_)`"
}
declare_clippy_lint! {
/// ### What it does
/// Checks for iterators of `Result`s using ``.filter(Result::is_ok).map(Result::unwrap)` that may
/// be replaced with a `.flatten()` call.
///
/// ### Why is this bad?
/// `Result` implements `IntoIterator<Item = T>`. This means that `Result` can be flattened
/// automatically without suspicious-looking `unwrap` calls.
///
/// ### Example
/// ```no_run
/// let _ = std::iter::empty::<Result<i32, ()>>().filter(Result::is_ok).map(Result::unwrap);
/// ```
/// Use instead:
/// ```no_run
/// let _ = std::iter::empty::<Result<i32, ()>>().flatten();
/// ```
#[clippy::version = "1.76.0"]
pub RESULT_FILTER_MAP,
complexity,
"filtering `Result` for `Ok` then force-unwrapping, which can be one type-safe operation"
}
pub struct Methods {
avoid_breaking_exported_api: bool,
msrv: Msrv,
@ -3903,6 +3927,7 @@ pub fn new(
UNNECESSARY_FALLIBLE_CONVERSIONS,
JOIN_ABSOLUTE_PATHS,
OPTION_MAP_OR_ERR_OK,
RESULT_FILTER_MAP,
]);
/// Extracts a method call name, args, and `Span` of the method name.

6
tests/ui/option_filter_map.fixed
View File

@ -3,12 +3,18 @@
fn main() {
let _ = Some(Some(1)).flatten();
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = Some(Some(1)).flatten();
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = Some(1).map(odds_out).flatten();
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = Some(1).map(odds_out).flatten();
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = vec![Some(1)].into_iter().flatten();
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = vec![Some(1)].into_iter().flatten();
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = vec![1]
.into_iter()
.map(odds_out)

8
tests/ui/option_filter_map.rs
View File

@ -3,21 +3,29 @@
fn main() {
let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap);
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap());
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = Some(1).map(odds_out).filter(Option::is_some).map(Option::unwrap);
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = Some(1).map(odds_out).filter(|o| o.is_some()).map(|o| o.unwrap());
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = vec![Some(1)].into_iter().filter(Option::is_some).map(Option::unwrap);
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap());
//~^ ERROR: `filter` for `Some` followed by `unwrap`
let _ = vec![1]
.into_iter()
.map(odds_out)
.filter(Option::is_some)
//~^ ERROR: `filter` for `Some` followed by `unwrap`
.map(Option::unwrap);
let _ = vec![1]
.into_iter()
.map(odds_out)
.filter(|o| o.is_some())
//~^ ERROR: `filter` for `Some` followed by `unwrap`
.map(|o| o.unwrap());
}

16
tests/ui/option_filter_map.stderr
View File

@ -8,48 +8,50 @@ LL | let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap);
= help: to override `-D warnings` add `#[allow(clippy::option_filter_map)]`
error: `filter` for `Some` followed by `unwrap`
--> $DIR/option_filter_map.rs:6:27
--> $DIR/option_filter_map.rs:7:27
|
LL | let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `Some` followed by `unwrap`
--> $DIR/option_filter_map.rs:7:35
--> $DIR/option_filter_map.rs:9:35
|
LL | let _ = Some(1).map(odds_out).filter(Option::is_some).map(Option::unwrap);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `Some` followed by `unwrap`
--> $DIR/option_filter_map.rs:8:35
--> $DIR/option_filter_map.rs:11:35
|
LL | let _ = Some(1).map(odds_out).filter(|o| o.is_some()).map(|o| o.unwrap());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `Some` followed by `unwrap`
--> $DIR/option_filter_map.rs:10:39
--> $DIR/option_filter_map.rs:14:39
|
LL | let _ = vec![Some(1)].into_iter().filter(Option::is_some).map(Option::unwrap);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `Some` followed by `unwrap`
--> $DIR/option_filter_map.rs:11:39
--> $DIR/option_filter_map.rs:16:39
|
LL | let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `Some` followed by `unwrap`
--> $DIR/option_filter_map.rs:15:10
--> $DIR/option_filter_map.rs:21:10
|
LL | .filter(Option::is_some)
| __________^
LL | |
LL | | .map(Option::unwrap);
| |____________________________^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `Some` followed by `unwrap`
--> $DIR/option_filter_map.rs:20:10
--> $DIR/option_filter_map.rs:27:10
|
LL | .filter(|o| o.is_some())
| __________^
LL | |
LL | | .map(|o| o.unwrap());
| |____________________________^ help: consider using `flatten` instead: `flatten()`

27
tests/ui/result_filter_map.fixed Normal file
View File

@ -0,0 +1,27 @@
#![warn(clippy::result_filter_map)]
#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)]
fn odds_out(x: i32) -> Result<i32, ()> {
if x % 2 == 0 { Ok(x) } else { Err(()) }
}
fn main() {
// Unlike the Option version, Result does not have a filter operation => we will check for iterators
// only
let _ = vec![Ok(1) as Result<i32, ()>]
.into_iter()
.flatten();
let _ = vec![Ok(1) as Result<i32, ()>]
.into_iter()
.flatten();
let _ = vec![1]
.into_iter()
.map(odds_out)
.flatten();
let _ = vec![1]
.into_iter()
.map(odds_out)
.flatten();
}

35
tests/ui/result_filter_map.rs Normal file
View File

@ -0,0 +1,35 @@
#![warn(clippy::result_filter_map)]
#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)]
fn odds_out(x: i32) -> Result<i32, ()> {
if x % 2 == 0 { Ok(x) } else { Err(()) }
}
fn main() {
// Unlike the Option version, Result does not have a filter operation => we will check for iterators
// only
let _ = vec![Ok(1) as Result<i32, ()>]
.into_iter()
.filter(Result::is_ok)
//~^ ERROR: `filter` for `Ok` followed by `unwrap`
.map(Result::unwrap);
let _ = vec![Ok(1) as Result<i32, ()>]
.into_iter()
.filter(|o| o.is_ok())
//~^ ERROR: `filter` for `Ok` followed by `unwrap`
.map(|o| o.unwrap());
let _ = vec![1]
.into_iter()
.map(odds_out)
.filter(Result::is_ok)
//~^ ERROR: `filter` for `Ok` followed by `unwrap`
.map(Result::unwrap);
let _ = vec![1]
.into_iter()
.map(odds_out)
.filter(|o| o.is_ok())
//~^ ERROR: `filter` for `Ok` followed by `unwrap`
.map(|o| o.unwrap());
}

41
tests/ui/result_filter_map.stderr Normal file
View File

@ -0,0 +1,41 @@
error: `filter` for `Ok` followed by `unwrap`
--> $DIR/result_filter_map.rs:13:10
|
LL | .filter(Result::is_ok)
| __________^
LL | |
LL | | .map(Result::unwrap);
| |____________________________^ help: consider using `flatten` instead: `flatten()`
|
= note: `-D clippy::result-filter-map` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::result_filter_map)]`
error: `filter` for `Ok` followed by `unwrap`
--> $DIR/result_filter_map.rs:19:10
|
LL | .filter(|o| o.is_ok())
| __________^
LL | |
LL | | .map(|o| o.unwrap());
| |____________________________^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `Ok` followed by `unwrap`
--> $DIR/result_filter_map.rs:26:10
|
LL | .filter(Result::is_ok)
| __________^
LL | |
LL | | .map(Result::unwrap);
| |____________________________^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `Ok` followed by `unwrap`
--> $DIR/result_filter_map.rs:32:10
|
LL | .filter(|o| o.is_ok())
| __________^
LL | |
LL | | .map(|o| o.unwrap());
| |____________________________^ help: consider using `flatten` instead: `flatten()`
error: aborting due to 4 previous errors