Reborrow &mut on every move

[Jules-Bertholet]

View all comments

---

From the Reference:

Moved and copied types

When a place expression is evaluated in a value expression context, or is bound by value in a pattern, it denotes the value held in that memory location.

If the type of that value implements Copy, then the value will be copied.

In the remaining situations, if that type is Sized, then it may be possible to move the value.

[…]

This PR adds one exception to the above rules: if the type of the value is an &mut _ mutable reference, the value will now always be reborrowed instead of moved.

This allows the following to compile:

fn generic(_: impl Sized) {}
fn assert_fnmut(_: &mut impl FnMut()) {}

struct Foo<T>(T);

fn main() {
    let mut_ref = &mut ();
    generic(mut_ref);
    {
        mut_ref
    };
    let _local = mut_ref;
    let _ = || mut_ref;
    let mut _tup: (&mut (),) = (&mut (),);
    _tup.0 = mut_ref;
    Foo(mut_ref);
    Foo { 0: mut_ref };
    let mut f = || {
        let _y: &mut _ = mut_ref;
    };
    f();
    f();
    assert_fnmut(&mut f);
    generic(mut_ref);

    let mut_ref_ref = &mut &mut ();
    generic(*mut_ref_ref);
    let _local = *mut_ref_ref;
    let _ = || *mut_ref_ref;
    let mut _tup: (&mut (),) = (&mut (),);
    _tup.0 = *mut_ref_ref;
    Foo(*mut_ref_ref);
    Foo { 0: *mut_ref_ref };
    let mut f = || {
        let _y: &mut _ = *mut_ref_ref;
    };
    f();
    f();
    assert_fnmut(&mut f);
    generic(*mut_ref_ref);
}

Unfortunately, the following continues to not compile:

fn assert_fnmut(_: &mut impl FnMut()) {}
fn main() {
    let x = &mut ();
    let mut f = || {
        //~^ ERROR expected a closure that implements the `FnMut` trait, but this closure only implements `FnOnce` [E0525]
        let _y = x;
    };
    f();
    f();
    assert_fnmut(f);
}

This last case is unfortunately very difficult to fix at present. See #47478 for discussion.

When #![feature(reborrow)] (#145612) is enabled, we also insert reborrows for concrete types implementing Reborrow.

@rustbot label A-MIR A-borrow-checker T-lang needs-fcp

[lcnr]

should then probably also remove the explicit reborrows in HIR typeck?

View changes since this review

[dianne]

should then probably also remove the explicit reborrows in HIR typeck?

would that work without changing closure upvar inference to match? e.g. I think this closure captures x by mut ref (and accordingly is a FnMut) only because _y's declaration has a type annotation on it that causes coercing to insert a reborrow:

fn main() {
    let x = &mut ();
    let mut f = || {
        let _y: &mut _ = x;
    };
    f();
    f();
}

without the annotation, it captures x by value and fails to compile

edit: oh, of course, this makes the MIR for constructing the closure do the reborrow since it uses consume_by_copy_reborrow_or_move

edit 2: but it'd still be inferred to be a FnOnce, wouldn't it, if it has by-value captures?

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

[rustbot]

r? @adwinwhite

rustbot has assigned @adwinwhite.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 77 candidates
• Random selection from 19 candidates

[Jules-Bertholet]

#163494 (comment): I don't understand MIR optimizations, don't feel confident touching these tests. Someone who knows more than I do will need to take a look

[Jules-Bertholet]

@rustbot label F-reborrow

[rustbot]

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

[rustbot]

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

Some changes occurred in match checking

cc @Nadrieril

[Jules-Bertholet]

I am trying to fix the remaining mir-opt regression, but it's a little tough. If I improve CopyProp so that it recognizes &mut → &mut reborrows as a form of copy, that in turn pessimizes ReferencePropagation because it will consider fewer locals fully replaceable.

[Jules-Bertholet]

Hmm, so it looks like all the optimizations that are lost in CopyProp get recovered in ReferencePropagation. So there is only an actual optimization regression in opt-level=0 builds (where ReferencePropagation is not enabled). So maybe this is fine?

[rustbot]

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @vakaras

[nnethercote]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: c842e75 (c842e7577a58e3d96e5e86c7b7f0649bb84ae4f8)
Base parent: c36f145 (c36f1457196e315bc204b9564a6a5a7fe7f5a51f)

[rust-timer]

Finished benchmarking commit (c842e75): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.9%, -0.1%]	9
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-0.6%	[-0.9%, -0.1%]	9

Max RSS (memory usage)

Results (primary 1.1%, secondary -1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.8%	[2.9%, 4.7%]	2
Regressions ❌ (secondary)	4.3%	[4.3%, 4.3%]	1
Improvements ✅ (primary)	-4.3%	[-4.3%, -4.3%]	1
Improvements ✅ (secondary)	-2.9%	[-4.2%, -0.7%]	3
All ❌✅ (primary)	1.1%	[-4.3%, 4.7%]	3

Cycles

Results (secondary 4.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.1%	[2.9%, 6.2%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.5%]	7
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.1%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.2%, 0.5%]	11

Bootstrap: 489.203s -> 490.429s (0.25%)
Artifact size: 406.46 MiB -> 406.52 MiB (0.02%)

[Jules-Bertholet]

@rustbot label I-lang-nominated

[RalfJung]

Suggested change

	/// This is produced by the [`ExprKind::CoerceShared`],
	/// This is produced by [`ExprKind::CoerceShared`],

(I know this is pre-existing, but you're touching this comment anyway)

View changes since the review