[regression - rust2018]: unused_mut lint false positives on nightly

[gnzlbg]

I believe this is a regression in rust2018 nightly (it wasn't triggering in earlier rust2018 builds).

---

Playground

#![deny(warnings)]  

#[allow(unreachable_code)]
pub fn sum_nan() {
    // return;

    let mut v = 0;
    assert_eq!(v, 0);
    v = 1;
    assert_eq!(v, 1);
}

fn main() {}

works correctly, but uncommenting the return; (play) errors with:

error: variable does not need to be mutable
 --> src/main.rs:7:9
  |
7 |     let mut v = 0;
  |         ----^
  |         |
  |         help: remove this `mut`

This only happens with Rust2018 on nightly (Rust2015 works fine).

[matthewjasper]

Well, the code compiles (in Rust2018) without mut, so the lint's not exactly wrong, even if the behaviour is surprising.

[gnzlbg]

@matthewjasper this is caused by the NLL feature, so maybe this is a consequence of #54943 ?

[pnkfelix]

#54943 is about well-formedness, which I don't think is part of the code that tags whether a mut was necessary or not (which is what is triggering the unused_mut warning that is then triggering the deny(warnings) you have in there.

This is an interesting scenario in terms of what one can do to mitigate this...

[gnzlbg]

@pnkfelix i've been thinking a bit about the well-formedness of unreachable code because of this. AFAICT with NLL currently unreachable code needs to be syntactically correct, to typeck and to unsafeck, but it does not need to dropck (double drops are currently allowed) or borrowck (this example).

In Rust2015 unreachable code also needs to dropck and borrowck.

[gnzlbg]

The first question that should be answered is whether these changes in the well-formedness requirements of unreachable code are by design or an accident (cc @nikomatsakis ). If this is by design, then I'd suppose they have gone through an RFC and we can close this, or leave this open with the intent of maybe adding a better warning about this.

If they are by accident, then the first thing to do is to decide what should be the correct behavior here (e.g. stick to what Rust2015 does or change that to something else?).

[nikomatsakis]

This is considered an acceptable regression, per our usual policy on lints (see e.g. the motivation for RFC 1993. However, we may want to avoid linting about unused-mut on unreachable variables like this, just to be less annoying.

[pnkfelix]

(in particular the fact that the binding is itself unreachable makes this lint seem especially weird, at least to me...)

[pnkfelix]

Also, I am not sure whether I was sufficiently clear in my earlier comment: #54943 is using the phrase "well-formed" as a particular technical term. "Well-formed" is a predicate on types that is meant to evaluate whether an individual type "makes sense" before you start considering whether other expressions are instances of that type; it is documented (at least in part) in part of RFC 1214.

In other words, I do not think #54943 is related to this issue.

[gnzlbg]

Another variant of this bug is:

#[allow(unreachable_code)]
pub fn sum_nan() {
    return;

    let v = 0;
    assert_eq!(v, 0);
    std::mem::drop(v);
    std::mem::drop(v);
    assert_eq!(v, 1);
}

where no errors are emitted even though the unreachable code has a double drop, or this code:

#[allow(unreachable_code, unused_mut)]
pub fn sum_nan() {
    return;

    let mut v = 0;
    assert_eq!(v, 0);
    let x = &mut v;
    assert_eq!(*x, 0);
    *x = 1;
    let y = &mut v;
    assert_eq!(*y, 1);
    assert_eq!(*x, 1);
}

where two mutable references can be created and compile fine in unreachable code.

[pnkfelix]

@gnzlbg wrote:

The first question that should be answered is whether these changes in the well-formedness requirements of unreachable code are by design or an accident (cc @nikomatsakis ). If this is by design, then I'd suppose they have gone through an RFC and we can close this, or leave this open with the intent of maybe adding a better warning about this.

I think at least some of the changes you are describing here are definitely by design.

The NLL RFC stated that lifetimes were going to be based on the control-flow graph. This included details like "the lifetime 'a must only outlive those portions of 'b that are reachable from P."

Giiven that this was always going to be a control-flow sensitive analysis, it seems entirely understandable that the new borrow-check analysis is going to only consider code that is actually reachable in the control flow.

So, my initial reaction is that the examples you describe are not all that surprising to me.

---

Having said that ... that same RFC did document a number of changes that were being included in order to deal with infinite loops.

• (A stray thought: Would it be bonkers to use similar false unwind edges after return statements...?)
• Update: it almost certainly wouldn't be trivial to add such a change; after all, part of the point of adding the false unwind edge after loops was to make it look like destructors would run. But return statements already have that effect... I guess we could make the false-edges conditional (which is similar to what we do in the false-edges we add to match arms...)

[pnkfelix]

triage: I have tagged this as NLL-diagnostics (rather than NLL-complete).

My reasoning: Even though this is an example where one might argue that NLL is causing the compiler to regress (and fail to compile previously compiled code, which would usually yield an NLL-complete issue), that failure to compile is a consequence of the use of #![deny(warnings)], and our standard policy is that the compiler is free to add new warnings; cap-lints is meant to take care of downstream crates that have not been updated to account for any such warnings, as @nikomatsakis stated above.

[pnkfelix]

Triage: I don't think any hypothetical "fix" (if we make one) would be beta-backported. So the only question is what priority to assign.

With regards to prioritization... I'm torn between P-high or P-medium on this one. I think I'll wait to discuss it at the NLL team meeting tonight.

[pnkfelix]

Interestingly, the following example provided by @matthewjasper has the same behavior (of triggering the lint in question) on both the 2015 and 2018 editions (play)

#![deny(warnings)]  

#[allow(unreachable_code)]
pub fn sum_nan() {
    let mut v;
    v = 0;
    assert_eq!(v, 0);
    return;
    v = 1;
    assert_eq!(v, 1);
}

fn main() {}

(I mention this mostly because I am not sure exactly what mental model one is meant to apply to reason about when the lint here will and won't fire...)

[pnkfelix]

making executive decision to tag this as P-medium

[gnzlbg]

Thank you all!