From b50a014fa07897b9471d67d8f2fc7f72c6321fba Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Sat, 24 Oct 2020 23:48:31 +0200 Subject: [PATCH] Avoid triggering default_trait_access Do not trigger `default_trait_access` on a span already linted by `field_reassigned_with_default`. --- ...ld_reassign_with_default.rs => default.rs} | 80 +++++++++++++++++-- clippy_lints/src/default_trait_access.rs | 62 -------------- clippy_lints/src/lib.rs | 16 ++-- src/lintlist/mod.rs | 4 +- 4 files changed, 81 insertions(+), 81 deletions(-) rename clippy_lints/src/{field_reassign_with_default.rs => default.rs} (76%) delete mode 100644 clippy_lints/src/default_trait_access.rs diff --git a/clippy_lints/src/field_reassign_with_default.rs b/clippy_lints/src/default.rs similarity index 76% rename from clippy_lints/src/field_reassign_with_default.rs rename to clippy_lints/src/default.rs index e86da93ff5dc..626216ea786f 100644 --- a/clippy_lints/src/field_reassign_with_default.rs +++ b/clippy_lints/src/default.rs @@ -1,12 +1,36 @@ -use crate::utils::{contains_name, match_def_path, paths, qpath_res, snippet, span_lint_and_note}; +use crate::utils::{any_parent_is_automatically_derived, contains_name, match_def_path, paths, qpath_res, snippet}; +use crate::utils::{span_lint_and_note, span_lint_and_sugg}; use if_chain::if_chain; +use rustc_data_structures::fx::FxHashSet; +use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::{Block, Expr, ExprKind, PatKind, QPath, Stmt, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self, Adt, Ty}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::symbol::{Ident, Symbol}; +use rustc_span::Span; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +declare_clippy_lint! { + /// **What it does:** Checks for literal calls to `Default::default()`. + /// + /// **Why is this bad?** It's more clear to the reader to use the name of the type whose default is + /// being gotten than the generic `Default`. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// // Bad + /// let s: String = Default::default(); + /// + /// // Good + /// let s = String::default(); + /// ``` + pub DEFAULT_TRAIT_ACCESS, + pedantic, + "checks for literal calls to `Default::default()`" +} declare_clippy_lint! { /// **What it does:** Checks for immediate reassignment of fields initialized @@ -19,12 +43,14 @@ declare_clippy_lint! { /// **Example:** /// Bad: /// ``` + /// # #[derive(Default)] /// # struct A { i: i32 } /// let mut a: A = Default::default(); /// a.i = 42; /// ``` /// Use instead: /// ``` + /// # #[derive(Default)] /// # struct A { i: i32 } /// let a = A { /// i: 42, @@ -36,9 +62,46 @@ declare_clippy_lint! { "binding initialized with Default should have its fields set in the initializer" } -declare_lint_pass!(FieldReassignWithDefault => [FIELD_REASSIGN_WITH_DEFAULT]); +#[derive(Default)] +pub struct DefaultPass { + // Spans linted by `field_reassign_with_default`. + reassigned_linted: FxHashSet, +} + +impl_lint_pass!(DefaultPass => [DEFAULT_TRAIT_ACCESS, FIELD_REASSIGN_WITH_DEFAULT]); + +impl LateLintPass<'_> for DefaultPass { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if_chain! { + // Avoid cases already linted by `field_reassign_with_default` + if !self.reassigned_linted.contains(&expr.span); + if let ExprKind::Call(ref path, ..) = expr.kind; + if !any_parent_is_automatically_derived(cx.tcx, expr.hir_id); + if let ExprKind::Path(ref qpath) = path.kind; + if let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id(); + if match_def_path(cx, def_id, &paths::DEFAULT_TRAIT_METHOD); + // Detect and ignore ::default() because these calls do explicitly name the type. + if let QPath::Resolved(None, _path) = qpath; + then { + let expr_ty = cx.typeck_results().expr_ty(expr); + if let ty::Adt(def, ..) = expr_ty.kind() { + // TODO: Work out a way to put "whatever the imported way of referencing + // this type in this file" rather than a fully-qualified type. + let replacement = format!("{}::default()", cx.tcx.def_path_str(def.did)); + span_lint_and_sugg( + cx, + DEFAULT_TRAIT_ACCESS, + expr.span, + &format!("calling `{}` is more clear than this expression", replacement), + "try", + replacement, + Applicability::Unspecified, // First resolve the TODO above + ); + } + } + } + } -impl LateLintPass<'_> for FieldReassignWithDefault { fn check_block<'tcx>(&mut self, cx: &LateContext<'tcx>, block: &Block<'tcx>) { // find all binding statements like `let mut _ = T::default()` where `T::default()` is the // `default` method of the `Default` trait, and store statement index in current block being @@ -47,7 +110,7 @@ impl LateLintPass<'_> for FieldReassignWithDefault { // start from the `let mut _ = _::default();` and look at all the following // statements, see if they re-assign the fields of the binding - for (stmt_idx, binding_name, binding_type) in binding_statements_using_default { + for (stmt_idx, binding_name, binding_type, span) in binding_statements_using_default { // the last statement of a block cannot trigger the lint if stmt_idx == block.stmts.len() - 1 { break; @@ -145,6 +208,7 @@ impl LateLintPass<'_> for FieldReassignWithDefault { sugg ), ); + self.reassigned_linted.insert(span); } } } @@ -171,7 +235,7 @@ fn is_expr_default<'tcx>(expr: &'tcx Expr<'tcx>, cx: &LateContext<'tcx>) -> bool fn enumerate_bindings_using_default<'tcx>( cx: &LateContext<'tcx>, block: &Block<'tcx>, -) -> Vec<(usize, Symbol, Ty<'tcx>)> { +) -> Vec<(usize, Symbol, Ty<'tcx>, Span)> { block .stmts .iter() @@ -189,7 +253,7 @@ fn enumerate_bindings_using_default<'tcx>( if let Some(ref expr) = local.init; if is_expr_default(expr, cx); then { - Some((idx, ident.name, ty)) + Some((idx, ident.name, ty, expr.span)) } else { None } diff --git a/clippy_lints/src/default_trait_access.rs b/clippy_lints/src/default_trait_access.rs deleted file mode 100644 index 3048436d9a7b..000000000000 --- a/clippy_lints/src/default_trait_access.rs +++ /dev/null @@ -1,62 +0,0 @@ -use if_chain::if_chain; -use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, QPath}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty; -use rustc_session::{declare_lint_pass, declare_tool_lint}; - -use crate::utils::{any_parent_is_automatically_derived, match_def_path, paths, span_lint_and_sugg}; - -declare_clippy_lint! { - /// **What it does:** Checks for literal calls to `Default::default()`. - /// - /// **Why is this bad?** It's more clear to the reader to use the name of the type whose default is - /// being gotten than the generic `Default`. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust - /// // Bad - /// let s: String = Default::default(); - /// - /// // Good - /// let s = String::default(); - /// ``` - pub DEFAULT_TRAIT_ACCESS, - pedantic, - "checks for literal calls to `Default::default()`" -} - -declare_lint_pass!(DefaultTraitAccess => [DEFAULT_TRAIT_ACCESS]); - -impl<'tcx> LateLintPass<'tcx> for DefaultTraitAccess { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if_chain! { - if let ExprKind::Call(ref path, ..) = expr.kind; - if !any_parent_is_automatically_derived(cx.tcx, expr.hir_id); - if let ExprKind::Path(ref qpath) = path.kind; - if let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id(); - if match_def_path(cx, def_id, &paths::DEFAULT_TRAIT_METHOD); - // Detect and ignore ::default() because these calls do explicitly name the type. - if let QPath::Resolved(None, _path) = qpath; - then { - let expr_ty = cx.typeck_results().expr_ty(expr); - if let ty::Adt(def, ..) = expr_ty.kind() { - // TODO: Work out a way to put "whatever the imported way of referencing - // this type in this file" rather than a fully-qualified type. - let replacement = format!("{}::default()", cx.tcx.def_path_str(def.did)); - span_lint_and_sugg( - cx, - DEFAULT_TRAIT_ACCESS, - expr.span, - &format!("calling `{}` is more clear than this expression", replacement), - "try", - replacement, - Applicability::Unspecified, // First resolve the TODO above - ); - } - } - } - } -} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5a2d502b0a65..ad65e09455b1 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -175,7 +175,7 @@ mod copies; mod copy_iterator; mod create_dir; mod dbg_macro; -mod default_trait_access; +mod default; mod dereference; mod derive; mod disallowed_method; @@ -198,7 +198,6 @@ mod excessive_bools; mod exit; mod explicit_write; mod fallible_impl_from; -mod field_reassign_with_default; mod float_equality_without_abs; mod float_literal; mod floating_point_arithmetic; @@ -531,7 +530,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: ©_iterator::COPY_ITERATOR, &create_dir::CREATE_DIR, &dbg_macro::DBG_MACRO, - &default_trait_access::DEFAULT_TRAIT_ACCESS, + &default::DEFAULT_TRAIT_ACCESS, + &default::FIELD_REASSIGN_WITH_DEFAULT, &dereference::EXPLICIT_DEREF_METHODS, &derive::DERIVE_HASH_XOR_EQ, &derive::DERIVE_ORD_XOR_PARTIAL_ORD, @@ -570,7 +570,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &exit::EXIT, &explicit_write::EXPLICIT_WRITE, &fallible_impl_from::FALLIBLE_IMPL_FROM, - &field_reassign_with_default::FIELD_REASSIGN_WITH_DEFAULT, &float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS, &float_literal::EXCESSIVE_PRECISION, &float_literal::LOSSY_FLOAT_LITERAL, @@ -1033,7 +1032,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box neg_cmp_op_on_partial_ord::NoNegCompOpForPartialOrd); store.register_late_pass(|| box unwrap::Unwrap); store.register_late_pass(|| box duration_subsec::DurationSubsec); - store.register_late_pass(|| box default_trait_access::DefaultTraitAccess); store.register_late_pass(|| box indexing_slicing::IndexingSlicing); store.register_late_pass(|| box non_copy_const::NonCopyConst); store.register_late_pass(|| box ptr_offset_with_cast::PtrOffsetWithCast); @@ -1084,7 +1082,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: let enum_variant_name_threshold = conf.enum_variant_name_threshold; store.register_early_pass(move || box enum_variants::EnumVariantNames::new(enum_variant_name_threshold)); store.register_early_pass(|| box tabs_in_doc_comments::TabsInDocComments); - store.register_late_pass(|| box field_reassign_with_default::FieldReassignWithDefault); + store.register_late_pass(|| box default::DefaultPass::default()); store.register_late_pass(|| box unused_self::UnusedSelf); store.register_late_pass(|| box mutable_debug_assertion::DebugAssertWithMutCall); store.register_late_pass(|| box exit::Exit); @@ -1197,7 +1195,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&copies::MATCH_SAME_ARMS), LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION), LintId::of(©_iterator::COPY_ITERATOR), - LintId::of(&default_trait_access::DEFAULT_TRAIT_ACCESS), + LintId::of(&default::DEFAULT_TRAIT_ACCESS), LintId::of(&dereference::EXPLICIT_DEREF_METHODS), LintId::of(&derive::EXPL_IMPL_CLONE_ON_COPY), LintId::of(&derive::UNSAFE_DERIVE_DESERIALIZE), @@ -1301,6 +1299,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&comparison_chain::COMPARISON_CHAIN), LintId::of(&copies::IFS_SAME_COND), LintId::of(&copies::IF_SAME_THEN_ELSE), + LintId::of(&default::FIELD_REASSIGN_WITH_DEFAULT), LintId::of(&derive::DERIVE_HASH_XOR_EQ), LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD), LintId::of(&doc::MISSING_SAFETY_DOC), @@ -1324,7 +1323,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&eval_order_dependence::DIVERGING_SUB_EXPRESSION), LintId::of(&eval_order_dependence::EVAL_ORDER_DEPENDENCE), LintId::of(&explicit_write::EXPLICIT_WRITE), - LintId::of(&field_reassign_with_default::FIELD_REASSIGN_WITH_DEFAULT), LintId::of(&float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS), LintId::of(&float_literal::EXCESSIVE_PRECISION), LintId::of(&format::USELESS_FORMAT), @@ -1556,13 +1554,13 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS), LintId::of(&collapsible_if::COLLAPSIBLE_IF), LintId::of(&comparison_chain::COMPARISON_CHAIN), + LintId::of(&default::FIELD_REASSIGN_WITH_DEFAULT), LintId::of(&doc::MISSING_SAFETY_DOC), LintId::of(&doc::NEEDLESS_DOCTEST_MAIN), LintId::of(&enum_variants::ENUM_VARIANT_NAMES), LintId::of(&enum_variants::MODULE_INCEPTION), LintId::of(&eq_op::OP_REF), LintId::of(&eta_reduction::REDUNDANT_CLOSURE), - LintId::of(&field_reassign_with_default::FIELD_REASSIGN_WITH_DEFAULT), LintId::of(&float_literal::EXCESSIVE_PRECISION), LintId::of(&formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING), LintId::of(&formatting::SUSPICIOUS_ELSE_FORMATTING), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 3a45a7aa85bb..cb720d1547a7 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -345,7 +345,7 @@ vec![ group: "pedantic", desc: "checks for literal calls to `Default::default()`", deprecation: None, - module: "default_trait_access", + module: "default", }, Lint { name: "deprecated_cfg_attr", @@ -618,7 +618,7 @@ vec![ group: "style", desc: "binding initialized with Default should have its fields set in the initializer", deprecation: None, - module: "field_reassign_with_default", + module: "default", }, Lint { name: "filetype_is_file",