Skip to content

Allow filter_map_identity when the closure is typed #12562

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 31, 2024
Merged

Allow filter_map_identity when the closure is typed #12562

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9655231
Allow `filter_map_identity` when the closure is typed
PartiallyUntyped Mar 25, 2024
60937bf
:adjust applicability for typed identity closures in `filter_map_iden…
PartiallyUntyped Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions clippy_lints/src/methods/filter_map_identity.rs
View file
Open in desktop
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::MaybeIncorrect);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Unspecified (or HasPlaceholders).

MaybeIncorrect suggestions should still compile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've interpreted "valid Rust code" as it parsing correctly, everything else can fail I'm pretty sure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember @xFrednet commenting about it here

But oh well, the fact that we have a different understanding is all the more reason that the documentation/naming for Applicability could be better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustfix somewhere has an issue to rename these variants to make the difference clearer. I think it's mostly stuck on finding better names:

rust-lang/cargo#13028

}
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
Open in desktop
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
Open in desktop
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
Open in desktop
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