-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(lint): deref and deref_mut method explicit calls #3258
Closed
+186
−1
Closed
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
858a902
fix build failure
tommilligan c512509
Working basic dereference clip
tommilligan f62ca84
Added additional tests and comments for deref
tommilligan 656b75d
Renamed to explicit_deref_method
tommilligan 2a0fb02
Move explicit_deref_method from complexity to pedantic
tommilligan a6a7593
Revert "fix build failure"
tommilligan 66447a5
ran update_lints
tommilligan b241010
Added failing test for deref in macro
tommilligan 1e52f80
Ignore when part of macro expansion
tommilligan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
use crate::rustc::hir::{Expr, ExprKind, QPath}; | ||
use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; | ||
use crate::rustc::{declare_tool_lint, lint_array}; | ||
use if_chain::if_chain; | ||
use crate::utils::span_lint_and_sugg; | ||
|
||
/// **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.deref().unwrap(); | ||
/// let f = a.deref().unwrap(); | ||
/// ``` | ||
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_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), | ||
); | ||
}, | ||
_ => () | ||
}; | ||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
#![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; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does your lint exclude these cases? Since
_.deref().len()
gets linted, I have a feeling, that also_.unwrap()
will get linted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite correct - should be the other way round.
d.unwrap().deref()
would not be linted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/rust-lang-nursery/rust-clippy/pull/3258/files#diff-280822c8730d5eac6da4c4b86a9a0fd6R34
https://github.com/rust-lang-nursery/rust-clippy/pull/3258/files#diff-21b45ffb0b35b75693c82b44153ebd60R36
In a method chain,
deref
will only be linted if it is the first method in the chain.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I missed that, thanks!