From e1d47cd5f1c8158671d4ae766235afd8a58783cb Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 7 Mar 2019 07:42:38 +0100 Subject: [PATCH 1/7] Refactor: Extract `trait_ref_of_method` function --- clippy_lints/src/assign_ops.rs | 9 ++------- clippy_lints/src/missing_const_for_fn.rs | 17 ++--------------- clippy_lints/src/suspicious_trait_impl.rs | 7 ++----- clippy_lints/src/utils/mod.rs | 21 +++++++++++++++++++++ 4 files changed, 27 insertions(+), 27 deletions(-) diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index b24e0cdfced4..b0bf250023d4 100644 --- a/clippy_lints/src/assign_ops.rs +++ b/clippy_lints/src/assign_ops.rs @@ -1,4 +1,4 @@ -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; @@ -140,13 +140,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 hir::Node::Item(item) = cx.tcx.hir().get_by_hir_id(parent_impl); - if let hir::ItemKind::Impl(_, _, _, _, Some(trait_ref), _, _) = - &item.node; + if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn); if trait_ref.path.def.def_id() == trait_id; then { return; } } diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs index 19757f32e6b4..5bc949a6688a 100644 --- a/clippy_lints/src/missing_const_for_fn.rs +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -1,5 +1,4 @@ -use crate::utils::{is_entrypoint_fn, span_lint}; -use if_chain::if_chain; +use crate::utils::{is_entrypoint_fn, span_lint, trait_ref_of_method}; 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 diff --git a/clippy_lints/src/suspicious_trait_impl.rs b/clippy_lints/src/suspicious_trait_impl.rs index 0b242d614d00..76ca1dc28482 100644 --- a/clippy_lints/src/suspicious_trait_impl.rs +++ b/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 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(trait_ref) = trait_ref_of_method(cx, parent_fn); if let Some(idx) = trait_ids.iter().position(|&tid| tid == trait_ref.path.def.def_id()); if binop != expected_ops[idx]; then{ diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 70ba0592caa3..6713f47690a1 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -301,6 +301,27 @@ 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 `Point` trait in this example: +/// +/// ```rust +/// trait Point; +/// +/// impl std::ops::Add for Point {} +/// ``` +pub fn trait_ref_of_method(cx: &LateContext<'_, '_>, hir_id: HirId) -> Option { + // 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() { From 5c9221f880cc3b2b73ec742317cb02d008e51818 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 7 Mar 2019 08:14:26 +0100 Subject: [PATCH 2/7] Refactor: Cleanup one part of assign_ops lint Removes a lot of indentation and separates lint emission from lint logic. Only touches the `hir::ExprKind::AssignOp` part of the lint. --- clippy_lints/src/assign_ops.rs | 92 +++++++++++++++++----------------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index b24e0cdfced4..63ec98a5b3ae 100644 --- a/clippy_lints/src/assign_ops.rs +++ b/clippy_lints/src/assign_ops.rs @@ -68,52 +68,14 @@ 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 { - let lint = |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 - ); - } - }, - ); - }; - // lhs op= l op r - if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, l) { - lint(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); - } + if op.node != binop.node { return; } + // lhs op= l op r + if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, l) { + 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_misrefactored_assign_op(cx, expr, *op, rhs, lhs, l); } } }, @@ -234,6 +196,44 @@ 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 { From 65694cc6c8df092353a3dec046d7101857e979e7 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 8 Mar 2019 09:10:41 +0100 Subject: [PATCH 3/7] Fix doctest --- clippy_lints/src/utils/mod.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 6713f47690a1..7d370b4754c2 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -306,9 +306,15 @@ pub fn implements_trait<'a, 'tcx>( /// Use this if you want to find the `TraitRef` of the `Point` trait in this example: /// /// ```rust -/// trait Point; +/// struct Point(isize, isize); /// -/// impl std::ops::Add for Point {} +/// 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 { // Get the implemented trait for the current function From 837d675afd3219155b429947d71458de53614b1d Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Fri, 8 Mar 2019 09:40:12 +0100 Subject: [PATCH 4/7] Update clippy_lints/src/utils/mod.rs Co-Authored-By: phansch --- clippy_lints/src/utils/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 7d370b4754c2..1a5b4f12b224 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -303,7 +303,7 @@ pub fn implements_trait<'a, 'tcx>( /// Get the `hir::TraitRef` of the trait the given method is implemented for /// -/// Use this if you want to find the `TraitRef` of the `Point` trait in this example: +/// Use this if you want to find the `TraitRef` of the `Add` trait in this example: /// /// ```rust /// struct Point(isize, isize); From 131b89b54ecdefcf7576f556c637441aa81f2da0 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 8 Mar 2019 09:42:09 +0100 Subject: [PATCH 5/7] fmt --- clippy_lints/src/assign_ops.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index b0bf250023d4..87e92c2bd245 100644 --- a/clippy_lints/src/assign_ops.rs +++ b/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, trait_ref_of_method, 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; From 9494f22f06f29ed1c47314b10ed62cfc1d4aff80 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 8 Mar 2019 09:44:22 +0100 Subject: [PATCH 6/7] cargo fmt --- clippy_lints/src/assign_ops.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index 63ec98a5b3ae..83bfd53017f2 100644 --- a/clippy_lints/src/assign_ops.rs +++ b/clippy_lints/src/assign_ops.rs @@ -68,7 +68,9 @@ 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 { return; } + if op.node != binop.node { + return; + } // lhs op= l op r if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, l) { lint_misrefactored_assign_op(cx, expr, *op, rhs, lhs, r); @@ -196,20 +198,24 @@ 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) { +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)) - { + 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)); + let long = format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r)); db.span_suggestion( expr.span, &format!( From f04acdd46353322c5e9c44c85e4129083dcaf082 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 8 Mar 2019 09:50:20 +0100 Subject: [PATCH 7/7] Document match_path, improve match_qpath docs --- clippy_lints/src/utils/mod.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 70ba0592caa3..83c7d586e64b 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/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()