Avoid triggering default_trait_access

ebroto · ebroto · commit b8ec12b66599 · 2020-10-24T23:48:31.000+02:00
Do not trigger `default_trait_access` on a span already linted by `field_reassigned_with_default`.
diff --git a/clippy_lints/src/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<Span>,
+}
+
+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 <Foo as Default>::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
diff --git 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,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &copy_iterator::COPY_ITERATOR,
         &create_dir::CREATE_DIR,
         &dbg_macro::DBG_MACRO,
-        &default_trait_access::DEFAULT_TRAIT_ACCESS,
+        &default::DEFAULT_TRAIT_ACCESS,
         &dereference::EXPLICIT_DEREF_METHODS,
         &derive::DERIVE_HASH_XOR_EQ,
         &derive::DERIVE_ORD_XOR_PARTIAL_ORD,
@@ -570,7 +569,7 @@ 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,
+        &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(&copy_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),
@@ -1324,7 +1322,7 @@ 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(&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),
@@ -1562,7 +1560,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         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(&default::FIELD_REASSIGN_WITH_DEFAULT),
         LintId::of(&float_literal::EXCESSIVE_PRECISION),
         LintId::of(&formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),
         LintId::of(&formatting::SUSPICIOUS_ELSE_FORMATTING),
