Skip to content

Commit

Permalink
Auto merge of rust-lang#12562 - m-rph:12501, r=y21
Browse files Browse the repository at this point in the history 
Allow `filter_map_identity` when the closure is typed

This extends the `filter_map_identity` lint to support typed closures.

For untyped closures, we know that the program compiles, and therefore we can safely suggest using flatten.

For typed closures, they may participate in type resolution. In this case we use `Applicability::MaybeIncorrect`.

Details:
https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Should.20.60filter_map_identity.60.20lint.20when.20closures.20are.20typed.3F

changelog: `filter_map_identity` will now suggest using flatten for typed closures.

r? `@y21` && `@Centri3`
  • Loading branch information
@bors
bors committed Mar 31, 2024
2 parents 3787a0c + 60937bf commit 797d50d
Show file tree
Hide file tree
Showing 4 changed files with 288 additions and 36 deletions.
18 changes: 15 additions & 3 deletions clippy_lints/src/methods/filter_map_identity.rs
View file
Original file line number Diff line number Diff line change
@@ -1,22 +1,34 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::{is_expr_untyped_identity_function, is_trait_method};
use clippy_utils::{is_expr_identity_function, is_expr_untyped_identity_function, is_trait_method};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::LateContext;
use rustc_span::{sym, Span};

use super::FILTER_MAP_IDENTITY;

fn is_identity(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<Applicability> {
if is_expr_untyped_identity_function(cx, expr) {
return Some(Applicability::MachineApplicable);
}
if is_expr_identity_function(cx, expr) {
return Some(Applicability::Unspecified);
}
None
}

pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_map_arg: &hir::Expr<'_>, filter_map_span: Span) {
if is_trait_method(cx, expr, sym::Iterator) && is_expr_untyped_identity_function(cx, filter_map_arg) {
if is_trait_method(cx, expr, sym::Iterator)
&& let Some(applicability) = is_identity(cx, filter_map_arg)
{
span_lint_and_sugg(
cx,
FILTER_MAP_IDENTITY,
filter_map_span.with_hi(expr.span.hi()),
"use of `filter_map` with an identity function",
"try",
"flatten()".to_string(),
Applicability::MachineApplicable,
applicability,
);
}
}
86 changes: 76 additions & 10 deletions tests/ui/filter_map_identity.fixed
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,83 @@
#![allow(unused_imports, clippy::needless_return)]
#![allow(unused_imports, clippy::needless_return, clippy::useless_vec)]
#![warn(clippy::filter_map_identity)]
#![feature(stmt_expr_attributes)]

use std::option::Option;
struct NonCopy;
use std::convert::identity;

fn non_copy_vec() -> Vec<Option<NonCopy>> {
todo!()
}

fn copy_vec<T: Copy>() -> Vec<Option<T>> {
todo!()
}

fn copy_vec_non_inferred() -> Vec<Option<i32>> {
todo!()
}

fn opaque<T: Default>() -> impl IntoIterator<Item = Option<T>> {
vec![Some(T::default())]
}

fn main() {
let iterator = vec![Some(1), None, Some(2)].into_iter();
let _ = iterator.flatten();
{
// into_iter
copy_vec_non_inferred().into_iter().flatten();
//~^ ERROR: use of
copy_vec_non_inferred().into_iter().flatten();
//~^ ERROR: use of
copy_vec_non_inferred().into_iter().flatten();
//~^ ERROR: use of
copy_vec_non_inferred().into_iter().flatten();
//~^ ERROR: use of
copy_vec_non_inferred().into_iter().flatten();
//~^ ERROR: use of

non_copy_vec().into_iter().flatten();
//~^ ERROR: use of
non_copy_vec().into_iter().flatten();
//~^ ERROR: use of

non_copy_vec().into_iter().flatten();
//~^ ERROR: use of
non_copy_vec().into_iter().flatten();
//~^ ERROR: use of
non_copy_vec().into_iter().flatten();
//~^ ERROR: use of
non_copy_vec().into_iter().flatten();
//~^ ERROR: use of

let iterator = vec![Some(1), None, Some(2)].into_iter();
let _ = iterator.flatten();
copy_vec::<i32>().into_iter().flatten();
//~^ ERROR: use of
copy_vec::<i32>().into_iter().flatten();
//~^ ERROR: use of
copy_vec::<i32>().into_iter().flatten();
//~^ ERROR: use of
copy_vec::<i32>().into_iter().flatten();
//~^ ERROR: use of

use std::convert::identity;
let iterator = vec![Some(1), None, Some(2)].into_iter();
let _ = iterator.flatten();
// we are forced to pass the type in the call.
copy_vec::<i32>().into_iter().flatten();
//~^ ERROR: use of
copy_vec::<i32>().into_iter().flatten();
//~^ ERROR: use of
copy_vec::<i32>().into_iter().flatten();
//~^ ERROR: use of
copy_vec::<i32>().into_iter().flatten();
//~^ ERROR: use of
#[rustfmt::skip]
copy_vec::<i32>().into_iter().flatten();
//~^ ERROR: use of
#[rustfmt::skip]
copy_vec::<i32>().into_iter().flatten();
//~^ ERROR: use of

let iterator = vec![Some(1), None, Some(2)].into_iter();
let _ = iterator.flatten();
// note, the compiler requires that we pass the type to `opaque`. This is mostly for reference,
// it behaves the same as copy_vec.
opaque::<i32>().into_iter().flatten();
//~^ ERROR: use of
}
}
86 changes: 76 additions & 10 deletions tests/ui/filter_map_identity.rs
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,83 @@
#![allow(unused_imports, clippy::needless_return)]
#![allow(unused_imports, clippy::needless_return, clippy::useless_vec)]
#![warn(clippy::filter_map_identity)]
#![feature(stmt_expr_attributes)]

use std::option::Option;
struct NonCopy;
use std::convert::identity;

fn non_copy_vec() -> Vec<Option<NonCopy>> {
todo!()
}

fn copy_vec<T: Copy>() -> Vec<Option<T>> {
todo!()
}

fn copy_vec_non_inferred() -> Vec<Option<i32>> {
todo!()
}

fn opaque<T: Default>() -> impl IntoIterator<Item = Option<T>> {
vec![Some(T::default())]
}

fn main() {
let iterator = vec![Some(1), None, Some(2)].into_iter();
let _ = iterator.filter_map(|x| x);
{
// into_iter
copy_vec_non_inferred().into_iter().filter_map(|x| x);
//~^ ERROR: use of
copy_vec_non_inferred().into_iter().filter_map(std::convert::identity);
//~^ ERROR: use of
copy_vec_non_inferred().into_iter().filter_map(identity);
//~^ ERROR: use of
copy_vec_non_inferred().into_iter().filter_map(|x| return x);
//~^ ERROR: use of
copy_vec_non_inferred().into_iter().filter_map(|x| return x);
//~^ ERROR: use of

non_copy_vec().into_iter().filter_map(|x| x);
//~^ ERROR: use of
non_copy_vec().into_iter().filter_map(|x| x);
//~^ ERROR: use of

non_copy_vec().into_iter().filter_map(std::convert::identity);
//~^ ERROR: use of
non_copy_vec().into_iter().filter_map(identity);
//~^ ERROR: use of
non_copy_vec().into_iter().filter_map(|x| return x);
//~^ ERROR: use of
non_copy_vec().into_iter().filter_map(|x| return x);
//~^ ERROR: use of

let iterator = vec![Some(1), None, Some(2)].into_iter();
let _ = iterator.filter_map(std::convert::identity);
copy_vec::<i32>().into_iter().filter_map(|x: Option<_>| x);
//~^ ERROR: use of
copy_vec::<i32>().into_iter().filter_map(|x: Option<_>| x);
//~^ ERROR: use of
copy_vec::<i32>().into_iter().filter_map(|x: Option<_>| return x);
//~^ ERROR: use of
copy_vec::<i32>().into_iter().filter_map(|x: Option<_>| return x);
//~^ ERROR: use of

use std::convert::identity;
let iterator = vec![Some(1), None, Some(2)].into_iter();
let _ = iterator.filter_map(identity);
// we are forced to pass the type in the call.
copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| x);
//~^ ERROR: use of
copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| x);
//~^ ERROR: use of
copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| return x);
//~^ ERROR: use of
copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| return x);
//~^ ERROR: use of
#[rustfmt::skip]
copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| -> Option<i32> {{ x }});
//~^ ERROR: use of
#[rustfmt::skip]
copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| -> Option<i32> {{ return x }});
//~^ ERROR: use of

let iterator = vec![Some(1), None, Some(2)].into_iter();
let _ = iterator.filter_map(|x| return x);
// note, the compiler requires that we pass the type to `opaque`. This is mostly for reference,
// it behaves the same as copy_vec.
opaque::<i32>().into_iter().filter_map(|x| x);
//~^ ERROR: use of
}
}
134 changes: 121 additions & 13 deletions tests/ui/filter_map_identity.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,29 +1,137 @@
error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:6:22
--> tests/ui/filter_map_identity.rs:28:45
|
LL | let _ = iterator.filter_map(|x| x);
| ^^^^^^^^^^^^^^^^^ help: try: `flatten()`
LL | copy_vec_non_inferred().into_iter().filter_map(|x| x);
| ^^^^^^^^^^^^^^^^^ help: try: `flatten()`
|
= note: `-D clippy::filter-map-identity` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::filter_map_identity)]`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:9:22
--> tests/ui/filter_map_identity.rs:30:45
|
LL | let _ = iterator.filter_map(std::convert::identity);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`
LL | copy_vec_non_inferred().into_iter().filter_map(std::convert::identity);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:13:22
--> tests/ui/filter_map_identity.rs:32:45
|
LL | let _ = iterator.filter_map(identity);
| ^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`
LL | copy_vec_non_inferred().into_iter().filter_map(identity);
| ^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:16:22
--> tests/ui/filter_map_identity.rs:34:45
|
LL | let _ = iterator.filter_map(|x| return x);
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`
LL | copy_vec_non_inferred().into_iter().filter_map(|x| return x);
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: aborting due to 4 previous errors
error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:36:45
|
LL | copy_vec_non_inferred().into_iter().filter_map(|x| return x);
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:39:36
|
LL | non_copy_vec().into_iter().filter_map(|x| x);
| ^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:41:36
|
LL | non_copy_vec().into_iter().filter_map(|x| x);
| ^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:44:36
|
LL | non_copy_vec().into_iter().filter_map(std::convert::identity);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:46:36
|
LL | non_copy_vec().into_iter().filter_map(identity);
| ^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:48:36
|
LL | non_copy_vec().into_iter().filter_map(|x| return x);
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:50:36
|
LL | non_copy_vec().into_iter().filter_map(|x| return x);
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:53:39
|
LL | copy_vec::<i32>().into_iter().filter_map(|x: Option<_>| x);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:55:39
|
LL | copy_vec::<i32>().into_iter().filter_map(|x: Option<_>| x);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:57:39
|
LL | copy_vec::<i32>().into_iter().filter_map(|x: Option<_>| return x);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:59:39
|
LL | copy_vec::<i32>().into_iter().filter_map(|x: Option<_>| return x);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:63:39
|
LL | copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| x);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:65:39
|
LL | copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| x);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:67:39
|
LL | copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| return x);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:69:39
|
LL | copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| return x);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:72:43
|
LL | copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| -> Option<i32> {{ x }});
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:75:43
|
LL | copy_vec::<i32>().into_iter().filter_map(|x: Option<i32>| -> Option<i32> {{ return x }});
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: use of `filter_map` with an identity function
--> tests/ui/filter_map_identity.rs:80:37
|
LL | opaque::<i32>().into_iter().filter_map(|x| x);
| ^^^^^^^^^^^^^^^^^ help: try: `flatten()`

error: aborting due to 22 previous errors

0 comments on commit 797d50d

Please sign in to comment.