Skip to content

Conversation

ChayimFriedman2
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 commented Aug 24, 2023

This is safe because a &RefMut cannot change the borrow flags.

We cannot impl Send for RefMut because of map_split(); we cannot impl Sync for Ref because of clone().

This is insta-stable and needs a FCP.

This precludes the addition of an API that transforms &RefMut to Ref, so we need to choose what we want.

@rustbot
Copy link
Collaborator

rustbot commented Aug 24, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 24, 2023
@rust-log-analyzer

This comment has been minimized.

This is safe because a `&RefMut` cannot change the borrow flags.

We cannot impl `Send` for `RefMut` because of `map_split()`; we cannot impl `Sync` for `Ref` because of `clone()`.
@m-ou-se
Copy link
Member

m-ou-se commented Aug 24, 2023

Do you have a use case for this? Wouldn't it be better to just share the &T or &mut T rather than the RefMut<T>?

@ChayimFriedman2
Copy link
Contributor Author

@m-ou-se I don't, but I think that we tend to impl traits as we can, no?

@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 28, 2023
@dtolnay dtolnay assigned dtolnay and unassigned m-ou-se Sep 28, 2023
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

This precludes the addition of an API that transforms &RefMut to Ref, so we need to choose what we want.

I'd be interested in seeing a prototype of what a sound API for this might look like, to better understand what options we are closing off.

Is it fn(&'short RefMut<'long, T>) -> Ref<'short, T>? I think I can see why that would be sound. The original RefMut remains borrowed for 'short, ensuring it cannot be used to mutate the T, consequently ensuring that no mutation of T is possible during 'short so having other Ref is okay.

And indeed, that would make it problematic to have RefMut<T>: Sync because one could send &RefMut<T> to 2 different threads, each of which performs the conversion to Ref, racing on the reference count.

@ChayimFriedman2
Copy link
Contributor Author

@dtolnay Yes, that is what I had in mind.

@dtolnay
Copy link
Member

dtolnay commented Sep 28, 2023

@rust-lang/libs-api:
@rfcbot fcp close

Given that there is no known use case benefiting from RefMut<T>: Sync, keeping options open for the API described in #115157 (review) seems more valuable.

Workaround: send the &T instead of &RefMut<T>.

@rfcbot
Copy link

rfcbot commented Sep 28, 2023

Team member @dtolnay has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Sep 28, 2023
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 5, 2023
@rfcbot
Copy link

rfcbot commented Oct 5, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 15, 2023
@rfcbot
Copy link

rfcbot commented Oct 15, 2023

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@dtolnay dtolnay closed this Oct 15, 2023
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks anyway for the PR!

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 19, 2023
@ChayimFriedman2 ChayimFriedman2 deleted the refmut-sync-1 branch August 4, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants