Skip to content

Commit

Permalink
Auto merge of rust-lang#8930 - kyoto7250:issue_8920, r=Alexendoo
Browse files Browse the repository at this point in the history
fix(manual_find_map and manual_filter_map): check clone method

close rust-lang#8920

Added conditional branching when the clone method is used.

Thank you in advance.

---

changelog: check `clone()` and other variant preserving methods in [`manual_find_map`] and [`manual_filter_map`]
  • Loading branch information
bors committed Jun 4, 2022
2 parents d9ddce8 + 42cf985 commit 542d474
Show file tree
Hide file tree
Showing 7 changed files with 389 additions and 4 deletions.
27 changes: 25 additions & 2 deletions clippy_lints/src/methods/filter_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::def::Res;
use rustc_hir::{Expr, ExprKind, PatKind, QPath, UnOp};
use rustc_hir::{Expr, ExprKind, PatKind, PathSegment, QPath, UnOp};
use rustc_lint::LateContext;
use rustc_span::source_map::Span;
use rustc_span::symbol::{sym, Symbol};
Expand Down Expand Up @@ -155,7 +155,15 @@ pub(super) fn check<'tcx>(
}
false
};
if SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg);

if match map_arg.kind {
ExprKind::MethodCall(method, [original_arg], _) => {
acceptable_methods(method)
&& SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, original_arg)
},
_ => SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg)
};

then {
let span = filter_span.with_hi(expr.span.hi());
let (filter_name, lint) = if is_find {
Expand All @@ -171,3 +179,18 @@ pub(super) fn check<'tcx>(
}
}
}

fn acceptable_methods(method: &PathSegment<'_>) -> bool {
let methods: [Symbol; 8] = [
sym::clone,
sym::as_ref,
sym!(copied),
sym!(cloned),
sym!(as_deref),
sym!(as_mut),
sym!(as_deref_mut),
sym!(to_owned),
];

methods.contains(&method.ident.name)
}
50 changes: 50 additions & 0 deletions tests/ui/manual_filter_map.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,53 @@ fn to_opt<T>(_: T) -> Option<T> {
fn to_res<T>(_: T) -> Result<T, ()> {
unimplemented!()
}

struct Issue8920<'a> {
option_field: Option<String>,
result_field: Result<String, ()>,
ref_field: Option<&'a usize>,
}

fn issue_8920() {
let mut vec = vec![Issue8920 {
option_field: Some(String::from("str")),
result_field: Ok(String::from("str")),
ref_field: Some(&1),
}];

let _ = vec
.iter()
.filter_map(|f| f.option_field.clone());

let _ = vec
.iter()
.filter_map(|f| f.ref_field.cloned());

let _ = vec
.iter()
.filter_map(|f| f.ref_field.copied());

let _ = vec
.iter()
.filter_map(|f| f.result_field.clone().ok());

let _ = vec
.iter()
.filter_map(|f| f.result_field.as_ref().ok());

let _ = vec
.iter()
.filter_map(|f| f.result_field.as_deref().ok());

let _ = vec
.iter_mut()
.filter_map(|f| f.result_field.as_mut().ok());

let _ = vec
.iter_mut()
.filter_map(|f| f.result_field.as_deref_mut().ok());

let _ = vec
.iter()
.filter_map(|f| f.result_field.to_owned().ok());
}
59 changes: 59 additions & 0 deletions tests/ui/manual_filter_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,62 @@ fn to_opt<T>(_: T) -> Option<T> {
fn to_res<T>(_: T) -> Result<T, ()> {
unimplemented!()
}

struct Issue8920<'a> {
option_field: Option<String>,
result_field: Result<String, ()>,
ref_field: Option<&'a usize>,
}

fn issue_8920() {
let mut vec = vec![Issue8920 {
option_field: Some(String::from("str")),
result_field: Ok(String::from("str")),
ref_field: Some(&1),
}];

let _ = vec
.iter()
.filter(|f| f.option_field.is_some())
.map(|f| f.option_field.clone().unwrap());

let _ = vec
.iter()
.filter(|f| f.ref_field.is_some())
.map(|f| f.ref_field.cloned().unwrap());

let _ = vec
.iter()
.filter(|f| f.ref_field.is_some())
.map(|f| f.ref_field.copied().unwrap());

let _ = vec
.iter()
.filter(|f| f.result_field.is_ok())
.map(|f| f.result_field.clone().unwrap());

let _ = vec
.iter()
.filter(|f| f.result_field.is_ok())
.map(|f| f.result_field.as_ref().unwrap());

let _ = vec
.iter()
.filter(|f| f.result_field.is_ok())
.map(|f| f.result_field.as_deref().unwrap());

let _ = vec
.iter_mut()
.filter(|f| f.result_field.is_ok())
.map(|f| f.result_field.as_mut().unwrap());

let _ = vec
.iter_mut()
.filter(|f| f.result_field.is_ok())
.map(|f| f.result_field.as_deref_mut().unwrap());

let _ = vec
.iter()
.filter(|f| f.result_field.is_ok())
.map(|f| f.result_field.to_owned().unwrap());
}
74 changes: 73 additions & 1 deletion tests/ui/manual_filter_map.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,77 @@ error: `filter(..).map(..)` can be simplified as `filter_map(..)`
LL | let _ = (0..).filter(|&n| to_res(n).is_ok()).map(|a| to_res(a).unwrap_or(1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `filter_map(|a| to_res(a).ok())`

error: aborting due to 3 previous errors
error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:54:10
|
LL | .filter(|f| f.option_field.is_some())
| __________^
LL | | .map(|f| f.option_field.clone().unwrap());
| |_________________________________________________^ help: try: `filter_map(|f| f.option_field.clone())`

error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:59:10
|
LL | .filter(|f| f.ref_field.is_some())
| __________^
LL | | .map(|f| f.ref_field.cloned().unwrap());
| |_______________________________________________^ help: try: `filter_map(|f| f.ref_field.cloned())`

error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:64:10
|
LL | .filter(|f| f.ref_field.is_some())
| __________^
LL | | .map(|f| f.ref_field.copied().unwrap());
| |_______________________________________________^ help: try: `filter_map(|f| f.ref_field.copied())`

error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:69:10
|
LL | .filter(|f| f.result_field.is_ok())
| __________^
LL | | .map(|f| f.result_field.clone().unwrap());
| |_________________________________________________^ help: try: `filter_map(|f| f.result_field.clone().ok())`

error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:74:10
|
LL | .filter(|f| f.result_field.is_ok())
| __________^
LL | | .map(|f| f.result_field.as_ref().unwrap());
| |__________________________________________________^ help: try: `filter_map(|f| f.result_field.as_ref().ok())`

error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:79:10
|
LL | .filter(|f| f.result_field.is_ok())
| __________^
LL | | .map(|f| f.result_field.as_deref().unwrap());
| |____________________________________________________^ help: try: `filter_map(|f| f.result_field.as_deref().ok())`

error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:84:10
|
LL | .filter(|f| f.result_field.is_ok())
| __________^
LL | | .map(|f| f.result_field.as_mut().unwrap());
| |__________________________________________________^ help: try: `filter_map(|f| f.result_field.as_mut().ok())`

error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:89:10
|
LL | .filter(|f| f.result_field.is_ok())
| __________^
LL | | .map(|f| f.result_field.as_deref_mut().unwrap());
| |________________________________________________________^ help: try: `filter_map(|f| f.result_field.as_deref_mut().ok())`

error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:94:10
|
LL | .filter(|f| f.result_field.is_ok())
| __________^
LL | | .map(|f| f.result_field.to_owned().unwrap());
| |____________________________________________________^ help: try: `filter_map(|f| f.result_field.to_owned().ok())`

error: aborting due to 12 previous errors

50 changes: 50 additions & 0 deletions tests/ui/manual_find_map.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,53 @@ fn to_opt<T>(_: T) -> Option<T> {
fn to_res<T>(_: T) -> Result<T, ()> {
unimplemented!()
}

struct Issue8920<'a> {
option_field: Option<String>,
result_field: Result<String, ()>,
ref_field: Option<&'a usize>,
}

fn issue_8920() {
let mut vec = vec![Issue8920 {
option_field: Some(String::from("str")),
result_field: Ok(String::from("str")),
ref_field: Some(&1),
}];

let _ = vec
.iter()
.find_map(|f| f.option_field.clone());

let _ = vec
.iter()
.find_map(|f| f.ref_field.cloned());

let _ = vec
.iter()
.find_map(|f| f.ref_field.copied());

let _ = vec
.iter()
.find_map(|f| f.result_field.clone().ok());

let _ = vec
.iter()
.find_map(|f| f.result_field.as_ref().ok());

let _ = vec
.iter()
.find_map(|f| f.result_field.as_deref().ok());

let _ = vec
.iter_mut()
.find_map(|f| f.result_field.as_mut().ok());

let _ = vec
.iter_mut()
.find_map(|f| f.result_field.as_deref_mut().ok());

let _ = vec
.iter()
.find_map(|f| f.result_field.to_owned().ok());
}
59 changes: 59 additions & 0 deletions tests/ui/manual_find_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,62 @@ fn to_opt<T>(_: T) -> Option<T> {
fn to_res<T>(_: T) -> Result<T, ()> {
unimplemented!()
}

struct Issue8920<'a> {
option_field: Option<String>,
result_field: Result<String, ()>,
ref_field: Option<&'a usize>,
}

fn issue_8920() {
let mut vec = vec![Issue8920 {
option_field: Some(String::from("str")),
result_field: Ok(String::from("str")),
ref_field: Some(&1),
}];

let _ = vec
.iter()
.find(|f| f.option_field.is_some())
.map(|f| f.option_field.clone().unwrap());

let _ = vec
.iter()
.find(|f| f.ref_field.is_some())
.map(|f| f.ref_field.cloned().unwrap());

let _ = vec
.iter()
.find(|f| f.ref_field.is_some())
.map(|f| f.ref_field.copied().unwrap());

let _ = vec
.iter()
.find(|f| f.result_field.is_ok())
.map(|f| f.result_field.clone().unwrap());

let _ = vec
.iter()
.find(|f| f.result_field.is_ok())
.map(|f| f.result_field.as_ref().unwrap());

let _ = vec
.iter()
.find(|f| f.result_field.is_ok())
.map(|f| f.result_field.as_deref().unwrap());

let _ = vec
.iter_mut()
.find(|f| f.result_field.is_ok())
.map(|f| f.result_field.as_mut().unwrap());

let _ = vec
.iter_mut()
.find(|f| f.result_field.is_ok())
.map(|f| f.result_field.as_deref_mut().unwrap());

let _ = vec
.iter()
.find(|f| f.result_field.is_ok())
.map(|f| f.result_field.to_owned().unwrap());
}
Loading

0 comments on commit 542d474

Please sign in to comment.