From 8c526eefd382ed72e3d984ddda087385906a8d91 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 30 Jun 2024 15:22:39 +0200 Subject: [PATCH] Move interior mutability into ProcMacroSrvProcess --- .../crates/proc-macro-api/src/lib.rs | 37 +++++-------------- .../crates/proc-macro-api/src/process.rs | 37 +++++++++++-------- 2 files changed, 31 insertions(+), 43 deletions(-) diff --git a/src/tools/rust-analyzer/crates/proc-macro-api/src/lib.rs b/src/tools/rust-analyzer/crates/proc-macro-api/src/lib.rs index 0de76689ab09..c64ec77487d9 100644 --- a/src/tools/rust-analyzer/crates/proc-macro-api/src/lib.rs +++ b/src/tools/rust-analyzer/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>, + process: Arc, 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>, + process: Arc, 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>, } @@ -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, ) -> io::Result { 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, 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, 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) => { diff --git a/src/tools/rust-analyzer/crates/proc-macro-api/src/process.rs b/src/tools/rust-analyzer/crates/proc-macro-api/src/process.rs index 718a96dc80fe..1c37082bb29c 100644 --- a/src/tools/rust-analyzer/crates/proc-macro-api/src/process.rs +++ b/src/tools/rust-analyzer/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, + version: u32, + mode: SpanMode, +} + +#[derive(Debug)] +struct ProcessSrvState { process: Process, stdin: ChildStdin, stdout: BufReader, /// Populated when the server exits. server_exited: Option, - 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 { + fn version_check(&self) -> Result { let request = Request::ApiVersionCheck {}; let response = self.send_task(request)?; @@ -86,7 +90,7 @@ impl ProcMacroProcessSrv { } } - fn enable_rust_analyzer_spans(&mut self) -> Result { + fn enable_rust_analyzer_spans(&self) -> Result { 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, 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 { - if let Some(server_error) = &self.server_exited { + pub(crate) fn send_task(&self, req: Request) -> Result { + 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,