From d524ef1f42f50caf72f5d4bbc358507b40fa8948 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sat, 7 Mar 2020 15:33:27 +0100 Subject: [PATCH] Use only check_expr with parent expr and precedence --- CHANGELOG.md | 2 +- README.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 | 4 +- tests/ui/dereference.fixed | 20 ++++--- tests/ui/dereference.rs | 18 +++--- tests/ui/dereference.stderr | 28 ++++++++- 9 files changed, 81 insertions(+), 102 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a83a71fb54b..07daeaf1f9cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1227,7 +1227,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/README.md b/README.md index 2181c296e9b2..7d6fcbc90986 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 361 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 362 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: 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 937a5db9a24d..3e816ab57ce4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -506,7 +506,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, @@ -1072,7 +1072,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 923ac04bf80e..6cb1f694fd5e 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -21,9 +21,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 257f9b0d60bb..52c7f0924125 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 361] = [ +pub const ALL_LINTS: [Lint; 362] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -526,7 +526,7 @@ pub const ALL_LINTS: [Lint; 361] = [ 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