Skip to content

Commit

Permalink
Auto merge of #3861 - flip1995:rollup, r=flip1995
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bors
bors committed Mar 9, 2019
2 parents bd6b5a1 + c32135a commit 8213d25
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 74 deletions.
109 changes: 56 additions & 53 deletions clippy_lints/src/assign_ops.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);
}
}
},
Expand All @@ -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; }
}
Expand Down Expand Up @@ -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 {
Expand Down
17 changes: 2 additions & 15 deletions clippy_lints/src/missing_const_for_fn.rs
View file
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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;
}
},
Expand All @@ -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
Expand Down
7 changes: 2 additions & 5 deletions clippy_lints/src/suspicious_trait_impl.rs
View file
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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{
Expand Down
48 changes: 47 additions & 1 deletion clippy_lints/src/utils/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()
Expand Down Expand Up @@ -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() {
Expand Down

0 comments on commit 8213d25

Please sign in to comment.