Incorrect unused mut warning in 1.46.0-nightly (f455e46ea 2020-06-20)

[dignifiedquire]

CI on https://github.com/async-rs/async-std suddenly started failing as we run with deny warnings.

the error is

error: variable does not need to be mutable
##[error]  --> src/io/stderr.rs:92:9
   |
92 |         mut self: Pin<&mut Self>,
   |         ----^^^^
   |         |
   |         help: remove this `mut`
   |
   = note: `-D unused-mut` implied by `-D warnings`

error: variable does not need to be mutable

for the following code

impl Write for Stderr {
    fn poll_write(
        mut self: Pin<&mut Self>,
        cx: &mut Context<'_>,
        buf: &[u8],
    ) -> Poll<io::Result<usize>> {
        let state = &mut *self.0.lock().unwrap();

    // ... 
}

which stops compiling if the referenced mut is removed.

(Sorry in advance if this is known, but I couldn't find a matching issue)

CI run: https://github.com/async-rs/async-std/pull/822/checks?check_run_id=793134400
Source Code: https://github.com/async-rs/async-std/blob/master/src/io/stderr.rs#L90-L96

Edit: It seems that the nightly version might not be what triggered it, but rather a change in the underlying implementation of the lock that is being called above.

No warnings under

• rustc 1.45.0-nightly (1836e3b 2020-05-06)
• rustc 1.44.1 (c7087fe 2020-06-17)

This issue has been assigned to @nbdd0121 via this comment.

[LeSeulArtichaut]

Let's find the cause of this regression.
@rustbot ping cleanup

[rustbot]

Hey Cleanup Crew ICE-breakers! This bug has been identified as a good
"Cleanup ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @AminArria @camelid @chrissimpkins @contrun @DutchGhost @elshize @ethanboxx @h-michael @HallerPatrick @hdhoang @hellow554 @imtsuki @kanru @KarlK90 @LeSeulArtichaut @MAdrianMattocks @matheus-consoli @mental32 @nmccarty @Noah-Kennedy @pard68 @PeytonT @pierreN @Redblueflame @RobbieClarken @RobertoSnap @robjtede @SarthakSingh31 @senden9 @shekohex @sinato @spastorino @turboladen @woshilapin @yerke

[SNCPlay42]

Reduced somewhat:

use std::pin::Pin;
use std::sync::Mutex;

pub struct Stderr(Mutex<()>);

pub fn poll_write(mut x: Pin<&mut Stderr>) {
    let _ = &mut *x.0.lock().unwrap();
}

[SNCPlay42]

Bisected with #![deny(warnings)]:

searched nightlies: from nightly-2020-05-06 to nightly-2020-06-21
regressed nightly: nightly-2020-06-20
searched commits: from e55d3f9 to 2d8bd9b
regressed commit: 72417d8

bisected with cargo-bisect-rustc v0.5.2

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start 2020-05-06 

[LeSeulArtichaut]

Just bisected, found similar results. Thanks @SNCPlay42!

Culprit PR is #73504 which is a rollup, however I don't see any obvious culprit in there...

[LeSeulArtichaut]

Actually, I think #72280 is the only possible culprit here, cc @nbdd0121 @nikomatsakis

[LeSeulArtichaut]

Consider this code, derived from #73592 (comment)

use std::pin::Pin;
use std::sync::Mutex;

pub struct Stderr(Mutex<()>);

pub fn poll_write(x: Pin<&mut Stderr>) { // `x` is not `mut` here
    let _ = &mut *x.0.lock().unwrap();
}

This does not compile with stable or beta compiler, but compiles fine with the nightly compiler. I suspect this is a soundness issue, given that I think it was expected for #72280 to only fix diagnostics, and not let code like this compile.

[nbdd0121]

#72280 does not only fix diagnostics. It fixes two cases that should compile but didn't. I am looking into this right now.

[nbdd0121]

I don't think this is a soundness issue

Consider this code, derived from #73592 (comment)

use std::pin::Pin;
use std::sync::Mutex;

pub struct Stderr(Mutex<()>);

pub fn poll_write(x: Pin<&mut Stderr>) { // `x` is not `mut` here
    let _ = &mut *x.0.lock().unwrap();
}

This does not compile with stable or beta compiler, but compiles fine with the nightly compiler. I suspect this is a soundness issue, given that I think it was expected for #72280 to only fix diagnostics, and not let code like this compile.

I might be missing something obvious, but why should this not compile? Clearly Deref<Target=Stderr> is sufficient for this to run, because Mutex::lock takes &self.

[nbdd0121]

I digged deeper and pin-pointed the reason for the behaviour change:

Previous we type-check the receiver of the method with needs = Needs::MutPlace when the result is type-checked with needs = Needs::MutPlace.

rust/src/librustc_typeck/check/expr.rs

Line 867 in 7bdf7d0

let rcvr_t = self.check_expr_with_needs(&rcvr, needs);

This means that DerefMut of Mutex will be used even though Deref would be sufficient. For types like Mutex/RefCell which gives something with DerefMut this is overly restrictive and unnecessary, so the compilation fails in stable.

This compiles in stable:

use std::pin::Pin;
use std::sync::Mutex;
use std::ops::Deref;

pub struct Stderr(Mutex<()>);

pub fn poll_write(x: Pin<&mut Stderr>) {
    let _ = &mut *Deref::deref(&x).0.lock().unwrap();
}

My PR fixes this and will only use DerefMut when necessary, so mut is no longer needed (actually, it should be a bug that it is previously required in this case).

[nbdd0121]

Given that rustc is allowed to generate additional warning in new versions, My opinion is that no fixes are necessary on rustc. I would recommend #[allow(unused_mut)] to be used, or spell out Deref::deref explicitly to mute the warning on nightly while let the code compile on stable.

[SNCPlay42]

I was worried that the OP says

which stops compiling if the referenced mut is removed.

But this appears to be incorrect, removing the mut succesfully compiles in the original crate when using the same nightly rustc that produced the warning. So there isn't different behaviour between the original crate and the reduced test case.

[dignifiedquire]

I retested to make sure to cover the cases mentioned above. When removing the mut

• stable 1.44.1
• rustc 1.45.0-nightly (1836e3b 2020-05-06)

both fail with the following error

   Compiling async-std v1.6.2 (/Users/dignifiedquire/opensource/async-std)
error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
  --> src/io/stderr.rs:96:27
   |
92 |         self: Pin<&mut Self>,
   |         ---- help: consider changing this to be mutable: `mut self`
...
96 |         let state = &mut *self.0.lock().unwrap();
   |                           ^^^^ cannot borrow as mutable

error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
  --> src/io/stderr.rs:96:27
   |
92 |         self: Pin<&mut Self>,
   |         ---- help: consider changing this to be mutable: `mut self`
...
96 |         let state = &mut *self.0.lock().unwrap();
   |                           ^^^^ cannot borrow as mutable

error: aborting due to previous error

However the nightly (1.46.0-nightly (f455e46 2020-06-20)) version does compile.

[nikomatsakis]

I agree with @nbdd0121 that this appears to be a bug fix, and that mut should not have been required.

[LeSeulArtichaut]

I think we should then close this issue as working as intended?

[nbdd0121]

We may want to add a UI test to ensure that the code without mut compiles and the code with mut triggers a warning.

[nikomatsakis]

Yes, +1 to a suitable test.

[nbdd0121]

@rustbot claim