ANNOT: Reassign immutable [E0384]

[Shiroy]

Detect reassignation of immutable bindings and mark the corresponding statements as an error. (E0384 of #886)

I'm trying to write a quick fix for this issue but I encounter some difficulties because the original bindings may come from either a let expression or a function argument. I need some tips to write that fix.

[alygin]

I think this annotation can only be implemented for a very limited set of cases at the moment. Otherwise, it will be a source of great deal of false positives. When I put this error to the list I thought of the implementation under the following circumstances:

1. The left side resolves to a let- or static-declaration, or to a function parameter.
2. No complex matching patterns involved.
3. No asterisks involved.

That's not much, but at least that looks feasible. The next step would be struct fields reassigning, or some simple patterns support. But that's harder to do properly than it might seem at first glance.

[Shiroy]

I also figured out a case I don't really know how I can detect and handle

let x: int32
//...
x = 3; //First assignation -> should work (actually it doesn't)
//...
x = 5; //Second assignation -> shouldn't work (this part is OK)

Or more tricky

let x: int32
//...
if (stuff()) {
    x = 5;
} else {
    x = 3;
}

What are your suggestions to handle that ?

[alygin]

I would start with ignoring all cases when a left side resolves to a let-declaration without assignment. Otherwise you need to have an absolutely accurate resolve engine to guarantee proper reassigning analysis. And we have no such engine yet.

[Shiroy]

Ok, I'll do that.

How could such an engine work ? What is the big picture ?

[alygin]

I had a kind of prototype for this annotation. It visited RsPathExprs, resolved their paths to RsPatBindings, then I searched for a parent RsLetDecl (RsConstant or RsValueParameter should also work) and checked the mutability specifier.

Add those checks on absence of complex patterns and asterisks in the path, on presence of an assignment in let-declarations, drop analysis every time you can't resolve something, and that should be enough for the most straightforward cases. Chances are there won't be false positives :)

[alygin]

Though I think binary expressions are more suitable elements to start such analysis than RsPathExprs. My prototype actually was for borrow checking on function calls, so it used RsPathExprs. But the general idea was the same.

[alygin]

Looks as a reasonably safe start, we can extend this annotation to other cases in the future. 👍

[alygin]

Can be simplified:

return (declaration as? RsPatBinding)?.isMut ?: false

[alygin]

The exact rustc message is "re-assignment of immutable variable foo", so I'd suggest to use "Re-assignment of immutable variable foo [E0384]". A simpler form would be without the variable name, though it sometimes useful to have it right in the error message.

[alygin]

And the if () formatting.

[alygin]

I usually add error numbers to the tests, like testE0384_ReassigningImmutableVariable() and put them in the numerical order. Maybe we could stick to such rule to keep them manageable as their number grows?

[alygin]

And I would also add a couple of tests to guarantee regression checks. For instance, a test for let without assignment, a test for assignment through asterisk (we should ignore them).

[matklad]

LGTM!

Thannks @Shiroy and @alygin !

[matklad]

The nice next step would be to add a quick fix to add a mut.