[WIP] - new lint: using let mut a = a at start of function
#5138
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,96 @@ | ||||||||
| use rustc_lint::{EarlyLintPass, EarlyContext}; | ||||||||
| use rustc_session::{declare_lint_pass, declare_tool_lint}; | ||||||||
| use syntax::ast::*; | ||||||||
| use syntax::visit::FnKind; | ||||||||
| use rustc_span::Span; | ||||||||
| use rustc_errors::DiagnosticBuilder; | ||||||||
| use crate::utils::{span_lint_and_then, multispan_sugg}; | ||||||||
| use if_chain::if_chain; | ||||||||
|
|
||||||||
| declare_clippy_lint! { | ||||||||
| /// **What it does:** checks if any fn parameters have been assigned to a local mutable | ||||||||
| /// variable. | ||||||||
| /// | ||||||||
| /// **Why is this bad?** reduces the complexity of the code by removing a redundant local | ||||||||
| /// mutable variable. | ||||||||
| /// | ||||||||
| /// **Known problems:** None. | ||||||||
| /// | ||||||||
| /// **Example:** | ||||||||
| /// | ||||||||
| /// ```rust | ||||||||
| /// fn f(a: Vec<bool>) { | ||||||||
| /// let mut a = a; | ||||||||
| /// // rest of code | ||||||||
| /// } | ||||||||
| /// ``` | ||||||||
| /// | ||||||||
| /// could be defined as | ||||||||
| /// | ||||||||
| /// ```rust | ||||||||
| /// fn f(mut a: Vec<bool>) { | ||||||||
| /// // rest of code | ||||||||
| /// } | ||||||||
| /// ``` | ||||||||
| pub FN_PARAM_REDEF_AS_MUTABLE, | ||||||||
|
There was a problem hiding this comment. bikeshedding: I think we usually call them variable "bindings" rather than "definitions". So, this can be named There was a problem hiding this comment. Sweet, I'll rename it cause that sounds a lot better. |
||||||||
| complexity, | ||||||||
| "local variables that can be eliminated by updating fn params mutability" | ||||||||
| } | ||||||||
|
|
||||||||
| declare_lint_pass!(FnParamRedefAsMutable => [FN_PARAM_REDEF_AS_MUTABLE]); | ||||||||
|
|
||||||||
| impl EarlyLintPass for FnParamRedefAsMutable { | ||||||||
| fn check_fn(&mut self, cx: &EarlyContext<'_>, fn_kind: FnKind<'_>, fn_decl: &FnDecl, span: Span, _: NodeId) { | ||||||||
| if let FnKind::ItemFn(_, _, _, block) | FnKind::Method(_, _, _, block) = fn_kind { | ||||||||
| for stmt in &block.stmts { | ||||||||
| check_statement(cx, fn_decl, span, stmt); | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| fn check_statement(cx: &EarlyContext<'_>, fn_decl: &FnDecl, fn_span: Span, stmt: &Stmt) { | ||||||||
| if_chain! { | ||||||||
| // Check to see if the local variable is defined as mutable | ||||||||
| if let StmtKind::Local(ref local) = stmt.kind; | ||||||||
| if let PatKind::Ident(mode, ..) = local.pat.kind; | ||||||||
| if let BindingMode::ByValue(mutability) = mode; | ||||||||
| if let Mutability::Mut = mutability; | ||||||||
|
Comment on lines
+57
to
+58
There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. Sweet, learning more Rust as I go! |
||||||||
|
|
||||||||
| if let Some(ref expr) = local.init; | ||||||||
| if let ExprKind::Path(_, ref path) = expr.kind; | ||||||||
| if let Some(ref segment) = path.segments.last(); | ||||||||
| if let name = segment.ident.name; | ||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||
|
|
||||||||
| // The path to fn parameters is 1 in length. | ||||||||
| if path.segments.len() == 1; | ||||||||
| then { | ||||||||
| for param in &fn_decl.inputs { | ||||||||
| if_chain! { | ||||||||
| if let PatKind::Ident(param_mode, ident, ..) = param.pat.kind; | ||||||||
| // Make sure they have the same name & it's not mutable | ||||||||
| if ident.name == name; | ||||||||
| if let BindingMode::ByValue(param_mut) = param_mode; | ||||||||
| if let Mutability::Not = param_mut; | ||||||||
|
Comment on lines
+73
to
+74
There was a problem hiding this comment.
Suggested change
|
||||||||
|
|
||||||||
| then { | ||||||||
| let sugg = |db: &mut DiagnosticBuilder<'_>| { | ||||||||
| db.span_help(param.span, "consider making this param `mut`"); | ||||||||
| db.span_help(stmt.span, "consider removing this local variable"); | ||||||||
|
|
||||||||
| multispan_sugg(db, "...".to_string(), vec![]); | ||||||||
|
There was a problem hiding this comment. I'm going to remove this, initially I was going to use this to do what line 78~79 was doing. |
||||||||
| }; | ||||||||
|
|
||||||||
| span_lint_and_then( | ||||||||
| cx, | ||||||||
| FN_PARAM_REDEF_AS_MUTABLE, | ||||||||
| fn_span, | ||||||||
| "a parameter was redefined as mutable, can be removed", | ||||||||
|
There was a problem hiding this comment. NIT:
Suggested change
|
||||||||
| sugg, | ||||||||
| ); | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| #![warn(clippy::fn_param_redef_as_mutable)] | ||
|
|
||
| fn foobar(a: Vec<bool>) { | ||
|
There was a problem hiding this comment. We'll need more tests:
fn f(x: usize) {
let y = x * 3;
let x = 42;
for i in 0..x {
println!("{}", i);
}
let mut z = x; // Should not get linted
z /= 2;
println!("x={}, y={}, z={}", x, y, z);
}There was a problem hiding this comment. I noticed that you used arg instead of param, is there a preference to the wording here that I should use in the change? Also just one thing that I think would be good for discussion; What's the intention behind not catching a rebind if they're not the same name of the arg? There was a problem hiding this comment.
Oh, I should have specified, what of the above test cases should get linted and what not. I added it in my comment above There was a problem hiding this comment.
arg is shorter to write 😄 I don't know, what they are called in rust, honestly. I think, I heard both. There was a problem hiding this comment. Looking at the rustc doc, they are called param: https://doc.rust-lang.org/nightly/nightly-rustc/syntax/ast/struct.Param.html There was a problem hiding this comment. I've heard arguments are the values that get passed to a function while parameters are the formal variables in the type signature. So in |
||
| let mut c = a; | ||
| } | ||
|
|
||
| fn main() { | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| error: a parameter was redefined as mutable, can be removed | ||
| --> $DIR/fn_param_redef_as_mutable.rs:3:1 | ||
| | | ||
| LL | / fn foobar(a: Vec<bool>) { | ||
| LL | | let mut c = a; | ||
| LL | | } | ||
| | |_^ | ||
| | | ||
| = note: `-D clippy::fn-param-redef-as-mutable` implied by `-D warnings` | ||
| help: consider making this param `mut` | ||
| --> $DIR/fn_param_redef_as_mutable.rs:3:11 | ||
| | | ||
| LL | fn foobar(a: Vec<bool>) { | ||
| | ^^^^^^^^^^^^^ | ||
| help: consider removing this local variable | ||
| --> $DIR/fn_param_redef_as_mutable.rs:4:5 | ||
| | | ||
| LL | let mut c = a; | ||
| | ^^^^^^^^^^^^^^ | ||
|
|
||
| error: aborting due to previous error | ||
|
|
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.
Making this mistake is bad because you add unnecessary complexity, not reduce it ;)
Maybe something like