Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce RefCell::{try_replace, try_replace_with, try_swap} #132011

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daboross
Copy link
Contributor

This adds Result-based equivalents to RefCell::replace, RefCell::replace_with and RefCell::swap.

This was proposed in #54493, and has not gone through an ACP or RFC process.

Concerns

I believe the design of these functions should be relatively uncontroversial, but their presence at all may be. It's unclear how much these would be used, and they do clutter the documentation of RefCell. The main purpose is completeness: offering a Result-based method for every panicking RefCell method.

Looking at history, RFC-2057 (PR, rendered), which added replace and swap, simply didn't address the possibility of try_replace and try_swap. The author dismissed them as not very useful.

Error types

#54493 proposed adding unique error types for each function. I've opted instead to reuse BorrowMutError, matching try_borrow_unguarded reusing BorrowError.

Process?

As I haven't gone through the rust-lang/rust PR process much yet, I'd love guidance on what to do with this. I figured writing this PR couldn't hurt, but it's unclear whether an ACP or RFC would be best here.

The prior additions of RefCell::{try_borrow, try_borrow_mut} and RefCell::{swap, replace} both used RFCs, but it's unclear to me if things have changed since then.

@rustbot
Copy link
Collaborator

rustbot commented Oct 21, 2024

r? @scottmcm

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

@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 Oct 21, 2024
@rust-log-analyzer

This comment has been minimized.

@daboross daboross force-pushed the refcell_try_replace_replace_with_swap branch from 13f5676 to 344aa2a Compare October 21, 2024 21:25
@scottmcm scottmcm added the needs-acp This change is blocked on the author creating an ACP. label Oct 22, 2024
@scottmcm
Copy link
Member

These days you don't need full RFCs just to add methods; and ACP is the right choice.

(Library RFCs are more for traits that lots of the ecosystem should implement, for example, or large additions of new functionality that set new precedents or scopes.)

@daboross
Copy link
Contributor Author

Great, makes sense!

I'll make an ACP issue now & link it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-acp This change is blocked on the author creating an ACP. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants