OpenE2K/rust
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Auto merge of #3662 - mikerite:fix-498, r=oli-obk

Browse Source 
Fix `map_clone` bad suggestion

`cloned` requires that the elements of the iterator must be references. This
change determines if that is the case by examining the type of the closure
argument and suggesting `.cloned` only if it is a reference. When the closure
argument is not a reference, it suggests removing the `map` call instead.

A minor problem with this change is that the new check sometimes overlaps
with the `clone_on_copy` lint.

Fixes #498
This commit is contained in:
bors 2019-01-15 09:04:09 +00:00
commit 1b89724b48
4 changed files with 81 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

71
clippy_lints/src/map_clone.rs
View File

@ -5,6 +5,7 @@ use crate::utils::{
use if_chain::if_chain;
use rustc::hir;
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
use rustc::ty;
use rustc::{declare_tool_lint, lint_array};
use rustc_errors::Applicability;
use syntax::ast::Ident;
@ -18,9 +19,7 @@ pub struct Pass;
///
/// **Why is this bad?** Readability, this can be written more concisely
///
/// **Known problems:** Sometimes `.cloned()` requires stricter trait
/// bound than `.map(|e| e.clone())` (which works because of the coercion).
/// See [#498](https://github.com/rust-lang-nursery/rust-clippy/issues/498).
/// **Known problems:** None
///
/// **Example:**
///
@ -69,19 +68,27 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
hir::PatKind::Ref(ref inner, _) => if let hir::PatKind::Binding(
hir::BindingAnnotation::Unannotated, _, name, None
) = inner.node {
lint(cx, e.span, args[0].span, name, closure_expr);
if ident_eq(name, closure_expr) {
lint(cx, e.span, args[0].span);
}
},
hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, _, name, None) => {
match closure_expr.node {
hir::ExprKind::Unary(hir::UnOp::UnDeref, ref inner) => {
if !cx.tables.expr_ty(inner).is_box() {
lint(cx, e.span, args[0].span, name, inner);
if ident_eq(name, inner) && !cx.tables.expr_ty(inner).is_box() {
lint(cx, e.span, args[0].span);
}
},
hir::ExprKind::MethodCall(ref method, _, ref obj) => {
if method.ident.as_str() == "clone"
if ident_eq(name, &obj[0]) && method.ident.as_str() == "clone"
&& match_trait_method(cx, closure_expr, &paths::CLONE_TRAIT) {
lint(cx, e.span, args[0].span, name, &obj[0]);
let obj_ty = cx.tables.expr_ty(&obj[0]);
if let ty::Ref(..) = obj_ty.sty {
lint(cx, e.span, args[0].span);
} else {
lint_needless_cloning(cx, e.span, args[0].span);
}
}
},
_ => {},
@ -94,22 +101,38 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
}
}
fn lint(cx: &LateContext<'_, '_>, replace: Span, root: Span, name: Ident, path: &hir::Expr) {
fn ident_eq(name: Ident, path: &hir::Expr) -> bool {
if let hir::ExprKind::Path(hir::QPath::Resolved(None, ref path)) = path.node {
if path.segments.len() == 1 && path.segments[0].ident == name {
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
MAP_CLONE,
replace,
"You are using an explicit closure for cloning elements",
"Consider calling the dedicated `cloned` method",
format!(
"{}.cloned()",
snippet_with_applicability(cx, root, "..", &mut applicability)
),
applicability,
)
}
path.segments.len() == 1 && path.segments[0].ident == name
} else {
false
}
}
fn lint_needless_cloning(cx: &LateContext<'_, '_>, root: Span, receiver: Span) {
span_lint_and_sugg(
cx,
MAP_CLONE,
root.trim_start(receiver).unwrap(),
"You are needlessly cloning iterator elements",
"Remove the map call",
String::new(),
Applicability::MachineApplicable,
)
}
fn lint(cx: &LateContext<'_, '_>, replace: Span, root: Span) {
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
MAP_CLONE,
replace,
"You are using an explicit closure for cloning elements",
"Consider calling the dedicated `cloned` method",
format!(
"{}.cloned()",
snippet_with_applicability(cx, root, "..", &mut applicability)
),
applicability,
)
}

12
tests/ui/map_clone.fixed
View File

@ -1,6 +1,7 @@
// run-rustfix
#![warn(clippy::all, clippy::pedantic)]
#![allow(clippy::iter_cloned_collect)]
#![allow(clippy::clone_on_copy)]
#![allow(clippy::missing_docs_in_private_items)]
fn main() {
@ -8,4 +9,15 @@ fn main() {
let _: Vec<String> = vec![String::new()].iter().cloned().collect();
let _: Vec<u32> = vec![42, 43].iter().cloned().collect();
let _: Option<u64> = Some(Box::new(16)).map(|b| *b);
// Don't lint these
let v = vec![5_i8; 6];
let a = 0;
let b = &a;
let _ = v.iter().map(|_x| *b);
let _ = v.iter().map(|_x| a.clone());
let _ = v.iter().map(|&_x| a);
// Issue #498
let _ = std::env::args();
}

12
tests/ui/map_clone.rs
View File

@ -1,6 +1,7 @@
// run-rustfix
#![warn(clippy::all, clippy::pedantic)]
#![allow(clippy::iter_cloned_collect)]
#![allow(clippy::clone_on_copy)]
#![allow(clippy::missing_docs_in_private_items)]
fn main() {
@ -8,4 +9,15 @@ fn main() {
let _: Vec<String> = vec![String::new()].iter().map(|x| x.clone()).collect();
let _: Vec<u32> = vec![42, 43].iter().map(|&x| x).collect();
let _: Option<u64> = Some(Box::new(16)).map(|b| *b);
// Don't lint these
let v = vec![5_i8; 6];
let a = 0;
let b = &a;
let _ = v.iter().map(|_x| *b);
let _ = v.iter().map(|_x| a.clone());
let _ = v.iter().map(|&_x| a);
// Issue #498
let _ = std::env::args().map(|v| v.clone());
}

14
tests/ui/map_clone.stderr
View File

@ -1,5 +1,5 @@
error: You are using an explicit closure for cloning elements
--> $DIR/map_clone.rs:7:22
--> $DIR/map_clone.rs:8:22
|
LL | let _: Vec<i8> = vec![5_i8; 6].iter().map(|x| *x).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `vec![5_i8; 6].iter().cloned()`
@ -7,16 +7,22 @@ LL | let _: Vec<i8> = vec![5_i8; 6].iter().map(|x| *x).collect();
= note: `-D clippy::map-clone` implied by `-D warnings`
error: You are using an explicit closure for cloning elements
--> $DIR/map_clone.rs:8:26
--> $DIR/map_clone.rs:9:26
|
LL | let _: Vec<String> = vec![String::new()].iter().map(|x| x.clone()).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()`
error: You are using an explicit closure for cloning elements
--> $DIR/map_clone.rs:9:23
--> $DIR/map_clone.rs:10:23
|
LL | let _: Vec<u32> = vec![42, 43].iter().map(|&x| x).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `vec![42, 43].iter().cloned()`
error: aborting due to 3 previous errors
error: You are needlessly cloning iterator elements
--> $DIR/map_clone.rs:22:29
|
LL | let _ = std::env::args().map(|v| v.clone());
| ^^^^^^^^^^^^^^^^^^^ help: Remove the map call
error: aborting due to 4 previous errors