Merge pull request #2844 from illicitonion/default_trait_access

oli-obk · web-flow · commit 4839c790aeb9 · 2018-06-19T10:10:20.000+02:00
Add default_trait_access lint
diff --git a/clippy_lints/src/default_trait_access.rs b/clippy_lints/src/default_trait_access.rs
@@ -0,0 +1,68 @@
+use rustc::hir::*;
+use rustc::lint::*;
+use rustc::ty::TypeVariants;
+
+use crate::utils::{any_parent_is_automatically_derived, match_def_path, opt_def_id, paths, span_lint_and_sugg};
+
+
+/// **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();
+/// ```
+declare_clippy_lint! {
+    pub DEFAULT_TRAIT_ACCESS,
+    style,
+    "checks for literal calls to Default::default()"
+}
+
+#[derive(Copy, Clone)]
+pub struct DefaultTraitAccess;
+
+impl LintPass for DefaultTraitAccess {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(DEFAULT_TRAIT_ACCESS)
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DefaultTraitAccess {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+        if_chain! {
+            if let ExprCall(ref path, ..) = expr.node;
+            if !any_parent_is_automatically_derived(cx.tcx, expr.id);
+            if let ExprPath(ref qpath) = path.node;
+            if let Some(def_id) = opt_def_id(cx.tables.qpath_def(qpath, path.hir_id));
+            if match_def_path(cx.tcx, def_id, &paths::DEFAULT_TRAIT_METHOD);
+            then {
+                match qpath {
+                    QPath::Resolved(..) => {
+                        // 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 expr_ty = cx.tables.expr_ty(expr);
+                        if let TypeVariants::TyAdt(..) = expr_ty.sty {
+                            let replacement = format!("{}::default()", expr_ty);
+                            span_lint_and_sugg(
+                                cx,
+                                DEFAULT_TRAIT_ACCESS,
+                                expr.span,
+                                &format!("Calling {} is more clear than this expression", replacement),
+                                "try",
+                                replacement);
+                         }
+                    },
+                    QPath::TypeRelative(..) => {},
+                }
+            }
+         }
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -111,6 +111,7 @@ pub mod collapsible_if;
 pub mod const_static_lifetime;
 pub mod copies;
 pub mod cyclomatic_complexity;
+pub mod default_trait_access;
 pub mod derive;
 pub mod doc;
 pub mod double_comparison;
@@ -430,6 +431,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box neg_cmp_op_on_partial_ord::NoNegCompOpForPartialOrd);
     reg.register_late_lint_pass(box unwrap::Pass);
     reg.register_late_lint_pass(box duration_subsec::DurationSubsec);
+    reg.register_late_lint_pass(box default_trait_access::DefaultTraitAccess);
 
 
     reg.register_lint_group("clippy_restriction", vec![
@@ -517,6 +519,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         copies::IF_SAME_THEN_ELSE,
         copies::IFS_SAME_COND,
         cyclomatic_complexity::CYCLOMATIC_COMPLEXITY,
+        default_trait_access::DEFAULT_TRAIT_ACCESS,
         derive::DERIVE_HASH_XOR_EQ,
         double_comparison::DOUBLE_COMPARISONS,
         double_parens::DOUBLE_PARENS,
@@ -715,6 +718,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT,
         collapsible_if::COLLAPSIBLE_IF,
         const_static_lifetime::CONST_STATIC_LIFETIME,
+        default_trait_access::DEFAULT_TRAIT_ACCESS,
         enum_variants::ENUM_VARIANT_NAMES,
         enum_variants::MODULE_INCEPTION,
         eq_op::OP_REF,
diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs
@@ -1129,3 +1129,17 @@ pub fn without_block_comments(lines: Vec<&str>) -> Vec<&str> {
 
     without
 }
+
+pub fn any_parent_is_automatically_derived(tcx: TyCtxt, node: NodeId) -> bool {
+    let map = &tcx.hir;
+    let mut prev_enclosing_node = None;
+    let mut enclosing_node = node;
+    while Some(enclosing_node) != prev_enclosing_node {
+        if is_automatically_derived(map.attrs(enclosing_node)) {
+            return true;
+        }
+        prev_enclosing_node = Some(enclosing_node);
+        enclosing_node = map.get_parent(enclosing_node);
+    }
+    false
+}
diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs
@@ -24,6 +24,7 @@ pub const C_VOID: [&str; 4] = ["std", "os", "raw", "c_void"];
 pub const C_VOID_LIBC: [&str; 2] = ["libc", "c_void"];
 pub const DEBUG_FMT_METHOD: [&str; 4] = ["core", "fmt", "Debug", "fmt"];
 pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"];
+pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"];
 pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"];
 pub const DOUBLE_ENDED_ITERATOR: [&str; 4] = ["core", "iter", "traits", "DoubleEndedIterator"];
 pub const DROP: [&str; 3] = ["core", "mem", "drop"];
diff --git a/tests/ui/default_trait_access.rs b/tests/ui/default_trait_access.rs
@@ -0,0 +1,100 @@
+#![warn(default_trait_access)]
+
+use std::default::Default as D2;
+use std::string;
+use std::default;
+
+fn main() {
+    let s1: String = Default::default();
+
+    let s2 = String::default();
+
+    let s3: String = D2::default();
+
+    let s4: String = std::default::Default::default();
+
+    let s5 = string::String::default();
+
+    let s6: String = default::Default::default();
+
+    let s7 = std::string::String::default();
+
+    let s8: String = DefaultFactory::make_t_badly();
+
+    let s9: String = DefaultFactory::make_t_nicely();
+
+    let s10 = DerivedDefault::default();
+
+    let s11: GenericDerivedDefault<String> = Default::default();
+
+    let s12 = GenericDerivedDefault::<String>::default();
+
+    let s13 = TupleDerivedDefault::default();
+
+    let s14: TupleDerivedDefault = Default::default();
+
+    let s15: ArrayDerivedDefault = Default::default();
+
+    let s16 = ArrayDerivedDefault::default();
+
+    let s17: TupleStructDerivedDefault = Default::default();
+
+    let s18 = TupleStructDerivedDefault::default();
+
+    println!(
+        "[{}] [{}] [{}] [{}] [{}] [{}] [{}] [{}] [{}] [{:?}] [{:?}] [{:?}] [{:?}] [{:?}] [{:?}] [{:?}] [{:?}] [{:?}]",
+        s1,
+        s2,
+        s3,
+        s4,
+        s5,
+        s6,
+        s7,
+        s8,
+        s9,
+        s10,
+        s11,
+        s12,
+        s13,
+        s14,
+        s15,
+        s16,
+        s17,
+        s18,
+    );
+}
+
+struct DefaultFactory;
+
+impl DefaultFactory {
+    pub fn make_t_badly<T: Default>() -> T {
+        Default::default()
+    }
+
+    pub fn make_t_nicely<T: Default>() -> T {
+        T::default()
+    }
+}
+
+#[derive(Debug, Default)]
+struct DerivedDefault {
+    pub s: String,
+}
+
+#[derive(Debug, Default)]
+struct GenericDerivedDefault<T: Default + std::fmt::Debug> {
+    pub s: T,
+}
+
+#[derive(Debug, Default)]
+struct TupleDerivedDefault {
+    pub s: (String, String),
+}
+
+#[derive(Debug, Default)]
+struct ArrayDerivedDefault {
+    pub s: [String; 10],
+}
+
+#[derive(Debug, Default)]
+struct TupleStructDerivedDefault(String);
diff --git a/tests/ui/default_trait_access.stderr b/tests/ui/default_trait_access.stderr
@@ -0,0 +1,52 @@
+error: Calling std::string::String::default() is more clear than this expression
+ --> $DIR/default_trait_access.rs:8:22
+  |
+8 |     let s1: String = Default::default();
+  |                      ^^^^^^^^^^^^^^^^^^ help: try: `std::string::String::default()`
+  |
+  = note: `-D default-trait-access` implied by `-D warnings`
+
+error: Calling std::string::String::default() is more clear than this expression
+  --> $DIR/default_trait_access.rs:12:22
+   |
+12 |     let s3: String = D2::default();
+   |                      ^^^^^^^^^^^^^ help: try: `std::string::String::default()`
+
+error: Calling std::string::String::default() is more clear than this expression
+  --> $DIR/default_trait_access.rs:14:22
+   |
+14 |     let s4: String = std::default::Default::default();
+   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::string::String::default()`
+
+error: Calling std::string::String::default() is more clear than this expression
+  --> $DIR/default_trait_access.rs:18:22
+   |
+18 |     let s6: String = default::Default::default();
+   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::string::String::default()`
+
+error: Calling GenericDerivedDefault<std::string::String>::default() is more clear than this expression
+  --> $DIR/default_trait_access.rs:28:46
+   |
+28 |     let s11: GenericDerivedDefault<String> = Default::default();
+   |                                              ^^^^^^^^^^^^^^^^^^ help: try: `GenericDerivedDefault<std::string::String>::default()`
+
+error: Calling TupleDerivedDefault::default() is more clear than this expression
+  --> $DIR/default_trait_access.rs:34:36
+   |
+34 |     let s14: TupleDerivedDefault = Default::default();
+   |                                    ^^^^^^^^^^^^^^^^^^ help: try: `TupleDerivedDefault::default()`
+
+error: Calling ArrayDerivedDefault::default() is more clear than this expression
+  --> $DIR/default_trait_access.rs:36:36
+   |
+36 |     let s15: ArrayDerivedDefault = Default::default();
+   |                                    ^^^^^^^^^^^^^^^^^^ help: try: `ArrayDerivedDefault::default()`
+
+error: Calling TupleStructDerivedDefault::default() is more clear than this expression
+  --> $DIR/default_trait_access.rs:40:42
+   |
+40 |     let s17: TupleStructDerivedDefault = Default::default();
+   |                                          ^^^^^^^^^^^^^^^^^^ help: try: `TupleStructDerivedDefault::default()`
+
+error: aborting due to 8 previous errors
+
diff --git a/tests/ui/default_trait_access.stdout b/tests/ui/default_trait_access.stdout
diff --git a/tests/ui/implicit_hasher.rs b/tests/ui/implicit_hasher.rs
@@ -30,12 +30,12 @@ impl Foo<i16> for HashMap<String, String> {
 
 impl<K: Hash + Eq, V, S: BuildHasher + Default> Foo<i32> for HashMap<K, V, S> {
     fn make() -> (Self, Self) {
-        (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
+        (HashMap::default(), HashMap::with_capacity_and_hasher(10, S::default()))
     }
 }
 impl<S: BuildHasher + Default> Foo<i64> for HashMap<String, String, S> {
     fn make() -> (Self, Self) {
-        (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))
+        (HashMap::default(), HashMap::with_capacity_and_hasher(10, S::default()))
     }
 }
 
@@ -53,12 +53,12 @@ impl Foo<i16> for HashSet<String> {
 
 impl<T: Hash + Eq, S: BuildHasher + Default> Foo<i32> for HashSet<T, S> {
     fn make() -> (Self, Self) {
-        (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))
+        (HashSet::default(), HashSet::with_capacity_and_hasher(10, S::default()))
     }
 }
 impl<S: BuildHasher + Default> Foo<i64> for HashSet<String, S> {
     fn make() -> (Self, Self) {
-        (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))
+        (HashSet::default(), HashSet::with_capacity_and_hasher(10, S::default()))
     }
 }
 
diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs
@@ -3,7 +3,8 @@
 
 #![warn(clippy, clippy_pedantic, option_unwrap_used)]
 #![allow(blacklisted_name, unused, print_stdout, non_ascii_literal, new_without_default,
-    new_without_default_derive, missing_docs_in_private_items, needless_pass_by_value)]
+    new_without_default_derive, missing_docs_in_private_items, needless_pass_by_value,
+    default_trait_access)]
 
 use std::collections::BTreeMap;
 use std::collections::HashMap;
diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr

Original file line number	Diff line number	Diff line change
`@@ -30,12 +30,12 @@ impl Foo<i16> for HashMap<String, String> {`
`30`	`30`
`31`	`31`	`impl<K: Hash + Eq, V, S: BuildHasher + Default> Foo<i32> for HashMap<K, V, S> {`
`32`	`32`	`fn make() -> (Self, Self) {`
`33`		`- (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))`
	`33`	`+ (HashMap::default(), HashMap::with_capacity_and_hasher(10, S::default()))`
`34`	`34`	`}`
`35`	`35`	`}`
`36`	`36`	`impl<S: BuildHasher + Default> Foo<i64> for HashMap<String, String, S> {`
`37`	`37`	`fn make() -> (Self, Self) {`
`38`		`- (HashMap::default(), HashMap::with_capacity_and_hasher(10, Default::default()))`
	`38`	`+ (HashMap::default(), HashMap::with_capacity_and_hasher(10, S::default()))`
`39`	`39`	`}`
`40`	`40`	`}`
`41`	`41`
`@@ -53,12 +53,12 @@ impl Foo<i16> for HashSet<String> {`
`53`	`53`
`54`	`54`	`impl<T: Hash + Eq, S: BuildHasher + Default> Foo<i32> for HashSet<T, S> {`
`55`	`55`	`fn make() -> (Self, Self) {`
`56`		`- (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))`
	`56`	`+ (HashSet::default(), HashSet::with_capacity_and_hasher(10, S::default()))`
`57`	`57`	`}`
`58`	`58`	`}`
`59`	`59`	`impl<S: BuildHasher + Default> Foo<i64> for HashSet<String, S> {`
`60`	`60`	`fn make() -> (Self, Self) {`
`61`		`- (HashSet::default(), HashSet::with_capacity_and_hasher(10, Default::default()))`
	`61`	`+ (HashSet::default(), HashSet::with_capacity_and_hasher(10, S::default()))`
`62`	`62`	`}`
`63`	`63`	`}`
`64`	`64`