Skip to content

Commit 8a227c7

Browse files
oli-obkoli-obk
authored
Merge pull request #1550 from sinkuu/should_assert_eq
Lint `assert!(x == y)`
2 parents 0682c95 + d2b9b7e commit 8a227c7

12 files changed

+130
-11
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,7 @@ All notable changes to this project will be documented in this file.
423423
[`shadow_same`]: https://github.com/Manishearth/rust-clippy/wiki#shadow_same
424424
[`shadow_unrelated`]: https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated
425425
[`short_circuit_statement`]: https://github.com/Manishearth/rust-clippy/wiki#short_circuit_statement
426+
[`should_assert_eq`]: https://github.com/Manishearth/rust-clippy/wiki#should_assert_eq
426427
[`should_implement_trait`]: https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait
427428
[`similar_names`]: https://github.com/Manishearth/rust-clippy/wiki#similar_names
428429
[`single_char_pattern`]: https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern

‎README.md

+2-1
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ transparently:
180180

181181
## Lints
182182

183-
There are 190 lints included in this crate:
183+
There are 191 lints included in this crate:
184184

185185
name | default | triggers on
186186
-----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -329,6 +329,7 @@ name
329329
[shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same) | allow | rebinding a name to itself, e.g. `let mut x = &mut x`
330330
[shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | allow | rebinding a name without even using the original value
331331
[short_circuit_statement](https://github.com/Manishearth/rust-clippy/wiki#short_circuit_statement) | warn | using a short circuit boolean condition as a statement
332+
[should_assert_eq](https://github.com/Manishearth/rust-clippy/wiki#should_assert_eq) | warn | using `assert` macro for asserting equality
332333
[should_implement_trait](https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait) | warn | defining a method that should be implementing a std trait
333334
[similar_names](https://github.com/Manishearth/rust-clippy/wiki#similar_names) | allow | similarly named items and bindings
334335
[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")`

‎clippy_lints/src/assign_ops.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps {
152152
let hir::Item_::ItemImpl(_, _, _, Some(ref trait_ref), _, _) = item.node,
153153
trait_ref.path.def.def_id() == trait_id
154154
], { return; }}
155-
implements_trait($cx, $ty, trait_id, vec![$rty])
155+
implements_trait($cx, $ty, trait_id, &[$rty], None)
156156
},)*
157157
_ => false,
158158
}

‎clippy_lints/src/lib.rs

+3
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ pub mod regex;
126126
pub mod returns;
127127
pub mod serde;
128128
pub mod shadow;
129+
pub mod should_assert_eq;
129130
pub mod strings;
130131
pub mod swap;
131132
pub mod temporary_assignment;
@@ -297,6 +298,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
297298
reg.register_early_lint_pass(box double_parens::DoubleParens);
298299
reg.register_late_lint_pass(box unused_io_amount::UnusedIoAmount);
299300
reg.register_late_lint_pass(box large_enum_variant::LargeEnumVariant::new(conf.enum_variant_size_threshold));
301+
reg.register_late_lint_pass(box should_assert_eq::ShouldAssertEq);
300302

301303
reg.register_lint_group("clippy_restrictions", vec![
302304
arithmetic::FLOAT_ARITHMETIC,
@@ -479,6 +481,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
479481
returns::LET_AND_RETURN,
480482
returns::NEEDLESS_RETURN,
481483
serde::SERDE_API_MISUSE,
484+
should_assert_eq::SHOULD_ASSERT_EQ,
482485
strings::STRING_LIT_AS_BYTES,
483486
swap::ALMOST_SWAPPED,
484487
swap::MANUAL_SWAP,

‎clippy_lints/src/methods.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -724,7 +724,7 @@ fn lint_or_fun_call(cx: &LateContext, expr: &hir::Expr, name: &str, args: &[hir:
724724
return false;
725725
};
726726

727-
if implements_trait(cx, arg_ty, default_trait_id, Vec::new()) {
727+
if implements_trait(cx, arg_ty, default_trait_id, &[], None) {
728728
span_lint_and_then(cx,
729729
OR_FUN_CALL,
730730
span,
@@ -1268,7 +1268,7 @@ fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option<ty::Ty<'a>> {
12681268
/// This checks whether a given type is known to implement Debug.
12691269
fn has_debug_impl<'a, 'b>(ty: ty::Ty<'a>, cx: &LateContext<'b, 'a>) -> bool {
12701270
match cx.tcx.lang_items.debug_trait() {
1271-
Some(debug) => implements_trait(cx, ty, debug, Vec::new()),
1271+
Some(debug) => implements_trait(cx, ty, debug, &[], None),
12721272
None => false,
12731273
}
12741274
}

‎clippy_lints/src/misc.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,7 @@ fn check_to_owned(cx: &LateContext, expr: &Expr, other: &Expr, left: bool, op: S
420420
None => return,
421421
};
422422

423-
if !implements_trait(cx, arg_ty, partial_eq_trait_id, vec![other_ty]) {
423+
if !implements_trait(cx, arg_ty, partial_eq_trait_id, &[other_ty], None) {
424424
return;
425425
}
426426

‎clippy_lints/src/new_without_default.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NewWithoutDefault {
115115
self_ty.walk_shallow().next().is_none(), // implements_trait does not work with generics
116116
same_tys(cx, self_ty, return_ty(cx, id), id),
117117
let Some(default_trait_id) = get_trait_def_id(cx, &paths::DEFAULT_TRAIT),
118-
!implements_trait(cx, self_ty, default_trait_id, Vec::new())
118+
!implements_trait(cx, self_ty, default_trait_id, &[], None)
119119
], {
120120
if let Some(sp) = can_derive_default(self_ty, cx, default_trait_id) {
121121
span_lint_and_then(cx,
@@ -156,7 +156,7 @@ fn can_derive_default<'t, 'c>(ty: ty::Ty<'t>, cx: &LateContext<'c, 't>, default_
156156
ty::TyAdt(adt_def, substs) if adt_def.is_struct() => {
157157
for field in adt_def.all_fields() {
158158
let f_ty = field.ty(cx.tcx, substs);
159-
if !implements_trait(cx, f_ty, default_trait_id, Vec::new()) {
159+
if !implements_trait(cx, f_ty, default_trait_id, &[], None) {
160160
return None;
161161
}
162162
}

‎clippy_lints/src/should_assert_eq.rs

+54
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
use rustc::lint::*;
2+
use rustc::hir::*;
3+
use utils::{is_direct_expn_of, implements_trait, span_lint};
4+
5+
/// **What it does:** Checks for `assert!(x == y)` which can be better written
6+
/// as `assert_eq!(x, y)` if `x` and `y` implement `Debug` trait.
7+
///
8+
/// **Why is this bad?** `assert_eq` provides better assertion failure reporting.
9+
///
10+
/// **Known problems:** Hopefully none.
11+
///
12+
/// **Example:**
13+
/// ```rust
14+
/// let (x, y) = (1, 2);
15+
///
16+
/// assert!(x == y); // assertion failed: x == y
17+
/// assert_eq!(x, y); // assertion failed: `(left == right)` (left: `1`, right: `2`)
18+
/// ```
19+
declare_lint! {
20+
pub SHOULD_ASSERT_EQ,
21+
Warn,
22+
"using `assert` macro for asserting equality"
23+
}
24+
25+
pub struct ShouldAssertEq;
26+
27+
impl LintPass for ShouldAssertEq {
28+
fn get_lints(&self) -> LintArray {
29+
lint_array![SHOULD_ASSERT_EQ]
30+
}
31+
}
32+
33+
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ShouldAssertEq {
34+
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) {
35+
if_let_chain! {[
36+
let ExprIf(ref cond, ..) = e.node,
37+
let ExprUnary(UnOp::UnNot, ref cond) = cond.node,
38+
let ExprBinary(ref binop, ref expr1, ref expr2) = cond.node,
39+
binop.node == BinOp_::BiEq,
40+
is_direct_expn_of(cx, e.span, "assert").is_some(),
41+
let Some(debug_trait) = cx.tcx.lang_items.debug_trait(),
42+
], {
43+
let ty1 = cx.tables.expr_ty(expr1);
44+
let ty2 = cx.tables.expr_ty(expr2);
45+
46+
let parent = cx.tcx.hir.get_parent(e.id);
47+
48+
if implements_trait(cx, ty1, debug_trait, &[], Some(parent)) &&
49+
implements_trait(cx, ty2, debug_trait, &[], Some(parent)) {
50+
span_lint(cx, SHOULD_ASSERT_EQ, e.span, "use `assert_eq` for better reporting");
51+
}
52+
}}
53+
}
54+
}

‎clippy_lints/src/utils/mod.rs

+9-3
Original file line numberDiff line numberDiff line change
@@ -317,13 +317,19 @@ pub fn implements_trait<'a, 'tcx>(
317317
cx: &LateContext<'a, 'tcx>,
318318
ty: ty::Ty<'tcx>,
319319
trait_id: DefId,
320-
ty_params: Vec<ty::Ty<'tcx>>
320+
ty_params: &[ty::Ty<'tcx>],
321+
parent_node_id: Option<NodeId>
321322
) -> bool {
322323
cx.tcx.populate_implementations_for_trait_if_necessary(trait_id);
323324

324325
let ty = cx.tcx.erase_regions(&ty);
325-
cx.tcx.infer_ctxt((), Reveal::All).enter(|infcx| {
326-
let obligation = cx.tcx.predicate_for_trait_def(traits::ObligationCause::dummy(), trait_id, 0, ty, &ty_params);
326+
let mut b = if let Some(id) = parent_node_id {
327+
cx.tcx.infer_ctxt(BodyId { node_id: id }, Reveal::All)
328+
} else {
329+
cx.tcx.infer_ctxt((), Reveal::All)
330+
};
331+
b.enter(|infcx| {
332+
let obligation = cx.tcx.predicate_for_trait_def(traits::ObligationCause::dummy(), trait_id, 0, ty, ty_params);
327333

328334
traits::SelectionContext::new(&infcx).evaluate_obligation_conservatively(&obligation)
329335
})

‎tests/ui/float_cmp.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -92,5 +92,5 @@ fn main() {
9292
let a: *const f32 = xs.as_ptr();
9393
let b: *const f32 = xs.as_ptr();
9494

95-
assert!(a == b); // no errors
95+
assert_eq!(a, b); // no errors
9696
}

‎tests/ui/should_assert_eq.rs

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
#![feature(plugin)]
2+
#![plugin(clippy)]
3+
4+
#![deny(should_assert_eq)]
5+
6+
#[derive(PartialEq, Eq)]
7+
struct NonDebug(i32);
8+
9+
#[derive(Debug, PartialEq, Eq)]
10+
struct Debug(i32);
11+
12+
fn main() {
13+
assert!(1 == 2);
14+
assert!(Debug(1) == Debug(2));
15+
assert!(NonDebug(1) == NonDebug(1)); // ok
16+
17+
test_generic(1, 2, 3, 4);
18+
}
19+
20+
fn test_generic<T: std::fmt::Debug + Eq, U: Eq>(x: T, y: T, z: U, w: U) {
21+
assert!(x == y);
22+
assert!(z == w); // ok
23+
}

‎tests/ui/should_assert_eq.stderr

+31
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
error: use `assert_eq` for better reporting
2+
--> $DIR/should_assert_eq.rs:13:5
3+
|
4+
13 | assert!(1 == 2);
5+
| ^^^^^^^^^^^^^^^^
6+
|
7+
note: lint level defined here
8+
--> $DIR/should_assert_eq.rs:4:9
9+
|
10+
4 | #![deny(should_assert_eq)]
11+
| ^^^^^^^^^^^^^^^^
12+
= note: this error originates in a macro outside of the current crate
13+
14+
error: use `assert_eq` for better reporting
15+
--> $DIR/should_assert_eq.rs:14:5
16+
|
17+
14 | assert!(Debug(1) == Debug(2));
18+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
19+
|
20+
= note: this error originates in a macro outside of the current crate
21+
22+
error: use `assert_eq` for better reporting
23+
--> $DIR/should_assert_eq.rs:21:5
24+
|
25+
21 | assert!(x == y);
26+
| ^^^^^^^^^^^^^^^^
27+
|
28+
= note: this error originates in a macro outside of the current crate
29+
30+
error: aborting due to 3 previous errors
31+

0 commit comments

Comments
 (0)