Deny-by-default is too strong for `arc_with_non_send_sync`

[Aaron1011]

Summary

The recently added arc_with_non_send_sync is deny-by-default. The description claims that "Wrapping a type in Arc doesn’t add thread safety to the underlying data, so data races could occur when touching the underlying data.". However, this is only true in the presence of unsafe code - when this lint fires, the Arc itself will be !Send/!Sync, preventing it from being shared between threads by safe code (and sound unsafe code).

This lint fired several times in my project - while it's a legitimate lint, I think warn-by-default would be a more reasonable setting.

Reproducer

I tried this code:

use std::cell::Cell;
use std::sync::Arc;

fn main() {
    let _a = Arc::new(Cell::new(0));
}

I expected to see this happen:

A warn-by-default lint.

Instead, this happened:

error: usage of `Arc<T>` where `T` is not `Send` or `Sync`
 --> src/main.rs:5:14
  |
5 |     let _a = Arc::new(Cell::new(0));
  |              ^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: consider using `Rc<T>` instead or wrapping `T` in a std::sync type like `Mutex<T>`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync
  = note: `#[deny(clippy::arc_with_non_send_sync)]` on by default

Version

rustc 1.72.0-nightly (839e9a6e1 2023-07-02)
binary: rustc
commit-hash: 839e9a6e1210934fd24b15548b811a97c77138fc
commit-date: 2023-07-02
host: x86_64-unknown-linux-gnu
release: 1.72.0-nightly
LLVM version: 16.0.5

Additional Labels

No response

[Centri3]

The description is a bit* misleading (*very), the code is useless. Arc::new with a !Send or !Sync should be the same as Rc::new, afaik.

The lint is a bit of "completely useless code" and "perf"; The code is useless, and slower than using Rc for absolutely no reason.

The description should be updated at some point.

[Aaron1011]

That's true, but I don't think that should be considered a correctness issue.

[kpreid]

Another factor that might be relevant to choosing the right message and category: the intent might be that the inner type is Sync, but a bug caused it not to be.

In that case, you want to know why the inner type is not Sync. The compiler's errors on an unsatisfied Sync bound will give you a trace of that, but this lint currently doesn't, so getting this lint error is less helpful than continuing until hitting an unsatisfied bound.

[dhardy]

Consider a simple wrapper like this:

pub struct MyMaybeSharable<T>(Arc<Mutex<T>>);
impl<T> MyMaybeSharable<T> {
    pub fn new(value: T) -> Self {
        MyMaybeSharable(Arc::new(Mutex::new(value)))
    }
}

With T: Send the whole will implement Send + Sync. Without T: Send it implements neither, so it's not unsafe.

Maybe the wrapper should have a T: Send bound to satisfy this lint? Counterpoint: some widely-usable component of a library where the value may or may not cross a thread boundary. Using Rc<RefCell<T>> is more efficient, but using Arc<Mutex<T>> is still valid code.

Since the code is valid and not unsafe and is triggered by generic code which may not even be used sub-optimally in practice, this lint should not even warn by default.

[kpreid]

@dhardy The case of a generic type inside the Mutex is #11076 which has already been fixed by #11077 — it should no longer warn.

[kpreid]

The denying version of this lint made it into 1.72.0-beta.1 (7ba605cd9 2023-07-11). Since #11077 is beta-nominated, I think it makes sense for this PR to be too.

@rustbot label +beta-nominated

[jfgoog]

I agree that this fix needs to go into 1.72.0.

I am testing the 1.72 beta for the Android Rust toolchain, and we have some code that uses Arc<tokio::sync::Mutex<T>> that erroneously triggers this error.

The Tokio docs even specifically mention Arc<Mutex<T>>, so I suspect we aren't the only ones who will encounter this problem:

https://github.com/tokio-rs/tokio/blob/a7d52c2fede5ebd8f6e49d52a4af77138e0bd6e1/tokio/src/sync/mutex.rs

[flip1995]

Removing beta-nominated from the issue, because both PRs addressing this are already labeled:

• arc_with_non_send_sync: reword and move to suspicious #11104
• [arc_with_non_send_sync]: don't lint if type has nested type parameters #11077