avoid eq_op in test code

rust-lang · Oct 14, 2021 · f04bab4 · f04bab4
1 parent 5c97b27
commit f04bab4
Show file tree

Hide file tree

Showing 5 changed files with 86 additions and 13 deletions.
diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs
@@ -1,7 +1,9 @@
 use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_then};
 use clippy_utils::source::snippet;
 use clippy_utils::ty::{implements_trait, is_copy};
-use clippy_utils::{ast_utils::is_useless_with_eq_exprs, eq_expr_value, higher, in_macro, is_expn_of};
+use clippy_utils::{
+    ast_utils::is_useless_with_eq_exprs, eq_expr_value, higher, in_macro, is_expn_of, is_in_test_function,
+};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, StmtKind};
@@ -81,7 +83,7 @@ impl<'tcx> LateLintPass<'tcx> for EqOp {
                         if macro_args.len() == 2;
                         let (lhs, rhs) = (macro_args[0], macro_args[1]);
                         if eq_expr_value(cx, lhs, rhs);
-
+                        if !is_in_test_function(cx.tcx, e.hir_id);
                         then {
                             span_lint(
                                 cx,
@@ -108,7 +110,10 @@ impl<'tcx> LateLintPass<'tcx> for EqOp {
             if macro_with_not_op(&left.kind) || macro_with_not_op(&right.kind) {
                 return;
             }
-            if is_useless_with_eq_exprs(op.node.into()) && eq_expr_value(cx, left, right) {
+            if is_useless_with_eq_exprs(op.node.into())
+                && eq_expr_value(cx, left, right)
+                && !is_in_test_function(cx.tcx, e.hir_id)
+            {
                 span_lint(
                     cx,
                     EQ_OP,

diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -69,11 +69,13 @@ use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::DefId;
 use rustc_hir::hir_id::{HirIdMap, HirIdSet};
 use rustc_hir::intravisit::{self, walk_expr, ErasedMap, FnKind, NestedVisitorMap, Visitor};
+use rustc_hir::itemlikevisit::ItemLikeVisitor;
 use rustc_hir::LangItem::{OptionNone, ResultErr, ResultOk};
 use rustc_hir::{
-    def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl,
-    ImplItem, ImplItemKind, IsAsync, Item, ItemKind, LangItem, Local, MatchSource, Mutability, Node, Param, Pat,
-    PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind, UnOp,
+    def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, ForeignItem, GenericArgs,
+    HirId, Impl, ImplItem, ImplItemKind, IsAsync, Item, ItemKind, LangItem, Local, MatchSource, Mutability, Node,
+    Param, Pat, PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind,
+    UnOp,
 };
 use rustc_lint::{LateContext, Level, Lint, LintContext};
 use rustc_middle::hir::exports::Export;
@@ -2064,16 +2066,57 @@ pub fn is_hir_ty_cfg_dependant(cx: &LateContext<'_>, ty: &hir::Ty<'_>) -> bool {
     false
 }
 
-/// Checks whether item either has `test` attribute applied, or
-/// is a module with `test` in its name.
-pub fn is_test_module_or_function(tcx: TyCtxt<'_>, item: &Item<'_>) -> bool {
-    if let Some(def_id) = tcx.hir().opt_local_def_id(item.hir_id()) {
-        if tcx.has_attr(def_id.to_def_id(), sym::test) {
-            return true;
+struct VisitConstTestStruct {
+    names: Vec<Symbol>,
+    found: bool,
+}
+impl<'hir> ItemLikeVisitor<'hir> for VisitConstTestStruct {
+    fn visit_item(&mut self, item: &Item<'_>) {
+        if let ItemKind::Const(ty, _body) = item.kind {
+            if let TyKind::Path(QPath::Resolved(_, path)) = ty.kind {
+                // We could also check for the type name `test::TestDescAndFn`
+                // and the `#[rustc_test_marker]` attribute?
+                if let Res::Def(DefKind::Struct, _) = path.res {
+                    if self.names.contains(&item.ident.name) {
+                        self.found = true;
+                    }
+                }
+            }
         }
     }
+    fn visit_trait_item(&mut self, _: &TraitItem<'_>) {}
+    fn visit_impl_item(&mut self, _: &ImplItem<'_>) {}
+    fn visit_foreign_item(&mut self, _: &ForeignItem<'_>) {}
+}
+
+/// Checks if the function containing the given `HirId` is a `#[test]` function
+pub fn is_in_test_function(tcx: TyCtxt<'_>, id: hir::HirId) -> bool {
+    let names: Vec<_> = tcx
+        .hir()
+        .parent_iter(id)
+        // Since you can nest functions we need to collect all until we leave
+        // function scope
+        .filter_map(|(_id, node)| {
+            if let Node::Item(item) = node {
+                if let ItemKind::Fn(_, _, _) = item.kind {
+                    return Some(item.ident.name);
+                }
+            }
+            None
+        })
+        .collect();
+    let parent_mod = tcx.parent_module(id);
+    let mut vis = VisitConstTestStruct { names, found: false };
+    tcx.hir().visit_item_likes_in_module(parent_mod, &mut vis);
+    vis.found
+}
 
-    matches!(item.kind, ItemKind::Mod(..)) && item.ident.name.as_str().split('_').any(|a| a == "test" || a == "tests")
+/// Checks whether item either has `test` attribute appelied, or
+/// is a module with `test` in its name.
+pub fn is_test_module_or_function(tcx: TyCtxt<'_>, item: &Item<'_>) -> bool {
+    is_in_test_function(tcx, item.hir_id())
+        || matches!(item.kind, ItemKind::Mod(..))
+            && item.ident.name.as_str().split('_').any(|a| a == "test" || a == "tests")
 }
 
 macro_rules! op_utils {

diff --git a/tests/compile-test.rs b/tests/compile-test.rs
@@ -149,6 +149,16 @@ fn run_ui(cfg: &mut compiletest::Config) {
     compiletest::run_tests(cfg);
 }
 
+fn run_ui_test(cfg: &mut compiletest::Config) {
+    cfg.mode = TestMode::Ui;
+    cfg.src_base = Path::new("tests").join("ui_test");
+    let _g = VarGuard::set("CARGO_MANIFEST_DIR", std::fs::canonicalize("tests").unwrap());
+    cfg.target_rustcflags
+        .get_or_insert_with(Default::default)
+        .push_str(" --test");
+    compiletest::run_tests(cfg);
+}
+
 fn run_internal_tests(cfg: &mut compiletest::Config) {
     // only run internal tests with the internal-tests feature
     if !RUN_INTERNAL_TESTS {
@@ -312,6 +322,7 @@ fn compile_test() {
     prepare_env();
     let mut config = default_config();
     run_ui(&mut config);
+    run_ui_test(&mut config);
     run_ui_toml(&mut config);
     run_ui_cargo(&mut config);
     run_internal_tests(&mut config);

diff --git a/tests/ui/eq_op_macros.rs b/tests/ui/eq_op_macros.rs
@@ -54,3 +54,11 @@ fn main() {
     let mut my_iter = my_vec.iter();
     assert_ne!(my_iter.next(), my_iter.next());
 }
+
+#[test]
+fn eq_op_shouldnt_trigger_in_tests() {
+    let a = 1;
+    let b = 2;
+    assert_eq!(a, a);
+    assert_eq!(a + b, b + a);
+}
diff --git a/tests/ui_test/eq_op.rs b/tests/ui_test/eq_op.rs
@@ -0,0 +1,6 @@
+#[test]
+fn eq_op_shouldnt_trigger_in_tests() {
+    let a = 1;
+    let result = a + 1 == 1 + a;
+    assert!(result);
+}