From 8cbf548f7e6cf5dea96acb836a7300e3e46f0764 Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Sat, 28 Jan 2017 14:02:49 +0100 Subject: [PATCH 1/4] Add suggestions to `EXPLICIT_[INTO_]ITER_LOOP` Also reduces the highlighted span to the expr containing the `.[into_]iter()` call (so the suggestion is probably applicable by rustfix.) Fixes #1484 --- clippy_lints/src/loops.rs | 48 +++++++++++++--------- tests/compile-fail/for_loop.rs | 75 +++++++++++++++++++++++++++------- 2 files changed, 90 insertions(+), 33 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 73926fed0f1..e3492fd00f7 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -588,28 +588,38 @@ fn check_for_loop_arg(cx: &LateContext, pat: &Pat, arg: &Expr, expr: &Expr) { if &*method_name.as_str() == "iter" || &*method_name.as_str() == "iter_mut" { if is_ref_iterable_type(cx, &args[0]) { let object = snippet(cx, args[0].span, "_"); - span_lint(cx, - EXPLICIT_ITER_LOOP, - expr.span, - &format!("it is more idiomatic to loop over `&{}{}` instead of `{}.{}()`", - if &*method_name.as_str() == "iter_mut" { - "mut " - } else { - "" - }, - object, - object, - method_name)); + let suggestion = format!("&{}{}", + if &*method_name.as_str() == "iter_mut" { + "mut " + } else { + "" + }, + object); + span_lint_and_then(cx, + EXPLICIT_ITER_LOOP, + arg.span, + &format!("it is more idiomatic to loop over `{}` instead of `{}.{}()`", + suggestion, + object, + method_name), + |db| { + db.span_suggestion(arg.span, "to write this more concisely, try looping over", suggestion); + }); } } else if &*method_name.as_str() == "into_iter" && match_trait_method(cx, arg, &paths::INTO_ITERATOR) { let object = snippet(cx, args[0].span, "_"); - span_lint(cx, - EXPLICIT_INTO_ITER_LOOP, - expr.span, - &format!("it is more idiomatic to loop over `{}` instead of `{}.{}()`", - object, - object, - method_name)); + span_lint_and_then(cx, + EXPLICIT_INTO_ITER_LOOP, + arg.span, + &format!("it is more idiomatic to loop over `{}` instead of `{}.{}()`", + object, + object, + method_name), + |db| { + db.span_suggestion(arg.span, + "to write this more concisely, try looping over", + object.to_string()); + }); } else if &*method_name.as_str() == "next" && match_trait_method(cx, arg, &paths::ITERATOR) { span_lint(cx, diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 659935e6098..5e13b778329 100644 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -286,39 +286,86 @@ fn main() { id_col[i] = 1f64; } - /* + // This is fine. for i in (10..0).map(|x| x * 2) { println!("{}", i); - }*/ + } - for _v in vec.iter() { } //~ERROR it is more idiomatic to loop over `&vec` - for _v in vec.iter_mut() { } //~ERROR it is more idiomatic to loop over `&mut vec` + for _v in vec.iter() { } + //~^ ERROR it is more idiomatic to loop over `&vec` + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION &vec + for _v in vec.iter_mut() { } + //~^ ERROR it is more idiomatic to loop over `&mut vec` + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION vec let out_vec = vec![1,2,3]; - for _v in out_vec.into_iter() { } //~ERROR it is more idiomatic to loop over `out_vec` instead of `out_vec.into_iter()` + for _v in out_vec.into_iter() { } + //~^ ERROR it is more idiomatic to loop over `out_vec` instead of `out_vec.into_iter()` + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION out_vec for _v in &vec { } // these are fine for _v in &mut vec { } // these are fine - for _v in [1, 2, 3].iter() { } //~ERROR it is more idiomatic to loop over `&[ + for _v in [1, 2, 3].iter() { } + //~^ ERROR it is more idiomatic to loop over `&[ + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION &[1, 2, 3] + for _v in (&mut [1, 2, 3]).iter() { } // no error - for _v in [0; 32].iter() {} //~ERROR it is more idiomatic to loop over `&[ + + for _v in [0; 32].iter() {} + //~^ ERROR it is more idiomatic to loop over `&[ + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION &[0; 32] + for _v in [0; 33].iter() {} // no error + let ll: LinkedList<()> = LinkedList::new(); - for _v in ll.iter() { } //~ERROR it is more idiomatic to loop over `&ll` + for _v in ll.iter() { } + //~^ ERROR it is more idiomatic to loop over `&ll` + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION &ll + let vd: VecDeque<()> = VecDeque::new(); - for _v in vd.iter() { } //~ERROR it is more idiomatic to loop over `&vd` + for _v in vd.iter() { } + //~^ ERROR it is more idiomatic to loop over `&vd` + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION &vd + let bh: BinaryHeap<()> = BinaryHeap::new(); - for _v in bh.iter() { } //~ERROR it is more idiomatic to loop over `&bh` + for _v in bh.iter() { } + //~^ ERROR it is more idiomatic to loop over `&bh` + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION &bh + let hm: HashMap<(), ()> = HashMap::new(); - for _v in hm.iter() { } //~ERROR it is more idiomatic to loop over `&hm` + for _v in hm.iter() { } + //~^ ERROR it is more idiomatic to loop over `&hm` + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION &hm + let bt: BTreeMap<(), ()> = BTreeMap::new(); - for _v in bt.iter() { } //~ERROR it is more idiomatic to loop over `&bt` + for _v in bt.iter() { } + //~^ ERROR it is more idiomatic to loop over `&bt` + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION &bt + let hs: HashSet<()> = HashSet::new(); - for _v in hs.iter() { } //~ERROR it is more idiomatic to loop over `&hs` + for _v in hs.iter() { } + //~^ ERROR it is more idiomatic to loop over `&hs` + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION &hs + let bs: BTreeSet<()> = BTreeSet::new(); - for _v in bs.iter() { } //~ERROR it is more idiomatic to loop over `&bs` + for _v in bs.iter() { } + //~^ ERROR it is more idiomatic to loop over `&bs` + //~| HELP to write this more concisely, try looping over + //~| SUGGESTION &bs + for _v in vec.iter().next() { } //~ERROR you are iterating over `Iterator::next()` From 6760b35e57c147fed70c693f952920c674a0eda4 Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Sat, 28 Jan 2017 14:12:35 +0100 Subject: [PATCH 2/4] Increase suggestion tests' context --- tests/compile-fail/for_loop.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 5e13b778329..2712d4a1023 100644 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -294,18 +294,18 @@ fn main() { for _v in vec.iter() { } //~^ ERROR it is more idiomatic to loop over `&vec` //~| HELP to write this more concisely, try looping over - //~| SUGGESTION &vec + //~| SUGGESTION for _v in &vec { for _v in vec.iter_mut() { } //~^ ERROR it is more idiomatic to loop over `&mut vec` //~| HELP to write this more concisely, try looping over - //~| SUGGESTION vec + //~| SUGGESTION for _v in &mut vec { let out_vec = vec![1,2,3]; for _v in out_vec.into_iter() { } //~^ ERROR it is more idiomatic to loop over `out_vec` instead of `out_vec.into_iter()` //~| HELP to write this more concisely, try looping over - //~| SUGGESTION out_vec + //~| SUGGESTION for _v in out_vec { for _v in &vec { } // these are fine for _v in &mut vec { } // these are fine @@ -313,14 +313,14 @@ fn main() { for _v in [1, 2, 3].iter() { } //~^ ERROR it is more idiomatic to loop over `&[ //~| HELP to write this more concisely, try looping over - //~| SUGGESTION &[1, 2, 3] + //~| SUGGESTION for _v in &[1, 2, 3] { for _v in (&mut [1, 2, 3]).iter() { } // no error for _v in [0; 32].iter() {} //~^ ERROR it is more idiomatic to loop over `&[ //~| HELP to write this more concisely, try looping over - //~| SUGGESTION &[0; 32] + //~| SUGGESTION for _v in &[0; 32] { for _v in [0; 33].iter() {} // no error @@ -328,43 +328,43 @@ fn main() { for _v in ll.iter() { } //~^ ERROR it is more idiomatic to loop over `&ll` //~| HELP to write this more concisely, try looping over - //~| SUGGESTION &ll + //~| SUGGESTION for _v in &ll { let vd: VecDeque<()> = VecDeque::new(); for _v in vd.iter() { } //~^ ERROR it is more idiomatic to loop over `&vd` //~| HELP to write this more concisely, try looping over - //~| SUGGESTION &vd + //~| SUGGESTION for _v in &vd { let bh: BinaryHeap<()> = BinaryHeap::new(); for _v in bh.iter() { } //~^ ERROR it is more idiomatic to loop over `&bh` //~| HELP to write this more concisely, try looping over - //~| SUGGESTION &bh + //~| SUGGESTION for _v in &bh { let hm: HashMap<(), ()> = HashMap::new(); for _v in hm.iter() { } //~^ ERROR it is more idiomatic to loop over `&hm` //~| HELP to write this more concisely, try looping over - //~| SUGGESTION &hm + //~| SUGGESTION for _v in &hm { let bt: BTreeMap<(), ()> = BTreeMap::new(); for _v in bt.iter() { } //~^ ERROR it is more idiomatic to loop over `&bt` //~| HELP to write this more concisely, try looping over - //~| SUGGESTION &bt + //~| SUGGESTION for _v in &bt { let hs: HashSet<()> = HashSet::new(); for _v in hs.iter() { } //~^ ERROR it is more idiomatic to loop over `&hs` //~| HELP to write this more concisely, try looping over - //~| SUGGESTION &hs + //~| SUGGESTION for _v in &hs { let bs: BTreeSet<()> = BTreeSet::new(); for _v in bs.iter() { } //~^ ERROR it is more idiomatic to loop over `&bs` //~| HELP to write this more concisely, try looping over - //~| SUGGESTION &bs + //~| SUGGESTION for _v in &bs { for _v in vec.iter().next() { } //~ERROR you are iterating over `Iterator::next()` From 2ba03c846733aebeaff7d17977eac8899d27f7fa Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Sat, 28 Jan 2017 14:17:37 +0100 Subject: [PATCH 3/4] Make Travis' rustfmt happy --- clippy_lints/src/loops.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index e3492fd00f7..f3dd2d4b8e3 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -616,9 +616,7 @@ fn check_for_loop_arg(cx: &LateContext, pat: &Pat, arg: &Expr, expr: &Expr) { object, method_name), |db| { - db.span_suggestion(arg.span, - "to write this more concisely, try looping over", - object.to_string()); + db.span_suggestion(arg.span, "to write this more concisely, try looping over", object.to_string()); }); } else if &*method_name.as_str() == "next" && match_trait_method(cx, arg, &paths::ITERATOR) { From 2357dfe8ee709c827d47e4f29d29516d666ce8a8 Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Sat, 28 Jan 2017 14:17:47 +0100 Subject: [PATCH 4/4] Remove useless part of test --- tests/compile-fail/for_loop.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 2712d4a1023..09b0431c656 100644 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -286,11 +286,6 @@ fn main() { id_col[i] = 1f64; } - // This is fine. - for i in (10..0).map(|x| x * 2) { - println!("{}", i); - } - for _v in vec.iter() { } //~^ ERROR it is more idiomatic to loop over `&vec` //~| HELP to write this more concisely, try looping over