From 02b1387e61dc90054d0466019c1d0fac9f80de3f Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Fri, 26 Dec 2025 18:45:39 -0800 Subject: [PATCH] Update AsyncApp result removal brief: remove AppContext::Result, macOS-first approach - Remove AppContext::Result associated type entirely (all contexts return T directly) - Remove Flatten trait (no longer needed without Result>) - 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 --- 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(-) create mode 100644 async-app-result-removal-migration.md diff --git a/async-app-result-removal-migration.md b/async-app-result-removal-migration.md new file mode 100644 index 0000000000000000000000000000000000000000..0a1daa1026f87b04e423abb4ecbdb93ba3e9ee76 --- /dev/null +++ b/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` 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 held across thread boundaries +let task: Task = 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` created in foreground, awaited in background + +### Action Items + +- [ ] Search for `background_spawn` calls that contain awaits on foreground work +- [ ] Audit `Task` 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 ` +- [ ] 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 = anyhow::Result; // 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 \ No newline at end of file diff --git a/async-app-result-removal-tracker.json b/async-app-result-removal-tracker.json index 27325ae56ec0be9d303a0320baec445b0968a16a..28aa8d53686d6b4d628146540e041ed1b0912953 100644 --- a/async-app-result-removal-tracker.json +++ b/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 to try_update() -> Option", "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 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 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"] + } } diff --git a/async-app-result-removal.md b/async-app-result-removal.md index f9a7e4c1b08b2b62f1258d7c6d1c5633e689e16f..f144ed2b1c3672826800cd6cd46544c4656ec7da 100644 --- a/async-app-result-removal.md +++ b/async-app-result-removal.md @@ -92,19 +92,42 @@ Apply the same pattern to: ### AppContext Trait -Change the associated type: +Remove the `Result` associated type entirely. All context types will now return values directly: + ```rust // Before +pub trait AppContext { + type Result; + fn new(...) -> Self::Result>; + fn update_entity(...) -> Self::Result; + // etc. +} + impl AppContext for AsyncApp { type Result = Result; + // ... } // After +pub trait AppContext { + fn new(...) -> Entity; + fn update_entity(...) -> R; + // etc. +} + impl AppContext for AsyncApp { - type Result = 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>` when `WeakEntity` (returns `Result`) is used with `AsyncApp` (also returns `Result`). With this change: +- `AsyncApp` methods return `T` directly +- `WeakEntity::update` etc. return `Result` (only for the weak upgrade) +- No `Result>` 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>` -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` to `try_update() -> Option` 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` 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` 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 \ No newline at end of file