FP branches_sharing_code: scoped mutability ?

[matthiaskrgr]

I have code that looks somewhat like this:

use std::path::Path;
pub fn main() {
    let dir = std::path::PathBuf::new(); // there was a different function here to get the path root but this will do
    let path_debug = if cfg!(windows) {
        let mut td = dir;
        td.push("debug");
        td.push("cargo-cache.exe");
        td
    } else {
        let mut td = dir;
        td.push("debug");
        td.push("cargo-cache");
        td
    };
    // ... use path_debug somehere...
}

i wrote it that way to reduce the mutability to the smallest scope:
There is no need for the variable path_debug to be mutable for its entire lifetime if I only need mutability for its construction.

The lint triggers here:

warning: all if blocks contain the same code at the start and the end. Here at the start
  --> src/test_helpers.rs:37:5
   |
37 | /     let path_debug = if cfg!(windows) {
38 | |         let mut td = target_dir;
39 | |         td.push("debug");
   | |_________________________^
   |
note: and here at the end
  --> src/test_helpers.rs:46:5
   |
46 | /         td
47 | |     };
   | |_____^
   = note: The end suggestion probably needs some adjustments to use the expression result correctly
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code
help: consider moving the start statements out like this
   |
37 |     let mut td = target_dir;
38 |     td.push("debug");
39 |     let path_debug = if cfg!(windows) {
   |
help: and consider moving the end statements out like this
   |
46 |     }
47 |     td;
   |

So the lint wants me to move

        let mut td = target_dir;
        td.push("debug");

out of the blocks which would expand the mutability of the var to its entire lifetime as far as I see.

@xFrednet do you think it makes sense to lint in such a case, or should the lint bail out somehow if it somehow detects that we have different mutabilities inside and outside of the scope (can clippy know that?)

One way to work around would be to do something like

let mut td = ...
td.push(...)
if ... {
   td.push(a)
} else {
   td.push(b)
}
let td = td;  // remove mutability

but the td = td is kinda ugly imo. :/

[xFrednet]

I considered such cases and decided that I would include them. The lint message even includes a message saying that this is not 100% equivalent:

= note: The end suggestion probably needs some adjustments to use the expression result correctly

I've seen the usage of let td = td; in some cases. (I usually proceed to just use the muted value though.) The lint will also add a second note if another value with the symbol td is in the outer scope.

We could consider adding a configuration for not moving values out 🤔

[xFrednet]

There has been two other bugs in connection of this lint. I'll add this to the known problems section until a configuration value is added or - we agreed on something else :)

[camsteffen]

I think it should still lint. You could write it like this.

let path_debug = {
    let mut td = dir;
    td.push("debug");
	if cfg!(windows) {
        td.push("cargo-cache.exe");
    } else {
        td.push("cargo-cache");
    };
	td
};

Not sure if it makes sense to actually suggest this though.