From 6bb426b0e37d0db4433fc6ffd67e7ffc26d6ab68 Mon Sep 17 00:00:00 2001
From: sinkuu <sinkuupump@gmail.com>
Date: Fri, 17 Feb 2017 17:32:51 +0900
Subject: [PATCH 1/4] Add `should_assert_eq` lint (fixes #641)
---
clippy_lints/src/lib.rs | 2 +
clippy_lints/src/should_assert_eq.rs | 59 ++++++++++++++++++++++++++++
clippy_lints/src/utils/paths.rs | 1 +
tests/ui/float_cmp.rs | 2 +-
tests/ui/should_assert_eq.rs | 16 ++++++++
tests/ui/should_assert_eq.stderr | 23 +++++++++++
6 files changed, 102 insertions(+), 1 deletion(-)
create mode 100644 clippy_lints/src/should_assert_eq.rs
create mode 100644 tests/ui/should_assert_eq.rs
create mode 100644 tests/ui/should_assert_eq.stderr
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 13ad8b41f96..4f99dff89b4 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -126,6 +126,7 @@ pub mod regex;
pub mod returns;
pub mod serde;
pub mod shadow;
+pub mod should_assert_eq;
pub mod strings;
pub mod swap;
pub mod temporary_assignment;
@@ -297,6 +298,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
reg.register_early_lint_pass(box double_parens::DoubleParens);
reg.register_late_lint_pass(box unused_io_amount::UnusedIoAmount);
reg.register_late_lint_pass(box large_enum_variant::LargeEnumVariant::new(conf.enum_variant_size_threshold));
+ reg.register_late_lint_pass(box should_assert_eq::ShouldAssertEq);
reg.register_lint_group("clippy_restrictions", vec![
arithmetic::FLOAT_ARITHMETIC,
diff --git a/clippy_lints/src/should_assert_eq.rs b/clippy_lints/src/should_assert_eq.rs
new file mode 100644
index 00000000000..9bacc64f61c
--- /dev/null
+++ b/clippy_lints/src/should_assert_eq.rs
@@ -0,0 +1,59 @@
+use rustc::lint::*;
+use rustc::hir::*;
+use utils::{paths, is_direct_expn_of, get_trait_def_id, implements_trait, span_lint};
+
+/// **What it does:** Checks for `assert!(x == y)` which can be written better
+/// as `assert_eq!(x, y)` if `x` and `y` implement `Debug` trait.
+///
+/// **Why is this bad?** `assert_eq` provides better assertion failure reporting.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// let (x, y) = (1, 2);
+///
+/// assert!(x == y); // assertion failed: x == y
+/// assert_eq!(x, y); // assertion failed: `(left == right)` (left: `1`, right: `2`)
+/// ```
+declare_lint! {
+ pub SHOULD_ASSERT_EQ,
+ Warn,
+ "using `assert` macro for asserting equality"
+}
+
+pub struct ShouldAssertEq;
+
+impl LintPass for ShouldAssertEq {
+ fn get_lints(&self) -> LintArray {
+ lint_array![SHOULD_ASSERT_EQ]
+ }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ShouldAssertEq {
+ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) {
+ if_let_chain! {[
+ let ExprIf(ref cond, ..) = e.node,
+ let ExprUnary(UnOp::UnNot, ref cond) = cond.node,
+ let ExprBinary(ref binop, ref expr1, ref expr2) = cond.node,
+ ], {
+ if is_direct_expn_of(cx, e.span, "assert").is_none() {
+ return;
+ }
+
+ if binop.node != BinOp_::BiEq {
+ return;
+ }
+
+ let debug_trait = get_trait_def_id(cx, &paths::DEBUG_TRAIT)
+ .expect("cannot find Debug trait");
+
+ let ty1 = cx.tables.expr_ty(expr1);
+ let ty2 = cx.tables.expr_ty(expr2);
+ if implements_trait(cx, ty1, debug_trait, vec![]) &&
+ implements_trait(cx, ty2, debug_trait, vec![]) {
+ span_lint(cx, SHOULD_ASSERT_EQ, e.span, "use `assert_eq` for better reporting");
+ }
+ }}
+ }
+}
diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs
index 5edff76d996..6ee35e3bd96 100644
--- a/clippy_lints/src/utils/paths.rs
+++ b/clippy_lints/src/utils/paths.rs
@@ -15,6 +15,7 @@ pub const CMP_MIN: [&'static str; 3] = ["core", "cmp", "min"];
pub const COW: [&'static str; 3] = ["collections", "borrow", "Cow"];
pub const CSTRING_NEW: [&'static str; 5] = ["std", "ffi", "c_str", "CString", "new"];
pub const DEBUG_FMT_METHOD: [&'static str; 4] = ["core", "fmt", "Debug", "fmt"];
+pub const DEBUG_TRAIT: [&'static str; 3] = ["core", "fmt", "Debug"];
pub const DEFAULT_TRAIT: [&'static str; 3] = ["core", "default", "Default"];
pub const DISPLAY_FMT_METHOD: [&'static str; 4] = ["core", "fmt", "Display", "fmt"];
pub const DROP: [&'static str; 3] = ["core", "mem", "drop"];
diff --git a/tests/ui/float_cmp.rs b/tests/ui/float_cmp.rs
index de897caca08..024dd9c242c 100644
--- a/tests/ui/float_cmp.rs
+++ b/tests/ui/float_cmp.rs
@@ -92,5 +92,5 @@ fn main() {
let a: *const f32 = xs.as_ptr();
let b: *const f32 = xs.as_ptr();
- assert!(a == b); // no errors
+ assert_eq!(a, b); // no errors
}
diff --git a/tests/ui/should_assert_eq.rs b/tests/ui/should_assert_eq.rs
new file mode 100644
index 00000000000..41e3a411383
--- /dev/null
+++ b/tests/ui/should_assert_eq.rs
@@ -0,0 +1,16 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(should_assert_eq)]
+
+#[derive(PartialEq, Eq)]
+struct NonDebug(i32);
+
+#[derive(Debug, PartialEq, Eq)]
+struct Debug(i32);
+
+fn main() {
+ assert!(1 == 2);
+ assert!(Debug(1) == Debug(2));
+ assert!(NonDebug(1) == NonDebug(1)); // ok
+}
diff --git a/tests/ui/should_assert_eq.stderr b/tests/ui/should_assert_eq.stderr
new file mode 100644
index 00000000000..3c51dbe0a9c
--- /dev/null
+++ b/tests/ui/should_assert_eq.stderr
@@ -0,0 +1,23 @@
+error: use `assert_eq` for better reporting
+ --> $DIR/should_assert_eq.rs:13:5
+ |
+13 | assert!(1 == 2);
+ | ^^^^^^^^^^^^^^^^
+ |
+note: lint level defined here
+ --> $DIR/should_assert_eq.rs:4:9
+ |
+4 | #![deny(should_assert_eq)]
+ | ^^^^^^^^^^^^^^^^
+ = note: this error originates in a macro outside of the current crate
+
+error: use `assert_eq` for better reporting
+ --> $DIR/should_assert_eq.rs:14:5
+ |
+14 | assert!(Debug(1) == Debug(2));
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+ |
+ = note: this error originates in a macro outside of the current crate
+
+error: aborting due to 2 previous errors
+
From 16e53c9868d2fe3fd8b2bd7b0f77764da689918a Mon Sep 17 00:00:00 2001
From: sinkuu <sinkuupump@gmail.com>
Date: Fri, 17 Feb 2017 17:48:19 +0900
Subject: [PATCH 2/4] Run update_lints.py
---
CHANGELOG.md | 1 +
README.md | 3 ++-
clippy_lints/src/lib.rs | 1 +
3 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 90f67e8aec7..1192195cd6a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -423,6 +423,7 @@ All notable changes to this project will be documented in this file.
[`shadow_same`]: https://github.com/Manishearth/rust-clippy/wiki#shadow_same
[`shadow_unrelated`]: https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated
[`short_circuit_statement`]: https://github.com/Manishearth/rust-clippy/wiki#short_circuit_statement
+[`should_assert_eq`]: https://github.com/Manishearth/rust-clippy/wiki#should_assert_eq
[`should_implement_trait`]: https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait
[`similar_names`]: https://github.com/Manishearth/rust-clippy/wiki#similar_names
[`single_char_pattern`]: https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern
diff --git a/README.md b/README.md
index 9e4b9fba1b6..842e654678b 100644
--- a/README.md
+++ b/README.md
@@ -180,7 +180,7 @@ transparently:
## Lints
-There are 190 lints included in this crate:
+There are 191 lints included in this crate:
name | default | triggers on
-----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -329,6 +329,7 @@ name
[shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same) | allow | rebinding a name to itself, e.g. `let mut x = &mut x`
[shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | allow | rebinding a name without even using the original value
[short_circuit_statement](https://github.com/Manishearth/rust-clippy/wiki#short_circuit_statement) | warn | using a short circuit boolean condition as a statement
+[should_assert_eq](https://github.com/Manishearth/rust-clippy/wiki#should_assert_eq) | warn | using `assert` macro for asserting equality
[should_implement_trait](https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait) | warn | defining a method that should be implementing a std trait
[similar_names](https://github.com/Manishearth/rust-clippy/wiki#similar_names) | allow | similarly named items and bindings
[single_char_pattern](https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern) | warn | using a single-character str where a char could be used, e.g. `_.split("x")`
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 4f99dff89b4..814daf7d645 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -481,6 +481,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
returns::LET_AND_RETURN,
returns::NEEDLESS_RETURN,
serde::SERDE_API_MISUSE,
+ should_assert_eq::SHOULD_ASSERT_EQ,
strings::STRING_LIT_AS_BYTES,
swap::ALMOST_SWAPPED,
swap::MANUAL_SWAP,
From 8cb2ec804dced5435aaea7f465eccddcdeddba19 Mon Sep 17 00:00:00 2001
From: sinkuu <sinkuupump@gmail.com>
Date: Fri, 17 Feb 2017 19:59:52 +0900
Subject: [PATCH 3/4] Support generic type
---
clippy_lints/src/assign_ops.rs | 2 +-
clippy_lints/src/methods.rs | 4 ++--
clippy_lints/src/misc.rs | 2 +-
clippy_lints/src/new_without_default.rs | 4 ++--
clippy_lints/src/should_assert_eq.rs | 18 ++++++++++++------
clippy_lints/src/utils/mod.rs | 12 +++++++++---
clippy_lints/src/utils/paths.rs | 1 -
tests/ui/should_assert_eq.rs | 7 +++++++
tests/ui/should_assert_eq.stderr | 10 +++++++++-
9 files changed, 43 insertions(+), 17 deletions(-)
diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs
index 71ebb996fc5..ea3e29cff12 100644
--- a/clippy_lints/src/assign_ops.rs
+++ b/clippy_lints/src/assign_ops.rs
@@ -152,7 +152,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps {
let hir::Item_::ItemImpl(_, _, _, Some(ref trait_ref), _, _) = item.node,
trait_ref.path.def.def_id() == trait_id
], { return; }}
- implements_trait($cx, $ty, trait_id, vec![$rty])
+ implements_trait($cx, $ty, trait_id, &[$rty], None)
},)*
_ => false,
}
diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs
index b74bd9266a4..9acf4c7e973 100644
--- a/clippy_lints/src/methods.rs
+++ b/clippy_lints/src/methods.rs
@@ -724,7 +724,7 @@ fn lint_or_fun_call(cx: &LateContext, expr: &hir::Expr, name: &str, args: &[hir:
return false;
};
- if implements_trait(cx, arg_ty, default_trait_id, Vec::new()) {
+ if implements_trait(cx, arg_ty, default_trait_id, &[], None) {
span_lint_and_then(cx,
OR_FUN_CALL,
span,
@@ -1268,7 +1268,7 @@ fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option<ty::Ty<'a>> {
/// This checks whether a given type is known to implement Debug.
fn has_debug_impl<'a, 'b>(ty: ty::Ty<'a>, cx: &LateContext<'b, 'a>) -> bool {
match cx.tcx.lang_items.debug_trait() {
- Some(debug) => implements_trait(cx, ty, debug, Vec::new()),
+ Some(debug) => implements_trait(cx, ty, debug, &[], None),
None => false,
}
}
diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs
index d397d425111..d2f794def0c 100644
--- a/clippy_lints/src/misc.rs
+++ b/clippy_lints/src/misc.rs
@@ -420,7 +420,7 @@ fn check_to_owned(cx: &LateContext, expr: &Expr, other: &Expr, left: bool, op: S
None => return,
};
- if !implements_trait(cx, arg_ty, partial_eq_trait_id, vec![other_ty]) {
+ if !implements_trait(cx, arg_ty, partial_eq_trait_id, &[other_ty], None) {
return;
}
diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_without_default.rs
index e4e98c26038..ece7e130c5f 100644
--- a/clippy_lints/src/new_without_default.rs
+++ b/clippy_lints/src/new_without_default.rs
@@ -115,7 +115,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NewWithoutDefault {
self_ty.walk_shallow().next().is_none(), // implements_trait does not work with generics
same_tys(cx, self_ty, return_ty(cx, id), id),
let Some(default_trait_id) = get_trait_def_id(cx, &paths::DEFAULT_TRAIT),
- !implements_trait(cx, self_ty, default_trait_id, Vec::new())
+ !implements_trait(cx, self_ty, default_trait_id, &[], None)
], {
if let Some(sp) = can_derive_default(self_ty, cx, default_trait_id) {
span_lint_and_then(cx,
@@ -156,7 +156,7 @@ fn can_derive_default<'t, 'c>(ty: ty::Ty<'t>, cx: &LateContext<'c, 't>, default_
ty::TyAdt(adt_def, substs) if adt_def.is_struct() => {
for field in adt_def.all_fields() {
let f_ty = field.ty(cx.tcx, substs);
- if !implements_trait(cx, f_ty, default_trait_id, Vec::new()) {
+ if !implements_trait(cx, f_ty, default_trait_id, &[], None) {
return None;
}
}
diff --git a/clippy_lints/src/should_assert_eq.rs b/clippy_lints/src/should_assert_eq.rs
index 9bacc64f61c..16933ea2747 100644
--- a/clippy_lints/src/should_assert_eq.rs
+++ b/clippy_lints/src/should_assert_eq.rs
@@ -1,13 +1,13 @@
use rustc::lint::*;
use rustc::hir::*;
-use utils::{paths, is_direct_expn_of, get_trait_def_id, implements_trait, span_lint};
+use utils::{is_direct_expn_of, implements_trait, span_lint};
/// **What it does:** Checks for `assert!(x == y)` which can be written better
/// as `assert_eq!(x, y)` if `x` and `y` implement `Debug` trait.
///
/// **Why is this bad?** `assert_eq` provides better assertion failure reporting.
///
-/// **Known problems:** None.
+/// **Known problems:** Hopefully none.
///
/// **Example:**
/// ```rust
@@ -45,13 +45,19 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ShouldAssertEq {
return;
}
- let debug_trait = get_trait_def_id(cx, &paths::DEBUG_TRAIT)
- .expect("cannot find Debug trait");
+ let debug_trait = if let Some(t) = cx.tcx.lang_items.debug_trait() {
+ t
+ } else {
+ return;
+ };
let ty1 = cx.tables.expr_ty(expr1);
let ty2 = cx.tables.expr_ty(expr2);
- if implements_trait(cx, ty1, debug_trait, vec![]) &&
- implements_trait(cx, ty2, debug_trait, vec![]) {
+
+ let parent = cx.tcx.hir.get_parent(e.id);
+
+ if implements_trait(cx, ty1, debug_trait, &[], Some(parent)) &&
+ implements_trait(cx, ty2, debug_trait, &[], Some(parent)) {
span_lint(cx, SHOULD_ASSERT_EQ, e.span, "use `assert_eq` for better reporting");
}
}}
diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs
index 1211b77ddb8..ce581febfe4 100644
--- a/clippy_lints/src/utils/mod.rs
+++ b/clippy_lints/src/utils/mod.rs
@@ -317,13 +317,19 @@ pub fn implements_trait<'a, 'tcx>(
cx: &LateContext<'a, 'tcx>,
ty: ty::Ty<'tcx>,
trait_id: DefId,
- ty_params: Vec<ty::Ty<'tcx>>
+ ty_params: &[ty::Ty<'tcx>],
+ parent_node_id: Option<NodeId>
) -> bool {
cx.tcx.populate_implementations_for_trait_if_necessary(trait_id);
let ty = cx.tcx.erase_regions(&ty);
- cx.tcx.infer_ctxt((), Reveal::All).enter(|infcx| {
- let obligation = cx.tcx.predicate_for_trait_def(traits::ObligationCause::dummy(), trait_id, 0, ty, &ty_params);
+ let mut b = if let Some(id) = parent_node_id {
+ cx.tcx.infer_ctxt(BodyId { node_id: id }, Reveal::All)
+ } else {
+ cx.tcx.infer_ctxt((), Reveal::All)
+ };
+ b.enter(|infcx| {
+ let obligation = cx.tcx.predicate_for_trait_def(traits::ObligationCause::dummy(), trait_id, 0, ty, ty_params);
traits::SelectionContext::new(&infcx).evaluate_obligation_conservatively(&obligation)
})
diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs
index 6ee35e3bd96..5edff76d996 100644
--- a/clippy_lints/src/utils/paths.rs
+++ b/clippy_lints/src/utils/paths.rs
@@ -15,7 +15,6 @@ pub const CMP_MIN: [&'static str; 3] = ["core", "cmp", "min"];
pub const COW: [&'static str; 3] = ["collections", "borrow", "Cow"];
pub const CSTRING_NEW: [&'static str; 5] = ["std", "ffi", "c_str", "CString", "new"];
pub const DEBUG_FMT_METHOD: [&'static str; 4] = ["core", "fmt", "Debug", "fmt"];
-pub const DEBUG_TRAIT: [&'static str; 3] = ["core", "fmt", "Debug"];
pub const DEFAULT_TRAIT: [&'static str; 3] = ["core", "default", "Default"];
pub const DISPLAY_FMT_METHOD: [&'static str; 4] = ["core", "fmt", "Display", "fmt"];
pub const DROP: [&'static str; 3] = ["core", "mem", "drop"];
diff --git a/tests/ui/should_assert_eq.rs b/tests/ui/should_assert_eq.rs
index 41e3a411383..5abf3566425 100644
--- a/tests/ui/should_assert_eq.rs
+++ b/tests/ui/should_assert_eq.rs
@@ -13,4 +13,11 @@ fn main() {
assert!(1 == 2);
assert!(Debug(1) == Debug(2));
assert!(NonDebug(1) == NonDebug(1)); // ok
+
+ test_generic(1, 2, 3, 4);
+}
+
+fn test_generic<T: std::fmt::Debug + Eq, U: Eq>(x: T, y: T, z: U, w: U) {
+ assert!(x == y);
+ assert!(z == w); // ok
}
diff --git a/tests/ui/should_assert_eq.stderr b/tests/ui/should_assert_eq.stderr
index 3c51dbe0a9c..68d287fda8d 100644
--- a/tests/ui/should_assert_eq.stderr
+++ b/tests/ui/should_assert_eq.stderr
@@ -19,5 +19,13 @@ error: use `assert_eq` for better reporting
|
= note: this error originates in a macro outside of the current crate
-error: aborting due to 2 previous errors
+error: use `assert_eq` for better reporting
+ --> $DIR/should_assert_eq.rs:21:5
+ |
+21 | assert!(x == y);
+ | ^^^^^^^^^^^^^^^^
+ |
+ = note: this error originates in a macro outside of the current crate
+
+error: aborting due to 3 previous errors
From d2b9b7ece2714170fae43ff9c5c5c3853d04ecef Mon Sep 17 00:00:00 2001
From: sinkuu <sinkuupump@gmail.com>
Date: Fri, 17 Feb 2017 22:37:32 +0900
Subject: [PATCH 4/4] Fix language & if_let_chain usage
---
clippy_lints/src/should_assert_eq.rs | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/clippy_lints/src/should_assert_eq.rs b/clippy_lints/src/should_assert_eq.rs
index 16933ea2747..e6296154126 100644
--- a/clippy_lints/src/should_assert_eq.rs
+++ b/clippy_lints/src/should_assert_eq.rs
@@ -2,7 +2,7 @@ use rustc::lint::*;
use rustc::hir::*;
use utils::{is_direct_expn_of, implements_trait, span_lint};
-/// **What it does:** Checks for `assert!(x == y)` which can be written better
+/// **What it does:** Checks for `assert!(x == y)` which can be better written
/// as `assert_eq!(x, y)` if `x` and `y` implement `Debug` trait.
///
/// **Why is this bad?** `assert_eq` provides better assertion failure reporting.
@@ -36,21 +36,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ShouldAssertEq {
let ExprIf(ref cond, ..) = e.node,
let ExprUnary(UnOp::UnNot, ref cond) = cond.node,
let ExprBinary(ref binop, ref expr1, ref expr2) = cond.node,
+ binop.node == BinOp_::BiEq,
+ is_direct_expn_of(cx, e.span, "assert").is_some(),
+ let Some(debug_trait) = cx.tcx.lang_items.debug_trait(),
], {
- if is_direct_expn_of(cx, e.span, "assert").is_none() {
- return;
- }
-
- if binop.node != BinOp_::BiEq {
- return;
- }
-
- let debug_trait = if let Some(t) = cx.tcx.lang_items.debug_trait() {
- t
- } else {
- return;
- };
-
let ty1 = cx.tables.expr_ty(expr1);
let ty2 = cx.tables.expr_ty(expr2);