Merge pull request #1082 from Manishearth/small-fixes

Small fixes

Author: Manishearth

clippy_lints/src/copies.rs

@@ -6,7 +6,7 @@ use std::collections::hash_map::Entry;
 use syntax::parse::token::InternedString;
 use syntax::util::small_vector::SmallVector;
 use utils::{SpanlessEq, SpanlessHash};
-use utils::{get_parent_expr, in_macro, span_note_and_lint};
+use utils::{get_parent_expr, in_macro, span_lint_and_then, span_note_and_lint, snippet};
 
 /// **What it does:** This lint checks for consecutive `ifs` with the same condition. This lint is
 /// `Warn` by default.
@@ -42,7 +42,8 @@ declare_lint! {
 /// purpose, you can factor them
 /// [using `|`](https://doc.rust-lang.org/book/patterns.html#multiple-patterns).
 ///
-/// **Known problems:** Hopefully none.
+/// **Known problems:** False positive possible with order dependent `match`
+/// (see issue [#860](https://github.com/Manishearth/rust-clippy/issues/860)).
 ///
 /// **Example:**
 /// ```rust,ignore
@@ -52,6 +53,23 @@ declare_lint! {
 ///     Baz => bar(), // <= oops
 /// }
 /// ```
+///
+/// This should probably be
+/// ```rust,ignore
+/// match foo {
+///     Bar => bar(),
+///     Quz => quz(),
+///     Baz => baz(), // <= fixed
+/// }
+/// ```
+///
+/// or if the original code was not a typo:
+/// ```rust,ignore
+/// match foo {
+///     Bar | Baz => bar(), // <= shows the intent better
+///     Quz => quz(),
+/// }
+/// ```
 declare_lint! {
     pub MATCH_SAME_ARMS,
     Warn,
@@ -143,12 +161,25 @@ fn lint_match_arms(cx: &LateContext, expr: &Expr) {
 
     if let ExprMatch(_, ref arms, MatchSource::Normal) = expr.node {
         if let Some((i, j)) = search_same(arms, hash, eq) {
-            span_note_and_lint(cx,
+            span_lint_and_then(cx,
                                MATCH_SAME_ARMS,
                                j.body.span,
                                "this `match` has identical arm bodies",
-                               i.body.span,
-                               "same as this");
+                               |db| {
+                db.span_note(i.body.span, "same as this");
+
+                // Note: this does not use `span_suggestion` on purpose: there is no clean way to
+                // remove the other arm. Building a span and suggest to replace it to "" makes an
+                // even more confusing error message. Also in order not to make up a span for the
+                // whole pattern, the suggestion is only shown when there is only one pattern. The
+                // user should know about `|` if they are already using it…
+
+                if i.pats.len() == 1 && j.pats.len() == 1 {
+                    let lhs = snippet(cx, i.pats[0].span, "<pat1>");
+                    let rhs = snippet(cx, j.pats[0].span, "<pat2>");
+                    db.span_note(i.body.span, &format!("consider refactoring into `{} | {}`", lhs, rhs));
+                }
+            });
         }
     }
 }

clippy_lints/src/loops.rs

@@ -1,6 +1,7 @@
 use reexport::*;
 use rustc::hir::*;
 use rustc::hir::def::Def;
+use rustc::hir::def_id::DefId;
 use rustc::hir::intravisit::{Visitor, walk_expr, walk_block, walk_decl};
 use rustc::hir::map::Node::NodeBlock;
 use rustc::lint::*;
@@ -337,7 +338,7 @@ fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, ex
         if let PatKind::Binding(_, ref ident, _) = pat.node {
             let mut visitor = VarVisitor {
                 cx: cx,
-                var: ident.node,
+                var: cx.tcx.expect_def(pat.id).def_id(),
                 indexed: HashMap::new(),
                 nonindex: false,
             };
@@ -667,15 +668,15 @@ impl<'a> Visitor<'a> for UsedVisitor {
 
 struct VarVisitor<'v, 't: 'v> {
     cx: &'v LateContext<'v, 't>, // context reference
-    var: Name, // var name to look for as index
+    var: DefId, // var name to look for as index
     indexed: HashMap<Name, Option<CodeExtent>>, // indexed variables, the extent is None for global
     nonindex: bool, // has the var been used otherwise?
 }
 
 impl<'v, 't> Visitor<'v> for VarVisitor<'v, 't> {
     fn visit_expr(&mut self, expr: &'v Expr) {
         if let ExprPath(None, ref path) = expr.node {
-            if path.segments.len() == 1 && path.segments[0].name == self.var {
+            if path.segments.len() == 1 && self.cx.tcx.expect_def(expr.id).def_id() == self.var {
                 // we are referencing our variable! now check if it's as an index
                 if_let_chain! {[
                     let Some(parexpr) = get_parent_expr(self.cx, expr),

clippy_lints/src/new_without_default.rs

@@ -7,15 +7,15 @@ use syntax::ast;
 use syntax::codemap::Span;
 use utils::paths;
 use utils::{get_trait_def_id, implements_trait, in_external_macro, return_ty, same_tys, span_lint_and_then};
+use utils::sugg::DiagnosticBuilderExt;
 
 /// **What it does:** This lints about type with a `fn new() -> Self` method
 /// and no implementation of
 /// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html)
 ///
 /// **Why is this bad?** User might expect to be able to use
 /// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html)
-/// as the type can be
-/// constructed without arguments.
+/// as the type can be constructed without arguments.
 ///
 /// **Known problems:** Hopefully none.
 ///
@@ -118,20 +118,26 @@ impl LateLintPass for NewWithoutDefault {
                                                      `Default` implementation for `{}`",
                                                     self_ty),
                                            |db| {
-                                               db.span_suggestion(span, "try this", "#[derive(Default)]".into());
-                                           });
+                            db.suggest_item_with_attr(cx, span, "try this", "#[derive(Default)]");
+                        });
                     } else {
                         span_lint_and_then(cx,
                                            NEW_WITHOUT_DEFAULT, span,
                                            &format!("you should consider adding a \
                                                     `Default` implementation for `{}`",
                                                     self_ty),
                                            |db| {
-                                               db.span_suggestion(span,
-                                                                  "try this",
-                                                                  format!("impl Default for {} {{ fn default() -> \
-                                                                          Self {{ {}::new() }} }}", self_ty, self_ty));
-                                           });
+                        db.suggest_prepend_item(cx,
+                                                  span,
+                                                  "try this",
+                                                  &format!(
+"impl Default for {} {{
+    fn default() -> Self {{
+        Self::new()
+    }}
+}}",
+                                                           self_ty));
+                        });
                     }
                 }}
             }

clippy_lints/src/utils/sugg.rs

@@ -1,11 +1,14 @@
 use rustc::hir;
-use rustc::lint::{EarlyContext, LateContext};
+use rustc::lint::{EarlyContext, LateContext, LintContext};
+use rustc_errors;
 use std::borrow::Cow;
+use std::fmt::Display;
 use std;
-use syntax::ast;
+use syntax::codemap::{CharPos, Span};
+use syntax::print::pprust::binop_to_string;
 use syntax::util::parser::AssocOp;
+use syntax::ast;
 use utils::{higher, snippet, snippet_opt};
-use syntax::print::pprust::binop_to_string;
 
 /// A helper type to build suggestion correctly handling parenthesis.
 pub enum Sugg<'a> {
@@ -20,7 +23,7 @@ pub enum Sugg<'a> {
 /// Literal constant `1`, for convenience.
 pub const ONE: Sugg<'static> = Sugg::NonParen(Cow::Borrowed("1"));
 
-impl<'a> std::fmt::Display for Sugg<'a> {
+impl<'a> Display for Sugg<'a> {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
         match *self {
             Sugg::NonParen(ref s) | Sugg::MaybeParen(ref s) | Sugg::BinOp(_, ref s) => {
@@ -126,7 +129,7 @@ impl<'a> Sugg<'a> {
     }
 
     /// Convenience method to create the `<lhs> as <rhs>` suggestion.
-    pub fn as_ty<R: std::fmt::Display>(self, rhs: R) -> Sugg<'static> {
+    pub fn as_ty<R: Display>(self, rhs: R) -> Sugg<'static> {
         make_assoc(AssocOp::As, &self, &Sugg::NonParen(rhs.to_string().into()))
     }
 
@@ -198,7 +201,7 @@ impl<T> ParenHelper<T> {
     }
 }
 
-impl<T: std::fmt::Display> std::fmt::Display for ParenHelper<T> {
+impl<T: Display> Display for ParenHelper<T> {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
         if self.paren {
             write!(f, "({})", self.wrapped)
@@ -354,3 +357,83 @@ fn astbinop2assignop(op: ast::BinOp) -> AssocOp {
         And | Eq | Ge | Gt | Le | Lt | Ne | Or => panic!("This operator does not exist"),
     })
 }
+
+/// Return the indentation before `span` if there are nothing but `[ \t]` before it on its line.
+fn indentation<T: LintContext>(cx: &T, span: Span) -> Option<String> {
+    let lo = cx.sess().codemap().lookup_char_pos(span.lo);
+    if let Some(line) = lo.file.get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */) {
+        if let Some((pos, _)) = line.char_indices().find(|&(_, c)| c != ' ' && c != '\t') {
+            // we can mix char and byte positions here because we only consider `[ \t]`
+            if lo.col == CharPos(pos) {
+                Some(line[..pos].into())
+            } else {
+                None
+            }
+        } else {
+            None
+        }
+    } else {
+        None
+    }
+}
+
+pub trait DiagnosticBuilderExt<T: LintContext> {
+    /// Suggests to add an attribute to an item.
+    ///
+    /// Correctly handles indentation of the attribute and item.
+    ///
+    /// # Example
+    ///
+    /// ```rust
+    /// db.suggest_item_with_attr(cx, item, "#[derive(Default)]");
+    /// ```
+    fn suggest_item_with_attr<D: Display+?Sized>(&mut self, cx: &T, item: Span, msg: &str, attr: &D);
+
+    /// Suggest to add an item before another.
+    ///
+    /// The item should not be indented (expect for inner indentation).
+    ///
+    /// # Example
+    ///
+    /// ```rust
+    /// db.suggest_prepend_item(cx, item,
+    /// "fn foo() {
+    ///     bar();
+    /// }");
+    /// ```
+    fn suggest_prepend_item(&mut self, cx: &T, item: Span, msg: &str, new_item: &str);
+}
+
+impl<'a, 'b, T: LintContext> DiagnosticBuilderExt<T> for rustc_errors::DiagnosticBuilder<'b> {
+    fn suggest_item_with_attr<D: Display+?Sized>(&mut self, cx: &T, item: Span, msg: &str, attr: &D) {
+        if let Some(indent) = indentation(cx, item) {
+            let span = Span {
+                hi: item.lo,
+                ..item
+            };
+
+            self.span_suggestion(span, msg, format!("{}\n{}", attr, indent));
+        }
+    }
+
+    fn suggest_prepend_item(&mut self, cx: &T, item: Span, msg: &str, new_item: &str) {
+        if let Some(indent) = indentation(cx, item) {
+            let span = Span {
+                hi: item.lo,
+                ..item
+            };
+
+            let mut first = true;
+            let new_item = new_item.lines().map(|l| {
+                if first {
+                    first = false;
+                    format!("{}\n", l)
+                } else {
+                    format!("{}{}\n", indent, l)
+                }
+            }).collect::<String>();
+
+            self.span_suggestion(span, msg, format!("{}\n{}", new_item, indent));
+        }
+    }
+}