Merge pull request #1060 from Manishearth/sugg

Improve suggestions

Author: llogiq

CHANGELOG.md

@@ -1,7 +1,10 @@
 # Change Log
 All notable changes to this project will be documented in this file.
 
-## 0.0.78 - 2016-07-02
+## 0.0.79 — ?
+* Major suggestions refactoring
+
+## 0.0.78 — 2016-07-02
 * Rustup to *rustc 1.11.0-nightly (01411937f 2016-07-01)*
 * New lints: [`wrong_transmute`, `double_neg`]
 * For compatibility, `cargo clippy` does not defines the `clippy` feature

README.md

@@ -25,7 +25,7 @@ name
 [almost_swapped](https://github.com/Manishearth/rust-clippy/wiki#almost_swapped)                                     | warn    | `foo = bar; bar = foo` sequence
 [approx_constant](https://github.com/Manishearth/rust-clippy/wiki#approx_constant)                                   | warn    | the approximate of a known float constant (in `std::f64::consts` or `std::f32::consts`) is found; suggests to use the constant
 [assign_op_pattern](https://github.com/Manishearth/rust-clippy/wiki#assign_op_pattern)                               | warn    | assigning the result of an operation on a variable to that same variable
-[assign_ops](https://github.com/Manishearth/rust-clippy/wiki#assign_ops)                                             | allow   | Any assignment operation
+[assign_ops](https://github.com/Manishearth/rust-clippy/wiki#assign_ops)                                             | allow   | any assignment operation
 [bad_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#bad_bit_mask)                                         | warn    | expressions of the form `_ & mask == select` that will only ever return `true` or `false` (because in the example `select` containing bits that `mask` doesn't have)
 [blacklisted_name](https://github.com/Manishearth/rust-clippy/wiki#blacklisted_name)                                 | warn    | usage of a blacklisted/placeholder name
 [block_in_if_condition_expr](https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_expr)             | warn    | braces can be eliminated in conditions that are expressions, e.g `if { true } ...`

clippy_lints/src/array_indexing.rs

@@ -6,7 +6,7 @@ use rustc_const_eval::eval_const_expr_partial;
 use rustc_const_math::ConstInt;
 use rustc::hir::*;
 use syntax::ast::RangeLimits;
-use utils;
+use utils::{self, higher};
 
 /// **What it does:** Check for out of bounds array indexing with a constant index.
 ///
@@ -77,7 +77,7 @@ impl LateLintPass for ArrayIndexing {
                 }
 
                 // Index is a constant range
-                if let Some(range) = utils::unsugar_range(index) {
+                if let Some(range) = higher::range(index) {
                     let start = range.start
                         .map(|start| eval_const_expr_partial(cx.tcx, start, ExprTypeChecked, None))
                         .map(|v| v.ok());
@@ -94,7 +94,7 @@ impl LateLintPass for ArrayIndexing {
                 }
             }
 
-            if let Some(range) = utils::unsugar_range(index) {
+            if let Some(range) = higher::range(index) {
                 // Full ranges are always valid
                 if range.start.is_none() && range.end.is_none() {
                     return;

clippy_lints/src/assign_ops.rs

@@ -1,28 +1,29 @@
 use rustc::hir;
 use rustc::lint::*;
 use utils::{span_lint_and_then, span_lint, snippet_opt, SpanlessEq, get_trait_def_id, implements_trait};
+use utils::{higher, sugg};
 
-/// **What it does:** This lint checks for `+=` operations and similar
+/// **What it does:** This lint checks for `+=` operations and similar.
 ///
-/// **Why is this bad?** Projects with many developers from languages without those operations
-///                      may find them unreadable and not worth their weight
+/// **Why is this bad?** Projects with many developers from languages without those operations may
+/// find them unreadable and not worth their weight.
 ///
-/// **Known problems:** Types implementing `OpAssign` don't necessarily implement `Op`
+/// **Known problems:** Types implementing `OpAssign` don't necessarily implement `Op`.
 ///
 /// **Example:**
 /// ```
 /// a += 1;
 /// ```
 declare_restriction_lint! {
     pub ASSIGN_OPS,
-    "Any assignment operation"
+    "any assignment operation"
 }
 
-/// **What it does:** Check for `a = a op b` or `a = b commutative_op a` patterns
+/// **What it does:** Check for `a = a op b` or `a = b commutative_op a` patterns.
 ///
-/// **Why is this bad?** These can be written as the shorter `a op= b`
+/// **Why is this bad?** These can be written as the shorter `a op= b`.
 ///
-/// **Known problems:** While forbidden by the spec, `OpAssign` traits may have implementations that differ from the regular `Op` impl
+/// **Known problems:** While forbidden by the spec, `OpAssign` traits may have implementations that differ from the regular `Op` impl.
 ///
 /// **Example:**
 ///
@@ -50,24 +51,14 @@ impl LateLintPass for AssignOps {
     fn check_expr(&mut self, cx: &LateContext, expr: &hir::Expr) {
         match expr.node {
             hir::ExprAssignOp(op, ref lhs, ref rhs) => {
-                if let (Some(l), Some(r)) = (snippet_opt(cx, lhs.span), snippet_opt(cx, rhs.span)) {
-                    span_lint_and_then(cx, ASSIGN_OPS, expr.span, "assign operation detected", |db| {
-                        match rhs.node {
-                            hir::ExprBinary(op2, _, _) if op2 != op => {
-                                db.span_suggestion(expr.span,
-                                                   "replace it with",
-                                                   format!("{} = {} {} ({})", l, l, op.node.as_str(), r));
-                            }
-                            _ => {
-                                db.span_suggestion(expr.span,
-                                                   "replace it with",
-                                                   format!("{} = {} {} {}", l, l, op.node.as_str(), r));
-                            }
-                        }
-                    });
-                } else {
-                    span_lint(cx, ASSIGN_OPS, expr.span, "assign operation detected");
-                }
+                span_lint_and_then(cx, ASSIGN_OPS, expr.span, "assign operation detected", |db| {
+                    let lhs = &sugg::Sugg::hir(cx, lhs, "..");
+                    let rhs = &sugg::Sugg::hir(cx, rhs, "..");
+
+                    db.span_suggestion(expr.span,
+                                       "replace it with",
+                                       format!("{} = {}", lhs, sugg::make_binop(higher::binop(op.node), lhs, rhs)));
+                });
             }
             hir::ExprAssign(ref assignee, ref e) => {
                 if let hir::ExprBinary(op, ref l, ref r) = e.node {

clippy_lints/src/collapsible_if.rs

@@ -13,11 +13,10 @@
 //! This lint is **warn** by default
 
 use rustc::lint::*;
-use std::borrow::Cow;
-use syntax::codemap::Spanned;
 use syntax::ast;
 
-use utils::{in_macro, snippet, snippet_block, span_lint_and_then};
+use utils::{in_macro, snippet_block, span_lint_and_then};
+use utils::sugg::Sugg;
 
 /// **What it does:** This lint checks for nested `if`-statements which can be collapsed by
 /// `&&`-combining their conditions and for `else { if .. }` expressions that can be collapsed to
@@ -104,31 +103,17 @@ fn check_collapsible_no_if_let(
             return;
         }
         span_lint_and_then(cx, COLLAPSIBLE_IF, expr.span, "this if statement can be collapsed", |db| {
+            let lhs = Sugg::ast(cx, check, "..");
+            let rhs = Sugg::ast(cx, check_inner, "..");
             db.span_suggestion(expr.span,
                                "try",
-                               format!("if {} && {} {}",
-                                       check_to_string(cx, check),
-                                       check_to_string(cx, check_inner),
+                               format!("if {} {}",
+                                       lhs.and(rhs),
                                        snippet_block(cx, content.span, "..")));
         });
     }}
 }
 
-fn requires_brackets(e: &ast::Expr) -> bool {
-    match e.node {
-        ast::ExprKind::Binary(Spanned { node: n, .. }, _, _) if n == ast::BinOpKind::Eq => false,
-        _ => true,
-    }
-}
-
-fn check_to_string(cx: &EarlyContext, e: &ast::Expr) -> Cow<'static, str> {
-    if requires_brackets(e) {
-        format!("({})", snippet(cx, e.span, "..")).into()
-    } else {
-        snippet(cx, e.span, "..")
-    }
-}
-
 /// If the block contains only one expression, returns it.
 fn expr_block(block: &ast::Block) -> Option<&ast::Expr> {
     let mut it = block.stmts.iter();

clippy_lints/src/entry.rs

@@ -121,21 +121,21 @@ impl<'a, 'tcx, 'v, 'b> Visitor<'v> for InsertVisitor<'a, 'tcx, 'b> {
             SpanlessEq::new(self.cx).eq_expr(self.key, &params[1])
         ], {
             span_lint_and_then(self.cx, MAP_ENTRY, self.span,
-                               &format!("usage of `contains_key` followed by `insert` on `{}`", self.ty), |db| {
+                               &format!("usage of `contains_key` followed by `insert` on a `{}`", self.ty), |db| {
                 if self.sole_expr {
                     let help = format!("{}.entry({}).or_insert({})",
                                        snippet(self.cx, self.map.span, "map"),
                                        snippet(self.cx, params[1].span, ".."),
                                        snippet(self.cx, params[2].span, ".."));
 
-                    db.span_suggestion(self.span, "Consider using", help);
+                    db.span_suggestion(self.span, "consider using", help);
                 }
                 else {
-                    let help = format!("Consider using `{}.entry({})`",
+                    let help = format!("{}.entry({})",
                                        snippet(self.cx, self.map.span, "map"),
                                        snippet(self.cx, params[1].span, ".."));
 
-                    db.span_note(self.span, &help);
+                    db.span_suggestion(self.span, "consider using", help);
                 }
             });
         }}

clippy_lints/src/formatting.rs

@@ -148,7 +148,7 @@ fn check_consecutive_ifs(cx: &EarlyContext, first: &ast::Expr, second: &ast::Exp
     }
 }
 
-/// Match `if` or `else if` expressions and return the `then` and `else` block.
+/// Match `if` or `if let` expressions and return the `then` and `else` block.
 fn unsugar_if(expr: &ast::Expr) -> Option<(&P<ast::Block>, &Option<P<ast::Expr>>)> {
     match expr.node {
         ast::ExprKind::If(_, ref then, ref else_) |

clippy_lints/src/lib.rs

@@ -3,6 +3,7 @@
 #![feature(box_syntax)]
 #![feature(collections)]
 #![feature(custom_attribute)]
+#![feature(dotdot_in_tuple_patterns)]
 #![feature(question_mark)]
 #![feature(rustc_private)]
 #![feature(slice_patterns)]
@@ -36,6 +37,7 @@ extern crate quine_mc_cluskey;
 
 extern crate rustc_serialize;
 
+extern crate rustc_errors;
 extern crate rustc_plugin;
 extern crate rustc_const_eval;
 extern crate rustc_const_math;

clippy_lints/src/loops.rs

@@ -9,15 +9,14 @@ use rustc::middle::region::CodeExtent;
 use rustc::ty;
 use rustc_const_eval::EvalHint::ExprTypeChecked;
 use rustc_const_eval::eval_const_expr_partial;
-use std::borrow::Cow;
 use std::collections::HashMap;
 use syntax::ast;
+use utils::sugg;
 
-use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, in_external_macro,
-            span_help_and_lint, is_integer_literal, get_enclosing_block, span_lint_and_then, unsugar_range,
-            walk_ptrs_ty, recover_for_loop};
+use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, multispan_sugg, in_external_macro,
+            span_help_and_lint, is_integer_literal, get_enclosing_block, span_lint_and_then, higher,
+            walk_ptrs_ty};
 use utils::paths;
-use utils::UnsugaredRange;
 
 /// **What it does:** This lint checks for looping over the range of `0..len` of some collection just to get the values by index.
 ///
@@ -224,7 +223,7 @@ impl LintPass for Pass {
 
 impl LateLintPass for Pass {
     fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
-        if let Some((pat, arg, body)) = recover_for_loop(expr) {
+        if let Some((pat, arg, body)) = higher::for_loop(expr) {
             check_for_loop(cx, pat, arg, body, expr);
         }
         // check for `loop { if let {} else break }` that could be `while let`
@@ -333,7 +332,7 @@ fn check_for_loop(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &E
 /// Check for looping over a range and then indexing a sequence with it.
 /// The iteratee must be a range literal.
 fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &Expr) {
-    if let Some(UnsugaredRange { start: Some(ref start), ref end, .. }) = unsugar_range(arg) {
+    if let Some(higher::Range { start: Some(ref start), ref end, limits }) = higher::range(arg) {
         // the var must be a single name
         if let PatKind::Binding(_, ref ident, _) = pat.node {
             let mut visitor = VarVisitor {
@@ -361,49 +360,58 @@ fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, ex
 
                 let starts_at_zero = is_integer_literal(start, 0);
 
-                let skip: Cow<_> = if starts_at_zero {
-                    "".into()
+                let skip = if starts_at_zero {
+                    "".to_owned()
                 } else {
-                    format!(".skip({})", snippet(cx, start.span, "..")).into()
+                    format!(".skip({})", snippet(cx, start.span, ".."))
                 };
 
-                let take: Cow<_> = if let Some(ref end) = *end {
+                let take = if let Some(ref end) = *end {
                     if is_len_call(end, &indexed) {
-                        "".into()
+                        "".to_owned()
                     } else {
-                        format!(".take({})", snippet(cx, end.span, "..")).into()
+                        match limits {
+                            ast::RangeLimits::Closed => {
+                                let end = sugg::Sugg::hir(cx, end, "<count>");
+                                format!(".take({})", end + sugg::ONE)
+                            }
+                            ast::RangeLimits::HalfOpen => {
+                                format!(".take({})", snippet(cx, end.span, ".."))
+                            }
+                        }
                     }
                 } else {
-                    "".into()
+                    "".to_owned()
                 };
 
                 if visitor.nonindex {
-                    span_lint(cx,
-                              NEEDLESS_RANGE_LOOP,
-                              expr.span,
-                              &format!("the loop variable `{}` is used to index `{}`. Consider using `for ({}, \
-                                        item) in {}.iter().enumerate(){}{}` or similar iterators",
-                                       ident.node,
-                                       indexed,
-                                       ident.node,
-                                       indexed,
-                                       take,
-                                       skip));
+                    span_lint_and_then(cx,
+                                       NEEDLESS_RANGE_LOOP,
+                                       expr.span,
+                                       &format!("the loop variable `{}` is used to index `{}`", ident.node, indexed),
+                                       |db| {
+                        multispan_sugg(db, "consider using an iterator".to_string(), &[
+                            (pat.span, &format!("({}, <item>)", ident.node)),
+                            (arg.span, &format!("{}.iter().enumerate(){}{}", indexed, take, skip)),
+                        ]);
+                    });
                 } else {
                     let repl = if starts_at_zero && take.is_empty() {
                         format!("&{}", indexed)
                     } else {
                         format!("{}.iter(){}{}", indexed, take, skip)
                     };
 
-                    span_lint(cx,
-                              NEEDLESS_RANGE_LOOP,
-                              expr.span,
-                              &format!("the loop variable `{}` is only used to index `{}`. \
-                                        Consider using `for item in {}` or similar iterators",
-                                       ident.node,
-                                       indexed,
-                                       repl));
+                    span_lint_and_then(cx,
+                                       NEEDLESS_RANGE_LOOP,
+                                       expr.span,
+                                       &format!("the loop variable `{}` is only used to index `{}`.", ident.node, indexed),
+                                       |db| {
+                        multispan_sugg(db, "consider using an iterator".to_string(), &[
+                            (pat.span, "<item>"),
+                            (arg.span, &repl),
+                        ]);
+                    });
                 }
             }
         }
@@ -427,7 +435,7 @@ fn is_len_call(expr: &Expr, var: &Name) -> bool {
 
 fn check_for_loop_reverse_range(cx: &LateContext, arg: &Expr, expr: &Expr) {
     // if this for loop is iterating over a two-sided range...
-    if let Some(UnsugaredRange { start: Some(ref start), end: Some(ref end), limits }) = unsugar_range(arg) {
+    if let Some(higher::Range { start: Some(ref start), end: Some(ref end), limits }) = higher::range(arg) {
         // ...and both sides are compile-time constant integers...
         if let Ok(start_idx) = eval_const_expr_partial(cx.tcx, start, ExprTypeChecked, None) {
             if let Ok(end_idx) = eval_const_expr_partial(cx.tcx, end, ExprTypeChecked, None) {
@@ -588,33 +596,34 @@ fn check_for_loop_explicit_counter(cx: &LateContext, arg: &Expr, body: &Expr, ex
 
 /// Check for the `FOR_KV_MAP` lint.
 fn check_for_loop_over_map_kv(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &Expr) {
+    let pat_span = pat.span;
+
     if let PatKind::Tuple(ref pat, _) = pat.node {
         if pat.len() == 2 {
-            let (pat_span, kind) = match (&pat[0].node, &pat[1].node) {
-                (key, _) if pat_is_wild(key, body) => (&pat[1].span, "values"),
-                (_, value) if pat_is_wild(value, body) => (&pat[0].span, "keys"),
+            let (new_pat_span, kind) = match (&pat[0].node, &pat[1].node) {
+                (key, _) if pat_is_wild(key, body) => (pat[1].span, "value"),
+                (_, value) if pat_is_wild(value, body) => (pat[0].span, "key"),
                 _ => return,
             };
 
-            let arg_span = match arg.node {
-                ExprAddrOf(MutImmutable, ref expr) => expr.span,
+            let (arg_span, arg) = match arg.node {
+                ExprAddrOf(MutImmutable, ref expr) => (arg.span, &**expr),
                 ExprAddrOf(MutMutable, _) => return, // for _ in &mut _, there is no {values,keys}_mut method
-                _ => arg.span,
+                _ => (arg.span, arg),
             };
 
             let ty = walk_ptrs_ty(cx.tcx.expr_ty(arg));
             if match_type(cx, ty, &paths::HASHMAP) || match_type(cx, ty, &paths::BTREEMAP) {
                 span_lint_and_then(cx,
                                    FOR_KV_MAP,
                                    expr.span,
-                                   &format!("you seem to want to iterate on a map's {}", kind),
+                                   &format!("you seem to want to iterate on a map's {}s", kind),
                                    |db| {
-                    db.span_suggestion(expr.span,
-                                       "use the corresponding method",
-                                       format!("for {} in {}.{}() {{ .. }}",
-                                               snippet(cx, *pat_span, ".."),
-                                               snippet(cx, arg_span, ".."),
-                                               kind));
+                    let map = sugg::Sugg::hir(cx, arg, "map");
+                    multispan_sugg(db, "use the corresponding method".into(), &[
+                        (pat_span, &snippet(cx, new_pat_span, kind)),
+                        (arg_span, &format!("{}.{}s()", map.maybe_par(), kind)),
+                    ]);
                 });
             }
         }