Apply suggestions from code review

Co-authored-by: Jan Haller <bromeon@gmail.com>

godot-core/src/task/async_runtime.rs

@@ -57,7 +57,7 @@ pub fn spawn(future: impl Future<Output = ()> + 'static) -> TaskHandle {
     // Once thread-safe futures are possible the restriction can be lifted.
     assert!(
         crate::init::is_main_thread(),
-        "godot_task can only be used on the main thread!"
+        "godot_task() can only be used on the main thread"
     );
 
     let (task_handle, godot_waker) = ASYNC_RUNTIME.with_runtime_mut(move |rt| {
@@ -99,8 +99,8 @@ impl TaskHandle {
     pub fn cancel(self) {
         ASYNC_RUNTIME.with_runtime_mut(|rt| {
             let Some(task) = rt.tasks.get(self.index) else {
-                // getting the task from the runtime might return None if the runtime has already been deinitialized. In this case we just
-                // ignore the cancel request as the entire runtime has already been canceled.
+                // Getting the task from the runtime might return None if the runtime has already been deinitialized. In this case, we just
+                // ignore the cancel request, as the entire runtime has already been canceled.
                 return;
             };
 
@@ -124,7 +124,7 @@ impl TaskHandle {
     /// Synchronously checks if the task is still pending or has already completed.
     pub fn is_pending(&self) -> bool {
         ASYNC_RUNTIME.with_runtime(|rt| {
-            let slot = rt.tasks.get(self.index).expect("Slot at index must exist!");
+            let slot = rt.tasks.get(self.index).unwrap_or_else(|| panic!("missing future slot at index {}", self.index));
 
             if slot.id != self.id {
                 return false;
@@ -141,7 +141,7 @@ impl TaskHandle {
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Async Runtime
 
-const ASYNC_RUNTIME_DEINIT_PANIC_MESSAGE: &str = "The async runtime is being accessed after it has been deinitialized. This Should not be possible and is most likely a bug.";
+const ASYNC_RUNTIME_DEINIT_PANIC_MESSAGE: &str = "The async runtime is being accessed after it has been deinitialized. This should not be possible and is most likely a bug.";
 
 thread_local! {
     static ASYNC_RUNTIME: RefCell<Option<AsyncRuntime>> = RefCell::new(Some(AsyncRuntime::new()));
@@ -225,10 +225,10 @@ impl<T> FutureSlot<T> {
     fn park(&mut self, value: T) {
         match self.value {
             FutureSlotState::Empty | FutureSlotState::Gone => {
-                panic!("Future slot is currently unoccupied, future can not be parked here!");
+                panic!("cannot park future in slot which is unoccupied")
             }
             FutureSlotState::Pending(_) => {
-                panic!("Future slot is already occupied by a different future!")
+                panic!("cannot park future in slot, which is already occupied by a different future")
             }
             FutureSlotState::Polling => {
                 self.value = FutureSlotState::Pending(value);
@@ -237,8 +237,8 @@ impl<T> FutureSlot<T> {
     }
 }
 
-#[derive(Default)]
 /// The storage for the pending tasks of the async runtime.
+#[derive(Default)]
 struct AsyncRuntime {
     tasks: Vec<FutureSlot<Pin<Box<dyn Future<Output = ()>>>>>,
     next_task_id: u64,
@@ -382,7 +382,7 @@ fn poll_future(godot_waker: Arc<GodotWaker>) {
             FutureSlotState::Gone => None,
 
             FutureSlotState::Polling => {
-                panic!("The same GodotWaker has been called recursively, this is not expected!");
+                unreachable!("the same GodotWaker has been called recursively");
             }
 
             FutureSlotState::Pending(future) => Some(future),
@@ -396,7 +396,8 @@ fn poll_future(godot_waker: Arc<GodotWaker>) {
 
     let error_context = || "Godot async task failed";
 
-    // Future should be unwind-safe as we will immediately drop it in cases where a panic occurs.
+    // If Future::poll() panics, the future is immediately dropped and cannot be accessed again,
+    // thus any state that may not have been unwind-safe cannot be observed later.
     let mut future = AssertUnwindSafe(future);
 
     let panic_result = handle_panic(error_context, move || {

godot-core/src/task/futures.rs

@@ -20,8 +20,8 @@ use crate::registry::signal::TypedSignal;
 
 /// The panicking counter part to the [`FallibleSignalFuture`].
 ///
-/// This future works in the same ways as the `FallibleSignalFuture`, but panics when the signal object is freed instead of resolving to an
-/// [`Result::Err`]
+/// This future works in the same way as `FallibleSignalFuture`, but panics when the signal object is freed, instead of resolving to a
+/// [`Result::Err`].
 pub struct SignalFuture<R: ParamTuple + Sync + Send>(FallibleSignalFuture<R>);
 
 impl<R: ParamTuple + Sync + Send> SignalFuture<R> {
@@ -115,7 +115,7 @@ impl<R> Display for SignalFutureResolver<R> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(
             f,
-            "GuaranteedSignalFutureResolver::<{}>",
+            "SignalFutureResolver::<{}>",
             std::any::type_name::<R>()
         )
     }
@@ -274,7 +274,7 @@ impl Signal {
 
     /// Creates a future for this signal.
     ///
-    /// The future will resolve the next time the `Signal` is emitted, but might panic if the signal object is freed.
+    /// The future will resolve the next time the signal is emitted, but might panic if the signal object is freed.
     /// See [`SignalFuture`] for details.
     ///
     /// Since the `Signal` type does not contain information on the signal argument types, the future output type has to be inferred from
@@ -295,7 +295,7 @@ impl<C: WithBaseField, R: ParamTuple + Sync + Send> TypedSignal<'_, C, R> {
 
     /// Creates a future for this signal.
     ///
-    /// The future will resolve the next time the `Signal` is emitted, but might panic if the signal object is freed.
+    /// The future will resolve the next time the signal is emitted, but might panic if the signal object is freed.
     /// See [`SignalFuture`] for details.
     pub fn to_future(&self) -> SignalFuture<R> {
         SignalFuture::new(self.to_untyped())