Fix reborrow_info early return skipping field validation

[ikow]

reborrow_info validates that all data fields of a struct implementing Reborrow are either Copy or themselves Reborrow. When iterating the fields, finding a Reborrow field returned Ok(()) immediately, so any remaining fields were never validated. This let a struct with a non-Copy, non-Reborrow second field implement Reborrow.

Changing the early return Ok(()) to continue makes the loop validate every field.

Tracking issue: #145612

[rustbot]

r? @dingxiangfei2009

rustbot has assigned @dingxiangfei2009.
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

[dingxiangfei2009]

cc @aapoalas

Ah right, this is indeed a typo.

[dingxiangfei2009]

Let's ./x t tests/ui/reborrow/reborrow_multi_field_validation.rs --bless once.

View changes since the review

[aapoalas]

Oh; well that'd explain a few problems! Thank you <3

View changes since this review

[oli-obk]

Please strip the commit "co authored by LLM" thing. Either it is your work, or it is not. If it weren't, we could just use the tools ourselves instead of filtering our reviews through you to your LLM.

Similarly do not include "test" sections in your PR description. We do see test changes in the diff.

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Kivooeo]

How much was Claude involved into replacing return Ok(()) with continue?

[ds84182]

Genuine question:

Why does Reborrow not have a Copy supertrait bound, rather than enforcing all fields are Copy?

What happens if the Reborrow type also impls Drop?

A type that is both implicitly bitwise copyable but also impls Drop seems undesirable as copies can be elided and are supposed to be generally cheap (ignoring large Copy types).

I bring this up because my gut feeling is that while this PR fixes the desired behavior, it seems like Reborrow ultimately duplicates Copy as it just seems to be (at least to me) a Copy impl that also changes a lifetime. So perhaps the correct fix is to require types that impl Reborrow to also impl Copy, then allow the existing codepath for verifying Copy impls to handle this.

[aapoalas]

Genuine question:

Why does Reborrow not have a Copy supertrait bound, rather than enforcing all fields are Copy?

Fairly simple, actually: because Copy does not just mean "trivially memcpy-able" but also "any number of copies may freely exist at the same time", ie. the semantics of a Copy type are different from a Reborrow type. This is most visible in generic code:

fn foo<T: Copy + Debug>(t: T) {
  let u = t;
  let v = t;
  println!("{t} {u} {v}");
}

This should cause a borrow checker error if called with a type that implements Reborrow despite that type also being Copy. That's no good.

An argument has actually been made that the semi-reverse should be true: that perhaps there should be a blanket implementation of Reborrow for any T: Copy!

What happens if the Reborrow type also impls Drop?

This (and imply Copy) should be checked against in the coherence check of Reborrow, is my opinion. eg. Box<T> obviously cannot be reborrowed: every receiver would drop its copy, leading to double-free. A custom type with a Drop that logs or has some other non-destructive side-effect would be sound, but it would mean that the number of reborrows performed is actually observable / load-bearing, and therefore the compiler could no longer optimise out redundant copies.