Preserve generic reborrow while fixing identity moves

[P8L1]

Fixes a move-vs-generic-reborrow selection issue in typeck without changing the intended generic Reborrow behaviour.

In ordinary identity move/coercion contexts, a T -> T use should not become Adjust::GenericReborrow merely because the target ADT implements Reborrow. This PR makes those ordinary identity contexts try normal unification first; if normal unification succeeds, the expression remains an ordinary move/copy.

cc @aapoalas

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
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 73 candidates
• Random selection from 20 candidates

[aapoalas]

I don't think this should be necessary, and I believe this would not match the semantics that we expect from comparing to &mut _.

If you dump the MIR for an identity function that takes a &mut _ you should see that it performs two reborrows, one from argument to local and one from local to return value. I assume with this change an identity function of a Reborrow type would no longer show the same form but would instead just have a single move. If one day &mut _ unifies with Reborrow then that could cause issues.

Further, I think this change would not actually fix the general issue. eg. I expect this function would not work:

fn foo<'a>(x: CustomMut<'a>) -> Result<(), CustomMut<'a>> {
  bar(x)?; // <--
  println!("{x:?}");
  Ok(())
}

// this function would work
fn bar<'a>(x: CustomMut<'a>) -> Result<(), CustomMut<'a>> {
  Err(x)
}

I expect that the marked line with ? usage would not compile as a reborrow will happen there, capturing the local x and then making the borrow checker angry at us trying to return a local value.

We have talks ongoing at the Rust all-hands to correct the implementation / design to resolve the problem of borrowing local places in reborrowing generally.

View changes since this review

[P8L1]

I don't think this should be necessary, and I believe this would not match the semantics that we expect from comparing to &mut _.

If you dump the MIR for an identity function that takes a &mut _ you should see that it performs two reborrows, one from argument to local and one from local to return value. I assume with this change an identity function of a Reborrow type would no longer show the same form but would instead just have a single move. If one day &mut _ unifies with Reborrow then that could cause issues.

Further, I think this change would not actually fix the general issue. eg. I expect this function would not work:

fn foo<'a>(x: CustomMut<'a>) -> Result<(), CustomMut<'a>> {
  bar(x)?; // <--
  println!("{x:?}");
  Ok(())
}

// this function would work
fn bar<'a>(x: CustomMut<'a>) -> Result<(), CustomMut<'a>> {
  Err(x)
}

I expect that the marked line with ? usage would not compile as a reborrow will happen there, capturing the local x and then making the borrow checker angry at us trying to return a local value.

We have talks ongoing at the Rust all-hands to correct the implementation / design to resolve the problem of borrowing local places in reborrowing generally.

View changes since this review

Thanks, I think you are right that this PR is treating a symptom at the wrong layer.

I checked the ordering change again. The PR makes Coerce::coerce try plain unification before coerce_reborrow only when allow_two_phase == AllowTwoPhase::No, so identity coercions in ordinary non-argument contexts can remain a move/copy. That does make the direct CustomMut<'a> -> CustomMut<'a> identity case compile, and the MIR for the scratch identity function is just:

_0 = move _1;
return;

That differs from the &mut identity shape you described. Dumping MIR for:

pub fn id_mut<'a, T>(x: &'a mut T) -> &'a mut T { x }

with -Zunpretty=mir -Zmir-opt-level=0 shows the two reborrows:

_2 = &mut (*_1);
_0 = &mut (*_2);

I also tested the ? example. On this PR, the bar(x)?; println!("{x:?}"); Ok(()) shape still fails with E0515/E0502: the reborrow captures the local x, and returning the residual requires that borrow to live for 'a. In my scratch version, even fn bar<'a>(x: CustomMut<'a>) -> Result<(), CustomMut<'a>> { Err(x) } hit the same local-place reborrow issue because Err(x) goes through a function-argument coercion path where this PR intentionally preserves reborrow-first behavior. A non-? call/use-after shape with a diverging bar_never(x) compiled, but that does not address the residual-return case.

I will be closing this PR, please let me know what happens at All Hands.