Skip to content

Rollup of 3 pull requests #3861

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 9, 2019
Merged

Rollup of 3 pull requests #3861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e1d47cd
Refactor: Extract `trait_ref_of_method` function
phansch Mar 7, 2019
5c9221f
Refactor: Cleanup one part of assign_ops lint
phansch Mar 7, 2019
65694cc
Fix doctest
phansch Mar 8, 2019
837d675
Update clippy_lints/src/utils/mod.rs
flip1995 Mar 8, 2019
131b89b
fmt
phansch Mar 8, 2019
9494f22
cargo fmt
phansch Mar 8, 2019
f04acdd
Document match_path, improve match_qpath docs
phansch Mar 8, 2019
1902384
Rollup merge of #3851 - phansch:refactor_trait_stuff, r=flip1995
flip1995 Mar 9, 2019
f69351e
Rollup merge of #3852 - phansch:refactor_assign_ops, r=flip1995
flip1995 Mar 9, 2019
c32135a
Rollup merge of #3857 - phansch:document_path_qpath, r=flip1995
flip1995 Mar 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 56 additions & 53 deletions clippy_lints/src/assign_ops.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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