OpAssign can cause unused_assignments false positives

[durka]

This code

#![feature(augmented_assignments, op_assign_traits)]

use std::ops::AddAssign;

struct MutRef<'a>(&'a mut i32);

impl<'a> AddAssign<i32> for MutRef<'a> {
    fn add_assign(&mut self, rhs: i32) {
        *self.0 += rhs;
    }
}

fn main() {
    let mut a = 1;
    {
        let mut b = MutRef(&mut a);
        b += 1;
    }
    println!("{}", a);
}

reads the variable b by calling its AddAssign implementation. However, the warning still triggers. If you expand b.add_assign(1) manually, there is no warning.

cc @bluss. Found by "jatentaki" on IRC.

[Aatch]

It's likely that the lint isn't aware of OpAssign and is therefore technically correct in it's assessment that b += 1 has an assignment that isn't read (since it's assuming it's the same as b = b + 1, not true).

[durka]

@Aatch yes, presumably the warning should only trigger if the operands to += aren't primitives. But... you don't need a warning on AddAssign for Wrapping, etc. I guess it could exempt impls that are in core/std? Or there could be some annotation for impls like #[side_effect_free_operator]? Bleh.

[Aatch]

@durka hmm. The thing is, I think most implementations are going to be logically side-effect free. Ramp is, since doing a += b can't affect some other variable due to unique ownership constraints. Maybe a check for contained &mut and/or interior mutability to disable the warning? It wouldn't be perfect, but it would probably be a decent balance.

Yet another option is to detect the overloaded case and print out a slightly different message that makes it clear that the lint isn't necessarily "correct" here. There is some precedent in clang where it warns on doing if (a = b) as you might mean if (a == b) instead, but suggests using if ((a = b)) if you really did mean that to silence the warning. Similarly, prepending a _ to the variable will silence this warning. I think this case is a bit of an anti-pattern, so I'm not too worried about being perfect. People generally expect a += b to be semantically equivalent to a = a + b, so having a += b have (visible) side-effects is probably something we want to discourage anyway.

[bluss]

@Aatch This originally came up in ndarray which as a mutable view type ArrayViewMut<'a, Elem, Dimension> where using it with assign-ops will definitely be common. The "ArrayViewMut" represents for example a quadrant or other sliced-out part of an array.

So in that context this warning wants to be fixed.

[bluss]

Ndarray supports both Array += &Array; and Array += Scalar; btw. I think those being sugar for in place modification of the array data is fine.

[durka]

Changing a += 1; to a.add_assign(1); silences the warning even when a: i32. So clearly it's not perfectly accurate.

We could put an attribute on the AddAssign impl but I think it would have to be called #[impure] and we probably don't want to open that can of worms :)