Misguiding help on error "mut must be attached to each individual binding"

[tomprogrammer]

I tried this code to create a mutable variable binding to tuple.0 while also dereferencing it in the same pattern. Of course this code is not correct. I think the intention can be guessed because of the order in which & and mut appear, especially in the parenthesized variant.

#![allow(unused_variables)]
fn main() {
    let tuple: (&u8, &[u8]) = (&0, &[0, 0]);
    let (mut &last, rest) = tuple;
    let (mut (&last), rest) = tuple;
    // `last` should be type of `u8`
    // EDIT: This can be minimized to:
    let mut &x = &0;
}

This is the correct code, where the tokens & and mut are separated by parentheses to avoid parsing as &mut. This is also perfectly logical since patterns are read from the outside inwards destructuring the given value. My fault was to interpret & and mut like operators which got me stuck at the code I've written above.

#![allow(unused_variables)]
fn main() {
    let tuple: (&u8, &[u8]) = (&0, &[0, 0]);
    let (&(mut last), rest) = tuple;
    // EDIT: This can be minimized to:
    let &(mut x) = &0;
}

The issue is that the help output by the compiler isn't actually helping much because it changes semantics of the code. The compiler suggests to use &mut which would dereference a mutable reference when used in a pattern. That is very different from dereferencing a shared reference and mutably binding a variable.

Since &mut consists of two tokens & and mut the compiler tries to show you that the two token are in the wrong order I guess. In my opinion this is less probable than trying to express the pattern &(mut variable) wrongly as mut &variable. For the variant mut (&variable) the help is not correct anyway since the compiler ignores the parentheses.

I propose to change the help text to suggest &(mut last) instead. I'm not sure whether there are cases where this help would be wrong also.

Current error and help:

error: `mut` must be attached to each individual binding
 --> src/main.rs:5:10
  |
4 |     let (mut &last, rest) = tuple;
  |          ^^^^^^^^^ help: add `mut` to each binding: `&mut last`
  |
  = note: `mut` may be followed by `variable` and `variable @ pattern`

error: `mut` must be attached to each individual binding
 --> src/main.rs:6:10
  |
5 |     let (mut (&last), rest) = tuple;
  |          ^^^^^^^^^^^ help: add `mut` to each binding: `(&mut last)`
  |
  = note: `mut` may be followed by `variable` and `variable @ pattern`

warning: unnecessary parentheses around pattern
 --> src/main.rs:6:10
  |
5 |     let (mut (&last), rest) = tuple;
  |          ^^^^^^^^^^^ help: remove these parentheses
  |
  = note: `#[warn(unused_parens)]` on by default

Meta

All the compiler channels produce the same output.
rustc --version --verbose:
Stable:

rustc 1.48.0 (7eac88abb 2020-11-16)
binary: rustc
commit-hash: 7eac88abb2e57e752f3302f02be5f3ce3d7adfb4
commit-date: 2020-11-16
host: x86_64-unknown-linux-gnu
release: 1.48.0
LLVM version: 11.0

Beta:

rustc 1.49.0-beta.4 (877c7cbe1 2020-12-10)
binary: rustc
commit-hash: 877c7cbe142a373f93d38a23741dcc3a0a17a2af
commit-date: 2020-12-10
host: x86_64-unknown-linux-gnu
release: 1.49.0-beta.4

Nightly:

rustc 1.50.0-nightly (f74583445 2020-12-18)
binary: rustc
commit-hash: f74583445702e2e27ec4415376f2c540a83d7ded
commit-date: 2020-12-18
host: x86_64-unknown-linux-gnu
release: 1.50.0-nightly

[tomprogrammer]

I already tried to research how I would improve diagnostics here. These are my notes:

The error is constructed in the rustc_parse::parser::pat module. The relevant functions are:

    /// Parse a mutable binding with the `mut` token already eaten.
    fn parse_pat_ident_mut(&mut self) -> PResult<'a, PatKind> {
        let mut_span = self.prev_token.span;

        if self.eat_keyword(kw::Ref) {
            return self.recover_mut_ref_ident(mut_span);
        }

        self.recover_additional_muts();

        // Make sure we don't allow e.g. `let mut $p;` where `$p:pat`.
        if let token::Interpolated(ref nt) = self.token.kind {
            if let token::NtPat(_) = **nt {
                self.expected_ident_found().emit();
            }
        }

        // Parse the pattern we hope to be an identifier.
        let mut pat = self.parse_pat(Some("identifier"))?;

        // If we don't have `mut $ident (@ pat)?`, error.
        if let PatKind::Ident(BindingMode::ByValue(m @ Mutability::Not), ..) = &mut pat.kind {
            // Don't recurse into the subpattern.
            // `mut` on the outer binding doesn't affect the inner bindings.
            *m = Mutability::Mut;
        } else {
            // Add `mut` to any binding in the parsed pattern.
            let changed_any_binding = Self::make_all_value_bindings_mutable(&mut pat);
            self.ban_mut_general_pat(mut_span, &pat, changed_any_binding);
        }

        Ok(pat.into_inner().kind)
    }

For the pattern mut &variable pat should be:

PatKind::Ref(PatKind::Ident(BindingMode::ByValue(Mutability::Not), ..), Mutability::Not) // (1)

After make_all_value_bindings_mutable it will be (if I my assumptions about the visitor are correct):

PatKind::Ref(PatKind::Ident(BindingMode::ByValue(Mutability::Mut), ..), Mutability::Not) // (2)

    /// Turn all by-value immutable bindings in a pattern into mutable bindings.
    /// Returns `true` if any change was made.
    fn make_all_value_bindings_mutable(pat: &mut P<Pat>) -> bool {
        struct AddMut(bool);
        impl MutVisitor for AddMut {
            fn visit_pat(&mut self, pat: &mut P<Pat>) {
                if let PatKind::Ident(BindingMode::ByValue(m @ Mutability::Not), ..) = &mut pat.kind
                {
                    self.0 = true;
                    *m = Mutability::Mut;
                }
                noop_visit_pat(pat, self);
            }
        }

        let mut add_mut = AddMut(false);
        add_mut.visit_pat(pat);
        add_mut.0
    }

Here the changed pattern is pretty-printed. The pattern

PatKind::Ref(PatKind::Ident(BindingMode::ByValue(Mutability::Mut), ..), Mutability::Not) // (2)

is printed to be &mut variable (derived from the help message). But this doesn't round trip through parsing again, because then it'll be:

PatKind::Ref(PatKind::Ident(BindingMode::ByValue(Mutability::Not), ..), Mutability::Mut) // (3)

Here the change in semantics is visible. If the pretty-printer would have printed &(mut variable) the pattern would be

PatKind::Ref(PatKind::Paren(PatKind::Ident(BindingMode::ByValue(Mutability::Mut), ..)), Mutability::Not) // (4)

which is think should be equal to the original pattern (2) with the mutable binding.

    /// Error on `mut $pat` where `$pat` is not an ident.
    fn ban_mut_general_pat(&self, lo: Span, pat: &Pat, changed_any_binding: bool) {
        let span = lo.to(pat.span);
        let fix = pprust::pat_to_string(&pat);
        let (problem, suggestion) = if changed_any_binding {
            ("`mut` must be attached to each individual binding", "add `mut` to each binding")
        } else {
            ("`mut` must be followed by a named binding", "remove the `mut` prefix")
        };
        self.struct_span_err(span, problem)
            .span_suggestion(span, suggestion, fix, Applicability::MachineApplicable)
            .note("`mut` may be followed by `variable` and `variable @ pattern`")
            .emit();
    }

Therefore I think this code is correct, but the pretty printer should print the pattern differently so it matches the reparsed result better.

I hope this helps. Maybe someone more confident with the parsing and pretty-printing code could take a look on this.

[tomprogrammer]

I managed to solve the issue, let's see what the review brings.