-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add rebind_fn_arg_as_mut
lint
#6245
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
use crate::utils::{snippet, span_lint_and_then}; | ||
use if_chain::if_chain; | ||
use rustc_errors::Applicability; | ||
use rustc_hir::{ | ||
BindingAnnotation, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, Local, Node, PatKind, QPath, TraitFn, | ||
TraitItem, TraitItemKind, | ||
}; | ||
use rustc_lint::{LateContext, LateLintPass}; | ||
use rustc_middle::lint::in_external_macro; | ||
use rustc_session::{declare_lint_pass, declare_tool_lint}; | ||
|
||
declare_clippy_lint! { | ||
/// **What it does:** Checks for function arguments declared as not mutable and | ||
/// later rebound as mutable. | ||
/// | ||
/// **Why is this bad?** It can be easily improved by just declaring the function | ||
/// argument as mutable and removing the unnecessary assignment. | ||
/// | ||
/// **Known problems:** The function argument might have been shadowed by another | ||
/// value before the assignment. | ||
/// | ||
/// **Example:** | ||
/// | ||
/// ```rust | ||
/// fn foo_bad(bar: Vec<i32>) -> Vec<i32> { | ||
/// let mut bar = bar; | ||
/// bar.push(42); | ||
/// bar | ||
/// } | ||
/// ``` | ||
/// Use instead: | ||
/// ```rust | ||
/// fn foo(mut bar: Vec<i32>) -> Vec<i32> { | ||
/// bar.push(42); | ||
/// bar | ||
/// } | ||
/// ``` | ||
pub REBIND_FN_ARG_AS_MUT, | ||
style, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ebroto what do you think of the lint level for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be a
If we want to leave these cases for a follow-up, I would add the lint to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I can address your first suggestion this weekend. As to the second, if by fixing you mean generating an applicable suggestion, I only didn't do it because the output looked awful: a suggestion to remove an entire line just prints out a blank that might confuse the user if they don't pay attention to the line number. If it's possible to make it better than that, let me know! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for not being precise. I meant to avoid the false positive when the parameter has been shadowed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, right. But actually, I think that is fixed by my new impl. I'm comparing the resolution id for the variable (path) with the HIR id of the parameter's pattern. The thing is, I'm starting to think we should expand this lint to cover not only function parameters but any binding. Is there some case in which There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that is already covered by the shadow_same lint. |
||
"non-mutable function argument rebound as mutable" | ||
} | ||
|
||
declare_lint_pass!(RebindFnArgAsMut => [REBIND_FN_ARG_AS_MUT]); | ||
|
||
impl LateLintPass<'_> for RebindFnArgAsMut { | ||
fn check_local(&mut self, cx: &LateContext<'tcx>, local: &Local<'tcx>) { | ||
if_chain! { | ||
if !in_external_macro(cx.tcx.sess, local.span); | ||
|
||
// LHS | ||
if let PatKind::Binding(BindingAnnotation::Mutable, _, name, None) = local.pat.kind; | ||
|
||
// RHS | ||
if let Some(init) = local.init; | ||
if let ExprKind::Path(QPath::Resolved(_, path)) = init.kind; | ||
if path.segments.len() == 1; | ||
if path.segments[0].ident == name; | ||
|
||
if let rustc_hir::def::Res::Local(id) = path.res; | ||
|
||
// Fn | ||
let parent_id = cx.tcx.hir().get_parent_item(id); | ||
|
||
if let Node::Item(&Item { kind: ItemKind::Fn(_, _, body_id), .. }) | ||
| Node::ImplItem(&ImplItem { kind: ImplItemKind::Fn(_, body_id), .. }) | ||
| Node::TraitItem(&TraitItem { kind: TraitItemKind::Fn(_, TraitFn::Provided(body_id)), .. }) | ||
= cx.tcx.hir().get(parent_id); | ||
|
||
let body = cx.tcx.hir().body(body_id); | ||
if let Some(param) = body.params.iter().find(|param| param.pat.hir_id == id); | ||
if let PatKind::Binding(BindingAnnotation::Unannotated, ..) = param.pat.kind; | ||
|
||
then { | ||
span_lint_and_then( | ||
cx, | ||
REBIND_FN_ARG_AS_MUT, | ||
param.pat.span, | ||
&format!("argument `{}` is declared as not mutable, and later rebound as mutable", name), | ||
|diag| { | ||
diag.span_suggestion( | ||
param.pat.span, | ||
"consider just declaring as mutable", | ||
format!("mut {}", snippet(cx, param.pat.span, "_")), | ||
Applicability::MaybeIncorrect, | ||
); | ||
diag.span_help(local.span, "and remove this"); | ||
}, | ||
); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
// edition:2018 | ||
|
||
#![warn(clippy::rebind_fn_arg_as_mut)] | ||
|
||
fn f(x: bool) { | ||
let mut x = x; | ||
} | ||
|
||
trait T { | ||
fn tm1(x: bool) { | ||
let mut x = x; | ||
} | ||
fn tm2(x: bool); | ||
} | ||
|
||
struct S; | ||
|
||
impl S { | ||
fn m(x: bool) { | ||
let mut x = x; | ||
} | ||
} | ||
|
||
impl T for S { | ||
fn tm2(x: bool) { | ||
let mut x = x; | ||
} | ||
} | ||
|
||
fn f_no_lint(mut x: bool) { | ||
let mut x = x; | ||
} | ||
|
||
async fn expansion<T>(_: T) {} | ||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
error: argument `x` is declared as not mutable, and later rebound as mutable | ||
--> $DIR/rebind_fn_arg_as_mut.rs:5:6 | ||
| | ||
LL | fn f(x: bool) { | ||
| ^ help: consider just declaring as mutable: `mut x` | ||
| | ||
= note: `-D clippy::rebind-fn-arg-as-mut` implied by `-D warnings` | ||
help: and remove this | ||
--> $DIR/rebind_fn_arg_as_mut.rs:6:5 | ||
| | ||
LL | let mut x = x; | ||
| ^^^^^^^^^^^^^^ | ||
|
||
error: argument `x` is declared as not mutable, and later rebound as mutable | ||
--> $DIR/rebind_fn_arg_as_mut.rs:10:12 | ||
| | ||
LL | fn tm1(x: bool) { | ||
| ^ help: consider just declaring as mutable: `mut x` | ||
| | ||
help: and remove this | ||
--> $DIR/rebind_fn_arg_as_mut.rs:11:9 | ||
| | ||
LL | let mut x = x; | ||
| ^^^^^^^^^^^^^^ | ||
|
||
error: argument `x` is declared as not mutable, and later rebound as mutable | ||
--> $DIR/rebind_fn_arg_as_mut.rs:19:10 | ||
| | ||
LL | fn m(x: bool) { | ||
| ^ help: consider just declaring as mutable: `mut x` | ||
| | ||
help: and remove this | ||
--> $DIR/rebind_fn_arg_as_mut.rs:20:9 | ||
| | ||
LL | let mut x = x; | ||
| ^^^^^^^^^^^^^^ | ||
|
||
error: argument `x` is declared as not mutable, and later rebound as mutable | ||
--> $DIR/rebind_fn_arg_as_mut.rs:25:12 | ||
| | ||
LL | fn tm2(x: bool) { | ||
| ^ help: consider just declaring as mutable: `mut x` | ||
| | ||
help: and remove this | ||
--> $DIR/rebind_fn_arg_as_mut.rs:26:9 | ||
| | ||
LL | let mut x = x; | ||
| ^^^^^^^^^^^^^^ | ||
|
||
error: aborting due to 4 previous errors | ||
|
Uh oh!
There was an error while loading. Please reload this page.