Skip to content

Commit

Permalink
Check for Deref trait impl + add fixed version
Browse files Browse the repository at this point in the history
  • Loading branch information
ThibsG committed Feb 29, 2020
1 parent d40bb0c commit c4ac113
Showing 5 changed files with 149 additions and 40 deletions.
77 changes: 44 additions & 33 deletions clippy_lints/src/dereference.rs
Original file line number Diff line number Diff line change
@@ -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,46 +99,54 @@ 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);
}
}
}
}
}
}

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,
);
}
},
_ => (),
}
2 changes: 2 additions & 0 deletions clippy_lints/src/utils/paths.rs
Original file line number Diff line number Diff line change
@@ -21,7 +21,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 DOUBLE_ENDED_ITERATOR: [&str; 4] = ["core", "iter", "traits", "DoubleEndedIterator"];
79 changes: 79 additions & 0 deletions tests/ui/dereference.fixed
Original file line number Diff line number Diff line change
@@ -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();
}
17 changes: 17 additions & 0 deletions tests/ui/dereference.rs
Original file line number Diff line number Diff line change
@@ -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();
}
14 changes: 7 additions & 7 deletions tests/ui/dereference.stderr
Original file line number Diff line number Diff line change
@@ -1,43 +1,43 @@
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`
|
= 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`

0 comments on commit c4ac113

Please sign in to comment.