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

Auto merge of #12080 - PartiallyTyped:12058, r=xFrednet

Browse Source 
Fixed ICE introduced in #12004

Issue: in https://github.com/rust-lang/rust-clippy/pull/12004, we emit a lint for `filter(Option::is_some)`. If the
parent expression is a `.map` we don't emit that lint as there exists a
more specialized lint for that.

The ICE introduced in https://github.com/rust-lang/rust-clippy/pull/12004 is a consequence of the assumption that a
parent expression after a filter would be a method call with the filter
call being the receiver. However, it is entirely possible to have a
closure of the form

```
|| { vec![Some(1), None].into_iter().filter(Option::is_some) }
```
The previous implementation looked at the parent expression; namely the
closure, and tried to check the parameters by indexing [0] on an empty
list.

This commit is an overhaul of the lint with significantly more FP tests
and checks.

Impl details:

1. We verify that the filter method we are in is a proper trait method
   to avoid FPs.
2. We check that the parent expression is not a map by checking whether
   it exists; if is a trait method; and then a method call.
3. We check that we don't have comments in the span.
4. We verify that we are in an Iterator of Option and Result.
5. We check the contents of the filter.
   1. For closures we peel it. If it is not a single expression, we don't
     lint. We then try again by checking the peeled expression.
   2. For paths, we do a typecheck to avoid FPs for types that impl
     functions with the same names.
   3. For calls, we verify the type, via the path, and that the param of
     the closure is the single argument to the call.
   4. For method calls we verify that the receiver is the parameter of
     the closure. Since we handle single, non-block exprs, the
     parameter can't be shadowed, so no FP.

This commit also adds additional FP tests.

Fixes: #12058

Adding `@xFrednet` as you've the most context for this as you reviewed it last time.

`@rustbot` r? `@xFrednet`

---

changelog: none
(Will be backported and therefore don't effect stable)
This commit is contained in:
bors 2024-01-07 17:21:28 +00:00
commit 7fbaba856b
8 changed files with 1093 additions and 102 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -26,13 +26,14 @@ fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) ->
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)
&& !body.params.is_empty()
{
let arg_id = body.params[0].pat.hir_id;
return arg_id == *local;
}
false

208
clippy_lints/src/methods/iter_filter.rs
View File

@ -1,80 +1,181 @@
use clippy_utils::ty::get_iterator_item_ty;
use hir::ExprKind;
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 clippy_utils::{get_parent_expr, 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::symbol::{sym, Ident, 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, ..) => {
///
/// Returns true if the expression is a method call to `method_name`
/// e.g. `a.method_name()` or `Option::method_name`.
///
/// The type-checker verifies for us that the method accepts the right kind of items
/// (e.g. `Option::is_some` accepts `Option<_>`), so we don't need to check that.
///
/// How to capture each case:
///
/// `.filter(|a| { std::option::Option::is_some(a) })`
/// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ <- this is a closure, getting unwrapped and
/// recursively checked.
/// `std::option::Option::is_some(a)`
/// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ <- this is a call. It unwraps to a path with
/// `QPath::TypeRelative`. Since this is a type relative path, we need to check the method name, the
/// type, and that the parameter of the closure is passed in the call. This part is the dual of
/// `receiver.method_name()` below.
///
/// `filter(std::option::Option::is_some);`
/// ^^^^^^^^^^^^^^^^^^^^^^^^^^^ <- this is a type relative path, like above, we check the
/// type and the method name.
///
/// `filter(|a| a.is_some());`
/// ^^^^^^^^^^^^^^^ <- this is a method call inside a closure,
/// we check that the parameter of the closure is the receiver of the method call and don't allow
/// any other parameters.
fn is_method(
cx: &LateContext<'_>,
expr: &hir::Expr<'_>,
type_symbol: Symbol,
method_name: Symbol,
params: &[&hir::Pat<'_>],
) -> bool {
fn pat_is_recv(ident: Ident, param: &hir::Pat<'_>) -> bool {
match param.kind {
hir::PatKind::Binding(_, _, other, _) => ident == other,
hir::PatKind::Ref(pat, _) => pat_is_recv(ident, pat),
_ => false,
}
}
match expr.kind {
hir::ExprKind::MethodCall(hir::PathSegment { ident, .. }, recv, ..) => {
// compare the identifier of the receiver to the parameter
// we are in a filter => closure has a single parameter and a single, non-block
// expression, this means that the parameter shadows all outside variables with
// the same name => avoid FPs. If the parameter is not the receiver, then this hits
// outside variables => avoid FP
if ident.name == method_name
&& let hir::ExprKind::Path(path) = &receiver.kind
&& let Res::Local(ref local) = cx.qpath_res(path, receiver.hir_id)
&& let ExprKind::Path(QPath::Resolved(None, path)) = recv.kind
&& let &[seg] = path.segments
&& params.iter().any(|p| pat_is_recv(seg.ident, p))
{
return arg_id == *local;
return true;
}
false
},
_ => false,
// This is used to check for complete paths via `|a| std::option::Option::is_some(a)`
// this then unwraps to a path with `QPath::TypeRelative`
// we pass the params as they've been passed to the current call through the closure
hir::ExprKind::Call(expr, [param]) => {
// this will hit the `QPath::TypeRelative` case and check that the method name is correct
if is_method(cx, expr, type_symbol, method_name, params)
// we then check that this is indeed passing the parameter of the closure
&& let ExprKind::Path(QPath::Resolved(None, path)) = param.kind
&& let &[seg] = path.segments
&& params.iter().any(|p| pat_is_recv(seg.ident, p))
{
return true;
}
false
},
hir::ExprKind::Path(QPath::TypeRelative(ty, mname)) => {
let ty = cx.typeck_results().node_type(ty.hir_id);
if let Some(did) = cx.tcx.get_diagnostic_item(type_symbol)
&& ty.ty_adt_def() == cx.tcx.type_of(did).skip_binder().ty_adt_def()
{
return mname.ident.name == method_name;
}
false
},
hir::ExprKind::Closure(&hir::Closure { body, .. }) => {
let body = cx.tcx.hir().body(body);
let closure_expr = peel_blocks(body.value);
let params = body.params.iter().map(|param| param.pat).collect::<Vec<_>>();
is_method(cx, closure_expr, type_symbol, method_name, params.as_slice())
},
_ => 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 {
if let Some(expr) = get_parent_expr(cx, expr)
&& is_trait_method(cx, expr, sym::Iterator)
&& let hir::ExprKind::MethodCall(path, _, _, _) = expr.kind
&& path.ident.name == rustc_span::sym::map
{
return true;
}
false
}
enum FilterType {
IsSome,
IsOk,
}
/// Returns the `FilterType` of the expression if it is a filter over an Iter<Option> or
/// Iter<Result> with the parent expression not being a map, and not having a comment in the span of
/// the filter. If it is not a filter over an Iter<Option> or Iter<Result> then it returns None
///
/// How this is done:
/// 1. we know that this is invoked in a method call with `filter` as the method name via `mod.rs`
/// 2. we check that we are in a trait method. Therefore we are in an
/// `(x as Iterator).filter({filter_arg})` method call.
/// 3. we check that the parent expression is not a map. This is because we don't want to lint
/// twice, and we already have a specialized lint for that.
/// 4. we check that the span of the filter does not contain a comment.
/// 5. we get the type of the `Item` in the `Iterator`, and compare against the type of Option and
/// Result.
/// 6. we finally check the contents of the filter argument to see if it is a call to `is_some` or
/// `is_ok`.
/// 7. if all of the above are true, then we return the `FilterType`
fn expression_type(
cx: &LateContext<'_>,
expr: &hir::Expr<'_>,
filter_arg: &hir::Expr<'_>,
filter_span: Span,
) -> Option<FilterType> {
if !is_trait_method(cx, expr, sym::Iterator)
|| parent_is_map(cx, expr)
|| span_contains_comment(cx.sess().source_map(), filter_span.with_hi(expr.span.hi()))
{
return None;
}
if let hir::ExprKind::MethodCall(_, receiver, _, _) = expr.kind
&& let receiver_ty = cx.typeck_results().expr_ty(receiver)
&& let Some(iter_item_ty) = get_iterator_item_ty(cx, receiver_ty)
{
if let Some(opt_defid) = cx.tcx.get_diagnostic_item(sym::Option)
&& let opt_ty = cx.tcx.type_of(opt_defid).skip_binder()
&& iter_item_ty.ty_adt_def() == opt_ty.ty_adt_def()
&& is_method(cx, filter_arg, sym::Option, sym!(is_some), &[])
{
return Some(FilterType::IsSome);
}
if let Some(opt_defid) = cx.tcx.get_diagnostic_item(sym::Result)
&& let opt_ty = cx.tcx.type_of(opt_defid).skip_binder()
&& iter_item_ty.ty_adt_def() == opt_ty.ty_adt_def()
&& is_method(cx, filter_arg, sym::Result, sym!(is_ok), &[])
{
return Some(FilterType::IsOk);
}
}
None
}
#[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(
// we are in a filter inside an iterator
match expression_type(cx, expr, filter_arg, filter_span) {
None => (),
Some(FilterType::IsOk) => span_lint_and_sugg(
cx,
ITER_FILTER_IS_OK,
filter_span.with_hi(expr.span.hi()),
@ -82,6 +183,15 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_arg: &hir
"consider using `flatten` instead",
reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, filter_span)).into_owned(),
Applicability::HasPlaceholders,
);
),
Some(FilterType::IsSome) => 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,
),
}
}

179
tests/ui/iter_filter_is_ok.fixed
View File

@ -1,21 +1,72 @@
#![warn(clippy::iter_filter_is_ok)]
#![allow(
clippy::map_identity,
clippy::result_filter_map,
clippy::needless_borrow,
clippy::redundant_closure
)]
fn main() {
{
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Ok(1), Err(2)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Ok(1), Err(2)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
.flatten();
//~^ HELP: consider using `flatten` instead
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
.flatten();
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Ok(1), Err(2)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
}
}
fn avoid_linting_when_filter_has_side_effects() {
// Don't lint below
let mut counter = 0;
let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| {
counter += 1;
o.is_ok()
});
}
fn avoid_linting_when_commented() {
let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| {
// Roses are red,
// Violets are blue,
@ -24,3 +75,131 @@ fn main() {
o.is_ok()
});
}
fn ice_12058() {
// check that checking the parent node doesn't cause an ICE
// by indexing the parameters of a closure without parameters
Some(1).or_else(|| {
vec![Ok(1), Err(())].into_iter().filter(|z| *z != Ok(2));
None
});
}
fn avoid_linting_map() {
// should not lint
let _ = vec![Ok(1), Err(())]
.into_iter()
.filter(|o| o.is_ok())
.map(|o| o.unwrap());
// should not lint
let _ = vec![Ok(1), Err(())].into_iter().filter(|o| o.is_ok()).map(|o| o);
}
fn avoid_false_positive_due_to_is_ok_and_iterator_impl() {
#[derive(Default, Clone)]
struct Foo {}
impl Foo {
fn is_ok(&self) -> bool {
true
}
}
impl Iterator for Foo {
type Item = Foo;
fn next(&mut self) -> Option<Self::Item> {
Some(Foo::default())
}
}
let data = vec![Foo::default()];
// should not lint
let _ = data.clone().into_iter().filter(Foo::is_ok);
// should not lint
let _ = data.clone().into_iter().filter(|f| f.is_ok());
}
fn avoid_false_positive_due_to_is_ok_and_into_iterator_impl() {
#[derive(Default, Clone)]
struct Foo {}
impl Foo {
fn is_ok(&self) -> bool {
true
}
}
let data = vec![Foo::default()];
// should not lint
let _ = data.clone().into_iter().filter(Foo::is_ok);
// should not lint
let _ = data.clone().into_iter().filter(|f| f.is_ok());
}
fn avoid_fp_for_trivial() {
let _ = vec![Ok(1), Err(()), Ok(3)]
.into_iter()
// should not lint
.filter(|o| (Err(()) as Result<i32, ()>).is_ok());
}
fn avoid_false_positive_due_to_method_name() {
fn is_ok(x: &Result<i32, i32>) -> bool {
x.is_ok()
}
vec![Ok(1), Err(2), Ok(3)].into_iter().filter(is_ok);
// should not lint
}
fn avoid_fp_due_to_trait_type() {
struct Foo {
bar: i32,
}
impl Foo {
fn is_ok(obj: &Result<i32, i32>) -> bool {
obj.is_ok()
}
}
vec![Ok(1), Err(2), Ok(3)].into_iter().filter(Foo::is_ok);
// should not lint
}
fn avoid_fp_with_call_to_outside_var() {
let outside: Result<i32, ()> = Ok(1);
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| outside.is_ok());
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| Result::is_ok(&outside));
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| std::result::Result::is_ok(&outside));
}
fn avoid_fp_with_call_to_outside_var_mix_match_types() {
let outside = Some(1);
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| outside.is_some());
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| Option::is_some(&outside));
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| std::option::Option::is_some(&outside));
}

181
tests/ui/iter_filter_is_ok.rs
View File

@ -1,21 +1,72 @@
#![warn(clippy::iter_filter_is_ok)]
#![allow(
clippy::map_identity,
clippy::result_filter_map,
clippy::needless_borrow,
clippy::redundant_closure
)]
fn main() {
{
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(Result::is_ok);
//~^ HELP: consider using `flatten` instead
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|a| a.is_ok());
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| { o.is_ok() });
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|&a| a.is_ok());
//~^ HELP: consider using `flatten` instead
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|&a| a.is_ok());
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Ok(1), Err(2)].into_iter().filter(|&o| { o.is_ok() });
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
.filter(std::result::Result::is_ok);
//~^ HELP: consider using `flatten` instead
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
.filter(|a| std::result::Result::is_ok(a));
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|a| { std::result::Result::is_ok(a) });
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|ref a| a.is_ok());
//~^ HELP: consider using `flatten` instead
let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|ref a| a.is_ok());
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Ok(1), Err(2)].into_iter().filter(|ref o| { o.is_ok() });
//~^ HELP: consider using `flatten` instead
}
}
fn avoid_linting_when_filter_has_side_effects() {
// Don't lint below
let mut counter = 0;
let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| {
counter += 1;
o.is_ok()
});
}
fn avoid_linting_when_commented() {
let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| {
// Roses are red,
// Violets are blue,
@ -24,3 +75,131 @@ fn main() {
o.is_ok()
});
}
fn ice_12058() {
// check that checking the parent node doesn't cause an ICE
// by indexing the parameters of a closure without parameters
Some(1).or_else(|| {
vec![Ok(1), Err(())].into_iter().filter(|z| *z != Ok(2));
None
});
}
fn avoid_linting_map() {
// should not lint
let _ = vec![Ok(1), Err(())]
.into_iter()
.filter(|o| o.is_ok())
.map(|o| o.unwrap());
// should not lint
let _ = vec![Ok(1), Err(())].into_iter().filter(|o| o.is_ok()).map(|o| o);
}
fn avoid_false_positive_due_to_is_ok_and_iterator_impl() {
#[derive(Default, Clone)]
struct Foo {}
impl Foo {
fn is_ok(&self) -> bool {
true
}
}
impl Iterator for Foo {
type Item = Foo;
fn next(&mut self) -> Option<Self::Item> {
Some(Foo::default())
}
}
let data = vec![Foo::default()];
// should not lint
let _ = data.clone().into_iter().filter(Foo::is_ok);
// should not lint
let _ = data.clone().into_iter().filter(|f| f.is_ok());
}
fn avoid_false_positive_due_to_is_ok_and_into_iterator_impl() {
#[derive(Default, Clone)]
struct Foo {}
impl Foo {
fn is_ok(&self) -> bool {
true
}
}
let data = vec![Foo::default()];
// should not lint
let _ = data.clone().into_iter().filter(Foo::is_ok);
// should not lint
let _ = data.clone().into_iter().filter(|f| f.is_ok());
}
fn avoid_fp_for_trivial() {
let _ = vec![Ok(1), Err(()), Ok(3)]
.into_iter()
// should not lint
.filter(|o| (Err(()) as Result<i32, ()>).is_ok());
}
fn avoid_false_positive_due_to_method_name() {
fn is_ok(x: &Result<i32, i32>) -> bool {
x.is_ok()
}
vec![Ok(1), Err(2), Ok(3)].into_iter().filter(is_ok);
// should not lint
}
fn avoid_fp_due_to_trait_type() {
struct Foo {
bar: i32,
}
impl Foo {
fn is_ok(obj: &Result<i32, i32>) -> bool {
obj.is_ok()
}
}
vec![Ok(1), Err(2), Ok(3)].into_iter().filter(Foo::is_ok);
// should not lint
}
fn avoid_fp_with_call_to_outside_var() {
let outside: Result<i32, ()> = Ok(1);
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| outside.is_ok());
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| Result::is_ok(&outside));
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| std::result::Result::is_ok(&outside));
}
fn avoid_fp_with_call_to_outside_var_mix_match_types() {
let outside = Some(1);
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| outside.is_some());
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| Option::is_some(&outside));
let _ = vec![Ok(1), Err(2), Ok(3)]
.into_iter()
// should not lint
.filter(|o| std::option::Option::is_some(&outside));
}

62
tests/ui/iter_filter_is_ok.stderr
View File

@ -1,5 +1,5 @@
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:4:52
--> $DIR/iter_filter_is_ok.rs:11:56
|
LL | let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(Result::is_ok);
| ^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
@ -8,16 +8,70 @@ LL | let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(Result::is_ok);
= help: to override `-D warnings` add `#[allow(clippy::iter_filter_is_ok)]`
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:6:52
--> $DIR/iter_filter_is_ok.rs:13:56
|
LL | let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|a| a.is_ok());
| ^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:10:45
--> $DIR/iter_filter_is_ok.rs:16:49
|
LL | let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| { o.is_ok() });
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: aborting due to 3 previous errors
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:21:56
|
LL | let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|&a| a.is_ok());
| ^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:24:56
|
LL | let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|&a| a.is_ok());
| ^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:28:49
|
LL | let _ = vec![Ok(1), Err(2)].into_iter().filter(|&o| { o.is_ok() });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:35:14
|
LL | .filter(std::result::Result::is_ok);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:40:14
|
LL | .filter(|a| std::result::Result::is_ok(a));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:43:56
|
LL | let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|a| { std::result::Result::is_ok(a) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:48:56
|
LL | let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|ref a| a.is_ok());
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:51:56
|
LL | let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|ref a| a.is_ok());
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_ok` on iterator over `Result`s
--> $DIR/iter_filter_is_ok.rs:55:49
|
LL | let _ = vec![Ok(1), Err(2)].into_iter().filter(|ref o| { o.is_ok() });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: aborting due to 12 previous errors

223
tests/ui/iter_filter_is_some.fixed
View File

@ -1,23 +1,70 @@
#![warn(clippy::iter_filter_is_some)]
#![allow(
clippy::map_identity,
clippy::result_filter_map,
clippy::needless_borrow,
clippy::option_filter_map,
clippy::redundant_closure
)]
use std::collections::HashMap;
fn main() {
let _ = vec![Some(1)].into_iter().flatten();
{
let _ = vec![Some(1), None, Some(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
let _ = vec![Some(1)].into_iter().flatten();
let _ = vec![Some(1), None, Some(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Some(1), None, Some(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Some(1), None, Some(3)]
.into_iter()
.flatten();
//~^ HELP: consider using `flatten` instead
let _ = vec![Some(1), None, Some(3)]
.into_iter()
.flatten();
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Some(1), None, Some(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Some(1), None, Some(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Some(1)].into_iter().flatten();
let _ = vec![Some(1), None, Some(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Some(1), None, Some(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Some(1), None, Some(3)].into_iter().flatten();
//~^ HELP: consider using `flatten` instead
}
}
fn avoid_linting_when_filter_has_side_effects() {
// Don't lint below
let mut counter = 0;
let _ = vec![Some(1)].into_iter().filter(|o| {
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|o| {
counter += 1;
o.is_some()
});
}
let _ = vec![Some(1)].into_iter().filter(|o| {
fn avoid_linting_when_commented() {
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|o| {
// Roses are red,
// Violets are blue,
// `Err` is not an `Option`,
@ -25,3 +72,169 @@ fn main() {
o.is_some()
});
}
fn ice_12058() {
// check that checking the parent node doesn't cause an ICE
// by indexing the parameters of a closure without parameters
Some(1).or_else(|| {
vec![Some(1), None, Some(3)].into_iter().filter(|z| *z != Some(2));
None
});
}
fn avoid_linting_map() {
// should not lint
let _ = vec![Some(1), None, Some(3)]
.into_iter()
.filter(|o| o.is_some())
.map(|o| o.unwrap());
// should not lint
let _ = vec![Some(1), None, Some(3)]
.into_iter()
.filter(|o| o.is_some())
.map(|o| o);
}
fn avoid_false_positive_due_to_is_some_and_iterator_impl() {
#[derive(Default, Clone)]
struct Foo {}
impl Foo {
fn is_some(&self) -> bool {
true
}
}
impl Iterator for Foo {
type Item = Foo;
fn next(&mut self) -> Option<Self::Item> {
Some(Foo::default())
}
}
let data = vec![Foo::default()];
// should not lint
let _ = data.clone().into_iter().filter(Foo::is_some);
// should not lint
let _ = data.clone().into_iter().filter(|f| f.is_some());
}
fn avoid_false_positive_due_to_is_some_and_into_iterator_impl() {
#[derive(Default, Clone)]
struct Foo {}
impl Foo {
fn is_some(&self) -> bool {
true
}
}
let data = vec![Foo::default()];
// should not lint
let _ = data.clone().into_iter().filter(Foo::is_some);
// should not lint
let _ = data.clone().into_iter().filter(|f| f.is_some());
}
fn avoid_unpack_fp() {
let _ = vec![(Some(1), None), (None, Some(3))]
.into_iter()
// should not lint
.filter(|(a, _)| a.is_some());
let _ = vec![(Some(1), None), (None, Some(3))]
.into_iter()
// should not lint
.filter(|(a, _)| a.is_some())
.collect::<Vec<_>>();
let m = HashMap::from([(1, 1)]);
let _ = vec![1, 2, 4].into_iter().filter(|a| m.get(a).is_some());
// should not lint
}
fn avoid_fp_for_external() {
let value = HashMap::from([(1, 1)]);
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| value.get(&1).is_some());
let value = Option::Some(1);
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| value.is_some());
}
fn avoid_fp_for_trivial() {
let value = HashMap::from([(1, 1)]);
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| Some(1).is_some());
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| None::<i32>.is_some());
}
fn avoid_false_positive_due_to_method_name() {
fn is_some(x: &Option<i32>) -> bool {
x.is_some()
}
vec![Some(1), None, Some(3)].into_iter().filter(is_some);
// should not lint
}
fn avoid_fp_due_to_trait_type() {
struct Foo {
bar: i32,
}
impl Foo {
fn is_some(obj: &Option<i32>) -> bool {
obj.is_some()
}
}
vec![Some(1), None, Some(3)].into_iter().filter(Foo::is_some);
// should not lint
}
fn avoid_fp_with_call_to_outside_var() {
let outside = Some(1);
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| outside.is_some());
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| Option::is_some(&outside));
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| std::option::Option::is_some(&outside));
}
fn avoid_fp_with_call_to_outside_var_mix_match_types() {
let outside: Result<i32, ()> = Ok(1);
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| outside.is_ok());
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| Result::is_ok(&outside));
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| std::result::Result::is_ok(&outside));
}

223
tests/ui/iter_filter_is_some.rs
View File

@ -1,23 +1,70 @@
#![warn(clippy::iter_filter_is_some)]
#![allow(
clippy::map_identity,
clippy::result_filter_map,
clippy::needless_borrow,
clippy::option_filter_map,
clippy::redundant_closure
)]
use std::collections::HashMap;
fn main() {
let _ = vec![Some(1)].into_iter().filter(Option::is_some);
{
let _ = vec![Some(1), None, Some(3)].into_iter().filter(Option::is_some);
//~^ HELP: consider using `flatten` instead
let _ = vec![Some(1)].into_iter().filter(|o| o.is_some());
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|a| a.is_some());
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|o| { o.is_some() });
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Some(1), None, Some(3)]
.into_iter()
.filter(std::option::Option::is_some);
//~^ HELP: consider using `flatten` instead
let _ = vec![Some(1), None, Some(3)]
.into_iter()
.filter(|a| std::option::Option::is_some(a));
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|a| { std::option::Option::is_some(a) });
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|&a| a.is_some());
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Some(1)].into_iter().filter(|o| { o.is_some() });
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|&o| { o.is_some() });
//~^ HELP: consider using `flatten` instead
}
{
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|ref a| a.is_some());
//~^ HELP: consider using `flatten` instead
#[rustfmt::skip]
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|ref o| { o.is_some() });
//~^ HELP: consider using `flatten` instead
}
}
fn avoid_linting_when_filter_has_side_effects() {
// Don't lint below
let mut counter = 0;
let _ = vec![Some(1)].into_iter().filter(|o| {
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|o| {
counter += 1;
o.is_some()
});
}
let _ = vec![Some(1)].into_iter().filter(|o| {
fn avoid_linting_when_commented() {
let _ = vec![Some(1), None, Some(3)].into_iter().filter(|o| {
// Roses are red,
// Violets are blue,
// `Err` is not an `Option`,
@ -25,3 +72,169 @@ fn main() {
o.is_some()
});
}
fn ice_12058() {
// check that checking the parent node doesn't cause an ICE
// by indexing the parameters of a closure without parameters
Some(1).or_else(|| {
vec![Some(1), None, Some(3)].into_iter().filter(|z| *z != Some(2));
None
});
}
fn avoid_linting_map() {
// should not lint
let _ = vec![Some(1), None, Some(3)]
.into_iter()
.filter(|o| o.is_some())
.map(|o| o.unwrap());
// should not lint
let _ = vec![Some(1), None, Some(3)]
.into_iter()
.filter(|o| o.is_some())
.map(|o| o);
}
fn avoid_false_positive_due_to_is_some_and_iterator_impl() {
#[derive(Default, Clone)]
struct Foo {}
impl Foo {
fn is_some(&self) -> bool {
true
}
}
impl Iterator for Foo {
type Item = Foo;
fn next(&mut self) -> Option<Self::Item> {
Some(Foo::default())
}
}
let data = vec![Foo::default()];
// should not lint
let _ = data.clone().into_iter().filter(Foo::is_some);
// should not lint
let _ = data.clone().into_iter().filter(|f| f.is_some());
}
fn avoid_false_positive_due_to_is_some_and_into_iterator_impl() {
#[derive(Default, Clone)]
struct Foo {}
impl Foo {
fn is_some(&self) -> bool {
true
}
}
let data = vec![Foo::default()];
// should not lint
let _ = data.clone().into_iter().filter(Foo::is_some);
// should not lint
let _ = data.clone().into_iter().filter(|f| f.is_some());
}
fn avoid_unpack_fp() {
let _ = vec![(Some(1), None), (None, Some(3))]
.into_iter()
// should not lint
.filter(|(a, _)| a.is_some());
let _ = vec![(Some(1), None), (None, Some(3))]
.into_iter()
// should not lint
.filter(|(a, _)| a.is_some())
.collect::<Vec<_>>();
let m = HashMap::from([(1, 1)]);
let _ = vec![1, 2, 4].into_iter().filter(|a| m.get(a).is_some());
// should not lint
}
fn avoid_fp_for_external() {
let value = HashMap::from([(1, 1)]);
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| value.get(&1).is_some());
let value = Option::Some(1);
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| value.is_some());
}
fn avoid_fp_for_trivial() {
let value = HashMap::from([(1, 1)]);
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| Some(1).is_some());
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| None::<i32>.is_some());
}
fn avoid_false_positive_due_to_method_name() {
fn is_some(x: &Option<i32>) -> bool {
x.is_some()
}
vec![Some(1), None, Some(3)].into_iter().filter(is_some);
// should not lint
}
fn avoid_fp_due_to_trait_type() {
struct Foo {
bar: i32,
}
impl Foo {
fn is_some(obj: &Option<i32>) -> bool {
obj.is_some()
}
}
vec![Some(1), None, Some(3)].into_iter().filter(Foo::is_some);
// should not lint
}
fn avoid_fp_with_call_to_outside_var() {
let outside = Some(1);
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| outside.is_some());
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| Option::is_some(&outside));
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| std::option::Option::is_some(&outside));
}
fn avoid_fp_with_call_to_outside_var_mix_match_types() {
let outside: Result<i32, ()> = Ok(1);
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| outside.is_ok());
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| Result::is_ok(&outside));
let _ = vec![Some(1), None, Some(3)]
.into_iter()
// should not lint
.filter(|o| std::result::Result::is_ok(&outside));
}

56
tests/ui/iter_filter_is_some.stderr
View File

@ -1,23 +1,65 @@
error: `filter` for `is_some` on iterator over `Option`
--> $DIR/iter_filter_is_some.rs:4:39
--> $DIR/iter_filter_is_some.rs:14:58
|
LL | let _ = vec![Some(1)].into_iter().filter(Option::is_some);
LL | let _ = vec![Some(1), None, Some(3)].into_iter().filter(Option::is_some);
| ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
|
= note: `-D clippy::iter-filter-is-some` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::iter_filter_is_some)]`
error: `filter` for `is_some` on iterator over `Option`
--> $DIR/iter_filter_is_some.rs:6:39
--> $DIR/iter_filter_is_some.rs:16:58
|
LL | let _ = vec![Some(1)].into_iter().filter(|o| o.is_some());
LL | let _ = vec![Some(1), None, Some(3)].into_iter().filter(|a| a.is_some());
| ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_some` on iterator over `Option`
--> $DIR/iter_filter_is_some.rs:10:39
--> $DIR/iter_filter_is_some.rs:19:58
|
LL | let _ = vec![Some(1)].into_iter().filter(|o| { o.is_some() });
LL | let _ = vec![Some(1), None, Some(3)].into_iter().filter(|o| { o.is_some() });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: aborting due to 3 previous errors
error: `filter` for `is_some` on iterator over `Option`
--> $DIR/iter_filter_is_some.rs:26:14
|
LL | .filter(std::option::Option::is_some);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_some` on iterator over `Option`
--> $DIR/iter_filter_is_some.rs:31:14
|
LL | .filter(|a| std::option::Option::is_some(a));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_some` on iterator over `Option`
--> $DIR/iter_filter_is_some.rs:34:58
|
LL | let _ = vec![Some(1), None, Some(3)].into_iter().filter(|a| { std::option::Option::is_some(a) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_some` on iterator over `Option`
--> $DIR/iter_filter_is_some.rs:39:58
|
LL | let _ = vec![Some(1), None, Some(3)].into_iter().filter(|&a| a.is_some());
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_some` on iterator over `Option`
--> $DIR/iter_filter_is_some.rs:43:58
|
LL | let _ = vec![Some(1), None, Some(3)].into_iter().filter(|&o| { o.is_some() });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_some` on iterator over `Option`
--> $DIR/iter_filter_is_some.rs:48:58
|
LL | let _ = vec![Some(1), None, Some(3)].into_iter().filter(|ref a| a.is_some());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: `filter` for `is_some` on iterator over `Option`
--> $DIR/iter_filter_is_some.rs:52:58
|
LL | let _ = vec![Some(1), None, Some(3)].into_iter().filter(|ref o| { o.is_some() });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
error: aborting due to 10 previous errors