New lint [local_assigned_single_value]

[Centri3]

This one was pretty tough. I still don't know much about MIR so I likely overcomplicated some of it but I hope it's at least of decent quality :)

I don't know of any FPs (that I haven't fixed already), but there's tons of FNs, most notably let (x, y) = (1, 1) and the like. It's pretty barebones currently,

Closes #4016

changelog: New lint [local_assigned_single_value]

[rustbot]

r? @giraffate

(rustbot has picked a reviewer for you, use r? to override)

[Centri3]

r? @Jarcho (apparently you know MIR?)

[Centri3]

The implementation is pretty much done now, I may refactor again though. I still see no FPs as it's overly pessimistic, if it can't prove with 100% confidence it's constant it won't lint.

There are still missed cases, though, like casting a C-like enum or getting the field of a constant ADT

[bors]

☔ The latest upstream changes (presumably #10951) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #10884) made this pull request unmergeable. Please resolve the merge conflicts.

[Jarcho]

Just did an initial review. Didn't check the evaluation logic yet, but there's a bit of work to do with the current comments.

[Jarcho]

This is better represented as an enum. Something like:

enum State {
    Uninitialized,
    SingleValue(Value, Vec<Span>),
    MultiValue,
}

[Centri3]

Agreed

[Jarcho]

The map should be initialized to a state such that only user bound mutable variables check the assigned value. i.e. matches!(l.local_info, LocalInfo::User(_)) && l.mutability == Mutability::Mut)

[Centri3]

Well, local_info is actually ClearCrossCrate now so we can't. It's not available past borrowck and friends and results in an ICE if used in clippy.

We could perhaps use var_debug_info or something, or LocalKind::Temp.

[Jarcho]

You can't just ignore assignments from macros. The most permissive thing you can do here is to block linting for any locals which are assigned in macros. Trying to lint macros will be pain if you want to avoid false positives.

[Jarcho]

This whole chain should just be a match.

[Jarcho]

This is only ok if the source projection is not mutable.

[Jarcho]

This is only ok if the source projection is not mutable.

[Jarcho]

This is only ok if the source projection is not mutable.

[Jarcho]

This is only ok if the source projection is not mutable.

[Centri3]

Thanks for reviewing! I agree that there's a ton of work to do on this. I'll probably rewrite what you didn't review yet as it's even uglier. Unfortunately I might not be able to get to this as I've been having issues with both my physical and mental health, but I'll get to it in due time ^^

[Centri3]

I'm closing this for now as I have no intention to come back to it. If anybody else wants to work on this in the next few months, you're free to.

Some things I feel like should be note to anybody working on this in the future: MIR might not be the best for this. Currently, let x = A::B as i32 where A is repr(i32) and has implicit discriminators is lowered to something like let _ = 1i32 as i32, which means we have no way of ignoring this as it shouldn't be linted. This can be fixed upstream, however, there's no real way to determine if an assignment comes before another. So something like

let mut x = 1;
let mut y = 1;
x = y;
y = x;

Will either get into a loop or not lint, despite we should lint it. If this operates on the HIR, we can ignore y = x when evaluating x = y, but: HIR will be tricky. let (mut x, mut y) = (1, 2) will need to be evaluated properly, same with

let (&&&x, [y, z], A { x: _, w, .. }) = (&&&&&&1, [1, 2], A { x: 0, y: 0, z: 0, w: 0 }

and anything else. It'll be a very complex lint no matter what.