Update AsyncApp result removal brief: remove AppContext::Result, macOS-first approach

Mikayla Maki created 2 days ago

- Remove AppContext::Result associated type entirely (all contexts return T directly)
- Remove Flatten trait (no longer needed without Result<Result<T>>)
- Restructure phases: macOS first, then other platforms, then API changes
- Split codebase migration (phases 3-5) to separate brief
- Add async-app-result-removal-migration.md for future ~500+ callsite updates

Change summary

async-app-result-removal-migration.md | 172 +++++++++++++++++++++++
async-app-result-removal-tracker.json | 210 +++++++++-------------------
async-app-result-removal.md           | 103 ++++++++-----
3 files changed, 302 insertions(+), 183 deletions(-)

Detailed changes

async-app-result-removal-migration.md

@@ -0,0 +1,172 @@
+# Project: AsyncApp Result Removal - Codebase Migration
+
+## Prerequisites
+
+This brief depends on the completion of `async-app-result-removal.md`, which:
+- Adds trampoline check infrastructure in dispatchers
+- Updates `AsyncApp` API with `try_update()` / `update()` pattern
+- Removes `AppContext::Result<T>` associated type
+- Removes `Flatten` trait
+
+## Overview
+
+This phase migrates the entire codebase to use the new `AsyncApp` API. Since foreground tasks are now automatically cancelled when the app dies, most callsites can remove their error handling.
+
+## Phase 1: Audit Cross-Boundary Awaits
+
+### Goal
+
+Identify places where background tasks await foreground tasks, as these are the edge cases that need `try_update()`.
+
+### Search Patterns
+
+```
+# Background tasks awaiting foreground work
+background_executor().spawn(...).await
+
+# Channels receiving from foreground tasks
+receiver.recv().await  # where sender is in foreground
+
+# Task<T> held across thread boundaries
+let task: Task<T> = cx.spawn(...)  # then moved to background
+```
+
+### Known Patterns to Check
+
+1. **cx.spawn(...).await from background** - If a background task stores an `AsyncApp` and calls `spawn`, then awaits the result
+2. **Channel patterns** - Foreground sends via channel, background receives
+3. **Task handles passed to background** - A `Task<T>` created in foreground, awaited in background
+
+### Action Items
+
+- [ ] Search for `background_spawn` calls that contain awaits on foreground work
+- [ ] Audit `Task<T>` usage to find cross-boundary cases
+- [ ] Create list of callsites requiring `try_update()` instead of `update()`
+- [ ] Document any patterns that cannot be safely migrated
+
+## Phase 2: Codebase Migration
+
+### Migration Strategy
+
+1. **Automated pass**: Use search-and-replace for simple patterns
+2. **Manual review**: Handle complex cases requiring `try_update()`
+
+### Simple Patterns (Automated)
+
+```rust
+// Before
+cx.update(|app| { ... })?
+cx.update(|app| { ... }).ok();
+cx.update(|app| { ... }).unwrap();
+
+// After
+cx.update(|app| { ... })
+```
+
+### Complex Patterns (Manual)
+
+```rust
+// Background awaiting foreground - use try_update
+cx.background_spawn(async move {
+    // Before
+    let result = task.await?;
+    
+    // After - if task could be cancelled
+    let Some(result) = cx.try_update(|app| { ... }) else {
+        return; // or handle gracefully
+    };
+});
+```
+
+### Crate Priority Order
+
+Migrate in dependency order to catch issues early:
+
+1. `crates/gpui` - Core framework
+2. `crates/language` - Language support
+3. `crates/project` - Project management
+4. `crates/editor` - Editor core
+5. `crates/workspace` - Workspace management
+6. `crates/agent` and `crates/agent_ui` - AI agent
+7. Remaining crates alphabetically
+
+### Per-Crate Checklist
+
+For each crate:
+- [ ] Find all `AsyncApp` / `AsyncWindowContext` usage
+- [ ] Categorize: simple removal vs. needs `try_update()`
+- [ ] Apply changes
+- [ ] Run `cargo test -p <crate>` 
+- [ ] Run `./script/clippy`
+
+## Phase 3: Testing & Cleanup
+
+### Remove Dead Code
+
+After migration, search for and remove:
+
+```rust
+// Dead imports
+use anyhow::{Context, Result};  // if only used for AsyncApp errors
+
+// Dead error handling
+.context("app was released")
+.context("window was closed")
+
+// Unused Result type aliases
+type Result<T> = anyhow::Result<T>;  // if only used for AsyncApp
+```
+
+### Update Documentation
+
+- [ ] Update `AsyncApp` rustdoc to explain new semantics
+- [ ] Update GPUI documentation/examples
+- [ ] Add migration guide for external users (if any)
+
+### New Tests
+
+Add tests to prevent regression:
+
+- [ ] Test: `update()` works when app is alive
+- [ ] Test: `try_update()` returns `None` when app is gone
+- [ ] Test: Tasks are cancelled (not panicking) when app dies
+- [ ] Test: Nested tasks both cancel cleanly
+
+### Final Validation
+
+- [ ] `cargo test` (full suite)
+- [ ] `./script/clippy` 
+- [ ] Manual testing: heavy async workloads
+- [ ] Manual testing: rapid window open/close
+- [ ] Manual testing: quit app with pending tasks
+
+## Estimated Scope
+
+Based on grep analysis:
+- ~500+ callsites using `AsyncApp::update()` or similar
+- ~50 crates with potential changes
+- Most changes are mechanical `.unwrap()` / `?` removal
+- ~10-20 complex cases requiring `try_update()`
+
+## Risk Mitigation
+
+### Rollback Plan
+
+If issues are discovered post-merge:
+1. Revert migration commits (codebase changes)
+2. Keep infrastructure changes (they're backwards compatible)
+3. Re-evaluate edge cases
+
+### Incremental Rollout
+
+Consider migrating in stages:
+1. First: `crates/gpui` only - validate core behavior
+2. Second: High-traffic crates (`editor`, `workspace`, `project`)
+3. Third: Remaining crates
+
+## Files Reference
+
+Key files for finding callsites:
+- `crates/gpui/src/app/async_context.rs` - `AsyncApp` definition
+- Search: `update(|`, `update_entity(`, `read_entity(`
+- Search: `.unwrap()`, `.ok()`, `?` following async context calls

async-app-result-removal-tracker.json

@@ -4,7 +4,7 @@
   "phases": [
     {
       "id": 1,
-      "name": "Add Trampoline Check Infrastructure",
+      "name": "Add Trampoline Check Infrastructure (macOS First)",
       "status": "not_started",
       "tasks": [
         {
@@ -21,48 +21,36 @@
         },
         {
           "id": "1.3",
-          "description": "Update trampoline function in Linux dispatcher to check app status before run()",
-          "file": "crates/gpui/src/platform/linux/dispatcher.rs",
-          "status": "not_started"
-        },
-        {
-          "id": "1.4",
-          "description": "Update trampoline function in Windows dispatcher to check app status before run()",
-          "file": "crates/gpui/src/platform/windows/dispatcher.rs",
-          "status": "not_started"
-        },
-        {
-          "id": "1.5",
           "description": "Update trampoline function in Test dispatcher to check app status before run()",
           "file": "crates/gpui/src/platform/test/dispatcher.rs",
           "status": "not_started"
         },
         {
-          "id": "1.6",
+          "id": "1.4",
           "description": "Modify ForegroundExecutor::spawn_with_priority to accept optional app weak pointer",
           "file": "crates/gpui/src/executor.rs",
           "status": "not_started"
         },
         {
-          "id": "1.7",
+          "id": "1.5",
           "description": "Update AsyncApp::spawn to pass app weak pointer to executor",
           "file": "crates/gpui/src/app/async_context.rs",
           "status": "not_started"
         },
         {
-          "id": "1.8",
+          "id": "1.6",
           "description": "Write unit test verifying task cancellation when app is dropped",
           "file": "crates/gpui/src/executor.rs",
           "status": "not_started"
         },
         {
-          "id": "1.9",
+          "id": "1.7",
           "description": "Write unit test for nested tasks both cancelling cleanly",
           "file": "crates/gpui/src/executor.rs",
           "status": "not_started"
         },
         {
-          "id": "1.10",
+          "id": "1.8",
           "description": "Create async_cancellation.rs example demonstrating behavior",
           "file": "crates/gpui/examples/async_cancellation.rs",
           "status": "not_started"
@@ -71,218 +59,154 @@
     },
     {
       "id": 2,
-      "name": "Update AsyncApp API",
+      "name": "Extend to Other Platforms",
       "status": "not_started",
       "tasks": [
         {
           "id": "2.1",
+          "description": "Update trampoline function in Linux dispatcher to check app status before run()",
+          "file": "crates/gpui/src/platform/linux/dispatcher.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "2.2",
+          "description": "Update trampoline function in Windows dispatcher to check app status before run()",
+          "file": "crates/gpui/src/platform/windows/dispatcher.rs",
+          "status": "not_started"
+        }
+      ]
+    },
+    {
+      "id": 3,
+      "name": "Update AsyncApp API & Remove AppContext::Result",
+      "status": "not_started",
+      "tasks": [
+        {
+          "id": "3.1",
           "description": "Rename update() -> Result<R> to try_update() -> Option<R>",
           "file": "crates/gpui/src/app/async_context.rs",
           "status": "not_started"
         },
         {
-          "id": "2.2",
+          "id": "3.2",
           "description": "Add new update() -> R that panics if app is gone",
           "file": "crates/gpui/src/app/async_context.rs",
           "status": "not_started"
         },
         {
-          "id": "2.3",
+          "id": "3.3",
           "description": "Apply try_/non-try pattern to read_entity and update_entity",
           "file": "crates/gpui/src/app/async_context.rs",
           "status": "not_started"
         },
         {
-          "id": "2.4",
+          "id": "3.4",
           "description": "Apply try_/non-try pattern to read_global and update_global",
           "file": "crates/gpui/src/app/async_context.rs",
           "status": "not_started"
         },
         {
-          "id": "2.5",
+          "id": "3.5",
           "description": "Apply try_/non-try pattern to read_window and update_window",
           "file": "crates/gpui/src/app/async_context.rs",
           "status": "not_started"
         },
         {
-          "id": "2.6",
+          "id": "3.6",
           "description": "Apply try_/non-try pattern to new, reserve_entity, insert_entity",
           "file": "crates/gpui/src/app/async_context.rs",
           "status": "not_started"
         },
         {
-          "id": "2.7",
+          "id": "3.7",
           "description": "Apply try_/non-try pattern to refresh, open_window, subscribe, has_global",
           "file": "crates/gpui/src/app/async_context.rs",
           "status": "not_started"
         },
         {
-          "id": "2.8",
-          "description": "Update AppContext trait Result associated type for AsyncApp",
-          "file": "crates/gpui/src/app/async_context.rs",
-          "status": "not_started"
-        },
-        {
-          "id": "2.9",
+          "id": "3.8",
           "description": "Apply same changes to AsyncWindowContext",
           "file": "crates/gpui/src/app/async_context.rs",
           "status": "not_started"
-        }
-      ]
-    },
-    {
-      "id": 3,
-      "name": "Audit Cross-Boundary Awaits",
-      "status": "not_started",
-      "tasks": [
-        {
-          "id": "3.1",
-          "description": "Search for background tasks awaiting foreground tasks",
-          "file": null,
-          "status": "not_started",
-          "notes": "grep for patterns like background_executor().spawn(...).await"
-        },
-        {
-          "id": "3.2",
-          "description": "Identify all Task<T> values passed across thread boundaries",
-          "file": null,
-          "status": "not_started"
         },
         {
-          "id": "3.3",
-          "description": "Migrate identified cases to use try_update or handle Option",
-          "file": null,
-          "status": "not_started"
-        }
-      ]
-    },
-    {
-      "id": 4,
-      "name": "Codebase Migration",
-      "status": "not_started",
-      "tasks": [
-        {
-          "id": "4.1",
-          "description": "Update callsites in crates/gpui",
-          "file": null,
+          "id": "3.9",
+          "description": "Remove type Result<T> associated type from AppContext trait",
+          "file": "crates/gpui/src/gpui.rs",
           "status": "not_started"
         },
         {
-          "id": "4.2",
-          "description": "Update callsites in crates/editor",
-          "file": null,
+          "id": "3.10",
+          "description": "Update all AppContext trait method signatures to return R directly",
+          "file": "crates/gpui/src/gpui.rs",
           "status": "not_started"
         },
         {
-          "id": "4.3",
-          "description": "Update callsites in crates/workspace",
-          "file": null,
+          "id": "3.11",
+          "description": "Remove Flatten trait from gpui.rs",
+          "file": "crates/gpui/src/gpui.rs",
           "status": "not_started"
         },
         {
-          "id": "4.4",
-          "description": "Update callsites in crates/project",
-          "file": null,
+          "id": "3.12",
+          "description": "Update WeakEntity::update to remove Flatten bounds",
+          "file": "crates/gpui/src/app/entity_map.rs",
           "status": "not_started"
         },
         {
-          "id": "4.5",
-          "description": "Update callsites in crates/language",
-          "file": null,
+          "id": "3.13",
+          "description": "Update WeakEntity::read_with to remove Flatten bounds",
+          "file": "crates/gpui/src/app/entity_map.rs",
           "status": "not_started"
         },
         {
-          "id": "4.6",
-          "description": "Update callsites in crates/agent and crates/agent_ui",
-          "file": null,
+          "id": "3.14",
+          "description": "Update WeakEntity::update_in to remove Flatten bounds",
+          "file": "crates/gpui/src/app/entity_map.rs",
           "status": "not_started"
         },
         {
-          "id": "4.7",
-          "description": "Update callsites in remaining crates",
-          "file": null,
+          "id": "3.15",
+          "description": "Update ExampleContext in eval crate to match new AppContext signature",
+          "file": "crates/eval/src/example.rs",
           "status": "not_started"
         },
         {
-          "id": "4.8",
-          "description": "Fix all compilation errors",
-          "file": null,
-          "status": "not_started"
-        }
-      ]
-    },
-    {
-      "id": 5,
-      "name": "Testing and Cleanup",
-      "status": "not_started",
-      "tasks": [
-        {
-          "id": "5.1",
-          "description": "Remove dead error handling code",
-          "file": null,
-          "status": "not_started"
-        },
-        {
-          "id": "5.2",
+          "id": "3.16",
           "description": "Update AsyncApp documentation",
           "file": "crates/gpui/src/app/async_context.rs",
           "status": "not_started"
         },
         {
-          "id": "5.3",
-          "description": "Add test: try_update returns None when app gone",
-          "file": "crates/gpui/src/app/async_context.rs",
-          "status": "not_started"
-        },
-        {
-          "id": "5.4",
-          "description": "Add test: update works normally when app alive",
-          "file": "crates/gpui/src/app/async_context.rs",
-          "status": "not_started"
-        },
-        {
-          "id": "5.5",
-          "description": "Add test: spawn_executes_when_app_alive",
-          "file": "crates/gpui/src/app/async_context.rs",
-          "status": "not_started"
-        },
-        {
-          "id": "5.6",
-          "description": "Add test: update_entity works via AsyncApp",
-          "file": "crates/gpui/src/app/async_context.rs",
-          "status": "not_started"
-        },
-        {
-          "id": "5.7",
-          "description": "Verify async_cancellation example runs without panics",
-          "file": "crates/gpui/examples/async_cancellation.rs",
-          "status": "not_started"
-        },
-        {
-          "id": "5.8",
+          "id": "3.17",
           "description": "Run cargo test -p gpui",
           "file": null,
           "status": "not_started"
         },
         {
-          "id": "5.9",
+          "id": "3.18",
           "description": "Run ./script/clippy",
           "file": null,
           "status": "not_started"
         },
         {
-          "id": "5.10",
-          "description": "Run full test suite and fix any regressions",
-          "file": null,
+          "id": "3.19",
+          "description": "Verify async_cancellation example runs without panics",
+          "file": "crates/gpui/examples/async_cancellation.rs",
           "status": "not_started"
         },
         {
-          "id": "5.11",
+          "id": "3.20",
           "description": "Manual test: close windows with pending tasks, verify no panics",
           "file": null,
           "status": "not_started"
         }
       ]
     }
-  ]
+  ],
+  "future_work": {
+    "brief": "async-app-result-removal-migration.md",
+    "description": "Codebase migration to remove error handling from ~500+ callsites",
+    "phases": ["Audit Cross-Boundary Awaits", "Codebase Migration", "Testing and Cleanup"]
+  }
 }

async-app-result-removal.md

@@ -92,19 +92,42 @@ Apply the same pattern to:
 
 ### AppContext Trait
 
-Change the associated type:
+Remove the `Result<T>` associated type entirely. All context types will now return values directly:
+
 ```rust
 // Before
+pub trait AppContext {
+    type Result<T>;
+    fn new<T>(...) -> Self::Result<Entity<T>>;
+    fn update_entity<T, R>(...) -> Self::Result<R>;
+    // etc.
+}
+
 impl AppContext for AsyncApp {
     type Result<T> = Result<T>;
+    // ...
 }
 
 // After
+pub trait AppContext {
+    fn new<T>(...) -> Entity<T>;
+    fn update_entity<T, R>(...) -> R;
+    // etc.
+}
+
 impl AppContext for AsyncApp {
-    type Result<T> = T;
+    // All methods return values directly, panicking if app is gone
+    // ...
 }
 ```
 
+### Remove Flatten Trait
+
+The `Flatten` trait in `gpui.rs` exists solely to handle `Result<Result<T>>` when `WeakEntity` (returns `Result`) is used with `AsyncApp` (also returns `Result`). With this change:
+- `AsyncApp` methods return `T` directly
+- `WeakEntity::update` etc. return `Result<T>` (only for the weak upgrade)
+- No `Result<Result<T>>` case exists, so `Flatten` can be removed
+
 ### RunnableMeta
 
 Add optional app weak pointer:
@@ -117,48 +140,36 @@ pub struct RunnableMeta {
 
 ## Implementation Phases
 
-### Phase 1: Add Trampoline Check Infrastructure
+### Phase 1: Add Trampoline Check Infrastructure (macOS First)
+
+Start with macOS to validate the approach before implementing on other platforms.
 
 1. Modify `RunnableMeta` to include `Option<Weak<AppCell>>`
-2. Update trampoline functions in all dispatchers to check app status before running:
+2. Update trampoline function in Mac dispatcher to check app status before running:
    - `crates/gpui/src/platform/mac/dispatcher.rs`
+3. Update trampoline function in Test dispatcher (needed for tests):
+   - `crates/gpui/src/platform/test/dispatcher.rs`
+4. Modify spawn paths in `AsyncApp` to populate the app weak pointer in metadata
+5. Write tests to validate cancellation behavior on macOS
+
+### Phase 2: Extend to Other Platforms
+
+After validating on macOS:
+
+1. Update trampoline function in Linux dispatcher:
    - `crates/gpui/src/platform/linux/dispatcher.rs`
+2. Update trampoline function in Windows dispatcher:
    - `crates/gpui/src/platform/windows/dispatcher.rs`
-   - `crates/gpui/src/platform/test/dispatcher.rs`
-3. Modify spawn paths in `AsyncApp` to populate the app weak pointer in metadata
 
-### Phase 2: Update AsyncApp API
+### Phase 3: Update AsyncApp API & Remove AppContext::Result
 
 1. Rename `update() -> Result<R>` to `try_update() -> Option<R>`
 2. Add new `update() -> R` that panics if app is gone
 3. Apply same pattern to all fallible methods
-4. Update `AppContext` trait's associated `Result` type
-
-### Phase 3: Audit Cross-Boundary Awaits
-
-Search for patterns where background code awaits foreground tasks:
-```
-background_executor().spawn(...).await  // awaiting foreground work
-```
-
-Migrate these to use `try_update()` or handle the `Option`.
-
-Known patterns to check:
-- `cx.spawn(...).await` called from background context
-- Channels receiving from foreground tasks
-- Any `Task<T>` held across thread boundaries
-
-### Phase 4: Codebase Migration
-
-1. Update all callsites to remove `.unwrap()`, `?`, `.ok()` from `update()` calls
-2. Use `try_update()` where `Option` handling is needed
-3. This is a large mechanical change across ~500+ callsites
-
-### Phase 5: Cleanup
-
-1. Remove now-dead error handling code
-2. Update documentation
-3. Add tests validating cancellation behavior
+4. Remove `type Result<T>` from `AppContext` trait
+5. Update all trait method signatures to return `R` directly
+6. Remove `Flatten` trait from `gpui.rs`
+7. Update `WeakEntity::update`, `WeakEntity::read_with`, etc. to remove `Flatten` bounds
 
 ## Files to Modify
 
@@ -166,13 +177,17 @@ Known patterns to check:
 - `crates/gpui/src/app/async_context.rs` - AsyncApp implementation
 - `crates/gpui/src/executor.rs` - RunnableMeta, spawn functions
 - `crates/gpui/src/platform.rs` - RunnableVariant if needed
-- `crates/gpui/src/gpui.rs` - AppContext trait
+- `crates/gpui/src/gpui.rs` - AppContext trait (remove Result type), remove Flatten trait
+- `crates/gpui/src/app/entity_map.rs` - Update WeakEntity to remove Flatten usage
 
 ### Dispatcher Changes
-- `crates/gpui/src/platform/mac/dispatcher.rs` - trampoline function
-- `crates/gpui/src/platform/linux/dispatcher.rs` - trampoline function
-- `crates/gpui/src/platform/windows/dispatcher.rs` - trampoline function
-- `crates/gpui/src/platform/test/dispatcher.rs` - trampoline function
+- `crates/gpui/src/platform/mac/dispatcher.rs` - trampoline function (Phase 1)
+- `crates/gpui/src/platform/test/dispatcher.rs` - trampoline function (Phase 1)
+- `crates/gpui/src/platform/linux/dispatcher.rs` - trampoline function (Phase 2)
+- `crates/gpui/src/platform/windows/dispatcher.rs` - trampoline function (Phase 2)
+
+### External Crate Changes
+- `crates/eval/src/example.rs` - ExampleContext's AppContext impl (update to remove Result type)
 
 ### Realtime Tasks (No Changes Needed)
 Realtime/audio tasks use a separate code path with their own thread and channel. They don't use `AsyncApp` and won't be affected.
@@ -237,5 +252,13 @@ Entity operations via AsyncApp still work.
 1. `cargo test -p gpui` passes
 2. `cargo run -p gpui --example async_cancellation` works correctly
 3. `./script/clippy` passes
-4. Full test suite `cargo test` passes
+4. Full test suite `cargo test` passes (at least on macOS initially)
 5. Manual test: close windows with pending tasks, verify no panics
+
+## Future Work (Separate Brief)
+
+The following phases will be addressed in a separate brief after this work is validated:
+
+1. **Audit Cross-Boundary Awaits** - Search for patterns where background code awaits foreground tasks and migrate to `try_update()`
+2. **Codebase Migration** - Update all ~500+ callsites to remove `.unwrap()`, `?`, `.ok()` from `update()` calls
+3. **Cleanup** - Remove dead error handling code, update documentation