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

Suggestion to use Arc<T> is too indiscriminate #114687

Closed
dtolnay opened this issue Aug 10, 2023 · 3 comments · Fixed by #115311
Closed

Suggestion to use Arc<T> is too indiscriminate #114687

dtolnay opened this issue Aug 10, 2023 · 3 comments · Fixed by #115311
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented Aug 10, 2023

Code

fn main() {
    let ptr = &false as *const bool;

    std::thread::spawn(move || println!("{:p}", ptr)).join().unwrap();
}

Current output

error[E0277]: `*const bool` cannot be sent between threads safely
 --> src/main.rs:4:24
  |
4 |     std::thread::spawn(move || println!("{:p}", ptr)).join().unwrap();
  |     ------------------ -------^^^^^^^^^^^^^^^^^^^^^^
  |     |                  |
  |     |                  `*const bool` cannot be sent between threads safely
  |     |                  within this `[closure@src/main.rs:4:24: 4:31]`
  |     required by a bound introduced by this call
  |
  = help: within `[closure@src/main.rs:4:24: 4:31]`, the trait `Send` is not implemented for `*const bool`
  = note: consider using `std::sync::Arc<*const bool>`; for more information visit <https://doc.rust-lang.org/book/ch16-03-shared-state.html>
note: required because it's used within this closure
 --> src/main.rs:4:24
  |
4 |     std::thread::spawn(move || println!("{:p}", ptr)).join().unwrap();
  |                        ^^^^^^^
note: required by a bound in `spawn`
 --> /rustc/08d00b40aef2017fe6dba3ff7d6476efa0c10888/library/std/src/thread/mod.rs:680:1

Desired output

No response

Rationale and extra context

Filing to track the hesitation from #88936 (review).

[@nagisa]— Use of Arc is not always applicable, as both Send and Sync impls for Arc<T> require T: Send + Sync. Unless we can verify T bounds here, I don't think this on clause adds any more clarity over the default note?

[@estebank]— Thats fair, but was concerned the default would be too verbose for a case where we could be more straightforward. I can change it.

[@nagisa]— Well, we can always change this later too. r=me if you don't want to change it.

If T is not Send then Arc<T> is also not Send. Suggesting Arc<T> whenever Send is not implemented seems misguided.

Other cases

No response

Anything else?

No response

@dtolnay dtolnay added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 10, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 10, 2023
@dtolnay
Copy link
Member Author

dtolnay commented Aug 10, 2023

The link to https://doc.rust-lang.org/book/ch16-03-shared-state.html is a good addition and will be valuable for beginners. 💯

I think concretely the unhelpfulness is in regard to Arc<{Self}> which is (almost?) never going to be right. Something like Arc<Mutex<{Self}>> is more likely to be the right "silver bullet". I'd prefer not to suggest even that though. I think guiding toward the link and contextualizing with something like "consider whether an Arc could be incorporated to share this value between threads" would be most appropriate.

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 10, 2023
@compiler-errors compiler-errors added D-papercut Diagnostics: An error or lint that needs small tweaks. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. labels Aug 11, 2023
@reez12g
Copy link
Contributor

reez12g commented Aug 12, 2023

@rustbot claim

@m-ou-se
Copy link
Member

m-ou-se commented Aug 23, 2023

Suggesting Arc<T> for missing Send or Sync on T is always wrong. Arc<T> is never Send or Sync when T isn't. This message shouldn't be tweaked or changed to appear in fewer cases, but just removed.

An Arc is a tool to fix/avoid lifetimes, not for making anything Send or Sync. (A Mutex can make a Send-but-not-Sync thing Sync, but that also doesn't make a non-Send thing Send.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants