New lint: transitive assignments

What it does

Check for presence of assignment sequences like this:

b = a;
c = b;

Such a code is likely to be unclear or hide a real subtle bug. It might be possible that the original intent was to write this code instead:

c = b;
b = a;

, which is semantically equivalent to c = std::mem::replace(&mut b, a);. This variant properly threads the value held in b. The original code however simply drops the old value without a way to recover it, which is probably not intended and actually hides a bug. If it is not a case, it is still possible to rewrite the code in a less confusing way, depeding on if type of a, b and c implements Copy or not.

If type is Copy

It is possible to rewrite code to underline the fact that both b and c get a value coming from a:

b = a;
c = a;

If type is !Copy

In this case the intermediate assignment is simply redundant and the code can be collapsed to:

c = a;

However, assignment to a variable causes the destructor of previous value to run, so this assignment might be intetional in order to eagerly end lifetime of a value and probably observe the side effects of destructor. Nevertheless, this can be written in a more clear way without relying on side effects of assignments:

drop(b);
c = a;

Real-life example

Developer Tiemoko Ballo (github username tnballo) made a library scapegoat which is essentially a (behaviorly) replication of std::collections::BTreeMap. In the process of development they introduced a bug which was not caught by std's BTreeMap rustdoc tests (they admitted that "this is a real bug I inadvertently introduced early in scapegoat's development" in their article on differential testing). The bug was hidden in a following snippet of code:

loop {
    let min_node = &self.arena[min_idx];
    match min_node.left_idx() {
        // Continue search for min node
        Some(lt_idx) => {
            min_idx = lt_idx;
            min_parent_idx = min_idx;
        }
// More code here...

For reference, the correct version of code is:

loop {
    let min_node = &self.arena[min_idx];
    match min_node.left_idx() {
        // Continue search for min node
        Some(lt_idx) => {
            min_parent_idx = min_idx;
            min_idx = lt_idx;
        }
// More code here...

This is exactly the kind of bug which could be caught by the proposed lint.

Lint Name

transitive_assignments

Category

suspicious

Advantage

• The corrected variant is more clear to understand
• The corrected variant leaves less place to plant a subtle bug

Drawbacks

• Correctly suggesting a possible fix need to differentiate between Copy and non-Copy types, which complicates the implementation
• It might probably have high false positives rate, driving developers to supress it

Example

b = a;
c = b;

Could be written as:

drop(b);
c = a;

No files, tests, or entry points are named. Start by reviewing the proposed transitive_assignments examples and the stated Copy, non-Copy, destructor, and false-positive considerations. Done means the proposed suspicious lint behavior and its expected diagnostics are implemented and covered by appropriate tests.