Implement From<MutexGuard<'a, T>> for &'a Mutex<T>

[rubcc95]

Current Problem

Consider the following common pattern:

fn execute(items: impl Iterator<Item = Example>){
    let mutex: Mutex<usize> =  Mutex::new(0);
    let mut guard = mutex.lock().unwrap();
    for item in items {
        guard = item.execute_locked(&mutex, guard);
    }
}

struct Example;

impl Example {
    fn execute_locked<'a, T>(&self, mutex: &'a Mutex<T>, guard: MutexGuard<'a, T>) -> MutexGuard<'a, T> {
        drop(guard);
        //do something unlocked...
        mutex.lock().unwrap()
    }
}

Current patterns for working with mutex locks require passing both the mutex and its guard between methods, which leads to verbose code. This proposal aims to simplify and optimize mutex handling by allowing direct conversion from a MutexGuard to a reference to its original Mutex. The MutexGuard already contains an immutable reference to the Mutex. Moreover, since the MutexGuard is consumed when delivering the immutable reference to the Mutex, this feature does not incentivize the occurrence of deadlocks.

Proposed Solution

Implement From trait to allow direct conversion from MutexGuard to &Mutex:

impl<'a, T> From<MutexGuard<'a, T>> for &'a Mutex<T> {
    fn from(value: MutexGuard<'a, T>) -> Self {
        // Implementation leveraging internal lock field
    }
}

After implementation, the code would simplify to:

fn execute(items: impl Iterator<Item = Example>){
    let mutex: Mutex<usize> =  Mutex::new(0);
    let mut guard = mutex.lock().unwrap();
    for item in items {
        guard = item.execute_locked(guard);
    }
}

struct Example;

impl Example {
    fn execute_locked<'a, T>(&self, guard: MutexGuard<'a, T>) -> MutexGuard<'a, T> {
        drop(guard);
        // Access original mutex via From trait if needed
        let mutex: &Mutex<T> = guard.into();
        
        //do something unlocked...
        mutex.lock().unwrap()
    }
}

Benefits

1. Reduced method signature complexity
2. Less boilerplate code
3. More intuitive mutex handling
4. Maintains existing safety guarantees

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thomcc (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[joboet]

This makes sense to me, let's see what T-libs-api has to say about it.
r? libs-api

[rust-log-analyzer]

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#17 exporting to docker image format
#17 sending tarball 27.8s done
#17 DONE 33.2s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
   Compiling addr2line v0.22.0
error: implementation has missing stability attribute
   --> library/std/src/sync/mutex.rs:503:1
    |
503 | / impl<'a, T> From<MutexGuard<'a, T>> for &'a Mutex<T>{
504 | |     fn from(value: MutexGuard<'a, T>) -> Self{
505 | |         value.lock
507 | | }
    | |_^

error: could not compile `std` (lib) due to 1 previous error

[zachs18]

This seems useful. It might make more sense to have an associated function on MutexGuard like lock_api does, rather than a From impl.

[zopsicle]

This seems useful. It might make more sense to have an associated function on MutexGuard like lock_api does, rather than a From impl.

To elaborate on that: MutexGuard represents a locked mutex but the function returns an unlocked mutex. This IMO violates From's value preservation principle:

The conversion is value-preserving: the conceptual kind and meaning of the resulting value is the same.

Perhaps it should be called unlock.

[tgross35]

Given the discussed alternatives here, it is probably best for you to file an ACP. @rubcc95 could you do this? It is an issue template at https://github.com/rust-lang/libs-team/issues.

[rubcc95]

@tgross35 Of course. I'm on mobile, so I’ll do it tomorrow. As a side note, I take this opportunity to learn from your knowledge. I’m a self-taught programmer, and sometimes these community aspects seem a bit cumbersome to me, as I’m just starting with them... Would the correct approach in this situation have been to open a pull request directly instead of posting here?"

[tgross35]

You'll learn about the processes as you go :) There are some details here https://std-dev-guide.rust-lang.org/development/feature-lifecycle.html. The gist is that all new API needs to get approved by the libs-api team. Opening a PR and then requesting libs-api review is one way to do this and is usually okay for minimal changes. The other way is with an ACP, which is a better platform to discuss new API because (1) people are looking for API proposals there and can chime in, and (2) it avoids getting into the details of how something is implemented. Also an ACP is a good idea for bigger changes so you don't spend time implementing something that will need to change or may wind up rejected.

So there was nothing wrong with opening this PR for a simple change. But since a couple alternatives have come up, might as well move discussion to a dedicated place for feedback.

[bors]

☔ The latest upstream changes (presumably #134692) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@rubcc95 any updates on the ACP? if you have opened one already kindly link it in this issue (preferably in the issue description). Thanks