Skip to content

Add CoerceShared field-wise reborrow WF checks#157489

Open
P8L1 wants to merge 7 commits into
rust-lang:mainfrom
P8L1:split/coerceshared-wf-check
Open

Add CoerceShared field-wise reborrow WF checks#157489
P8L1 wants to merge 7 commits into
rust-lang:mainfrom
P8L1:split/coerceshared-wf-check

Conversation

@P8L1

@P8L1 P8L1 commented Jun 5, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

View all comments

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

r? @aapoalas

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 5, 2026
@P8L1

P8L1 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

@rustbot label +F-Reborrow

@rustbot rustbot added the F-reborrow `#![feature(reborrow)]`; see #145612 label Jun 5, 2026
@P8L1 P8L1 mentioned this pull request Jun 5, 2026
Closed
aapoalas
aapoalas requested changes Jun 5, 2026
View reviewed changes

@aapoalas aapoalas left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread compiler/rustc_hir_analysis/src/coherence/builtin.rs Outdated
Comment thread compiler/rustc_hir_analysis/src/coherence/builtin.rs
Comment thread compiler/rustc_hir_analysis/src/coherence/builtin.rs Outdated
Comment thread compiler/rustc_hir_analysis/src/coherence/builtin.rs Outdated
Comment thread compiler/rustc_hir_analysis/src/coherence/builtin.rs
Comment thread tests/ui/reborrow/coerce-shared-field-lifetime-swap.rs Outdated
Comment thread tests/ui/reborrow/auxiliary/reborrow_foreign_private.rs Outdated
Comment thread tests/ui/reborrow/coerce-shared-foreign-private-tuple-field.rs Outdated
Comment thread tests/ui/reborrow/coerce-shared-lifetime-mismatch.stderr
Comment thread tests/ui/reborrow/marker-coerce-shared-corrected-issue-156309.rs Outdated
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2026
@rustbot

rustbot commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

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

@P8L1

P8L1 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2026
@P8L1 P8L1 requested a review from aapoalas June 8, 2026 14:27
aapoalas
aapoalas requested changes Jun 11, 2026
View reviewed changes

@aapoalas aapoalas left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

View changes since this review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2026
@P8L1 P8L1 force-pushed the split/coerceshared-wf-check branch from c82f18a to 8a5f5aa Compare June 12, 2026 11:04
@P8L1 P8L1 requested a review from aapoalas June 12, 2026 11:19
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 12, 2026
@P8L1

P8L1 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rust-bors

This comment has been minimized.

Sign in to view
@P8L1

P8L1 commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

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

@P8L1 P8L1 force-pushed the split/coerceshared-wf-check branch from 46b362c to d5e890a Compare June 15, 2026 09:25
@rustbot

rustbot commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@P8L1

P8L1 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Ok, I had some time sooner than I expected I would. @aapoalas, I think I'm ready for another review.

@aapoalas

aapoalas commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned aapoalas Jun 15, 2026
@aapoalas

aapoalas commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

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

P8L1 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aapoalas aapoalas Awaiting requested review from aapoalas

Requested changes must be addressed to merge this pull request.

Assignees

@oli-obk oli-obk

Labels

F-reborrow `#![feature(reborrow)]`; see #145612 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@P8L1 @rustbot @aapoalas @oli-obk