Closure captures immutable reference when mutable reference is necessary

[ldm0]

Code

I tried this code:

fn main() {
    let mut schema_all = vec![];
    (0..42).for_each(|_x| match Err(()) as Result<(), _> {
        Ok(()) => schema_all.push(()),
        Err(_) => (),
    });
}

I expected to see this happen: cargo build success

Instead, this happened: cargo build failed

error[E0596]: cannot borrow `schema_all` as mutable, as it is behind a `&` reference
 --> src/main.rs:4:19
  |
4 |         Ok(()) => schema_all.push(()),
  |                   ^^^^^^^^^^ cannot borrow as mutable

warning: variable does not need to be mutable
 --> src/main.rs:2:9
  |
2 |     let mut schema_all = vec![];
  |         ----^^^^^^^^^^
  |         |
  |         help: remove this `mut`
  |
  = note: `#[warn(unused_mut)]` on by default

error: aborting due to previous error; 1 warning emitted

For more information about this error, try `rustc --explain E0596`.

Version it worked on

It most recently worked on: nightly-2021-03-16

Version with regression

rustc --version --verbose:

nightly-2021-03-17

[ldm0]

Regression in f5d8117 #82536

[LeSeulArtichaut]

cc @roxelo @arora-aman @nikomatsakis

[roxelo]

@rustbot claim

[roxelo]

The issue appears to span from this line: return_if_err!(mc.cat_pattern(discr_place.clone(), &arm.pat, |place, pat| {.
mc.cat_pattern returns an error which leads us to return before we walk the arms of the match. As a result, we don't know that we need to capture schema_all as a mutable reference.

I am still investigating why cat_pattern returns an error

[roxelo]

@LeSeulArtichaut I don't think this error is related to my changes anymore. I think my code instead led us to discover an ICE when as Result<(), _> is used. I think some work was done not so long ago on it.

For instance, these 2 code works:

fn main() {
    let mut schema_all = vec![];
    (0..42).for_each(|_x| match Err(()) { // removed as Result...
        Ok(()) => schema_all.push(()),
        Err(_) => (),
    });
}

fn main() {
    let mut schema_all = vec![];
    (0..42).for_each(|_x| match Err(()) as Result<(), _> {
        Ok(()) => schema_all.push(()),
        _ => (), // Use only _ here
    });
}

Otherwise, I took a look at why cat_pattern returns an error with the initial code snippet. When looking at the pattern Err(_) the cat_pattern returns an error here when unwrapping pat_ty_adjusted. This is because later on this function calls resolve_type_vars_or_error and ty.is_ty_var() here returns true.

In the meantime, a work around would be to first walk all of the arms of the match before trying to figure out whether the discreminent should be read or not. However, even with this fix, it is very likely that a similar issue would arise in the case that the discriminant also needs to be captured.

EDIT: In case we want to continue this conversation on zulip, I created a topic here: https://rust-lang.zulipchat.com/#narrow/stream/189812-t-compiler.2Fwg-rfc-2229/topic/Issue.20.2387814

[roxelo]

@rustbot label +A-edition-2021

[apiraino]

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-medium

EDIT: fixed link

[m-ou-se]

(@apiraino That link links to the thread of another issue.)