Skip to content

Add CoerceShared field-wise reborrow WF checks#157489

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

Add CoerceShared field-wise reborrow WF checks#157489
P8L1 wants to merge 14 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 Outdated
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 Outdated
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

This comment has been minimized.

Sign in to view
@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
@P8L1 P8L1 requested a review from oli-obk June 25, 2026 18:56
@rust-bors

This comment has been minimized.

Sign in to view
@aapoalas

aapoalas commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

@P8L1 I realised that we should actually be also checking the ZST fields for "Copy or Reborrow" as well: while it doesn't matter for runtime correctness, it does matter for type system soundness. It's possible that a user might create a non-Copy ZST type and attempt to use that in a field of a Reborrow type, and we should catch that as an error and report.

@P8L1

P8L1 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

lol im playing around with it now, will do 😆

@aapoalas aapoalas mentioned this pull request Jul 8, 2026
Open
4 tasks
@P8L1 P8L1 force-pushed the split/coerceshared-wf-check branch from f9ec797 to 2cbef8c Compare July 8, 2026 19:02
@rustbot

rustbot commented Jul 8, 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.

oli-obk
oli-obk requested changes Jul 9, 2026
View reviewed changes

@oli-obk oli-obk 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.

I think I mostly get it now, still need to think a bit about the interactions of the 3 steps in validate_coerce_shared_field

View changes since this review

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 Outdated
Comment thread compiler/rustc_hir_analysis/src/coherence/builtin.rs Outdated
Comment thread compiler/rustc_hir_analysis/src/coherence/builtin.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 Jul 9, 2026
@P8L1

P8L1 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

I made 96975b6 to address the ZST issues. I will now proceed to address the rest of Oli's review, then we can re-review

oli-obk
oli-obk reviewed Jul 9, 2026
View reviewed changes

@oli-obk oli-obk 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.

One thing I remembered but forgot to leave a comment for

View changes since this review

Comment thread compiler/rustc_hir_analysis/src/coherence/builtin.rs Outdated
@P8L1

P8L1 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Ok I just pushed ee0fb99. I think it addresses all of Oli's previous review findings. Ready for a re-review.

@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 Jul 9, 2026
@P8L1 P8L1 requested a review from oli-obk July 9, 2026 23:15
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
@oli-obk oli-obk Awaiting requested review from oli-obk

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.

5 participants

@P8L1 @rustbot @aapoalas @oli-obk @rust-log-analyzer