diff options
| author | bors <bors@rust-lang.org> | 2023-12-26 15:41:40 +0000 |
|---|---|---|
| committer | bors <bors@rust-lang.org> | 2023-12-26 15:41:40 +0000 |
| commit | 9dd2252b2c78013425816e6b288977bfafb1b659 (patch) | |
| tree | 561ceedd44281b69c8060cbc2f3f142e0181c8d1 /clippy_lints/src/methods | |
| parent | 99c783843dbbdc0ff1af984666341544c962845a (diff) | |
| parent | 85e1646afcf4010b8bb74b64762f247e73912884 (diff) | |
| download | rust-9dd2252b2c78013425816e6b288977bfafb1b659.tar.gz rust-9dd2252b2c78013425816e6b288977bfafb1b659.zip | |
Auto merge of #12004 - PartiallyTyped:11843, r=xFrednet
New lints `iter_filter_is_some` and `iter_filter_is_ok` Adds a pair of lints that check for cases of an iterator over `Result` and `Option` followed by `filter` without being followed by `map` as that is covered already by a different, specialized lint. Fixes #11843 PS, I also made some minor documentations fixes in a case where a double tick (`) was included. --- changelog: New Lint: [`iter_filter_is_some`] [#12004](https://github.com/rust-lang/rust/pull/12004) changelog: New Lint: [`iter_filter_is_ok`] [#12004](https://github.com/rust-lang/rust/pull/12004)
Diffstat (limited to 'clippy_lints/src/methods')
| -rw-r--r-- | clippy_lints/src/methods/filter_map.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/methods/iter_filter.rs | 87 | ||||
| -rw-r--r-- | clippy_lints/src/methods/mod.rs | 78 |
3 files changed, 163 insertions, 3 deletions
diff --git a/clippy_lints/src/methods/filter_map.rs b/clippy_lints/src/methods/filter_map.rs index a8ca82d20e7..7cfd3d346b6 100644 --- a/clippy_lints/src/methods/filter_map.rs +++ b/clippy_lints/src/methods/filter_map.rs @@ -22,6 +22,7 @@ fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) -> hir::ExprKind::Path(QPath::Resolved(_, segments)) => { segments.segments.last().unwrap().ident.name == method_name }, + hir::ExprKind::MethodCall(segment, _, _, _) => segment.ident.name == method_name, hir::ExprKind::Closure(&hir::Closure { body, .. }) => { let body = cx.tcx.hir().body(body); let closure_expr = peel_blocks(body.value); diff --git a/clippy_lints/src/methods/iter_filter.rs b/clippy_lints/src/methods/iter_filter.rs new file mode 100644 index 00000000000..ade8e3155fa --- /dev/null +++ b/clippy_lints/src/methods/iter_filter.rs @@ -0,0 +1,87 @@ +use rustc_lint::{LateContext, LintContext}; + +use super::{ITER_FILTER_IS_OK, ITER_FILTER_IS_SOME}; + +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::{indent_of, reindent_multiline}; +use clippy_utils::{is_trait_method, peel_blocks, span_contains_comment}; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_hir::def::Res; +use rustc_hir::QPath; +use rustc_span::symbol::{sym, Symbol}; +use rustc_span::Span; +use std::borrow::Cow; + +fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) -> bool { + match &expr.kind { + hir::ExprKind::Path(QPath::TypeRelative(_, mname)) => mname.ident.name == method_name, + hir::ExprKind::Path(QPath::Resolved(_, segments)) => { + segments.segments.last().unwrap().ident.name == method_name + }, + hir::ExprKind::MethodCall(segment, _, _, _) => segment.ident.name == method_name, + hir::ExprKind::Closure(&hir::Closure { body, .. }) => { + let body = cx.tcx.hir().body(body); + let closure_expr = peel_blocks(body.value); + let arg_id = body.params[0].pat.hir_id; + match closure_expr.kind { + hir::ExprKind::MethodCall(hir::PathSegment { ident, .. }, receiver, ..) => { + if ident.name == method_name + && let hir::ExprKind::Path(path) = &receiver.kind + && let Res::Local(ref local) = cx.qpath_res(path, receiver.hir_id) + { + return arg_id == *local; + } + false + }, + _ => false, + } + }, + _ => false, + } +} + +fn parent_is_map(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { + if let hir::Node::Expr(parent_expr) = cx.tcx.hir().get_parent(expr.hir_id) { + is_method(cx, parent_expr, rustc_span::sym::map) + } else { + false + } +} + +#[allow(clippy::too_many_arguments)] +pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_arg: &hir::Expr<'_>, filter_span: Span) { + let is_iterator = is_trait_method(cx, expr, sym::Iterator); + let parent_is_not_map = !parent_is_map(cx, expr); + + if is_iterator + && parent_is_not_map + && is_method(cx, filter_arg, sym!(is_some)) + && !span_contains_comment(cx.sess().source_map(), filter_span.with_hi(expr.span.hi())) + { + span_lint_and_sugg( + cx, + ITER_FILTER_IS_SOME, + filter_span.with_hi(expr.span.hi()), + "`filter` for `is_some` on iterator over `Option`", + "consider using `flatten` instead", + reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, filter_span)).into_owned(), + Applicability::HasPlaceholders, + ); + } + if is_iterator + && parent_is_not_map + && is_method(cx, filter_arg, sym!(is_ok)) + && !span_contains_comment(cx.sess().source_map(), filter_span.with_hi(expr.span.hi())) + { + span_lint_and_sugg( + cx, + ITER_FILTER_IS_OK, + filter_span.with_hi(expr.span.hi()), + "`filter` for `is_ok` on iterator over `Result`s", + "consider using `flatten` instead", + reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, filter_span)).into_owned(), + Applicability::HasPlaceholders, + ); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index a94524b3ba6..25b1ea526e2 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -38,6 +38,7 @@ mod into_iter_on_ref; mod is_digit_ascii_radix; mod iter_cloned_collect; mod iter_count; +mod iter_filter; mod iter_kv_map; mod iter_next_slice; mod iter_nth; @@ -1175,7 +1176,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for iterators of `Option`s using ``.filter(Option::is_some).map(Option::unwrap)` that may + /// 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? @@ -3755,7 +3756,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for iterators of `Result`s using ``.filter(Result::is_ok).map(Result::unwrap)` that may + /// 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? @@ -3776,6 +3777,58 @@ declare_clippy_lint! { "filtering `Result` for `Ok` then force-unwrapping, which can be one type-safe operation" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `.filter(Option::is_some)` that may be replaced with a `.flatten()` call. + /// This lint will require additional changes to the follow-up calls as it appects the type. + /// + /// ### Why is this bad? + /// This pattern is often followed by manual unwrapping of the `Option`. The simplification + /// results in more readable and succint code without the need for manual unwrapping. + /// + /// ### Example + /// ```no_run + /// // example code where clippy issues a warning + /// vec![Some(1)].into_iter().filter(Option::is_some); + /// + /// ``` + /// Use instead: + /// ```no_run + /// // example code which does not raise clippy warning + /// vec![Some(1)].into_iter().flatten(); + /// ``` + #[clippy::version = "1.76.0"] + pub ITER_FILTER_IS_SOME, + pedantic, + "filtering an iterator over `Option`s for `Some` can be achieved with `flatten`" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `.filter(Result::is_ok)` that may be replaced with a `.flatten()` call. + /// This lint will require additional changes to the follow-up calls as it appects the type. + /// + /// ### Why is this bad? + /// This pattern is often followed by manual unwrapping of `Result`. The simplification + /// results in more readable and succint code without the need for manual unwrapping. + /// + /// ### Example + /// ```no_run + /// // example code where clippy issues a warning + /// vec![Ok::<i32, String>(1)].into_iter().filter(Result::is_ok); + /// + /// ``` + /// Use instead: + /// ```no_run + /// // example code which does not raise clippy warning + /// vec![Ok::<i32, String>(1)].into_iter().flatten(); + /// ``` + #[clippy::version = "1.76.0"] + pub ITER_FILTER_IS_OK, + pedantic, + "filtering an iterator over `Result`s for `Ok` can be achieved with `flatten`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3928,6 +3981,8 @@ impl_lint_pass!(Methods => [ JOIN_ABSOLUTE_PATHS, OPTION_MAP_OR_ERR_OK, RESULT_FILTER_MAP, + ITER_FILTER_IS_SOME, + ITER_FILTER_IS_OK, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -4257,7 +4312,24 @@ impl Methods { string_extend_chars::check(cx, expr, recv, arg); extend_with_drain::check(cx, expr, recv, arg); }, - (name @ ("filter" | "find"), [arg]) => { + ("filter", [arg]) => { + if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) { + // if `arg` has side-effect, the semantic will change + iter_overeager_cloned::check( + cx, + expr, + recv, + recv2, + iter_overeager_cloned::Op::FixClosure(name, arg), + false, + ); + } + if self.msrv.meets(msrvs::ITER_FLATTEN) { + // use the sourcemap to get the span of the closure + iter_filter::check(cx, expr, arg, span); + } + }, + ("find", [arg]) => { if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) { // if `arg` has side-effect, the semantic will change iter_overeager_cloned::check( |
