From 4866309f9d570319b3f0f28793a3aac3e3b107c5 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 14 Jun 2018 08:57:27 +0100 Subject: [PATCH] Add default_trait_access lint --- clippy_lints/src/default_trait_access.rs | 63 ++++++++++++++++++++++++ clippy_lints/src/lib.rs | 4 ++ clippy_lints/src/utils/paths.rs | 1 + tests/ui/default_trait_access.rs | 39 +++++++++++++++ tests/ui/default_trait_access.stderr | 34 +++++++++++++ tests/ui/default_trait_access.stdout | 0 6 files changed, 141 insertions(+) create mode 100644 clippy_lints/src/default_trait_access.rs create mode 100644 tests/ui/default_trait_access.rs create mode 100644 tests/ui/default_trait_access.stderr create mode 100644 tests/ui/default_trait_access.stdout diff --git a/clippy_lints/src/default_trait_access.rs b/clippy_lints/src/default_trait_access.rs new file mode 100644 index 00000000000..0222f896e28 --- /dev/null +++ b/clippy_lints/src/default_trait_access.rs @@ -0,0 +1,63 @@ +use rustc::hir::*; +use rustc::lint::*; + +use crate::utils::{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 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 replacement = format!("{}::default()", cx.tables.expr_ty(expr)); + 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 index 52145341400..80c02db081e 100644 --- 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; @@ -425,6 +426,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![ @@ -512,6 +514,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, @@ -709,6 +712,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/paths.rs b/clippy_lints/src/utils/paths.rs index 5af1a151c8c..7606f4f8471 100644 --- 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 new file mode 100644 index 00000000000..9db875ee305 --- /dev/null +++ b/tests/ui/default_trait_access.rs @@ -0,0 +1,39 @@ +#![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(); + + println!("[{}] [{}] [{}] [{}] [{}] [{}] [{}] [{}] [{}]", s1, s2, s3, s4, s5, s6, s7, s8, s9); +} + +struct DefaultFactory; + +impl DefaultFactory { + pub fn make_t_badly() -> T { + Default::default() + } + + pub fn make_t_nicely() -> T { + T::default() + } +} diff --git a/tests/ui/default_trait_access.stderr b/tests/ui/default_trait_access.stderr new file mode 100644 index 00000000000..b2cb49e6c80 --- /dev/null +++ b/tests/ui/default_trait_access.stderr @@ -0,0 +1,34 @@ +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 T::default() is more clear than this expression + --> $DIR/default_trait_access.rs:33:9 + | +33 | Default::default() + | ^^^^^^^^^^^^^^^^^^ help: try: `T::default()` + +error: aborting due to 5 previous errors + diff --git a/tests/ui/default_trait_access.stdout b/tests/ui/default_trait_access.stdout new file mode 100644 index 00000000000..e69de29bb2d