Repository: rust-lang/rust-analyzer
Pull request #17059 (2024-04-13 11:32:48)

internal: make function builder create ast directly
I am working on #17050.
In the process, I noticed a place in the code that could be refactored.
Currently, the `function builder` creates the `ast` through the `function template` , but those two processes can be combined into one function.
I thought I should work on this first and created a PR.


Patch:
diff --git a/crates/flycheck/src/command.rs b/crates/flycheck/src/command.rs
index 8ba7018316..091146a001 100644
--- a/crates/flycheck/src/command.rs
+++ b/crates/flycheck/src/command.rs
@@ -4,13 +4,12 @@
 use std::{
     ffi::OsString,
     fmt, io,
-    marker::PhantomData,
     path::PathBuf,
     process::{ChildStderr, ChildStdout, Command, Stdio},
 };
 
 use command_group::{CommandGroup, GroupChild};
-use crossbeam_channel::Sender;
+use crossbeam_channel::{unbounded, Receiver, Sender};
 use stdx::process::streaming_output;
 
 /// Cargo output is structured as a one JSON per line. This trait abstracts parsing one line of
@@ -100,10 +99,10 @@ pub(crate) struct CommandHandle<T> {
     /// a read syscall dropping and therefore terminating the process is our best option.
     child: JodGroupChild,
     thread: stdx::thread::JoinHandle<io::Result<(bool, String)>>,
+    pub(crate) receiver: Receiver<T>,
     program: OsString,
     arguments: Vec<OsString>,
     current_dir: Option<PathBuf>,
-    _phantom: PhantomData<T>,
 }
 
 impl<T> fmt::Debug for CommandHandle<T> {
@@ -117,7 +116,7 @@ impl<T> fmt::Debug for CommandHandle<T> {
 }
 
 impl<T: ParseFromLine> CommandHandle<T> {
-    pub(crate) fn spawn(mut command: Command, sender: Sender<T>) -> std::io::Result<Self> {
+    pub(crate) fn spawn(mut command: Command) -> std::io::Result<Self> {
         command.stdout(Stdio::piped()).stderr(Stdio::piped()).stdin(Stdio::null());
         let mut child = command.group_spawn().map(JodGroupChild)?;
 
@@ -128,12 +127,13 @@ impl<T: ParseFromLine> CommandHandle<T> {
         let stdout = child.0.inner().stdout.take().unwrap();
         let stderr = child.0.inner().stderr.take().unwrap();
 
+        let (sender, receiver) = unbounded();
         let actor = CargoActor::<T>::new(sender, stdout, stderr);
         let thread = stdx::thread::Builder::new(stdx::thread::ThreadIntent::Worker)
             .name("CommandHandle".to_owned())
             .spawn(move || actor.run())
             .expect("failed to spawn thread");
-        Ok(CommandHandle { program, arguments, current_dir, child, thread, _phantom: PhantomData })
+        Ok(CommandHandle { program, arguments, current_dir, child, thread, receiver })
     }
 
     pub(crate) fn cancel(mut self) {
diff --git a/crates/flycheck/src/lib.rs b/crates/flycheck/src/lib.rs
index 5dfaaf7742..f935ec32d5 100644
--- a/crates/flycheck/src/lib.rs
+++ b/crates/flycheck/src/lib.rs
@@ -215,8 +215,6 @@ struct FlycheckActor {
     /// have to wrap sub-processes output handling in a thread and pass messages
     /// back over a channel.
     command_handle: Option<CommandHandle<CargoCheckMessage>>,
-    /// The receiver side of the channel mentioned above.
-    command_receiver: Option<Receiver<CargoCheckMessage>>,
 }
 
 enum Event {
@@ -242,7 +240,6 @@ impl FlycheckActor {
             sysroot_root,
             root: workspace_root,
             command_handle: None,
-            command_receiver: None,
         }
     }
 
@@ -251,13 +248,14 @@ impl FlycheckActor {
     }
 
     fn next_event(&self, inbox: &Receiver<StateChange>) -> Option<Event> {
+        let check_chan = self.command_handle.as_ref().map(|cargo| &cargo.receiver);
         if let Ok(msg) = inbox.try_recv() {
             // give restarts a preference so check outputs don't block a restart or stop
             return Some(Event::RequestStateChange(msg));
         }
         select! {
             recv(inbox) -> msg => msg.ok().map(Event::RequestStateChange),
-            recv(self.command_receiver.as_ref().unwrap_or(&never())) -> msg => Some(Event::CheckEvent(msg.ok())),
+            recv(check_chan.unwrap_or(&never())) -> msg => Some(Event::CheckEvent(msg.ok())),
         }
     }
 
@@ -286,12 +284,10 @@ impl FlycheckActor {
                     let formatted_command = format!("{:?}", command);
 
                     tracing::debug!(?command, "will restart flycheck");
-                    let (sender, receiver) = unbounded();
-                    match CommandHandle::spawn(command, sender) {
+                    match CommandHandle::spawn(command) {
                         Ok(command_handle) => {
                             tracing::debug!(command = formatted_command, "did restart flycheck");
                             self.command_handle = Some(command_handle);
-                            self.command_receiver = Some(receiver);
                             self.report_progress(Progress::DidStart);
                         }
                         Err(error) => {
@@ -307,7 +303,6 @@ impl FlycheckActor {
 
                     // Watcher finished
                     let command_handle = self.command_handle.take().unwrap();
-                    self.command_receiver.take();
                     let formatted_handle = format!("{:?}", command_handle);
 
                     let res = command_handle.join();
diff --git a/crates/ide-assists/src/handlers/generate_function.rs b/crates/ide-assists/src/handlers/generate_function.rs
index ff051fa870..db94a21a6d 100644
--- a/crates/ide-assists/src/handlers/generate_function.rs
+++ b/crates/ide-assists/src/handlers/generate_function.rs
@@ -175,8 +175,7 @@ fn add_func_to_accumulator(
         edit.edit_file(file);
 
         let target = function_builder.target.clone();
-        let function_template = function_builder.render();
-        let func = function_template.to_ast(ctx.config.snippet_cap, edit);
+        let func = function_builder.render(ctx.config.snippet_cap, edit);
 
         if let Some(name) = adt_name {
             let name = make::ty_path(make::ext::ident_path(&format!("{}", name.display(ctx.db()))));
@@ -205,37 +204,6 @@ fn get_adt_source(
     find_struct_impl(ctx, &adt_source, &[fn_name.to_owned()]).map(|impl_| (impl_, range.file_id))
 }
 
-struct FunctionTemplate {
-    fn_def: ast::Fn,
-    ret_type: Option<ast::RetType>,
-    should_focus_return_type: bool,
-    tail_expr: ast::Expr,
-}
-
-impl FunctionTemplate {
-    fn to_ast(&self, cap: Option<SnippetCap>, edit: &mut SourceChangeBuilder) -> ast::Fn {
-        let Self { fn_def, ret_type, should_focus_return_type, tail_expr } = self;
-
-        if let Some(cap) = cap {
-            if *should_focus_return_type {
-                // Focus the return type if there is one
-                match ret_type {
-                    Some(ret_type) => {
-                        edit.add_placeholder_snippet(cap, ret_type.clone());
-                    }
-                    None => {
-                        edit.add_placeholder_snippet(cap, tail_expr.clone());
-                    }
-                }
-            } else {
-                edit.add_placeholder_snippet(cap, tail_expr.clone());
-            }
-        }
-
-        fn_def.clone()
-    }
-}
-
 struct FunctionBuilder {
     target: GeneratedFunctionTarget,
     fn_name: ast::Name,
@@ -339,7 +307,7 @@ impl FunctionBuilder {
         })
     }
 
-    fn render(self) -> FunctionTemplate {
+    fn render(self, cap: Option<SnippetCap>, edit: &mut SourceChangeBuilder) -> ast::Fn {
         let placeholder_expr = make::ext::expr_todo();
         let fn_body = make::block_expr(vec![], Some(placeholder_expr));
         let visibility = match self.visibility {
@@ -361,17 +329,31 @@ impl FunctionBuilder {
         )
         .clone_for_update();
 
-        FunctionTemplate {
-            ret_type: fn_def.ret_type(),
-            // PANIC: we guarantee we always create a function body with a tail expr
-            tail_expr: fn_def
-                .body()
-                .expect("generated function should have a body")
-                .tail_expr()
-                .expect("function body should have a tail expression"),
-            should_focus_return_type: self.should_focus_return_type,
-            fn_def,
+        let ret_type = fn_def.ret_type();
+        // PANIC: we guarantee we always create a function body with a tail expr
+        let tail_expr = fn_def
+            .body()
+            .expect("generated function should have a body")
+            .tail_expr()
+            .expect("function body should have a tail expression");
+
+        if let Some(cap) = cap {
+            if self.should_focus_return_type {
+                // Focus the return type if there is one
+                match ret_type {
+                    Some(ret_type) => {
+                        edit.add_placeholder_snippet(cap, ret_type.clone());
+                    }
+                    None => {
+                        edit.add_placeholder_snippet(cap, tail_expr.clone());
+                    }
+                }
+            } else {
+                edit.add_placeholder_snippet(cap, tail_expr.clone());
+            }
         }
+
+        fn_def
     }
 }
 
diff --git a/crates/rust-analyzer/src/global_state.rs b/crates/rust-analyzer/src/global_state.rs
index a0101af309..8516ffa0df 100644
--- a/crates/rust-analyzer/src/global_state.rs
+++ b/crates/rust-analyzer/src/global_state.rs
@@ -86,10 +86,7 @@ pub(crate) struct GlobalState {
     pub(crate) last_flycheck_error: Option<String>,
 
     // Test explorer
-    pub(crate) test_run_session: Option<Vec<flycheck::CargoTestHandle>>,
-    pub(crate) test_run_sender: Sender<flycheck::CargoTestMessage>,
-    pub(crate) test_run_receiver: Receiver<flycheck::CargoTestMessage>,
-    pub(crate) test_run_remaining_jobs: usize,
+    pub(crate) test_run_session: Option<flycheck::CargoTestHandle>,
 
     // VFS
     pub(crate) loader: Handle<Box<dyn vfs::loader::Handle>, Receiver<vfs::loader::Message>>,
@@ -194,7 +191,6 @@ impl GlobalState {
             analysis_host.update_lru_capacities(capacities);
         }
         let (flycheck_sender, flycheck_receiver) = unbounded();
-        let (test_run_sender, test_run_receiver) = unbounded();
         let mut this = GlobalState {
             sender,
             req_queue: ReqQueue::default(),
@@ -223,9 +219,6 @@ impl GlobalState {
             last_flycheck_error: None,
 
             test_run_session: None,
-            test_run_sender,
-            test_run_receiver,
-            test_run_remaining_jobs: 0,
 
             vfs: Arc::new(RwLock::new((vfs::Vfs::default(), IntMap::default()))),
             vfs_config_version: 0,
diff --git a/crates/rust-analyzer/src/handlers/request.rs b/crates/rust-analyzer/src/handlers/request.rs
index e5c070bf77..77c4ad32cc 100644
--- a/crates/rust-analyzer/src/handlers/request.rs
+++ b/crates/rust-analyzer/src/handlers/request.rs
@@ -219,28 +219,14 @@ pub(crate) fn handle_run_test(
             .unwrap_or_default(),
         None => "".to_owned(),
     };
-    let test_path = if lca.is_empty() {
-        None
+    let handle = if lca.is_empty() {
+        flycheck::CargoTestHandle::new(None, state.config.cargo_test_options())
     } else if let Some((_, path)) = lca.split_once("::") {
-        Some(path)
+        flycheck::CargoTestHandle::new(Some(path), state.config.cargo_test_options())
     } else {
-        None
+        flycheck::CargoTestHandle::new(None, state.config.cargo_test_options())
     };
-    let mut handles = vec![];
-    for ws in &*state.workspaces {
-        if let ProjectWorkspace::Cargo { cargo, .. } = ws {
-            let handle = flycheck::CargoTestHandle::new(
-                test_path,
-                state.config.cargo_test_options(),
-                cargo.workspace_root(),
-                state.test_run_sender.clone(),
-            )?;
-            handles.push(handle);
-        }
-    }
-    // Each process send finished signal twice, once for stdout and once for stderr
-    state.test_run_remaining_jobs = 2 * handles.len();
-    state.test_run_session = Some(handles);
+    state.test_run_session = Some(handle?);
     Ok(())
 }
 
diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs
index ac270bc3c5..9459bd7c5d 100644
--- a/crates/rust-analyzer/src/main_loop.rs
+++ b/crates/rust-analyzer/src/main_loop.rs
@@ -7,7 +7,7 @@ use std::{
 };
 
 use always_assert::always;
-use crossbeam_channel::{select, Receiver};
+use crossbeam_channel::{never, select, Receiver};
 use ide_db::base_db::{SourceDatabase, SourceDatabaseExt, VfsPath};
 use lsp_server::{Connection, Notification, Request};
 use lsp_types::{notification::Notification as _, TextDocumentIdentifier};
@@ -220,7 +220,7 @@ impl GlobalState {
             recv(self.flycheck_receiver) -> task =>
                 Some(Event::Flycheck(task.unwrap())),
 
-            recv(self.test_run_receiver) -> task =>
+            recv(self.test_run_session.as_ref().map(|s| s.receiver()).unwrap_or(&never())) -> task =>
                 Some(Event::TestResult(task.unwrap())),
 
         }
@@ -337,7 +337,9 @@ impl GlobalState {
                         .entered();
                 self.handle_cargo_test_msg(message);
                 // Coalesce many test result event into a single loop turn
-                while let Ok(message) = self.test_run_receiver.try_recv() {
+                while let Some(message) =
+                    self.test_run_session.as_ref().and_then(|r| r.receiver().try_recv().ok())
+                {
                     self.handle_cargo_test_msg(message);
                 }
             }
@@ -790,11 +792,8 @@ impl GlobalState {
             }
             flycheck::CargoTestMessage::Suite => (),
             flycheck::CargoTestMessage::Finished => {
-                self.test_run_remaining_jobs = self.test_run_remaining_jobs.saturating_sub(1);
-                if self.test_run_remaining_jobs == 0 {
-                    self.send_notification::<lsp_ext::EndRunTest>(());
-                    self.test_run_session = None;
-                }
+                self.send_notification::<lsp_ext::EndRunTest>(());
+                self.test_run_session = None;
             }
             flycheck::CargoTestMessage::Custom { text } => {
                 self.send_notification::<lsp_ext::AppendOutputToRunTest>(text);


Test patch:
diff --git a/crates/flycheck/src/test_runner.rs b/crates/flycheck/src/test_runner.rs
index c136dd1366..94c05ebc74 100644
--- a/crates/flycheck/src/test_runner.rs
+++ b/crates/flycheck/src/test_runner.rs
@@ -3,8 +3,7 @@
 
 use std::process::Command;
 
-use crossbeam_channel::Sender;
-use paths::AbsPath;
+use crossbeam_channel::Receiver;
 use serde::Deserialize;
 use toolchain::Tool;
 
@@ -55,27 +54,20 @@ impl ParseFromLine for CargoTestMessage {
 
 #[derive(Debug)]
 pub struct CargoTestHandle {
-    _handle: CommandHandle<CargoTestMessage>,
+    handle: CommandHandle<CargoTestMessage>,
 }
 
 // Example of a cargo test command:
 // cargo test --workspace --no-fail-fast -- module::func -Z unstable-options --format=json
 
 impl CargoTestHandle {
-    pub fn new(
-        path: Option<&str>,
-        options: CargoOptions,
-        root: &AbsPath,
-        sender: Sender<CargoTestMessage>,
-    ) -> std::io::Result<Self> {
+    pub fn new(path: Option<&str>, options: CargoOptions) -> std::io::Result<Self> {
         let mut cmd = Command::new(Tool::Cargo.path());
         cmd.env("RUSTC_BOOTSTRAP", "1");
         cmd.arg("test");
         cmd.arg("--workspace");
         // --no-fail-fast is needed to ensure that all requested tests will run
         cmd.arg("--no-fail-fast");
-        cmd.arg("--manifest-path");
-        cmd.arg(root.join("Cargo.toml"));
         options.apply_on_command(&mut cmd);
         cmd.arg("--");
         if let Some(path) = path {
@@ -83,6 +75,10 @@ impl CargoTestHandle {
         }
         cmd.args(["-Z", "unstable-options"]);
         cmd.arg("--format=json");
-        Ok(Self { _handle: CommandHandle::spawn(cmd, sender)? })
+        Ok(Self { handle: CommandHandle::spawn(cmd)? })
+    }
+
+    pub fn receiver(&self) -> &Receiver<CargoTestMessage> {
+        &self.handle.receiver
     }
 }