Add CoerceShared field-wise reborrow WF checks

[P8L1]

View all comments

This PR attempts to add a well-formedness check for CoerceShared.
Split out of #157101

r? @aapoalas

[P8L1]

@rustbot label +F-Reborrow

[aapoalas]

Some issues but overall this looks pretty promising.

I'd kind of like the tests to all rather be added in the #157490 and then this PR to show which ones it fixes and which ones remain issues, but I'm not too partial on that point. I've reviewed the PR in the conventional comments style, so anything marked "thought" or "note" is just musings and not something you need to necessarily react to (unless you wish to), "question" is me being unsure but not something that would block the PR from merging, and "issues" and anything else marked "blocking" must be addressed before this can be considered for merging.

I'm also somewhat surprised that this PR didn't actually change any of the existing test results... I guess that's just a sign of our existing ui feature tests being insufficient :D

View changes since this review

[rustbot]

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

[P8L1]

@rustbot ready

[aapoalas]

Two issues remain from my point of view. I'll pass this for compiler reviewers next :)

View changes since this review

[P8L1]

@rustbot ready

[P8L1]

I will be rebasing this Wednesday I think, and I will also update it so it shows what tests it fixes

[aapoalas]

I just realised that while this is IMO the right way to proceed, we now kinda need temporary branches in the WF check impl that verify that the types are still trivially memcpy'able. Otherwise we get a good WF check but a too-simple-by-half implementation that doesn't know how to handle all the cases ... :)

[P8L1]

I just realised that while this is IMO the right way to proceed, we now kinda need temporary branches in the WF check impl that verify that the types are still trivially memcpy'able. Otherwise we get a good WF check but a too-simple-by-half implementation that doesn't know how to handle all the cases ... :)

Ah, yes, good point. 😄
I will add a temp branch on Wednesday

[P8L1]

Sorry, I'm a bit late. Just added temp branch.
@rustbot ready

[aapoalas]

Sorry, an issue appears with the temp branch as written right now :(

View changes since this review

[aapoalas]

Sorry, I'm a bit late.

No such thing - thank you for taking the time <3

[P8L1]

@rustbot ready

[oli-obk]

I haven't really reviewed the actual emission logic in depth yet, but it seems to reinvent a lot of checks we already have elsewhere. This has the danger of them diverging later on top of there possibly being subtle differences now

[P8L1]

Hi Oli, thanks for the review.
I'm definitely learning a lot.

I just pushed some changes.

I added HIR-based diagnostic context for CoerceShared impls so diagnostics can point at the impl, the source/self type, the target trait argument, and explicit source/target lifetimes where available. Then I added an impl anchor to field mismatch diagnostics, and changed inaccessible-field diagnostics to distinguish source-type vs target-type failures and label the relevant type use in the impl. I changed lifetime mismatch diagnostics to label the actual source/target lifetime spans when available, and missing-field diagnostics to label both the target field and the source type that lacks it.
I replaced the misleading multi-non-ZST field mismatch diagnostic with a dedicated temporary impl restriction diagnostic.

I moved the mut-ref-to-shared-ref field relation from structural equality of peeled referents to an obligation-based subtype relation in the coercion-compatible direction.

For the broader emission-logic concern, I checked this specifically. The full coercion/error-reporting infra lives in rustc_hir_typeck and depends on body/typeck context, while this WF check runs in rustc_hir_analysis/coherence. Pulling FnCtxt/coercion reporting into this layer would be the wrong dependency direction from my POV. But I may be wrong and would be more than happy to change it.

@rustbot ready