diff --git a/CHANGELOG.md b/CHANGELOG.md index b7ac3cace20..2c4bb42566d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1256,6 +1256,7 @@ Released 2018-09-13 [`expect_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#expect_fun_call [`expl_impl_clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#expl_impl_clone_on_copy [`explicit_counter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_counter_loop +[`explicit_deref_methods`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_deref_methods [`explicit_into_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_into_iter_loop [`explicit_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_iter_loop [`explicit_write`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_write diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs new file mode 100644 index 00000000000..68ec07e2bcb --- /dev/null +++ b/clippy_lints/src/dereference.rs @@ -0,0 +1,113 @@ +use crate::utils::{get_parent_expr, implements_trait, snippet, span_lint_and_sugg}; +use if_chain::if_chain; +use rustc_ast::util::parser::{ExprPrecedence, PREC_POSTFIX, PREC_PREFIX}; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Span; + +declare_clippy_lint! { + /// **What it does:** Checks for explicit `deref()` or `deref_mut()` method calls. + /// + /// **Why is this bad?** Derefencing by `&*x` or `&mut *x` is clearer and more concise, + /// when not part of a method chain. + /// + /// **Example:** + /// ```rust + /// use std::ops::Deref; + /// let a: &mut String = &mut String::from("foo"); + /// let b: &str = a.deref(); + /// ``` + /// Could be written as: + /// ```rust + /// let a: &mut String = &mut String::from("foo"); + /// let b = &*a; + /// ``` + /// + /// This lint excludes + /// ```rust,ignore + /// let _ = d.unwrap().deref(); + /// ``` + pub EXPLICIT_DEREF_METHODS, + pedantic, + "Explicit use of deref or deref_mut method while not in a method chain." +} + +declare_lint_pass!(Dereferencing => [ + EXPLICIT_DEREF_METHODS +]); + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { + if_chain! { + if !expr.span.from_expansion(); + if let ExprKind::MethodCall(ref method_name, _, ref args) = &expr.kind; + if args.len() == 1; + + then { + if let Some(parent_expr) = get_parent_expr(cx, expr) { + // Check if we have the whole call chain here + if let ExprKind::MethodCall(..) = parent_expr.kind { + return; + } + // Check for Expr that we don't want to be linted + let precedence = parent_expr.precedence(); + match precedence { + // Lint a Call is ok though + ExprPrecedence::Call | ExprPrecedence::AddrOf => (), + _ => { + if precedence.order() >= PREC_PREFIX && precedence.order() <= PREC_POSTFIX { + return; + } + } + } + } + let name = method_name.ident.as_str(); + lint_deref(cx, &*name, &args[0], args[0].span, expr.span); + } + } + } +} + +fn lint_deref(cx: &LateContext<'_, '_>, method_name: &str, call_expr: &Expr<'_>, var_span: Span, expr_span: Span) { + match method_name { + "deref" => { + if cx + .tcx + .lang_items() + .deref_trait() + .map_or(false, |id| implements_trait(cx, cx.tables.expr_ty(&call_expr), id, &[])) + { + span_lint_and_sugg( + cx, + EXPLICIT_DEREF_METHODS, + expr_span, + "explicit deref method call", + "try this", + format!("&*{}", &snippet(cx, var_span, "..")), + Applicability::MachineApplicable, + ); + } + }, + "deref_mut" => { + if cx + .tcx + .lang_items() + .deref_mut_trait() + .map_or(false, |id| implements_trait(cx, cx.tables.expr_ty(&call_expr), id, &[])) + { + span_lint_and_sugg( + cx, + EXPLICIT_DEREF_METHODS, + expr_span, + "explicit deref_mut method call", + "try this", + format!("&mut *{}", &snippet(cx, var_span, "..")), + Applicability::MachineApplicable, + ); + } + }, + _ => (), + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index cb9fcfca8a1..de1bab3a0b9 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -191,6 +191,7 @@ mod copies; mod copy_iterator; mod dbg_macro; mod default_trait_access; +mod dereference; mod derive; mod doc; mod double_comparison; @@ -513,6 +514,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: ©_iterator::COPY_ITERATOR, &dbg_macro::DBG_MACRO, &default_trait_access::DEFAULT_TRAIT_ACCESS, + &dereference::EXPLICIT_DEREF_METHODS, &derive::DERIVE_HASH_XOR_EQ, &derive::EXPL_IMPL_CLONE_ON_COPY, &doc::DOC_MARKDOWN, @@ -1039,6 +1041,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box verbose_file_reads::VerboseFileReads); store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default()); store.register_late_pass(|| box unnamed_address::UnnamedAddress); + store.register_late_pass(|| box dereference::Dereferencing); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1089,6 +1092,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION), LintId::of(©_iterator::COPY_ITERATOR), LintId::of(&default_trait_access::DEFAULT_TRAIT_ACCESS), + LintId::of(&dereference::EXPLICIT_DEREF_METHODS), LintId::of(&derive::EXPL_IMPL_CLONE_ON_COPY), LintId::of(&doc::DOC_MARKDOWN), LintId::of(&doc::MISSING_ERRORS_DOC), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 935ea180ebe..1d147e01066 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -528,6 +528,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "loops", }, + Lint { + name: "explicit_deref_methods", + group: "pedantic", + desc: "Explicit use of deref or deref_mut method while not in a method chain.", + deprecation: None, + module: "dereference", + }, Lint { name: "explicit_into_iter_loop", group: "pedantic", diff --git a/tests/ui/dereference.fixed b/tests/ui/dereference.fixed new file mode 100644 index 00000000000..459ca91b93b --- /dev/null +++ b/tests/ui/dereference.fixed @@ -0,0 +1,93 @@ +// run-rustfix + +#![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)] +#![warn(clippy::explicit_deref_methods)] + +use std::ops::{Deref, DerefMut}; + +fn concat(deref_str: &str) -> String { + format!("{}bar", deref_str) +} + +fn just_return(deref_str: &str) -> &str { + deref_str +} + +struct CustomVec(Vec); +impl Deref for CustomVec { + type Target = Vec; + + fn deref(&self) -> &Vec { + &self.0 + } +} + +fn main() { + let a: &mut String = &mut String::from("foo"); + + // these should require linting + + let b: &str = &*a; + + let b: &mut str = &mut *a; + + // both derefs should get linted here + let b: String = format!("{}, {}", &*a, &*a); + + println!("{}", &*a); + + #[allow(clippy::match_single_binding)] + match &*a { + _ => (), + } + + let b: String = concat(&*a); + + let b = &*just_return(a); + + let b: String = concat(&*just_return(a)); + + let b: &str = &*a.deref(); + + let opt_a = Some(a.clone()); + let b = &*opt_a.unwrap(); + + // following should not require linting + + let cv = CustomVec(vec![0, 42]); + let c = cv.deref()[0]; + + let b: &str = &*a.deref(); + + let b: String = a.deref().clone(); + + let b: usize = a.deref_mut().len(); + + let b: &usize = &a.deref().len(); + + let b: &str = &*a; + + let b: &mut str = &mut *a; + + macro_rules! expr_deref { + ($body:expr) => { + $body.deref() + }; + } + let b: &str = expr_deref!(a); + + // The struct does not implement Deref trait + #[derive(Copy, Clone)] + struct NoLint(u32); + impl NoLint { + pub fn deref(self) -> u32 { + self.0 + } + pub fn deref_mut(self) -> u32 { + self.0 + } + } + let no_lint = NoLint(42); + let b = no_lint.deref(); + let b = no_lint.deref_mut(); +} diff --git a/tests/ui/dereference.rs b/tests/ui/dereference.rs new file mode 100644 index 00000000000..8dc5272e67f --- /dev/null +++ b/tests/ui/dereference.rs @@ -0,0 +1,93 @@ +// run-rustfix + +#![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)] +#![warn(clippy::explicit_deref_methods)] + +use std::ops::{Deref, DerefMut}; + +fn concat(deref_str: &str) -> String { + format!("{}bar", deref_str) +} + +fn just_return(deref_str: &str) -> &str { + deref_str +} + +struct CustomVec(Vec); +impl Deref for CustomVec { + type Target = Vec; + + fn deref(&self) -> &Vec { + &self.0 + } +} + +fn main() { + let a: &mut String = &mut String::from("foo"); + + // these should require linting + + let b: &str = a.deref(); + + let b: &mut str = a.deref_mut(); + + // both derefs should get linted here + let b: String = format!("{}, {}", a.deref(), a.deref()); + + println!("{}", a.deref()); + + #[allow(clippy::match_single_binding)] + match a.deref() { + _ => (), + } + + let b: String = concat(a.deref()); + + let b = just_return(a).deref(); + + let b: String = concat(just_return(a).deref()); + + let b: &str = a.deref().deref(); + + let opt_a = Some(a.clone()); + let b = opt_a.unwrap().deref(); + + // following should not require linting + + let cv = CustomVec(vec![0, 42]); + let c = cv.deref()[0]; + + let b: &str = &*a.deref(); + + let b: String = a.deref().clone(); + + let b: usize = a.deref_mut().len(); + + let b: &usize = &a.deref().len(); + + let b: &str = &*a; + + let b: &mut str = &mut *a; + + macro_rules! expr_deref { + ($body:expr) => { + $body.deref() + }; + } + let b: &str = expr_deref!(a); + + // The struct does not implement Deref trait + #[derive(Copy, Clone)] + struct NoLint(u32); + impl NoLint { + pub fn deref(self) -> u32 { + self.0 + } + pub fn deref_mut(self) -> u32 { + self.0 + } + } + let no_lint = NoLint(42); + let b = no_lint.deref(); + let b = no_lint.deref_mut(); +} diff --git a/tests/ui/dereference.stderr b/tests/ui/dereference.stderr new file mode 100644 index 00000000000..d26b462a433 --- /dev/null +++ b/tests/ui/dereference.stderr @@ -0,0 +1,70 @@ +error: explicit deref method call + --> $DIR/dereference.rs:30:19 + | +LL | let b: &str = a.deref(); + | ^^^^^^^^^ help: try this: `&*a` + | + = note: `-D clippy::explicit-deref-methods` implied by `-D warnings` + +error: explicit deref_mut method call + --> $DIR/dereference.rs:32:23 + | +LL | let b: &mut str = a.deref_mut(); + | ^^^^^^^^^^^^^ help: try this: `&mut *a` + +error: explicit deref method call + --> $DIR/dereference.rs:35:39 + | +LL | let b: String = format!("{}, {}", a.deref(), a.deref()); + | ^^^^^^^^^ help: try this: `&*a` + +error: explicit deref method call + --> $DIR/dereference.rs:35:50 + | +LL | let b: String = format!("{}, {}", a.deref(), a.deref()); + | ^^^^^^^^^ help: try this: `&*a` + +error: explicit deref method call + --> $DIR/dereference.rs:37:20 + | +LL | println!("{}", a.deref()); + | ^^^^^^^^^ help: try this: `&*a` + +error: explicit deref method call + --> $DIR/dereference.rs:40:11 + | +LL | match a.deref() { + | ^^^^^^^^^ help: try this: `&*a` + +error: explicit deref method call + --> $DIR/dereference.rs:44:28 + | +LL | let b: String = concat(a.deref()); + | ^^^^^^^^^ help: try this: `&*a` + +error: explicit deref method call + --> $DIR/dereference.rs:46:13 + | +LL | let b = just_return(a).deref(); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*just_return(a)` + +error: explicit deref method call + --> $DIR/dereference.rs:48:28 + | +LL | let b: String = concat(just_return(a).deref()); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*just_return(a)` + +error: explicit deref method call + --> $DIR/dereference.rs:50:19 + | +LL | let b: &str = a.deref().deref(); + | ^^^^^^^^^^^^^^^^^ help: try this: `&*a.deref()` + +error: explicit deref method call + --> $DIR/dereference.rs:53:13 + | +LL | let b = opt_a.unwrap().deref(); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*opt_a.unwrap()` + +error: aborting due to 11 previous errors +