Consider removing mutex_atomic

[carllerche]

There are many reasons to use mutexes over atomics. Clippy is unable to correctly determine if an atomic is sufficient to replace the mutex use. Attempting to lint over could lead to pushing users to introduce bugs in their code.

I recommend removing this lint.

[Manishearth]

I feel like this lint is correct most of the time; and clippy lints are supposed to be used with a liberal sprinkling of allow(). There needs to be stronger, more detailed justification for removing a lint.

[carllerche]

My main point is the potential harm of taking action based on a false positive is high compared to other provided lints.

[carllerche]

(I can't comment on the rate of false positive besides saying I have only experienced a 100% false positive rate 😄 ).

[Manishearth]

I disagree, lints aren't as strong signals as compiler warnings, and Clippy's warnings are meant to be taken with a grain of salt. Here the signal is "there might be an issue", if you determine it isn't an issue, allow() it.

If you really think there's a problem here a stronger case needs to be made, with lots of examples.

[Manishearth]

I have only experienced a 100% false positive rate

Like I said, clippy is supposed to be used with a liberal sprinkling of allow(). Some lints just don't make sense for some codebases. It's not possible for our lints to be totally globally applicable. It's up to the codebase to make this judgement.

[carllerche]

Ok, feel free to close if you disagree.

[Manishearth]

(leaving open since other maintainers, and other people, may have different opinions)

[flip1995]

The lint mentions in its Known problems section that

This lint cannot detect if the mutex is actually used for waiting before a critical section.

I'm with @Manishearth here. I think in the general case this lint is correct and Atomics are sufficient. If you really need the mutex you should just allow the lint.

Removing this lint would require to show (with examples), that you need a Mutex in the general case.

[thombles]

I want to add a +1 since I ran into this reviewing a PR the other day where the submitter defaulted to using Relaxed. Replacing Mutex<bool> with AtomicBool like it shows in the example is easy, but now you need to reason correctly about Ordering. Speaking only for myself, I've read about three articles about memory ordering and I still don't know which combination of Acquire and Release to use in a given situation. I happen to know that using SeqCst everywhere is safe, maybe because I read that on stack overflow once. That's a whole lot of homework that goes unmentioned and if you don't follow through on it you might introduce heisenbugs.

I'm quite happy to use the allow feature as noted above but I feel like the lint is a footgun to begin with. Those who know how to use atomics correctly will notice this potential performance issue the moment they see it.

[taiki-e]

Filed #8260 to downgrade this lint to nursery group that allowed by default.

Seems that Carl originally proposed to remove this lint completely, but the maintainers rejected it, so I believe downgrading is a reasonable compromise for now.

[Noratrieb]

I agree that this lint is bad and should be removed. Atomics are very complicated and very easy to get wrong. Mutexes on the other hand are simple and a lot harder to get wrong. People who don't think of using an atomic in the first place shouldn't be directed to using an atomic instead, the mutex will be fine.

This like feels a little like if clippy suggested replacing safe but a little more expensive type casts with transmute. Yes, it's faster and often works just as well, but it has a higher maintenance burden and more risk of getting it wrong.

We want people to write correct code, not the fastest code imaginable. And if someone knows atomics well enough to use them, they will probably not need this lint in the first place.

[chorman0773]

I highly recommending removing this lint. Under complicated code, a person not knowing better would be directed to (incorrectly) use an atomic rather than a mutex, and bugs caused by this may be subtle and occur rarely in practice and may go undiscovered for some time, particularily if the code is rarely or never tested off of x86.
Even if the Mutex in the code could, in theory, be replaced with an atomic, doing so requires careful consideration and careful work, especially if an ordering stricter than Relaxed is needed.

Mutex<i32> and AtomicI32 can be (and are) used in substantially different ways.

I think in the general case this lint is correct and Atomics are sufficient. If you really need the mutex you should just allow the lint.

The problem is that people who would recognize when they do and do not need a mutex would also be able to recognize when they should use an atomic vs. when they should use a mutex. The lint will hit two groups of people:

• People who know what they are doing and have deliberately chosen to use a Mutex and thus will allow the lint, or
• People who do not, used Mutex as a default, and would not know whether or not they can replace Mutex with an atomic, and may do so without realising that their code is now broken or unsound.
  The first group of people don't need this lint, and the second group of people may be actively harmed by the lint.

[rdrpenguin04]

It seems this is universally agreed upon that this lint is bad. Should someone make a PR to move this to restriction or something like that?

[llogiq]

I'm actually not sure if restriction is the right group here. Maybe put it into leave it in nursery instead and also add a comment to sum up this discussion to the lint docs.

[taiki-e]

Well, this lint is already in nursery since #8260.

[rdrpenguin04]

The reason I feel nursery isn't a good fit is because, in general, the lints in nursery at least have potential to do the correct thing if the lint is finished; it's a "nursery" for future lints. In this case, the lint seems to be unhelpful in general, and the only change I see of it being useful is as a restriction. My own opinion though.

[ruuda]

This lint is also counter-productive advice when the Mutex<bool> is used with std::sync::Condvar::wait and variations, because Condvar::wait needs the mutex guard and can’t work with an AtomicBool. In fact, the example in the Condvar documentation for its canonical usage relies on Mutex<bool>. This was already raised separately in #1516, but I thought I’d highlight it here, because the lint’s docs link to this issue in particular.