From c5fe6ef100405ddb5b1c1ddb180d839212467791 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Sat, 14 Dec 2024 02:52:22 -0700 Subject: [PATCH] Hide the implementation of `Task` (#22009) The `Option` within `Ready` is confusing and using `None` for it can cause crashes. There was actually one instance of this! Release Notes: - N/A --- crates/editor/src/editor.rs | 4 +-- crates/gpui/src/executor.rs | 30 ++++++++++++++------ crates/lsp/src/lsp.rs | 2 +- crates/project/src/project.rs | 2 +- crates/repl/src/components/kernel_options.rs | 4 +-- crates/workspace/src/workspace.rs | 2 +- 6 files changed, 28 insertions(+), 16 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 01c41bb84c435372caf7f52eda42cf4b3fe373d0..61e4c228c7c9cb736b5756934b3553e519ec3ca6 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -9502,7 +9502,7 @@ impl Editor { let location_tasks = definitions .into_iter() .map(|definition| match definition { - HoverLink::Text(link) => Task::Ready(Some(Ok(Some(link.target)))), + HoverLink::Text(link) => Task::ready(Ok(Some(link.target))), HoverLink::InlayHint(lsp_location, server_id) => { editor.compute_target_location(lsp_location, server_id, cx) } @@ -9544,7 +9544,7 @@ impl Editor { cx: &mut ViewContext, ) -> Task>> { let Some(project) = self.project.clone() else { - return Task::Ready(Some(Ok(None))); + return Task::ready(Ok(None)); }; cx.spawn(move |editor, mut cx| async move { diff --git a/crates/gpui/src/executor.rs b/crates/gpui/src/executor.rs index 86d4d0058f54290fae2ea67790493310e17fd9db..7dc6098fef4f0acc81f8741652c5a426fd0c12e1 100644 --- a/crates/gpui/src/executor.rs +++ b/crates/gpui/src/executor.rs @@ -50,7 +50,10 @@ pub struct ForegroundExecutor { /// the task to continue running, but with no way to return a value. #[must_use] #[derive(Debug)] -pub enum Task { +pub struct Task(TaskState); + +#[derive(Debug)] +enum TaskState { /// A task that is ready to return a value Ready(Option), @@ -61,14 +64,23 @@ pub enum Task { impl Task { /// Creates a new task that will resolve with the value pub fn ready(val: T) -> Self { - Task::Ready(Some(val)) + Task(TaskState::Ready(Some(val))) + } + + /// Returns the task's result if it is already know. The only known usecase for this is for + /// skipping spawning another task that awaits on this one. + pub fn get_ready(self) -> Option { + match self { + Task(TaskState::Ready(val)) => val, + Task(TaskState::Spawned(_)) => None, + } } /// Detaching a task runs it to completion in the background pub fn detach(self) { match self { - Task::Ready(_) => {} - Task::Spawned(task) => task.detach(), + Task(TaskState::Ready(_)) => {} + Task(TaskState::Spawned(task)) => task.detach(), } } } @@ -94,8 +106,8 @@ impl Future for Task { fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll { match unsafe { self.get_unchecked_mut() } { - Task::Ready(val) => Poll::Ready(val.take().unwrap()), - Task::Spawned(task) => task.poll(cx), + Task(TaskState::Ready(val)) => Poll::Ready(val.take().unwrap()), + Task(TaskState::Spawned(task)) => task.poll(cx), } } } @@ -163,7 +175,7 @@ impl BackgroundExecutor { let (runnable, task) = async_task::spawn(future, move |runnable| dispatcher.dispatch(runnable, label)); runnable.schedule(); - Task::Spawned(task) + Task(TaskState::Spawned(task)) } /// Used by the test harness to run an async test in a synchronous fashion. @@ -340,7 +352,7 @@ impl BackgroundExecutor { move |runnable| dispatcher.dispatch_after(duration, runnable) }); runnable.schedule(); - Task::Spawned(task) + Task(TaskState::Spawned(task)) } /// in tests, start_waiting lets you indicate which task is waiting (for debugging only) @@ -460,7 +472,7 @@ impl ForegroundExecutor { dispatcher.dispatch_on_main_thread(runnable) }); runnable.schedule(); - Task::Spawned(task) + Task(TaskState::Spawned(task)) } inner::(dispatcher, Box::pin(future)) } diff --git a/crates/lsp/src/lsp.rs b/crates/lsp/src/lsp.rs index 65ba61f626b5ee2230926a4bdd7584a806a089b1..473a7ac64eb346decee192d4209cd3414f14c14a 100644 --- a/crates/lsp/src/lsp.rs +++ b/crates/lsp/src/lsp.rs @@ -443,7 +443,7 @@ impl LanguageServer { let stderr_captures = stderr_capture.clone(); cx.spawn(|_| Self::handle_stderr(stderr, io_handlers, stderr_captures).log_err()) }) - .unwrap_or_else(|| Task::Ready(Some(None))); + .unwrap_or_else(|| Task::ready(None)); let input_task = cx.spawn(|_| async move { let (stdout, stderr) = futures::join!(stdout_input_task, stderr_input_task); stdout.or(stderr) diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index f00d53544dbbeb1cc92da882f3d61dc7d9095653..1040ae3bec0bc3522ffceb5b2ea48c1964de8f23 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -2540,7 +2540,7 @@ impl Project { .read(cx) .list_toolchains(worktree_id, language_name, cx) }) - .unwrap_or(Task::Ready(None)) + .ok()? .await }) } else { diff --git a/crates/repl/src/components/kernel_options.rs b/crates/repl/src/components/kernel_options.rs index 85cd758f406c39c6e1159b97cd768cf8ccfdb1d9..c83e6fbc9c2fc6c5cac9dbb4dc5d4130dfadb633 100644 --- a/crates/repl/src/components/kernel_options.rs +++ b/crates/repl/src/components/kernel_options.rs @@ -98,7 +98,7 @@ impl PickerDelegate for KernelPickerDelegate { if query.is_empty() { self.filtered_kernels = all_kernels; - return Task::Ready(Some(())); + return Task::ready(()); } self.filtered_kernels = if query.is_empty() { @@ -110,7 +110,7 @@ impl PickerDelegate for KernelPickerDelegate { .collect() }; - return Task::Ready(Some(())); + return Task::ready(()); } fn confirm(&mut self, _secondary: bool, cx: &mut ViewContext>) { diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 398b0ea07cb9f413080ff23c8f56a3143d826c90..517c9a86da49716e8acb5002aa37168d2daac133 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -1651,7 +1651,7 @@ impl Workspace { F: 'static + FnOnce(&mut Workspace, &mut ViewContext) -> T, { if self.project.read(cx).is_local() { - Task::Ready(Some(Ok(callback(self, cx)))) + Task::ready(Ok(callback(self, cx))) } else { let env = self.project.read(cx).cli_environment(cx); let task = Self::new_local(Vec::new(), self.app_state.clone(), None, env, cx);