Warn when a local is "mut" but clearly not mutated

[jruderman]

I think this should warn:

fn main()
{
    let mut x = 3;
    assert x == 3;
}

[bblum]

+1. i'm all for compiler warnings that make folks tighten down their code.

[bstrie]

Would it work similarly to the unused variable detection, where it warns unless you stick an underscore on the front of the identifier?

[zmanji]

I would like to work on this issue but I don't know where to start. Can anyone provide me with some tips?

[nikomatsakis]

The liveness pass already pretty much computes the information you need. It can be found in src/rustc/middle/liveness.rs, but you probably want to come on IRC and ask me (nmatsakis) questions about it.

[nikomatsakis]

I'm not sure how familiar you are with a traditional liveness analysis, but the basic idea is that it tracks when variables may be used again (in which case they are live). Ours is a bit different in that it also tracks when variables may be reassigned in the future. Therefore, it should be possible to check, at the point where a variable is declared, whether it will ever be reassigned.

The easiest thing would be to issue a warning for mutable variables which have initializers but are never reassigned (such as the example given in this bug). Such a warning would be issued in check_local(). There is currently a check that says "if there is an initializer and the variable is immutable, ensure it is never assigned again". You could repurpose this to something like "if there is an initializer, and the variable is never reassigned again, warn if it is not immutable" (but of course it should still report the error it does today)

You might conceivably also try to issue warnings for variables that are assigned only once in the flow of execution:

let mut foo;
if cond { foo = 1; } else { foo = 2; }
...

as such variables also do not require a mut label. But this is trickier, I'm not sure if we have enough information available to distinguish that case from one like;

let mut foo;
foo = 3;
...
if cond { foo = 1; } else { foo = 2; }
...

That is, each assignment can determine whether there are ever any later assignments and whether the variable is ever used later. But here it seems like we would also need to know whether there have been any prior assignments. So I'd say just warn if there is an initializer and ignore this second case.

[nikomatsakis]

Note: Be sure to add a "compile-fail" test case as described here: https://github.com/mozilla/rust/wiki/Note-testsuite

This one is a bit tricky as you'll be detecting a warning, and the test suite likes for such tests to fail, so you'll probably have to add a secondary error. You can check out the test src/test/liveness-unused.rs as an example.

[ttaubert]

It's unfortunately not as easy as just adding another check to check_local(). Variables might need to be mutable not only for re-assignment:

let tmps = ~[];
for vec::each(args) |arg| {
    tmps.push(copy *arg);
}

let s = ~"foobar";
let head = str::shift_char(&mut s);

These both result in error: illegal borrow: creating mutable alias to immutable local variable.

[huonw]

I have little idea of the infrastructure (I had a look at middle/liveness.rs, which didn't help me much), but how difficult would it be to add an used_mutably_count to the analysis that counts the maximum number of times a variable could possibly be assigned or used in a context that requires mut (accounting for loops)?

e.g.

let mut a;
a = 1;
// used_mutably_count(a) == Known(1)

let mut b = 2;
b = 1;
// used_mutably_count(b) == Known(2)

let mut c = ~[];
for vec::each(args) |arg| {
    c.push(copy *args); 
}
// used_mutably_count(c) == Unknown

Then if used_mutably_count(var) == Known(1), rustc can warn that var should be immutable (or not, if var is already immutable).

It could be computed with rules like

umc(var, if cond { blk1 } else { blk2 }) = max(umc(var, blk1), umc(var, blk2))
umc(var, { stmt1; stmt2; ... }) = umc(var, stmt1) + umc(var, stmt2) + ...
umc(var, while expr { body }) = if umc(var, body) > Known(0) || umc(var, expr) > Known(0) { Unknown } else { Known(0) }
umc(var, let var = ...;) = Known(1)
umc(var, let var;) = Known(0)

where umc(var, ast) counts the maximum possibly assignments & mutable-uses of var in ast, the operations are such that the result is Unknown if any of the inputs are Unknown and Unknown > Known(k) for all k.

(I guess the count could even be compressed to UsedNever, UsedOnce and UsedMany.)

[graydon]

non-critical for 0.6, de-milestoning