Hide the implementation of `Task` (#22009)

Michael Sloan created 1 year ago

The `Option<T>` within `Ready` is confusing and using `None` for it can
cause crashes. There was actually one instance of this!

Release Notes:

- N/A

Change summary

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(-)

Detailed changes

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<Self>,
     ) -> Task<anyhow::Result<Option<Location>>> {
         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 {

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<T> {
+pub struct Task<T>(TaskState<T>);
+
+#[derive(Debug)]
+enum TaskState<T> {
     /// A task that is ready to return a value
     Ready(Option<T>),
 
@@ -61,14 +64,23 @@ pub enum Task<T> {
 impl<T> Task<T> {
     /// 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<T> {
+        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<T> Future for Task<T> {
 
     fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> {
         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::<R>(dispatcher, Box::pin(future))
     }

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)

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 {

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<Picker<Self>>) {

crates/workspace/src/workspace.rs

@@ -1651,7 +1651,7 @@ impl Workspace {
         F: 'static + FnOnce(&mut Workspace, &mut ViewContext<Workspace>) -> 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);