Move interior mutability into ProcMacroSrvProcess

crates/proc-macro-api/src/lib.rs

@@ -15,10 +15,8 @@ use indexmap::IndexSet;
 use paths::{AbsPath, AbsPathBuf};
 use rustc_hash::FxHashMap;
 use span::Span;
-use std::{
-    fmt, io,
-    sync::{Arc, Mutex},
-};
+use std::{fmt, io, sync::Arc};
+use tt::SmolStr;
 
 use serde::{Deserialize, Serialize};
 
@@ -48,9 +46,7 @@ pub struct ProcMacroServer {
     ///
     /// That means that concurrent salsa requests may block each other when expanding proc macros,
     /// which is unfortunate, but simple and good enough for the time being.
-    ///
-    /// Therefore, we just wrap the `ProcMacroProcessSrv` in a mutex here.
-    process: Arc<Mutex<ProcMacroProcessSrv>>,
+    process: Arc<ProcMacroProcessSrv>,
     path: AbsPathBuf,
 }
 
@@ -70,9 +66,9 @@ impl MacroDylib {
 /// we share a single expander process for all macros.
 #[derive(Debug, Clone)]
 pub struct ProcMacro {
-    process: Arc<Mutex<ProcMacroProcessSrv>>,
+    process: Arc<ProcMacroProcessSrv>,
     dylib_path: AbsPathBuf,
-    name: String,
+    name: SmolStr,
     kind: ProcMacroKind,
 }
 
@@ -89,7 +85,6 @@ impl PartialEq for ProcMacro {
 #[derive(Clone, Debug)]
 pub struct ServerError {
     pub message: String,
-    // io::Error isn't Clone for some reason
     pub io: Option<Arc<io::Error>>,
 }
 
@@ -104,10 +99,6 @@ impl fmt::Display for ServerError {
     }
 }
 
-pub struct MacroPanic {
-    pub message: String,
-}
-
 impl ProcMacroServer {
     /// Spawns an external process as the proc macro server and returns a client connected to it.
     pub fn spawn(
@@ -115,10 +106,7 @@ impl ProcMacroServer {
         env: &FxHashMap<String, String>,
     ) -> io::Result<ProcMacroServer> {
         let process = ProcMacroProcessSrv::run(process_path, env)?;
-        Ok(ProcMacroServer {
-            process: Arc::new(Mutex::new(process)),
-            path: process_path.to_owned(),
-        })
+        Ok(ProcMacroServer { process: Arc::new(process), path: process_path.to_owned() })
     }
 
     pub fn path(&self) -> &AbsPath {
@@ -127,15 +115,14 @@ impl ProcMacroServer {
 
     pub fn load_dylib(&self, dylib: MacroDylib) -> Result<Vec<ProcMacro>, ServerError> {
         let _p = tracing::info_span!("ProcMacroServer::load_dylib").entered();
-        let macros =
-            self.process.lock().unwrap_or_else(|e| e.into_inner()).find_proc_macros(&dylib.path)?;
+        let macros = self.process.find_proc_macros(&dylib.path)?;
 
         match macros {
             Ok(macros) => Ok(macros
                 .into_iter()
                 .map(|(name, kind)| ProcMacro {
                     process: self.process.clone(),
-                    name,
+                    name: name.into(),
                     kind,
                     dylib_path: dylib.path.clone(),
                 })
@@ -163,7 +150,7 @@ impl ProcMacro {
         call_site: Span,
         mixed_site: Span,
     ) -> Result<Result<tt::Subtree<Span>, PanicMessage>, ServerError> {
-        let version = self.process.lock().unwrap_or_else(|e| e.into_inner()).version();
+        let version = self.process.version();
         let current_dir = env.get("CARGO_MANIFEST_DIR");
 
         let mut span_data_table = IndexSet::default();
@@ -190,11 +177,7 @@ impl ProcMacro {
             },
         };
 
-        let response = self
-            .process
-            .lock()
-            .unwrap_or_else(|e| e.into_inner())
-            .send_task(msg::Request::ExpandMacro(Box::new(task)))?;
+        let response = self.process.send_task(msg::Request::ExpandMacro(Box::new(task)))?;
 
         match response {
             msg::Response::ExpandMacro(it) => {

crates/proc-macro-api/src/process.rs

@@ -3,7 +3,7 @@
 use std::{
     io::{self, BufRead, BufReader, Read, Write},
     process::{Child, ChildStdin, ChildStdout, Command, Stdio},
-    sync::Arc,
+    sync::{Arc, Mutex},
 };
 
 use paths::AbsPath;
@@ -17,13 +17,20 @@ use crate::{
 
 #[derive(Debug)]
 pub(crate) struct ProcMacroProcessSrv {
+    /// The state of the proc-macro server process, the protocol is currently strictly sequential
+    /// hence the lock on the state.
+    state: Mutex<ProcessSrvState>,
+    version: u32,
+    mode: SpanMode,
+}
+
+#[derive(Debug)]
+struct ProcessSrvState {
     process: Process,
     stdin: ChildStdin,
     stdout: BufReader<ChildStdout>,
     /// Populated when the server exits.
     server_exited: Option<ServerError>,
-    version: u32,
-    mode: SpanMode,
 }
 
 impl ProcMacroProcessSrv {
@@ -36,10 +43,7 @@ impl ProcMacroProcessSrv {
             let (stdin, stdout) = process.stdio().expect("couldn't access child stdio");
 
             io::Result::Ok(ProcMacroProcessSrv {
-                process,
-                stdin,
-                stdout,
-                server_exited: None,
+                state: Mutex::new(ProcessSrvState { process, stdin, stdout, server_exited: None }),
                 version: 0,
                 mode: SpanMode::Id,
             })
@@ -76,7 +80,7 @@ impl ProcMacroProcessSrv {
         self.version
     }
 
-    pub(crate) fn version_check(&mut self) -> Result<u32, ServerError> {
+    fn version_check(&self) -> Result<u32, ServerError> {
         let request = Request::ApiVersionCheck {};
         let response = self.send_task(request)?;
 
@@ -86,7 +90,7 @@ impl ProcMacroProcessSrv {
         }
     }
 
-    fn enable_rust_analyzer_spans(&mut self) -> Result<SpanMode, ServerError> {
+    fn enable_rust_analyzer_spans(&self) -> Result<SpanMode, ServerError> {
         let request = Request::SetConfig(crate::msg::ServerConfig {
             span_mode: crate::msg::SpanMode::RustAnalyzer,
         });
@@ -99,7 +103,7 @@ impl ProcMacroProcessSrv {
     }
 
     pub(crate) fn find_proc_macros(
-        &mut self,
+        &self,
         dylib_path: &AbsPath,
     ) -> Result<Result<Vec<(String, ProcMacroKind)>, String>, ServerError> {
         let request = Request::ListMacros { dylib_path: dylib_path.to_path_buf().into() };
@@ -112,20 +116,21 @@ impl ProcMacroProcessSrv {
         }
     }
 
-    pub(crate) fn send_task(&mut self, req: Request) -> Result<Response, ServerError> {
-        if let Some(server_error) = &self.server_exited {
+    pub(crate) fn send_task(&self, req: Request) -> Result<Response, ServerError> {
+        let state = &mut *self.state.lock().unwrap();
+        if let Some(server_error) = &state.server_exited {
             return Err(server_error.clone());
         }
 
         let mut buf = String::new();
-        send_request(&mut self.stdin, &mut self.stdout, req, &mut buf).map_err(|e| {
+        send_request(&mut state.stdin, &mut state.stdout, req, &mut buf).map_err(|e| {
             if e.io.as_ref().map(|it| it.kind()) == Some(io::ErrorKind::BrokenPipe) {
-                match self.process.child.try_wait() {
+                match state.process.child.try_wait() {
                     Ok(None) => e,
                     Ok(Some(status)) => {
                         let mut msg = String::new();
                         if !status.success() {
-                            if let Some(stderr) = self.process.child.stderr.as_mut() {
+                            if let Some(stderr) = state.process.child.stderr.as_mut() {
                                 _ = stderr.read_to_string(&mut msg);
                             }
                         }
@@ -133,7 +138,7 @@ impl ProcMacroProcessSrv {
                             message: format!("server exited with {status}: {msg}"),
                             io: None,
                         };
-                        self.server_exited = Some(server_error.clone());
+                        state.server_exited = Some(server_error.clone());
                         server_error
                     }
                     Err(_) => e,