Auto merge of #3861 - flip1995:rollup, r=flip1995

Rollup of 3 pull requests

Successful merges:

 - #3851 (Refactor: Extract `trait_ref_of_method` function)
 - #3852 (Refactor: Cleanup one part of assign_ops lint)
 - #3857 (Document match_path, improve match_qpath docs)

Failed merges:

r? @ghost

clippy_lints/src/assign_ops.rs

@@ -1,4 +1,6 @@
-use crate::utils::{get_trait_def_id, implements_trait, snippet_opt, span_lint_and_then, SpanlessEq};
+use crate::utils::{
+    get_trait_def_id, implements_trait, snippet_opt, span_lint_and_then, trait_ref_of_method, SpanlessEq,
+};
 use crate::utils::{higher, sugg};
 use if_chain::if_chain;
 use rustc::hir;
@@ -68,52 +70,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps {
         match &expr.node {
             hir::ExprKind::AssignOp(op, lhs, rhs) => {
                 if let hir::ExprKind::Binary(binop, l, r) = &rhs.node {
-                    if op.node == binop.node {
+                    if op.node != binop.node {
-                        let lint = |assignee: &hir::Expr, rhs_other: &hir::Expr| {
+                        return;
-                            span_lint_and_then(
-                                cx,
-                                MISREFACTORED_ASSIGN_OP,
-                                expr.span,
-                                "variable appears on both sides of an assignment operation",
-                                |db| {
-                                    if let (Some(snip_a), Some(snip_r)) =
-                                        (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span))
-                                    {
-                                        let a = &sugg::Sugg::hir(cx, assignee, "..");
-                                        let r = &sugg::Sugg::hir(cx, rhs, "..");
-                                        let long =
-                                            format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r));
-                                        db.span_suggestion(
-                                            expr.span,
-                                            &format!(
-                                                "Did you mean {} = {} {} {} or {}? Consider replacing it with",
-                                                snip_a,
-                                                snip_a,
-                                                op.node.as_str(),
-                                                snip_r,
-                                                long
-                                            ),
-                                            format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
-                                            Applicability::MachineApplicable,
-                                        );
-                                        db.span_suggestion(
-                                            expr.span,
-                                            "or",
-                                            long,
-                                            Applicability::MachineApplicable, // snippet
-                                        );
                     }
-                                },
-                            );
-                        };
                     // lhs op= l op r
                     if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, l) {
-                            lint(lhs, r);
+                        lint_misrefactored_assign_op(cx, expr, *op, rhs, lhs, r);
                     }
                     // lhs op= l commutative_op r
                     if is_commutative(op.node) && SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, r) {
-                            lint(lhs, l);
+                        lint_misrefactored_assign_op(cx, expr, *op, rhs, lhs, l);
-                        }
                     }
                 }
             },
@@ -140,13 +106,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps {
                                         };
                                         // check that we are not inside an `impl AssignOp` of this exact operation
                                         let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id);
-                                        let parent_impl = cx.tcx.hir().get_parent_item(parent_fn);
-                                        // the crate node is the only one that is not in the map
                                         if_chain! {
-                                            if parent_impl != hir::CRATE_HIR_ID;
+                                            if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
-                                            if let hir::Node::Item(item) = cx.tcx.hir().get_by_hir_id(parent_impl);
-                                            if let hir::ItemKind::Impl(_, _, _, _, Some(trait_ref), _, _) =
-                                                &item.node;
                                             if trait_ref.path.def.def_id() == trait_id;
                                             then { return; }
                                         }
@@ -234,6 +195,48 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps {
     }
 }
 
+fn lint_misrefactored_assign_op(
+    cx: &LateContext<'_, '_>,
+    expr: &hir::Expr,
+    op: hir::BinOp,
+    rhs: &hir::Expr,
+    assignee: &hir::Expr,
+    rhs_other: &hir::Expr,
+) {
+    span_lint_and_then(
+        cx,
+        MISREFACTORED_ASSIGN_OP,
+        expr.span,
+        "variable appears on both sides of an assignment operation",
+        |db| {
+            if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span)) {
+                let a = &sugg::Sugg::hir(cx, assignee, "..");
+                let r = &sugg::Sugg::hir(cx, rhs, "..");
+                let long = format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r));
+                db.span_suggestion(
+                    expr.span,
+                    &format!(
+                        "Did you mean {} = {} {} {} or {}? Consider replacing it with",
+                        snip_a,
+                        snip_a,
+                        op.node.as_str(),
+                        snip_r,
+                        long
+                    ),
+                    format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
+                    Applicability::MachineApplicable,
+                );
+                db.span_suggestion(
+                    expr.span,
+                    "or",
+                    long,
+                    Applicability::MachineApplicable, // snippet
+                );
+            }
+        },
+    );
+}
+
 fn is_commutative(op: hir::BinOpKind) -> bool {
     use rustc::hir::BinOpKind::*;
     match op {

clippy_lints/src/missing_const_for_fn.rs

@@ -1,5 +1,4 @@
-use crate::utils::{is_entrypoint_fn, span_lint};
+use crate::utils::{is_entrypoint_fn, span_lint, trait_ref_of_method};
-use if_chain::if_chain;
 use rustc::hir;
 use rustc::hir::intravisit::FnKind;
 use rustc::hir::{Body, Constness, FnDecl, HirId};
@@ -96,7 +95,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn {
                 }
             },
             FnKind::Method(_, sig, ..) => {
-                if is_trait_method(cx, hir_id) || already_const(sig.header) {
+                if trait_ref_of_method(cx, hir_id).is_some() || already_const(sig.header) {
                     return;
                 }
             },
@@ -115,18 +114,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn {
     }
 }
 
-fn is_trait_method(cx: &LateContext<'_, '_>, hir_id: HirId) -> bool {
-    // Get the implemented trait for the current function
-    let parent_impl = cx.tcx.hir().get_parent_item(hir_id);
-    if_chain! {
-        if parent_impl != hir::CRATE_HIR_ID;
-        if let hir::Node::Item(item) = cx.tcx.hir().get_by_hir_id(parent_impl);
-        if let hir::ItemKind::Impl(_, _, _, _, Some(_trait_ref), _, _) = &item.node;
-        then { return true; }
-    }
-    false
-}
-
 // We don't have to lint on something that's already `const`
 fn already_const(header: hir::FnHeader) -> bool {
     header.constness == Constness::Const

clippy_lints/src/suspicious_trait_impl.rs

@@ -1,4 +1,4 @@
-use crate::utils::{get_trait_def_id, span_lint};
+use crate::utils::{get_trait_def_id, span_lint, trait_ref_of_method};
 use if_chain::if_chain;
 use rustc::hir;
 use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
@@ -177,12 +177,9 @@ fn check_binop<'a>(
 
     // Get the actually implemented trait
     let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id);
-    let parent_impl = cx.tcx.hir().get_parent_item(parent_fn);
 
     if_chain! {
-        if parent_impl != hir::CRATE_HIR_ID;
+        if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
-        if let hir::Node::Item(item) = cx.tcx.hir().get_by_hir_id(parent_impl);
-        if let hir::ItemKind::Impl(_, _, _, _, Some(ref trait_ref), _, _) = item.node;
         if let Some(idx) = trait_ids.iter().position(|&tid| tid == trait_ref.path.def.def_id());
         if binop != expected_ops[idx];
         then{

clippy_lints/src/utils/mod.rs

@@ -190,7 +190,10 @@ pub fn single_segment_path(path: &QPath) -> Option<&PathSegment> {
     }
 }
 
-/// Match a `Path` against a slice of segment string literals.
+/// Match a `QPath` against a slice of segment string literals.
+///
+/// There is also `match_path` if you are dealing with a `rustc::hir::Path` instead of a
+/// `rustc::hir::QPath`.
 ///
 /// # Examples
 /// ```rust,ignore
@@ -210,6 +213,22 @@ pub fn match_qpath(path: &QPath, segments: &[&str]) -> bool {
     }
 }
 
+/// Match a `Path` against a slice of segment string literals.
+///
+/// There is also `match_qpath` if you are dealing with a `rustc::hir::QPath` instead of a
+/// `rustc::hir::Path`.
+///
+/// # Examples
+///
+/// ```rust,ignore
+/// if match_path(&trait_ref.path, &paths::HASH) {
+///     // This is the `std::hash::Hash` trait.
+/// }
+///
+/// if match_path(ty_path, &["rustc", "lint", "Lint"]) {
+///     // This is a `rustc::lint::Lint`.
+/// }
+/// ```
 pub fn match_path(path: &Path, segments: &[&str]) -> bool {
     path.segments
         .iter()
@@ -301,6 +320,33 @@ pub fn implements_trait<'a, 'tcx>(
         .enter(|infcx| infcx.predicate_must_hold_modulo_regions(&obligation))
 }
 
+/// Get the `hir::TraitRef` of the trait the given method is implemented for
+///
+/// Use this if you want to find the `TraitRef` of the `Add` trait in this example:
+///
+/// ```rust
+/// struct Point(isize, isize);
+///
+/// impl std::ops::Add for Point {
+///     type Output = Self;
+///
+///     fn add(self, other: Self) -> Self {
+///         Point(0, 0)
+///     }
+/// }
+/// ```
+pub fn trait_ref_of_method(cx: &LateContext<'_, '_>, hir_id: HirId) -> Option<TraitRef> {
+    // Get the implemented trait for the current function
+    let parent_impl = cx.tcx.hir().get_parent_item(hir_id);
+    if_chain! {
+        if parent_impl != hir::CRATE_HIR_ID;
+        if let hir::Node::Item(item) = cx.tcx.hir().get_by_hir_id(parent_impl);
+        if let hir::ItemKind::Impl(_, _, _, _, trait_ref, _, _) = &item.node;
+        then { return trait_ref.clone(); }
+    }
+    None
+}
+
 /// Check whether this type implements Drop.
 pub fn has_drop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
     match ty.ty_adt_def() {