rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.3k stars 12.72k forks source link

Suggest use self instead of &self when involving threads safely errors #118096

Open chenyukang opened 11 months ago

chenyukang commented 11 months ago

Code

use std::collections::VecDeque;
use std::sync::mpsc::Receiver;
use std::sync::Arc;
use std::sync::Mutex;
use std::thread;
use std::thread::JoinHandle;
pub trait Migration: Send + Sync {}

enum Command {
    Start,
    //Stop,
}

type MigrationTasks = VecDeque<(String, Arc<dyn Migration>)>;
struct MigrationWorker {
    tasks: Arc<Mutex<MigrationTasks>>,
    inbox: Receiver<Command>,
}

impl MigrationWorker {
    pub fn new(tasks: Arc<Mutex<MigrationTasks>>, inbox: Receiver<Command>) -> Self {
        Self { tasks, inbox }
    }

    pub fn start(&self) -> JoinHandle<()> {
        thread::spawn(move || {
            let msg = match self.inbox.recv() {
                Ok(msg) => Some(msg),
                Err(_err) => return,
            };

            if let Some(Command::Start) = msg {
                while let Some((name, _task)) = self.tasks.lock().unwrap().pop_front() {
                    eprintln!("start to run migrate in background: {}", name);
                }
            }
        })
    }
}

fn main() {
    let (tx, rx) = std::sync::mpsc::channel();
    let worker = MigrationWorker::new(Arc::new(Mutex::new(VecDeque::new())), rx);
    tx.send(Command::Start).unwrap();
    worker.start();
}

Current output

error[E0277]: `std::sync::mpsc::Receiver<Command>` cannot be shared between threads safely
   --> src/main.rs:26:23
    |
26  |           thread::spawn(move || {
    |  _________-------------_^
    | |         |
    | |         required by a bound introduced by this call
27  | |             let msg = match self.inbox.recv() {
28  | |                 Ok(msg) => Some(msg),
29  | |                 Err(_err) => return,
...   |
36  | |             }
37  | |         })
    | |_________^ `std::sync::mpsc::Receiver<Command>` cannot be shared between threads safely
    |
    = help: within `MigrationWorker`, the trait `Sync` is not implemented for `std::sync::mpsc::Receiver<Command>`
note: required because it appears within the type `MigrationWorker`
   --> src/main.rs:15:8
    |
15  | struct MigrationWorker {
    |        ^^^^^^^^^^^^^^^
    = note: required for `&MigrationWorker` to implement `Send`
note: required because it's used within this closure
   --> src/main.rs:26:23
    |
26  |         thread::spawn(move || {
    |                       ^^^^^^^
note: required by a bound in `spawn`
   --> /Users/yukang/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/thread/mod.rs:683:8
    |
680 | pub fn spawn<F, T>(f: F) -> JoinHandle<T>
    |        ----- required by a bound in this function
...
683 |     F: Send + 'static,
    |        ^^^^ required by this bound in `spawn`

For more information about this error, try `rustc --explain E0277`.

Desired output

Suggest changing `&self` to `self`

    pub fn start(self) -> JoinHandle<()> {

Rationale and extra context

This may be a relatively easy mistake to make, and the current error message does not seem to help beginners solve the problem very well.

Other cases

No response

Anything else?

When there is more code in fn start(e.g, we change self to &self in this line: https://github.com/nervosnetwork/ckb/blob/c77a927c1c44b5969b722c731f774c22ab194ff2/db-migration/src/lib.rs#L55), there may be this error messages, again, it's better if we could suggesting how to fix the code.

error[E0521]: borrowed data escapes outside of method
  --> db-migration/src/lib.rs:56:9
   |
55 |       pub fn start(&self) -> JoinHandle<()> {
   |                    -----
   |                    |
   |                    `self` is a reference that is only valid in the method body
   |                    let's call the lifetime of this reference `'1`
56 | /         thread::spawn(move || {
57 | |             let msg = match self.inbox.recv() {
58 | |                 Ok(msg) => Some(msg),
59 | |                 Err(_err) => return,
...  |
84 | |             }
85 | |         })
   | |          ^
   | |          |
   | |__________`self` escapes the method body here
   |            argument requires that `'1` must outlive `'static`
compiler-errors commented 11 months ago

I think in general, suggesting to change the signature is probably not a super great idea, unless we have very strong signal that it's going to be the right choice. For example, we need to at least check that the receiver is Sync, or else even &self is wrong.

But even then, I do think that after users apply the suggestion, they'll almost surely hit more borrow-checker errors. People don't typically have fn(self) when fn(&self) could work instead -- they're usually consuming some part of Self.