[WIP] - new lint: using `let mut a = a` at start of function #5138

sonng · 2020-02-06T00:54:59Z

Added in the right checks
Added better help hints

changelog: none

jyn514 · 2020-02-06T01:01:56Z

This looks a little broader than I was imagining ... For fn f(a: usize), it lints any let mut a = ... no matter how deep it is in the function. I was imagining only if it occurs before non-declaration statements, or maybe only if it's in the same scope.

e.g. I would like this to be linted:

fn f(a: usize) {
    let b = 1;
    let mut a = a;
}

but not this:

fn f(a: usize) {
    let b = a;
    loop {
         let mut a = a;
         a += 5;
    }

jyn514 · 2020-02-06T01:02:22Z

But maybe this is better, I'm not sure.

sonng · 2020-02-06T02:19:18Z

My understanding was if we're defining a local mutable variable pointing to a function parameter, then that local mutable variable is not needed and instead the function parameter's mutability could instead be changed.

The loop example you provided wouldn't trigger this lint cause it doesn't check inside of the loop statement.

From what I've learnt over the past week, this line here loops through each statement in that function. The function has only two statements, the let b = a; is a StmtKind::Local where the loop is a StmtKind::Expr. The loop is overlooked in this function.

edit: grammar

sinkuu · 2020-02-06T02:45:21Z

clippy_lints/src/fn_param_redef_as_mutable.rs

+    ///     // rest of code
+    /// }
+    /// ```
+    pub FN_PARAM_REDEF_AS_MUTABLE,


bikeshedding: I think we usually call them variable "bindings" rather than "definitions". So, this can be named FN_PARAM_REBIND_AS_MUT.

Sweet, I'll rename it cause that sounds a lot better.

flip1995

error: a parameter was redefined as mutable, can be removed
   --> clippy_lints/src/literal_representation.rs:198:5
    |
198 | /     fn split_digit_parts(digits: &str, float: bool) -> (&str, Option<&str>, Option<(char, &str)>) {
199 | |         let mut integer = digits;
200 | |         let mut fraction = None;
201 | |         let mut exponent = None;
...   |
224 | |         (integer, fraction, exponent)
225 | |     }
    | |_____^
    |
help: consider making this param `mut`
   --> clippy_lints/src/literal_representation.rs:198:26
    |
198 |     fn split_digit_parts(digits: &str, float: bool) -> (&str, Option<&str>, Option<(char, &str)>) {
    |                          ^^^^^^^^^^^^^
help: consider removing this local variable
   --> clippy_lints/src/literal_representation.rs:199:9
    |
199 |         let mut integer = digits;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^

This is a bug. The digits arg is binded to integer, but then gets used afterwards. If we'd declare the arg as mut, we would modify the semantics of the function.

To test if the arg is used afterwards, you can use:

rust-clippy/clippy_lints/src/utils/usage.rs

Lines 79 to 82 in a7b3b9f

    
           pub struct UsedVisitor { 
        
               pub var: ast::Name, // var to look for 
        
               pub used: bool,     // has the var been used otherwise? 
        
           }

Don't worry about the dogfood tests now. We can fix them, once this lint is implemented and tested by ui tests completely

flip1995 · 2020-02-06T08:23:46Z

clippy_lints/src/fn_param_redef_as_mutable.rs

+        if let BindingMode::ByValue(mutability) = mode;
+        if let Mutability::Mut = mutability;


Suggested change

if let BindingMode::ByValue(mutability) = mode;

if let Mutability::Mut = mutability;

if let BindingMode::ByValue(Mutability::Mut) = mode;

Sweet, learning more Rust as I go!

flip1995 · 2020-02-06T08:25:50Z

clippy_lints/src/fn_param_redef_as_mutable.rs

+        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;


Suggested change

if let name = segment.ident.name;

let name = segment.ident.name;

flip1995 · 2020-02-06T08:27:10Z

clippy_lints/src/fn_param_redef_as_mutable.rs

+                    if let BindingMode::ByValue(param_mut) = param_mode;
+                    if let Mutability::Not = param_mut;


Suggested change

if let BindingMode::ByValue(param_mut) = param_mode;

if let Mutability::Not = param_mut;

if let BindingMode::ByValue(Mutability::Not) = param_mode;

flip1995 · 2020-02-06T08:27:58Z

clippy_lints/src/fn_param_redef_as_mutable.rs

+                            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![]);


What does this do?

I'm going to remove this, initially I was going to use this to do what line 78~79 was doing.

flip1995 · 2020-02-06T08:28:19Z

clippy_lints/src/fn_param_redef_as_mutable.rs

+                            cx,
+                            FN_PARAM_REDEF_AS_MUTABLE,
+                            fn_span,
+                            "a parameter was redefined as mutable, can be removed",


NIT:

Suggested change

"a parameter was redefined as mutable, can be removed",

"a parameter was redefined as mutable and can be removed",

flip1995 · 2020-02-06T08:30:05Z

tests/ui/fn_param_redef_as_mutable.rs

@@ -0,0 +1,8 @@
+#![warn(clippy::fn_param_redef_as_mutable)]
+
+fn foobar(a: Vec<bool>) {


We'll need more tests:

more than one statement (lint)

binding is not in the top level block (no lint)

arg is used after binding (no lint)

name of rebind is the same as name of arg (lint)

name of rebind is the same as name of arg and arg is used afterwards. (lint)

This test: (no lint)

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); }

Playground

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?

What's the intention behind not catching a rebind if they're not the same name of the arg?

Oh, I should have specified, what of the above test cases should get linted and what not. I added it in my comment above

I noticed that you used arg instead of param

arg is shorter to write 😄 I don't know, what they are called in rust, honestly. I think, I heard both.

Looking at the rustc doc, they are called param: https://doc.rust-lang.org/nightly/nightly-rustc/syntax/ast/struct.Param.html

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 fn f(a: usize), a is a parameter, but in Vec::from("b"), "b" is an argument.

bors · 2020-02-06T15:33:52Z

☔ The latest upstream changes (presumably #5125) made this pull request unmergeable. Please resolve the merge conflicts.

- Added in the right checks - Added better help hints

bors · 2020-02-21T09:58:46Z

☔ The latest upstream changes (presumably #5202) made this pull request unmergeable. Please resolve the merge conflicts.

ghost · 2020-03-12T14:21:40Z

clippy_lints/src/fn_param_redef_as_mutable.rs

+    /// **Why is this bad?** reduces the complexity of the code by removing a redundant local
+    /// mutable variable.


Making this mistake is bad because you add unnecessary complexity, not reduce it ;)

Maybe something like

Suggested change

/// **Why is this bad?** reduces the complexity of the code by removing a redundant local

/// mutable variable.

/// **Why is this bad?** Introduces a local variables that increases complexity unnecessarily.

flip1995 · 2020-05-25T15:37:51Z

Thanks for contributing to Clippy! Sadly this PR was not updated in quite some time. If you waited on input from a reviewer, we're sorry that this fell under the radar. If you want to continue to work on this, just reopen the PR and/or ping a reviewer.

spazm · 2021-10-19T07:06:50Z

src/lintlist/mod.rs

+    Lint {
+        name: "fn_param_redef_as_mutable",
+        group: "complexity",
+        desc: "default lint description",


Update with real description

sinkuu reviewed Feb 6, 2020

View reviewed changes

flip1995 requested changes Feb 6, 2020

View reviewed changes

flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Feb 6, 2020

Added checks for redefinition of fn parameters as mutable.

8ef21ba

- Added in the right checks - Added better help hints

sonng force-pushed the master branch from 8876fa9 to 8ef21ba Compare February 10, 2020 21:34

flip1995 added the A-lint Area: New lints label Feb 21, 2020

ghost reviewed Mar 12, 2020

View reviewed changes

flip1995 closed this May 25, 2020

flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 25, 2020

chansuke pushed a commit to chansuke/rust-clippy that referenced this pull request Jul 2, 2020

Keep working on PR rust-lang#5138

075d1a9

spazm reviewed Oct 19, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] - new lint: using `let mut a = a` at start of function #5138

[WIP] - new lint: using `let mut a = a` at start of function #5138

sonng commented Feb 6, 2020

jyn514 commented Feb 6, 2020

jyn514 commented Feb 6, 2020

sonng commented Feb 6, 2020 •

edited

Loading

sinkuu Feb 6, 2020

sonng Feb 6, 2020

flip1995 left a comment •

edited

Loading

flip1995 Feb 6, 2020

sonng Feb 6, 2020

flip1995 Feb 6, 2020

flip1995 Feb 6, 2020

flip1995 Feb 6, 2020

sonng Feb 6, 2020

flip1995 Feb 6, 2020

flip1995 Feb 6, 2020 •

edited

Loading

sonng Feb 6, 2020

flip1995 Feb 7, 2020

flip1995 Feb 7, 2020

flip1995 Feb 7, 2020

jyn514 Feb 7, 2020

bors commented Feb 6, 2020

bors commented Feb 21, 2020

ghost Mar 12, 2020

flip1995 commented May 25, 2020

spazm Oct 19, 2021

	pub struct UsedVisitor {
	pub var: ast::Name, // var to look for
	pub used: bool, // has the var been used otherwise?
	}

		if let BindingMode::ByValue(mutability) = mode;
		if let Mutability::Mut = mutability;

	if let BindingMode::ByValue(mutability) = mode;
	if let Mutability::Mut = mutability;
	if let BindingMode::ByValue(Mutability::Mut) = mode;

	if let name = segment.ident.name;
	let name = segment.ident.name;

		if let BindingMode::ByValue(param_mut) = param_mode;
		if let Mutability::Not = param_mut;

	"a parameter was redefined as mutable, can be removed",
	"a parameter was redefined as mutable and can be removed",

		@@ -0,0 +1,8 @@
		#![warn(clippy::fn_param_redef_as_mutable)]

		fn foobar(a: Vec<bool>) {

		/// Why is this bad? reduces the complexity of the code by removing a redundant local
		/// mutable variable.

	/// Why is this bad? reduces the complexity of the code by removing a redundant local
	/// mutable variable.
	/// Why is this bad? Introduces a local variables that increases complexity unnecessarily.

[WIP] - new lint: using let mut a = a at start of function #5138

[WIP] - new lint: using let mut a = a at start of function #5138

Conversation

sonng commented Feb 6, 2020

jyn514 commented Feb 6, 2020

jyn514 commented Feb 6, 2020

sonng commented Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 6, 2020

bors commented Feb 21, 2020

Choose a reason for hiding this comment

flip1995 commented May 25, 2020

Choose a reason for hiding this comment

[WIP] - new lint: using `let mut a = a` at start of function #5138

[WIP] - new lint: using `let mut a = a` at start of function #5138

sonng commented Feb 6, 2020 •

edited

Loading

flip1995 left a comment •

edited

Loading

flip1995 Feb 6, 2020 •

edited

Loading