diff options
| -rw-r--r-- | clippy_lints/src/assign_ops.rs | 109 | ||||
| -rw-r--r-- | clippy_lints/src/missing_const_for_fn.rs | 17 | ||||
| -rw-r--r-- | clippy_lints/src/suspicious_trait_impl.rs | 7 | ||||
| -rw-r--r-- | clippy_lints/src/utils/mod.rs | 48 |
4 files changed, 107 insertions, 74 deletions
diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index b24e0cdfced..9e0b87bc377 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, 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 { - 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); } } }, @@ -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 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; } } @@ -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 { diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs index 19757f32e6b..5bc949a6688 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 0b242d614d0..76ca1dc2848 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 f6b9a77a573..2e341bc7435 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() @@ -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() { |