From b2d986850d795f8aa3bdea1d2c840b080c9df81c Mon Sep 17 00:00:00 2001 From: Tom Milligan Date: Wed, 3 Oct 2018 17:53:39 +0100 Subject: [PATCH 1/6] Working basic dereference clip --- clippy_lints/src/dereference.rs | 74 +++++++++++++++++++++++++++++++++ clippy_lints/src/lib.rs | 6 ++- tests/ui/dereference.rs | 55 ++++++++++++++++++++++++ tests/ui/dereference.stderr | 52 +++++++++++++++++++++++ 4 files changed, 186 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/dereference.rs create mode 100644 tests/ui/dereference.rs create mode 100644 tests/ui/dereference.stderr diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs new file mode 100644 index 000000000000..c29c0d466d10 --- /dev/null +++ b/clippy_lints/src/dereference.rs @@ -0,0 +1,74 @@ +use crate::rustc::hir::{Expr, ExprKind, QPath}; +use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use crate::rustc::{declare_tool_lint, lint_array}; +use crate::utils::{in_macro, span_lint_and_sugg}; +use if_chain::if_chain; + +/// **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 +/// let b = a.deref(); +/// let c = a.deref_mut(); +/// +/// // excludes +/// let e = d.unwrap().deref(); +/// ``` +declare_clippy_lint! { + pub EXPLICIT_DEREF_METHOD, + pedantic, + "Explicit use of deref or deref_mut method while not in a method chain." +} + +pub struct Pass; + +impl LintPass for Pass { + fn get_lints(&self) -> LintArray { + lint_array!(EXPLICIT_DEREF_METHOD) + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { + fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr) { + if in_macro(expr.span) { + return; + } + + if_chain! { + // if this is a method call + if let ExprKind::MethodCall(ref method_name, _, ref args) = &expr.node; + // on a Path (i.e. a variable/name, not another method) + if let ExprKind::Path(QPath::Resolved(None, path)) = &args[0].node; + then { + let name = method_name.ident.as_str(); + // alter help slightly to account for _mut + match &*name { + "deref" => { + span_lint_and_sugg( + cx, + EXPLICIT_DEREF_METHOD, + expr.span, + "explicit deref method call", + "try this", + format!("&*{}", path), + ); + }, + "deref_mut" => { + span_lint_and_sugg( + cx, + EXPLICIT_DEREF_METHOD, + expr.span, + "explicit deref_mut method call", + "try this", + format!("&mut *{}", path), + ); + }, + _ => () + }; + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index cb9fcfca8a1c..a6ddd6573a81 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -391,7 +391,7 @@ pub fn read_conf(args: &[rustc_ast::ast::NestedMetaItem], sess: &Session) -> Con } conf - }, + } Err((err, span)) => { sess.struct_span_err(span, err) .span_note(span, "Clippy will use default configuration") @@ -513,6 +513,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::DEREF_METHOD_EXPLICIT, &derive::DERIVE_HASH_XOR_EQ, &derive::EXPL_IMPL_CLONE_ON_COPY, &doc::DOC_MARKDOWN, @@ -1039,6 +1040,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::DerefMethodExplicit); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1089,6 +1091,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_METHOD), LintId::of(&derive::EXPL_IMPL_CLONE_ON_COPY), LintId::of(&doc::DOC_MARKDOWN), LintId::of(&doc::MISSING_ERRORS_DOC), @@ -1178,6 +1181,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(&dereference::EXPLICIT_DEREF_METHOD), LintId::of(&derive::DERIVE_HASH_XOR_EQ), LintId::of(&doc::MISSING_SAFETY_DOC), LintId::of(&doc::NEEDLESS_DOCTEST_MAIN), diff --git a/tests/ui/dereference.rs b/tests/ui/dereference.rs new file mode 100644 index 000000000000..7800cd84c242 --- /dev/null +++ b/tests/ui/dereference.rs @@ -0,0 +1,55 @@ +#![feature(tool_lints)] + +use std::ops::{Deref, DerefMut}; + +#[allow(clippy::many_single_char_names, clippy::clone_double_ref)] +#[allow(unused_variables)] +#[warn(clippy::explicit_deref_method)] +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(); + } + + { + let b: String = a.deref().clone(); + } + + { + let b: usize = a.deref_mut().len(); + } + + { + let b: &usize = &a.deref().len(); + } + + { + // only first deref should get linted here + let b: &str = a.deref().deref(); + } + + { + // both derefs should get linted here + let b: String = format!("{}, {}", a.deref(), a.deref()); + } + + // these should not require linting + { + let b: &str = &*a; + } + + { + let b: &mut str = &mut *a; + } + + { + macro_rules! expr_deref { ($body:expr) => { $body.deref() } } + let b: &str = expr_deref!(a); + } +} diff --git a/tests/ui/dereference.stderr b/tests/ui/dereference.stderr new file mode 100644 index 000000000000..a4c2487d06b9 --- /dev/null +++ b/tests/ui/dereference.stderr @@ -0,0 +1,52 @@ +error: explicit deref method call + --> $DIR/dereference.rs:13:23 + | +13 | let b: &str = a.deref(); + | ^^^^^^^^^ help: try this: `&*a` + | + = note: `-D clippy::explicit-deref-method` implied by `-D warnings` + +error: explicit deref_mut method call + --> $DIR/dereference.rs:17:27 + | +17 | let b: &mut str = a.deref_mut(); + | ^^^^^^^^^^^^^ help: try this: `&mut *a` + +error: explicit deref method call + --> $DIR/dereference.rs:21:25 + | +21 | let b: String = a.deref().clone(); + | ^^^^^^^^^ help: try this: `&*a` + +error: explicit deref_mut method call + --> $DIR/dereference.rs:25:24 + | +25 | let b: usize = a.deref_mut().len(); + | ^^^^^^^^^^^^^ help: try this: `&mut *a` + +error: explicit deref method call + --> $DIR/dereference.rs:29:26 + | +29 | let b: &usize = &a.deref().len(); + | ^^^^^^^^^ help: try this: `&*a` + +error: explicit deref method call + --> $DIR/dereference.rs:34:23 + | +34 | let b: &str = a.deref().deref(); + | ^^^^^^^^^ help: try this: `&*a` + +error: explicit deref method call + --> $DIR/dereference.rs:39:43 + | +39 | let b: String = format!("{}, {}", a.deref(), a.deref()); + | ^^^^^^^^^ help: try this: `&*a` + +error: explicit deref method call + --> $DIR/dereference.rs:39:54 + | +39 | let b: String = format!("{}, {}", a.deref(), a.deref()); + | ^^^^^^^^^ help: try this: `&*a` + +error: aborting due to 8 previous errors + From 6b4ab827469529f4eda5f1e9492abcb9ad9d209a Mon Sep 17 00:00:00 2001 From: ThibsG Date: Thu, 23 Jan 2020 16:28:01 +0100 Subject: [PATCH 2/6] Global rework + fix imports --- CHANGELOG.md | 1 + clippy_lints/src/dereference.rs | 60 ++++++++++++++++++--------------- clippy_lints/src/lib.rs | 7 ++-- src/lintlist/mod.rs | 7 ++++ tests/ui/dereference.rs | 59 ++++++++++++-------------------- tests/ui/dereference.stderr | 48 +++++++++++++------------- 6 files changed, 89 insertions(+), 93 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7ac3cace204..eb5a6af97d36 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_method`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_deref_method [`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 index c29c0d466d10..dea00e5aa3b6 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1,47 +1,51 @@ -use crate::rustc::hir::{Expr, ExprKind, QPath}; -use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; -use crate::rustc::{declare_tool_lint, lint_array}; +use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_errors::Applicability; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_tool_lint, declare_lint_pass}; use crate::utils::{in_macro, span_lint_and_sugg}; use if_chain::if_chain; -/// **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 -/// let b = a.deref(); -/// let c = a.deref_mut(); -/// -/// // excludes -/// let e = d.unwrap().deref(); -/// ``` 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 + /// let b = a.deref(); + /// let c = a.deref_mut(); + /// ``` + /// Could be written as: + /// ```rust + /// let b = &*a; + /// let c = &mut *a; + /// ``` + /// + /// This lint excludes + /// ```rust + /// let e = d.unwrap().deref(); + /// ``` pub EXPLICIT_DEREF_METHOD, pedantic, "Explicit use of deref or deref_mut method while not in a method chain." } -pub struct Pass; +declare_lint_pass!(Dereferencing => [ + EXPLICIT_DEREF_METHOD +]); -impl LintPass for Pass { - fn get_lints(&self) -> LintArray { - lint_array!(EXPLICIT_DEREF_METHOD) - } -} - -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { - fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr) { +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { if in_macro(expr.span) { return; } if_chain! { // if this is a method call - if let ExprKind::MethodCall(ref method_name, _, ref args) = &expr.node; + if let ExprKind::MethodCall(ref method_name, _, ref args) = &expr.kind; // on a Path (i.e. a variable/name, not another method) - if let ExprKind::Path(QPath::Resolved(None, path)) = &args[0].node; + if let ExprKind::Path(QPath::Resolved(None, path)) = &args[0].kind; then { let name = method_name.ident.as_str(); // alter help slightly to account for _mut @@ -54,6 +58,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { "explicit deref method call", "try this", format!("&*{}", path), + Applicability::MachineApplicable ); }, "deref_mut" => { @@ -64,6 +69,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { "explicit deref_mut method call", "try this", format!("&mut *{}", path), + Applicability::MachineApplicable ); }, _ => () diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a6ddd6573a81..6443caa89d62 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -391,7 +391,7 @@ pub fn read_conf(args: &[rustc_ast::ast::NestedMetaItem], sess: &Session) -> Con } conf - } + }, Err((err, span)) => { sess.struct_span_err(span, err) .span_note(span, "Clippy will use default configuration") @@ -513,7 +513,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::DEREF_METHOD_EXPLICIT, + &dereference::EXPLICIT_DEREF_METHOD, &derive::DERIVE_HASH_XOR_EQ, &derive::EXPL_IMPL_CLONE_ON_COPY, &doc::DOC_MARKDOWN, @@ -1040,7 +1040,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::DerefMethodExplicit); + store.register_late_pass(|| box dereference::Dereferencing); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1181,7 +1181,6 @@ 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(&dereference::EXPLICIT_DEREF_METHOD), LintId::of(&derive::DERIVE_HASH_XOR_EQ), LintId::of(&doc::MISSING_SAFETY_DOC), LintId::of(&doc::NEEDLESS_DOCTEST_MAIN), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 935ea180ebe2..3f1d31f0302c 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_method", + 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.rs b/tests/ui/dereference.rs index 7800cd84c242..07421eb715df 100644 --- a/tests/ui/dereference.rs +++ b/tests/ui/dereference.rs @@ -1,55 +1,38 @@ -#![feature(tool_lints)] +#![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)] +#![warn(clippy::explicit_deref_method)] use std::ops::{Deref, DerefMut}; -#[allow(clippy::many_single_char_names, clippy::clone_double_ref)] -#[allow(unused_variables)] -#[warn(clippy::explicit_deref_method)] fn main() { let a: &mut String = &mut String::from("foo"); // these should require linting - { - let b: &str = a.deref(); - } + let b: &str = a.deref(); - { - let b: &mut str = a.deref_mut(); - } + let b: &mut str = a.deref_mut(); - { - let b: String = a.deref().clone(); - } - - { - let b: usize = a.deref_mut().len(); - } - - { - let b: &usize = &a.deref().len(); - } + let b: String = a.deref().clone(); - { - // only first deref should get linted here - let b: &str = a.deref().deref(); - } + let b: usize = a.deref_mut().len(); - { - // both derefs should get linted here - let b: String = format!("{}, {}", a.deref(), a.deref()); - } + let b: &usize = &a.deref().len(); + + // only first deref should get linted here + let b: &str = a.deref().deref(); + + // both derefs should get linted here + let b: String = format!("{}, {}", a.deref(), a.deref()); // these should not require linting - { - let b: &str = &*a; - } - { - let b: &mut str = &mut *a; - } + let b: &str = &*a; + + let b: &mut str = &mut *a; - { - macro_rules! expr_deref { ($body:expr) => { $body.deref() } } - let b: &str = expr_deref!(a); + macro_rules! expr_deref { + ($body:expr) => { + $body.deref() + }; } + let b: &str = expr_deref!(a); } diff --git a/tests/ui/dereference.stderr b/tests/ui/dereference.stderr index a4c2487d06b9..7169b689a860 100644 --- a/tests/ui/dereference.stderr +++ b/tests/ui/dereference.stderr @@ -1,52 +1,52 @@ error: explicit deref method call - --> $DIR/dereference.rs:13:23 + --> $DIR/dereference.rs:10:19 | -13 | let b: &str = a.deref(); - | ^^^^^^^^^ help: try this: `&*a` +LL | let b: &str = a.deref(); + | ^^^^^^^^^ help: try this: `&*a` | = note: `-D clippy::explicit-deref-method` implied by `-D warnings` error: explicit deref_mut method call - --> $DIR/dereference.rs:17:27 + --> $DIR/dereference.rs:12:23 | -17 | let b: &mut str = a.deref_mut(); - | ^^^^^^^^^^^^^ help: try this: `&mut *a` +LL | let b: &mut str = a.deref_mut(); + | ^^^^^^^^^^^^^ help: try this: `&mut *a` error: explicit deref method call - --> $DIR/dereference.rs:21:25 + --> $DIR/dereference.rs:14:21 | -21 | let b: String = a.deref().clone(); - | ^^^^^^^^^ help: try this: `&*a` +LL | let b: String = a.deref().clone(); + | ^^^^^^^^^ help: try this: `&*a` error: explicit deref_mut method call - --> $DIR/dereference.rs:25:24 + --> $DIR/dereference.rs:16:20 | -25 | let b: usize = a.deref_mut().len(); - | ^^^^^^^^^^^^^ help: try this: `&mut *a` +LL | let b: usize = a.deref_mut().len(); + | ^^^^^^^^^^^^^ help: try this: `&mut *a` error: explicit deref method call - --> $DIR/dereference.rs:29:26 + --> $DIR/dereference.rs:18:22 | -29 | let b: &usize = &a.deref().len(); - | ^^^^^^^^^ help: try this: `&*a` +LL | let b: &usize = &a.deref().len(); + | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:34:23 + --> $DIR/dereference.rs:21:19 | -34 | let b: &str = a.deref().deref(); - | ^^^^^^^^^ help: try this: `&*a` +LL | let b: &str = a.deref().deref(); + | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:39:43 + --> $DIR/dereference.rs:24:39 | -39 | let b: String = format!("{}, {}", a.deref(), a.deref()); - | ^^^^^^^^^ help: try this: `&*a` +LL | let b: String = format!("{}, {}", a.deref(), a.deref()); + | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:39:54 + --> $DIR/dereference.rs:24:50 | -39 | let b: String = format!("{}, {}", a.deref(), a.deref()); - | ^^^^^^^^^ help: try this: `&*a` +LL | let b: String = format!("{}, {}", a.deref(), a.deref()); + | ^^^^^^^^^ help: try this: `&*a` error: aborting due to 8 previous errors From c1132434a7cf580a09d08a274bbfd2e776b324f8 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sun, 26 Jan 2020 19:48:30 +0100 Subject: [PATCH 3/6] Report using stmts and expr + tests --- clippy_lints/src/dereference.rs | 138 +++++++++++++++++++++++--------- tests/ui/dereference.rs | 36 +++++++-- tests/ui/dereference.stderr | 42 +++++----- 3 files changed, 148 insertions(+), 68 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index dea00e5aa3b6..9022617ebfc1 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1,9 +1,11 @@ -use rustc_hir::{Expr, ExprKind, QPath}; +use crate::utils::{get_parent_expr, method_calls, snippet, span_lint_and_sugg}; +use if_chain::if_chain; use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_hir::{Expr, ExprKind, QPath, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_tool_lint, declare_lint_pass}; -use crate::utils::{in_macro, span_lint_and_sugg}; -use if_chain::if_chain; +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. @@ -21,7 +23,7 @@ declare_clippy_lint! { /// let b = &*a; /// let c = &mut *a; /// ``` - /// + /// /// This lint excludes /// ```rust /// let e = d.unwrap().deref(); @@ -36,45 +38,105 @@ declare_lint_pass!(Dereferencing => [ ]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { - fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { - if in_macro(expr.span) { - return; + fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx hir::Stmt<'_>) { + if_chain! { + if let StmtKind::Local(ref local) = stmt.kind; + if let Some(ref init) = local.init; + + then { + match init.kind { + ExprKind::Call(ref _method, args) => { + for arg in args { + if_chain! { + // Caller must call only one other function (deref or deref_mut) + // otherwise it can lead to error prone suggestions (ex: &*a.len()) + let (method_names, arg_list, _) = method_calls(arg, 2); + if method_names.len() == 1; + // Caller must be a variable + let variables = arg_list[0]; + if variables.len() == 1; + if let ExprKind::Path(QPath::Resolved(None, _)) = variables[0].kind; + + then { + let name = method_names[0].as_str(); + lint_deref(cx, &*name, variables[0].span, arg.span); + } + } + } + } + ExprKind::MethodCall(ref method_name, _, ref args) => { + if init.span.from_expansion() { + return; + } + if_chain! { + if args.len() == 1; + if let ExprKind::Path(QPath::Resolved(None, _)) = args[0].kind; + // Caller must call only one other function (deref or deref_mut) + // otherwise it can lead to error prone suggestions (ex: &*a.len()) + let (method_names, arg_list, _) = method_calls(init, 2); + if method_names.len() == 1; + // Caller must be a variable + let variables = arg_list[0]; + if variables.len() == 1; + if let ExprKind::Path(QPath::Resolved(None, _)) = variables[0].kind; + + then { + let name = method_name.ident.as_str(); + lint_deref(cx, &*name, args[0].span, init.span); + } + } + } + _ => () + } + } } + } + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { if_chain! { - // if this is a method call if let ExprKind::MethodCall(ref method_name, _, ref args) = &expr.kind; - // on a Path (i.e. a variable/name, not another method) - if let ExprKind::Path(QPath::Resolved(None, path)) = &args[0].kind; + if args.len() == 1; + if let Some(parent) = get_parent_expr(cx, &expr); + then { - let name = method_name.ident.as_str(); - // alter help slightly to account for _mut - match &*name { - "deref" => { - span_lint_and_sugg( - cx, - EXPLICIT_DEREF_METHOD, - expr.span, - "explicit deref method call", - "try this", - format!("&*{}", path), - Applicability::MachineApplicable - ); - }, - "deref_mut" => { - span_lint_and_sugg( - cx, - EXPLICIT_DEREF_METHOD, - expr.span, - "explicit deref_mut method call", - "try this", - format!("&mut *{}", path), - Applicability::MachineApplicable - ); - }, - _ => () - }; + // Call and MethodCall exprs are better reported using statements + match parent.kind { + ExprKind::Call(_, _) => return, + ExprKind::MethodCall(_, _, _) => return, + _ => { + let name = method_name.ident.as_str(); + lint_deref(cx, &*name, args[0].span, expr.span); + } + } } } } } + +fn lint_deref(cx: &LateContext<'_, '_>, fn_name: &str, var_span: Span, expr_span: Span) { + match fn_name { + "deref" => { + span_lint_and_sugg( + cx, + EXPLICIT_DEREF_METHOD, + expr_span, + "explicit deref method call", + "try this", + format!("&*{}", &snippet(cx, var_span, "..")), + Applicability::MachineApplicable, + ); + }, + "deref_mut" => { + span_lint_and_sugg( + cx, + EXPLICIT_DEREF_METHOD, + expr_span, + "explicit deref_mut method call", + "try this", + format!("&mut *{}", &snippet(cx, var_span, "..")), + Applicability::MachineApplicable, + ); + }, + _ => (), + } +} diff --git a/tests/ui/dereference.rs b/tests/ui/dereference.rs index 07421eb715df..5de201fb22f0 100644 --- a/tests/ui/dereference.rs +++ b/tests/ui/dereference.rs @@ -3,28 +3,49 @@ use std::ops::{Deref, DerefMut}; +fn concat(deref_str: &str) -> String { + format!("{}bar", deref_str) +} + +fn just_return(deref_str: &str) -> &str { + deref_str +} + 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()); + + // following should not require linting + + let b = just_return(a).deref(); + + let b: String = concat(just_return(a).deref()); + let b: String = a.deref().clone(); let b: usize = a.deref_mut().len(); let b: &usize = &a.deref().len(); - // only first deref should get linted here let b: &str = a.deref().deref(); - // both derefs should get linted here - let b: String = format!("{}, {}", a.deref(), a.deref()); - - // these should not require linting - let b: &str = &*a; let b: &mut str = &mut *a; @@ -35,4 +56,7 @@ fn main() { }; } let b: &str = expr_deref!(a); + + let opt_a = Some(a); + let b = opt_a.unwrap().deref(); } diff --git a/tests/ui/dereference.stderr b/tests/ui/dereference.stderr index 7169b689a860..7e10add40b18 100644 --- a/tests/ui/dereference.stderr +++ b/tests/ui/dereference.stderr @@ -1,5 +1,5 @@ error: explicit deref method call - --> $DIR/dereference.rs:10:19 + --> $DIR/dereference.rs:19:19 | LL | let b: &str = a.deref(); | ^^^^^^^^^ help: try this: `&*a` @@ -7,46 +7,40 @@ LL | let b: &str = a.deref(); = note: `-D clippy::explicit-deref-method` implied by `-D warnings` error: explicit deref_mut method call - --> $DIR/dereference.rs:12:23 + --> $DIR/dereference.rs:21:23 | LL | let b: &mut str = a.deref_mut(); | ^^^^^^^^^^^^^ help: try this: `&mut *a` error: explicit deref method call - --> $DIR/dereference.rs:14:21 - | -LL | let b: String = a.deref().clone(); - | ^^^^^^^^^ help: try this: `&*a` - -error: explicit deref_mut method call - --> $DIR/dereference.rs:16:20 + --> $DIR/dereference.rs:24:39 | -LL | let b: usize = a.deref_mut().len(); - | ^^^^^^^^^^^^^ help: try this: `&mut *a` +LL | let b: String = format!("{}, {}", a.deref(), a.deref()); + | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:18:22 + --> $DIR/dereference.rs:24:50 | -LL | let b: &usize = &a.deref().len(); - | ^^^^^^^^^ help: try this: `&*a` +LL | let b: String = format!("{}, {}", a.deref(), a.deref()); + | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:21:19 + --> $DIR/dereference.rs:26:20 | -LL | let b: &str = a.deref().deref(); - | ^^^^^^^^^ help: try this: `&*a` +LL | println!("{}", a.deref()); + | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:24:39 + --> $DIR/dereference.rs:29:11 | -LL | let b: String = format!("{}, {}", a.deref(), a.deref()); - | ^^^^^^^^^ help: try this: `&*a` +LL | match a.deref() { + | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:24:50 + --> $DIR/dereference.rs:33:28 | -LL | let b: String = format!("{}, {}", a.deref(), a.deref()); - | ^^^^^^^^^ help: try this: `&*a` +LL | let b: String = concat(a.deref()); + | ^^^^^^^^^ help: try this: `&*a` -error: aborting due to 8 previous errors +error: aborting due to 7 previous errors From b6d43305502f15d3c3e1a49f488eedc5ce330b4f Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 25 Feb 2020 23:06:24 +0100 Subject: [PATCH 4/6] Check for Deref trait impl + add fixed version --- clippy_lints/src/dereference.rs | 77 ++++++++++++++++++-------------- clippy_lints/src/utils/paths.rs | 2 + tests/ui/dereference.fixed | 79 +++++++++++++++++++++++++++++++++ tests/ui/dereference.rs | 17 +++++++ tests/ui/dereference.stderr | 14 +++--- 5 files changed, 149 insertions(+), 40 deletions(-) create mode 100644 tests/ui/dereference.fixed diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 9022617ebfc1..d11614d489d2 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1,4 +1,6 @@ -use crate::utils::{get_parent_expr, method_calls, snippet, span_lint_and_sugg}; +use crate::utils::{ + get_parent_expr, get_trait_def_id, implements_trait, method_calls, paths, snippet, span_lint_and_sugg, +}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; @@ -15,18 +17,19 @@ declare_clippy_lint! { /// /// **Example:** /// ```rust - /// let b = a.deref(); - /// let c = a.deref_mut(); + /// 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; - /// let c = &mut *a; /// ``` /// /// This lint excludes - /// ```rust - /// let e = d.unwrap().deref(); + /// ```rust,ignore + /// let _ = d.unwrap().deref(); /// ``` pub EXPLICIT_DEREF_METHOD, pedantic, @@ -49,7 +52,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { for arg in args { if_chain! { // Caller must call only one other function (deref or deref_mut) - // otherwise it can lead to error prone suggestions (ex: &*a.len()) + // otherwise it can lead to error prone suggestions (ie: &*a.len()) let (method_names, arg_list, _) = method_calls(arg, 2); if method_names.len() == 1; // Caller must be a variable @@ -59,7 +62,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { then { let name = method_names[0].as_str(); - lint_deref(cx, &*name, variables[0].span, arg.span); + lint_deref(cx, &*name, &variables[0], variables[0].span, arg.span); } } } @@ -72,7 +75,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { if args.len() == 1; if let ExprKind::Path(QPath::Resolved(None, _)) = args[0].kind; // Caller must call only one other function (deref or deref_mut) - // otherwise it can lead to error prone suggestions (ex: &*a.len()) + // otherwise it can lead to error prone suggestions (ie: &*a.len()) let (method_names, arg_list, _) = method_calls(init, 2); if method_names.len() == 1; // Caller must be a variable @@ -82,7 +85,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { then { let name = method_name.ident.as_str(); - lint_deref(cx, &*name, args[0].span, init.span); + lint_deref(cx, &*name, init, args[0].span, init.span); } } } @@ -96,16 +99,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { if_chain! { if let ExprKind::MethodCall(ref method_name, _, ref args) = &expr.kind; if args.len() == 1; + if let ExprKind::Path(QPath::Resolved(None, _)) = args[0].kind; if let Some(parent) = get_parent_expr(cx, &expr); then { - // Call and MethodCall exprs are better reported using statements match parent.kind { - ExprKind::Call(_, _) => return, - ExprKind::MethodCall(_, _, _) => return, + // Already linted using statements + ExprKind::Call(_, _) | ExprKind::MethodCall(_, _, _) => (), _ => { let name = method_name.ident.as_str(); - lint_deref(cx, &*name, args[0].span, expr.span); + lint_deref(cx, &*name, &args[0], args[0].span, expr.span); } } } @@ -113,29 +116,37 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { } } -fn lint_deref(cx: &LateContext<'_, '_>, fn_name: &str, var_span: Span, expr_span: Span) { +fn lint_deref(cx: &LateContext<'_, '_>, fn_name: &str, call_expr: &Expr<'_>, var_span: Span, expr_span: Span) { match fn_name { "deref" => { - span_lint_and_sugg( - cx, - EXPLICIT_DEREF_METHOD, - expr_span, - "explicit deref method call", - "try this", - format!("&*{}", &snippet(cx, var_span, "..")), - Applicability::MachineApplicable, - ); + if get_trait_def_id(cx, &paths::DEREF_TRAIT) + .map_or(false, |id| implements_trait(cx, cx.tables.expr_ty(&call_expr), id, &[])) + { + span_lint_and_sugg( + cx, + EXPLICIT_DEREF_METHOD, + expr_span, + "explicit deref method call", + "try this", + format!("&*{}", &snippet(cx, var_span, "..")), + Applicability::MachineApplicable, + ); + } }, "deref_mut" => { - span_lint_and_sugg( - cx, - EXPLICIT_DEREF_METHOD, - expr_span, - "explicit deref_mut method call", - "try this", - format!("&mut *{}", &snippet(cx, var_span, "..")), - Applicability::MachineApplicable, - ); + if get_trait_def_id(cx, &paths::DEREF_MUT_TRAIT) + .map_or(false, |id| implements_trait(cx, cx.tables.expr_ty(&call_expr), id, &[])) + { + span_lint_and_sugg( + cx, + EXPLICIT_DEREF_METHOD, + expr_span, + "explicit deref_mut method call", + "try this", + format!("&mut *{}", &snippet(cx, var_span, "..")), + Applicability::MachineApplicable, + ); + } }, _ => (), } diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index d93f8a1e5609..a49299e5f6fb 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -25,7 +25,9 @@ pub const CSTRING: [&str; 4] = ["std", "ffi", "c_str", "CString"]; pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"]; pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"]; pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"]; +pub const DEREF_MUT_TRAIT: [&str; 4] = ["core", "ops", "deref", "DerefMut"]; pub const DEREF_MUT_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "DerefMut", "deref_mut"]; +pub const DEREF_TRAIT: [&str; 4] = ["core", "ops", "deref", "Deref"]; pub const DEREF_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "Deref", "deref"]; pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"]; pub const DISPLAY_TRAIT: [&str; 3] = ["core", "fmt", "Display"]; diff --git a/tests/ui/dereference.fixed b/tests/ui/dereference.fixed new file mode 100644 index 000000000000..292324eeacba --- /dev/null +++ b/tests/ui/dereference.fixed @@ -0,0 +1,79 @@ +// run-rustfix + +#![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)] +#![warn(clippy::explicit_deref_method)] + +use std::ops::{Deref, DerefMut}; + +fn concat(deref_str: &str) -> String { + format!("{}bar", deref_str) +} + +fn just_return(deref_str: &str) -> &str { + deref_str +} + +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); + + // following should not require linting + + let b = just_return(a).deref(); + + let b: String = concat(just_return(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.deref().deref(); + + let b: &str = &*a; + + let b: &mut str = &mut *a; + + macro_rules! expr_deref { + ($body:expr) => { + $body.deref() + }; + } + let b: &str = expr_deref!(a); + + let opt_a = Some(a); + let b = opt_a.unwrap().deref(); + + // 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 index 5de201fb22f0..25e1c29e7fa5 100644 --- a/tests/ui/dereference.rs +++ b/tests/ui/dereference.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)] #![warn(clippy::explicit_deref_method)] @@ -59,4 +61,19 @@ fn main() { let opt_a = Some(a); let b = opt_a.unwrap().deref(); + + // 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 index 7e10add40b18..97fab3a34e01 100644 --- a/tests/ui/dereference.stderr +++ b/tests/ui/dereference.stderr @@ -1,5 +1,5 @@ error: explicit deref method call - --> $DIR/dereference.rs:19:19 + --> $DIR/dereference.rs:21:19 | LL | let b: &str = a.deref(); | ^^^^^^^^^ help: try this: `&*a` @@ -7,37 +7,37 @@ LL | let b: &str = a.deref(); = note: `-D clippy::explicit-deref-method` implied by `-D warnings` error: explicit deref_mut method call - --> $DIR/dereference.rs:21:23 + --> $DIR/dereference.rs:23:23 | LL | let b: &mut str = a.deref_mut(); | ^^^^^^^^^^^^^ help: try this: `&mut *a` error: explicit deref method call - --> $DIR/dereference.rs:24:39 + --> $DIR/dereference.rs:26:39 | LL | let b: String = format!("{}, {}", a.deref(), a.deref()); | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:24:50 + --> $DIR/dereference.rs:26:50 | LL | let b: String = format!("{}, {}", a.deref(), a.deref()); | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:26:20 + --> $DIR/dereference.rs:28:20 | LL | println!("{}", a.deref()); | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:29:11 + --> $DIR/dereference.rs:31:11 | LL | match a.deref() { | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:33:28 + --> $DIR/dereference.rs:35:28 | LL | let b: String = concat(a.deref()); | ^^^^^^^^^ help: try this: `&*a` From 72b9ae2a10b13a209f09c5ace0a7b9763b740e62 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sat, 7 Mar 2020 15:33:27 +0100 Subject: [PATCH 5/6] Use only check_expr with parent expr and precedence --- CHANGELOG.md | 2 +- clippy_lints/src/dereference.rs | 103 +++++++++----------------------- clippy_lints/src/lib.rs | 4 +- clippy_lints/src/utils/paths.rs | 2 - src/lintlist/mod.rs | 2 +- tests/ui/dereference.fixed | 20 ++++--- tests/ui/dereference.rs | 18 +++--- tests/ui/dereference.stderr | 28 ++++++++- 8 files changed, 79 insertions(+), 100 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb5a6af97d36..2c4bb42566d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1256,7 +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_method`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_deref_method +[`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 index d11614d489d2..f5d82c549163 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1,10 +1,8 @@ -use crate::utils::{ - get_parent_expr, get_trait_def_id, implements_trait, method_calls, paths, snippet, span_lint_and_sugg, -}; +use crate::utils::{get_parent_expr, implements_trait, snippet, span_lint_and_sugg}; use if_chain::if_chain; +use rustc_ast::util::parser::ExprPrecedence; use rustc_errors::Applicability; -use rustc_hir as hir; -use rustc_hir::{Expr, ExprKind, QPath, StmtKind}; +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; @@ -31,100 +29,52 @@ declare_clippy_lint! { /// ```rust,ignore /// let _ = d.unwrap().deref(); /// ``` - pub EXPLICIT_DEREF_METHOD, + 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_METHOD + EXPLICIT_DEREF_METHODS ]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { - fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx hir::Stmt<'_>) { - if_chain! { - if let StmtKind::Local(ref local) = stmt.kind; - if let Some(ref init) = local.init; - - then { - match init.kind { - ExprKind::Call(ref _method, args) => { - for arg in args { - if_chain! { - // Caller must call only one other function (deref or deref_mut) - // otherwise it can lead to error prone suggestions (ie: &*a.len()) - let (method_names, arg_list, _) = method_calls(arg, 2); - if method_names.len() == 1; - // Caller must be a variable - let variables = arg_list[0]; - if variables.len() == 1; - if let ExprKind::Path(QPath::Resolved(None, _)) = variables[0].kind; - - then { - let name = method_names[0].as_str(); - lint_deref(cx, &*name, &variables[0], variables[0].span, arg.span); - } - } - } - } - ExprKind::MethodCall(ref method_name, _, ref args) => { - if init.span.from_expansion() { - return; - } - if_chain! { - if args.len() == 1; - if let ExprKind::Path(QPath::Resolved(None, _)) = args[0].kind; - // Caller must call only one other function (deref or deref_mut) - // otherwise it can lead to error prone suggestions (ie: &*a.len()) - let (method_names, arg_list, _) = method_calls(init, 2); - if method_names.len() == 1; - // Caller must be a variable - let variables = arg_list[0]; - if variables.len() == 1; - if let ExprKind::Path(QPath::Resolved(None, _)) = variables[0].kind; - - then { - let name = method_name.ident.as_str(); - lint_deref(cx, &*name, init, args[0].span, init.span); - } - } - } - _ => () - } - } - } - } - 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; - if let ExprKind::Path(QPath::Resolved(None, _)) = args[0].kind; - if let Some(parent) = get_parent_expr(cx, &expr); then { - match parent.kind { - // Already linted using statements - ExprKind::Call(_, _) | ExprKind::MethodCall(_, _, _) => (), - _ => { - let name = method_name.ident.as_str(); - lint_deref(cx, &*name, &args[0], args[0].span, expr.span); + 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 unary precedence + if let ExprPrecedence::Unary = parent_expr.precedence() { + return; } } + let name = method_name.ident.as_str(); + lint_deref(cx, &*name, &args[0], args[0].span, expr.span); } } } } -fn lint_deref(cx: &LateContext<'_, '_>, fn_name: &str, call_expr: &Expr<'_>, var_span: Span, expr_span: Span) { - match fn_name { +fn lint_deref(cx: &LateContext<'_, '_>, method_name: &str, call_expr: &Expr<'_>, var_span: Span, expr_span: Span) { + match method_name { "deref" => { - if get_trait_def_id(cx, &paths::DEREF_TRAIT) + 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_METHOD, + EXPLICIT_DEREF_METHODS, expr_span, "explicit deref method call", "try this", @@ -134,12 +84,15 @@ fn lint_deref(cx: &LateContext<'_, '_>, fn_name: &str, call_expr: &Expr<'_>, var } }, "deref_mut" => { - if get_trait_def_id(cx, &paths::DEREF_MUT_TRAIT) + 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_METHOD, + EXPLICIT_DEREF_METHODS, expr_span, "explicit deref_mut method call", "try this", diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6443caa89d62..b6b6a0e7d21f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -513,7 +513,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_METHOD, + &dereference::EXPLICIT_DEREF_METHODS, &derive::DERIVE_HASH_XOR_EQ, &derive::EXPL_IMPL_CLONE_ON_COPY, &doc::DOC_MARKDOWN, @@ -1091,7 +1091,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_METHOD), + 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/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index a49299e5f6fb..d93f8a1e5609 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -25,9 +25,7 @@ pub const CSTRING: [&str; 4] = ["std", "ffi", "c_str", "CString"]; pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"]; pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"]; pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"]; -pub const DEREF_MUT_TRAIT: [&str; 4] = ["core", "ops", "deref", "DerefMut"]; pub const DEREF_MUT_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "DerefMut", "deref_mut"]; -pub const DEREF_TRAIT: [&str; 4] = ["core", "ops", "deref", "Deref"]; pub const DEREF_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "Deref", "deref"]; pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"]; pub const DISPLAY_TRAIT: [&str; 3] = ["core", "fmt", "Display"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 3f1d31f0302c..1d147e01066f 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -529,7 +529,7 @@ pub static ref ALL_LINTS: Vec = vec![ module: "loops", }, Lint { - name: "explicit_deref_method", + name: "explicit_deref_methods", group: "pedantic", desc: "Explicit use of deref or deref_mut method while not in a method chain.", deprecation: None, diff --git a/tests/ui/dereference.fixed b/tests/ui/dereference.fixed index 292324eeacba..51e3d512cdba 100644 --- a/tests/ui/dereference.fixed +++ b/tests/ui/dereference.fixed @@ -1,7 +1,7 @@ // run-rustfix #![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)] -#![warn(clippy::explicit_deref_method)] +#![warn(clippy::explicit_deref_methods)] use std::ops::{Deref, DerefMut}; @@ -34,11 +34,18 @@ fn main() { let b: String = concat(&*a); - // following should not require linting + let b = &*just_return(a); + + let b: String = concat(&*just_return(a)); - let b = just_return(a).deref(); + let b: &str = &*a.deref(); - let b: String = concat(just_return(a).deref()); + let opt_a = Some(a.clone()); + let b = &*opt_a.unwrap(); + + // following should not require linting + + let b: &str = &*a.deref(); let b: String = a.deref().clone(); @@ -46,8 +53,6 @@ fn main() { let b: &usize = &a.deref().len(); - let b: &str = a.deref().deref(); - let b: &str = &*a; let b: &mut str = &mut *a; @@ -59,9 +64,6 @@ fn main() { } let b: &str = expr_deref!(a); - let opt_a = Some(a); - let b = opt_a.unwrap().deref(); - // The struct does not implement Deref trait #[derive(Copy, Clone)] struct NoLint(u32); diff --git a/tests/ui/dereference.rs b/tests/ui/dereference.rs index 25e1c29e7fa5..3a5953374112 100644 --- a/tests/ui/dereference.rs +++ b/tests/ui/dereference.rs @@ -1,7 +1,7 @@ // run-rustfix #![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)] -#![warn(clippy::explicit_deref_method)] +#![warn(clippy::explicit_deref_methods)] use std::ops::{Deref, DerefMut}; @@ -34,20 +34,25 @@ fn main() { let b: String = concat(a.deref()); - // following should not require linting - 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 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.deref().deref(); - let b: &str = &*a; let b: &mut str = &mut *a; @@ -59,9 +64,6 @@ fn main() { } let b: &str = expr_deref!(a); - let opt_a = Some(a); - let b = opt_a.unwrap().deref(); - // The struct does not implement Deref trait #[derive(Copy, Clone)] struct NoLint(u32); diff --git a/tests/ui/dereference.stderr b/tests/ui/dereference.stderr index 97fab3a34e01..d159214db2ff 100644 --- a/tests/ui/dereference.stderr +++ b/tests/ui/dereference.stderr @@ -4,7 +4,7 @@ error: explicit deref method call LL | let b: &str = a.deref(); | ^^^^^^^^^ help: try this: `&*a` | - = note: `-D clippy::explicit-deref-method` implied by `-D warnings` + = note: `-D clippy::explicit-deref-methods` implied by `-D warnings` error: explicit deref_mut method call --> $DIR/dereference.rs:23:23 @@ -42,5 +42,29 @@ error: explicit deref method call LL | let b: String = concat(a.deref()); | ^^^^^^^^^ help: try this: `&*a` -error: aborting due to 7 previous errors +error: explicit deref method call + --> $DIR/dereference.rs:37:13 + | +LL | let b = just_return(a).deref(); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*just_return(a)` + +error: explicit deref method call + --> $DIR/dereference.rs:39:28 + | +LL | let b: String = concat(just_return(a).deref()); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*just_return(a)` + +error: explicit deref method call + --> $DIR/dereference.rs:41:19 + | +LL | let b: &str = a.deref().deref(); + | ^^^^^^^^^^^^^^^^^ help: try this: `&*a.deref()` + +error: explicit deref method call + --> $DIR/dereference.rs:44:13 + | +LL | let b = opt_a.unwrap().deref(); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*opt_a.unwrap()` + +error: aborting due to 11 previous errors From 3c2bbcf00e3b97ab1fb03665b4afc7b7df0cd25e Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sat, 21 Mar 2020 19:34:56 +0100 Subject: [PATCH 6/6] Better precedence case management + more tests --- clippy_lints/src/dereference.rs | 15 +++++++++++---- clippy_lints/src/lib.rs | 1 + tests/ui/dereference.fixed | 12 ++++++++++++ tests/ui/dereference.rs | 12 ++++++++++++ tests/ui/dereference.stderr | 22 +++++++++++----------- 5 files changed, 47 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index f5d82c549163..68ec07e2bcb0 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1,6 +1,6 @@ use crate::utils::{get_parent_expr, implements_trait, snippet, span_lint_and_sugg}; use if_chain::if_chain; -use rustc_ast::util::parser::ExprPrecedence; +use rustc_ast::util::parser::{ExprPrecedence, PREC_POSTFIX, PREC_PREFIX}; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; @@ -51,9 +51,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing { if let ExprKind::MethodCall(..) = parent_expr.kind { return; } - // Check for unary precedence - if let ExprPrecedence::Unary = parent_expr.precedence() { - 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(); diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b6b6a0e7d21f..de1bab3a0b94 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; diff --git a/tests/ui/dereference.fixed b/tests/ui/dereference.fixed index 51e3d512cdba..459ca91b93b9 100644 --- a/tests/ui/dereference.fixed +++ b/tests/ui/dereference.fixed @@ -13,6 +13,15 @@ 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"); @@ -45,6 +54,9 @@ fn main() { // 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(); diff --git a/tests/ui/dereference.rs b/tests/ui/dereference.rs index 3a5953374112..8dc5272e67fa 100644 --- a/tests/ui/dereference.rs +++ b/tests/ui/dereference.rs @@ -13,6 +13,15 @@ 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"); @@ -45,6 +54,9 @@ fn main() { // 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(); diff --git a/tests/ui/dereference.stderr b/tests/ui/dereference.stderr index d159214db2ff..d26b462a4336 100644 --- a/tests/ui/dereference.stderr +++ b/tests/ui/dereference.stderr @@ -1,5 +1,5 @@ error: explicit deref method call - --> $DIR/dereference.rs:21:19 + --> $DIR/dereference.rs:30:19 | LL | let b: &str = a.deref(); | ^^^^^^^^^ help: try this: `&*a` @@ -7,61 +7,61 @@ LL | let b: &str = a.deref(); = note: `-D clippy::explicit-deref-methods` implied by `-D warnings` error: explicit deref_mut method call - --> $DIR/dereference.rs:23:23 + --> $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:26:39 + --> $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:26:50 + --> $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:28:20 + --> $DIR/dereference.rs:37:20 | LL | println!("{}", a.deref()); | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:31:11 + --> $DIR/dereference.rs:40:11 | LL | match a.deref() { | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:35:28 + --> $DIR/dereference.rs:44:28 | LL | let b: String = concat(a.deref()); | ^^^^^^^^^ help: try this: `&*a` error: explicit deref method call - --> $DIR/dereference.rs:37:13 + --> $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:39:28 + --> $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:41:19 + --> $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:44:13 + --> $DIR/dereference.rs:53:13 | LL | let b = opt_a.unwrap().deref(); | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*opt_a.unwrap()`