From e88c956e1e18a99af55af00c8917f8e975d534c5 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Tue, 12 Oct 2021 19:18:22 +0200 Subject: [PATCH] avoid `eq_op` in test code --- clippy_lints/src/eq_op.rs | 11 ++++-- clippy_utils/src/lib.rs | 78 ++++++++++++++++++++++++++++++++++----- tests/compile-test.rs | 14 +++++++ tests/ui_test/eq_op.rs | 15 ++++++++ 4 files changed, 105 insertions(+), 13 deletions(-) create mode 100644 tests/ui_test/eq_op.rs diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index 51d5094e8c9..655560afd42 100644 --- 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 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { 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 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { 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 index 09eee78f0d1..c540e1ebc9c 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -69,11 +69,13 @@ 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,72 @@ 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<'tcx> { + tcx: TyCtxt<'tcx>, + names: Vec, + found: bool, +} +impl<'hir> ItemLikeVisitor<'hir> for VisitConstTestStruct<'hir> { + 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 { + let has_test_marker = self + .tcx + .hir() + .attrs(item.hir_id()) + .iter() + .any(|a| a.has_name(sym::rustc_test_marker)); + if has_test_marker && 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<'_>) {} +} - matches!(item.kind, ItemKind::Mod(..)) && item.ident.name.as_str().split('_').any(|a| a == "test" || a == "tests") +/// Checks if the function containing the given `HirId` is a `#[test]` function +/// +/// Note: If you use this function, please add a `#[test]` case in `tests/ui_test`. +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 { + tcx, + names, + found: false, + }; + tcx.hir().visit_item_likes_in_module(parent_mod, &mut vis); + vis.found +} + +/// Checks whether item either has `test` attribute appelied, or +/// is a module with `test` in its name. +/// +/// Note: If you use this function, please add a `#[test]` case in `tests/ui_test`. +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 index e8b1640c869..c15835ef299 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -149,6 +149,19 @@ 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()); + let rustcflags = cfg.target_rustcflags.get_or_insert_with(Default::default); + let len = rustcflags.len(); + rustcflags.push_str(" --test"); + compiletest::run_tests(cfg); + if let Some(ref mut flags) = &mut cfg.target_rustcflags { + flags.truncate(len); + } +} + fn run_internal_tests(cfg: &mut compiletest::Config) { // only run internal tests with the internal-tests feature if !RUN_INTERNAL_TESTS { @@ -312,6 +325,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_test/eq_op.rs b/tests/ui_test/eq_op.rs new file mode 100644 index 00000000000..f2f5f1e588e --- /dev/null +++ b/tests/ui_test/eq_op.rs @@ -0,0 +1,15 @@ +#[warn(clippy::eq_op)] +#[test] +fn eq_op_shouldnt_trigger_in_tests() { + let a = 1; + let result = a + 1 == 1 + a; + assert!(result); +} + +#[test] +fn eq_op_macros_shouldnt_trigger_in_tests() { + let a = 1; + let b = 2; + assert_eq!(a, a); + assert_eq!(a + b, b + a); +}