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

LazyLock/Cell get_mut() and DerefMut #429

Closed
ChayimFriedman2 opened this issue Aug 16, 2024 · 5 comments
Closed

LazyLock/Cell get_mut() and DerefMut #429

ChayimFriedman2 opened this issue Aug 16, 2024 · 5 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@ChayimFriedman2
Copy link

ChayimFriedman2 commented Aug 16, 2024

Proposal

Problem statement

When porting rust-analyzer once_cell code to use std's lazy types, one place used &mut LazyCell<T> and wanted to get a &mut T from it (initializing if not initialized yet). once_cell provides DerefMut and get_mut() for that on its Lazy types, but std has no way to mutably access the contents of a LazyLock/Cell.

Motivating examples or use cases

The code I ported is here: https://github.com/rust-lang/rust-analyzer/blob/91aa3f46b32032d7d62c4e94e4ea973f63aacc8f/crates/hir-def/src/generics.rs#L600

That code could use Option (this is what I transformed it into), but that means having to carry around the initializer every time. Alternatively, there could be a LazyMut type, but the role seems to fit LazyCell/Lock perfectly and there is even no need for perf compromises. Also, even though I don't have a use-case for that, maybe someone need to access a Lazy sometimes mutably and sometimes immutably.

Solution sketch

/// There is no need for locking or reentrancy handling; this can be as simple as a function call.
impl<T> DerefMut for LazyCell<T> { ... }
impl LazyCell {
    pub fn get_mut(this: &mut Self) -> Option<&mut T>;
}

impl<T> DerefMut for LazyLock<T> { ... }
impl LazyLock {
    pub fn get_mut(this: &mut Self) -> Option<&mut T>;
}

Alternatives

We can provide only get_mut() or only DerefMut. I choose to provide both because DerefMut is easier to work with, but get_mut() is a standard in interior mutable types.

This could be emulated using Option, as I said.

Links and related work

https://docs.rs/once_cell/latest/once_cell/sync/struct.Lazy.html#method.get_mut
https://docs.rs/once_cell/latest/once_cell/sync/struct.Lazy.html#impl-DerefMut-for-Lazy%3CT,+F%3E
https://docs.rs/once_cell/latest/once_cell/unsync/struct.Lazy.html#method.get_mut
https://docs.rs/once_cell/latest/once_cell/unsync/struct.Lazy.html#impl-DerefMut-for-Lazy%3CT,+F%3E

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@ChayimFriedman2 ChayimFriedman2 added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Aug 16, 2024
@kennytm
Copy link
Member

kennytm commented Aug 16, 2024

If you're going to add the inherent method it should take this: &mut Self rather than &mut self.

The name should be force_mut to be consistent with the existing force method (and avoid confusing with the variant that returns an Option/Result)

@ChayimFriedman2
Copy link
Author

@kennytm Ah yes, you're right about &mut self.

About the name... force_mut() is consistent with force(), but get_mut() is consistent with other interior mutability containers. Personally I lean towards get_mut(), especially given that once_cell has chosen that name.

@kennytm
Copy link
Member

kennytm commented Aug 16, 2024

In other interior-mutable containers their get_mut() method are won't generate a value from scratch or mutate the container's state itself, unlike this proposal. This proposal requires executing the initializer if necessary so it is not trivial enough to use the get name. (That's also why once_lock::Lazy::get_mut returns an Option)

Containers get_mut()'s return type into_inner()'s return type Note
Arc, Rc Option<&mut T> Option<T> Returns None if !self.is_unique()
Mutex, RwLock LockResult<&mut T> LockResult<T> Returns Err if self.is_poisoned()
OnceLock, OnceCell Option<&mut T> Option<T> Returns None if !self.is_initialized()
Exclusive, ReentrantLock, Cell, RefCell, UnsafeCell, SyncUnsafeCell, AtomicT &mut T T -
once_cell::*::Lazy Option<&mut T> Result<T, F>, but method name is into_value() Returns None/Err if not yet initialized

If you add a get_mut() method it should

  1. not invoke the initializer
  2. have the return type somewhat consistent with .into_inner() (currently Tracking Issue for lazy_cell_into_inner rust#125623 returns Result<T, F>)

@ChayimFriedman2
Copy link
Author

@kennytm Oh! I didn't see that in once_cell it returns Option. Embarrassing. Edited the proposal.

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/libs-api meeting, and approved these. We'd like to have these, and we'd also like to have force_mut (similar to the existing force but returning &mut), and get (similar to get_mut but returning &T).

@joshtriplett joshtriplett added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

3 participants