The trait should be redesigned to be a better fit for intended use cases

[jonas-schievink]

After publishing the initial version, I noticed a few things:

• The trait was, for some reason, designed to be implemented for references. This is very confusing and almost no other trait does this. I believe the original motivation was to make it harder to cause deadlocks, but implementations will usually be for &SomeMutex, which means you can still deadlock. This also makes the lock method hard to call.
• The trait confuses the "interior mutability" aspect of mutexes with the "making Send data Sync" aspect, leading to expensive implementations (eg. Added arch specific mutex implementation cortex-m#209). Since we already have plenty of data structures like cells for interior mutability, it is more important to cover the "making Send data Sync" part.

I'm not yet sure how the trait should look, and what it would take to make it still useful for RTFM etc., but I think the current design is not optimal.

[korken89]

I think this should be done as an update to the RFC (https://github.com/rust-embedded/wg/blob/master/rfcs/0377-mutex-trait.md) to get proper frame of mind and comparison.

[PTaylor-us]

The trait confuses the "interior mutability" aspect of mutexes with the "making Send data Sync" aspect,

@jonas-schievink I don't quite understand what you mean here. I feel that the trait should support both interior mutability as well as Send and Sync. Is that what you're saying as well?

[jonas-schievink]

@PTaylor-FluenTech I'm not really sure yet. The issue is that the trait doesn't allow implementations without interior mutability. I don't know if that's a problem, as I haven't yet looked into the use cases this trait is supposed to enable.

[PTaylor-us]

@jonas-schievink I may be using the wrong terminology. I think the fact that a mutable borrow is required means the mutex is not providing interior mutability of the data it is holding.

[jonas-schievink]

Since the trait has to be implemented for &Mutex, not Mutex, the &mut self doesn't actually do much there. Also see rust-embedded/cortex-m#209 for why some interior mutability is necessary.

(yeah, this is fairly confusing)

[PTaylor-us]

Yeah, I think the necessary implementation of the trait for a reference seems to be a source of much difficulty. I know it was for me. So is the solution to make lock() use &self and be able to implement the trait on non-reference implementations? It's my understanding that the &mut self is what makes it necessary to implement for &Mutex. The RFC states that the reasoning for using &mut self was to allow for deadlock-free-by-construction (failed compilation if a deadlock is attempted). However there was a comment in the threads admitting that this form isn't necessary helpful in preventing deadlocks (I believe because the implementations are then for &Mutex which bypasses that deadlock-free-by-construction feature) (rust-embedded/wg#395 (comment)). Are we paying a steep cost for marginal benefit? Are these issues related to your issues with the current design?

[jonas-schievink]

Are these issues related to your issues with the current design?

Indeed, they're part of it. Not sure how to solve it yet (do we need 2 traits? are use cases that even benefit from a trait as opposed to inherent methods fine with an implicit RefCell?)

[Rahix]

Just as an example, this is the trait I've settled on in shared-bus:

pub trait BusMutex {
    type Bus;

    fn create(v: Self::Bus) -> Self;
    fn lock<R, F: FnOnce(&mut Self::Bus) -> R>(&self, f: F) -> R;
}

(The create() methods should probably be separate for a generic trait, as discussed previously)