From 64835d4f96e1f1b0dd0e7dafa54b3c71c2fb8ddb Mon Sep 17 00:00:00 2001 From: Vincent Esche Date: Tue, 12 May 2026 20:06:58 +0200 Subject: [PATCH 1/2] Expand `gpui-entity` and `gpui-async` skills with entity re-entrancy and `defer_in` lock rules --- .claude/skills/gpui-async/SKILL.md | 37 ++++++++ .claude/skills/gpui-entity/SKILL.md | 84 +++++++++++++++-- .../gpui-entity/references/best-practices.md | 91 +++++++++++++++---- 3 files changed, 186 insertions(+), 26 deletions(-) diff --git a/.claude/skills/gpui-async/SKILL.md b/.claude/skills/gpui-async/SKILL.md index e1c009f6e1..d047bb46c6 100644 --- a/.claude/skills/gpui-async/SKILL.md +++ b/.claude/skills/gpui-async/SKILL.md @@ -133,6 +133,43 @@ Tasks are automatically cancelled when dropped. Store in struct to keep alive. ## Common Pitfalls +### ❌ Don't: Use `defer_in` and then update the same entity through its handle + +`cx.defer_in(window, callback)` schedules `callback` to run **on the current entity** — GPUI re-acquires that entity's lock to execute it. Calling `entity.update(cx, …)` on the *same* entity from within the deferred callback re-enters the lock and panics: + +``` +cannot update … while it is already being updated +``` + +```rust +// ❌ Panic: list entity is locked for the defer_in; calling list.update re-enters +fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context>) { + cx.defer_in(window, |list_state, window, cx| { + parent.update(cx, |this, cx| { + this.inner_list.update(cx, |_, _| {}); // PANIC if inner_list == the deferred entity + }); + }); +} +``` + +```rust +// ✅ Correct: use the direct &mut reference — no lock needed +fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context>) { + cx.defer_in(window, |list_state, window, cx| { + // Access list data directly through the &mut reference + list_state.delegate_mut().some_method(); + + // Update a *different* entity — fine, different lock + parent.update(cx, |this, cx| { /* … */ }); + + // Sync list state directly after parent update — no lock needed + list_state.delegate_mut().update_snapshot(new_val); + }); +} +``` + +The rule: inside a `defer_in` callback, **never call `entity.update(cx, …)` or `entity.read(cx)` on the entity the `defer_in` was scheduled on**. Use the `&mut Entity` direct reference the callback provides instead. + ### ❌ Don't: Update entities from background tasks ```rust diff --git a/.claude/skills/gpui-entity/SKILL.md b/.claude/skills/gpui-entity/SKILL.md index 970e88ee74..a39598105a 100644 --- a/.claude/skills/gpui-entity/SKILL.md +++ b/.claude/skills/gpui-entity/SKILL.md @@ -120,17 +120,87 @@ entity.update(cx, |state, inner_cx| { }); ``` -### Avoid Nested Updates +### Avoid Nested Entity Updates + +Nested `entity.update(cx, …)` calls are dangerous. The default posture is: **do not nest them**. The sub-cases below clarify when a panic is guaranteed vs. merely possible. + +**Same entity → always panics.** +GPUI locks an entity for the entire duration of its update or render pass. Re-entering that same lock panics immediately: + +``` +cannot update … while it is already being updated +``` ```rust -// ✅ Good: Sequential updates -entity1.update(cx, |state, cx| { /* ... */ }); -entity2.update(cx, |state, cx| { /* ... */ }); +// ❌ Panic: updating entity_a from inside entity_a's own update +entity_a.update(cx, |state, cx| { + entity_a.update(cx, |_, _| {}); // PANIC — same lock +}); +``` -// ❌ Bad: Nested updates (may panic) -entity1.update(cx, |_, cx| { - entity2.update(cx, |_, cx| { /* ... */ }); +**Different entity → generally safe, but indirect cycles still panic.** +Each entity has its own lock, so updating `entity_b` from within `entity_a`'s update normally succeeds. However, if `entity_b`'s callback reaches back into `entity_a` — directly or through a chain — GPUI will attempt to re-acquire `entity_a`'s lock and panic. + +```rust +// ✅ Usually fine: different entities, no cycle +entity_a.update(cx, |_, cx| { + entity_b.update(cx, |_, _| {}); // OK — different lock }); + +// ❌ Panic: indirect cycle back to entity_a +entity_a.update(cx, |_, cx| { + entity_b.update(cx, |_, cx| { + entity_a.update(cx, |_, _| {}); // PANIC — entity_a is still locked + }); +}); +``` + +When in doubt, flatten the call sequence rather than nesting: finish the outer update, then update the second entity from outside. + +**`defer_in` does not bypass the lock.** `cx.defer_in(window, callback)` schedules `callback` to run on the current entity — meaning GPUI re-acquires the entity's lock to execute it. The re-entrancy rules apply equally inside the deferred callback: + +```rust +// ❌ Panic: defer_in re-locks entity_a; calling entity_a.update inside re-enters +impl SomeDelegate for MyAdapter { + fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context>) { + cx.defer_in(window, |list_state, window, cx| { + // list_state is locked for this callback! + parent.update(cx, |this, cx| { + this.list.update(cx, |_, _| {}); // PANIC — list is already locked above + }); + }); + } +} + +// ✅ Fix: use the direct &mut reference the callback provides +impl SomeDelegate for MyAdapter { + fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context>) { + cx.defer_in(window, |list_state, window, cx| { + // Access list data directly — no entity lock needed + list_state.delegate_mut().some_hook(); + + // Update the *parent* entity — different lock, safe + parent.update(cx, |this, cx| { /* … */ }); + + // Sync list state directly after parent update + list_state.delegate_mut().update_snapshot(new_val); + }); + } +} +``` + +**Snapshot pattern for render callbacks.** `render_item` (and any other rendering hook) runs inside the entity's render pass. It must never call `entity.read(cx)` or `entity.update(cx, …)` on any external entity. Instead, keep a plain `snapshot` field updated eagerly from *outside* render after every mutation: + +```rust +// ❌ Panic in render_item — ListState is already locked +fn render_item(&mut self, ix: IndexPath, window: &mut Window, cx: &mut Context>) -> … { + let checked = parent_entity.read(cx).selection.contains(&ix); // PANIC +} + +// ✅ Read from a plain snapshot field — no entity access +fn render_item(&mut self, ix: IndexPath, window: &mut Window, cx: &mut Context>) -> … { + let checked = self.selection_snapshot.iter().any(|(sel_ix, _)| sel_ix == &ix); +} ``` ## Common Use Cases diff --git a/.claude/skills/gpui-entity/references/best-practices.md b/.claude/skills/gpui-entity/references/best-practices.md index 81e023b06b..5e71f18630 100644 --- a/.claude/skills/gpui-entity/references/best-practices.md +++ b/.claude/skills/gpui-entity/references/best-practices.md @@ -4,34 +4,87 @@ Guidelines and best practices for effective entity management in GPUI. ## Avoiding Common Pitfalls -### Avoid Entity Borrowing Conflicts +### Avoid Re-entrant Entity Access (Same Entity) -**Problem:** Nested updates can cause borrow checker panics. +**Problem:** GPUI locks an entity for the entire duration of its render or update pass. Any attempt to `read` or `update` that *same* entity while the lock is held panics: + +``` +cannot update … while it is already being updated +``` ```rust -// ❌ Bad: Nested updates can panic -entity1.update(cx, |_, cx| { - entity2.update(cx, |_, cx| { - // This may panic if entities are related - }); +// ❌ Panic: entity_a updated from within entity_a's own update +entity_a.update(cx, |_, cx| { + entity_a.update(cx, |_, _| {}); // PANIC +}); + +// ✅ Fine: updating a *different* entity from within an update +entity_a.update(cx, |_, cx| { + entity_b.update(cx, |_, _| {}); // OK — different lock }); ``` -**Solution:** Perform updates sequentially. +**Note:** Updating two *different* entities in a nested fashion is safe. The restriction is strictly about re-entering the *same* entity's lock. + +### `defer_in` Re-locks the Entity — Same Rules Apply + +`cx.defer_in(window, callback)` schedules `callback` to run *on the entity the context refers to*. GPUI re-acquires that entity's lock to execute the deferred callback, so the re-entrancy rules apply equally inside it: ```rust -// ✅ Good: Sequential updates -entity1.update(cx, |state1, cx| { - // Update entity1 - state1.value = 42; - cx.notify(); -}); +// ❌ Panic: defer_in runs with ListState locked; calling list.update re-enters +fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context>) { + cx.defer_in(window, |list_state, window, cx| { + // list_state IS locked for this entire callback! + parent.update(cx, |this, cx| { + this.list.update(cx, |_, _| {}); // PANIC — re-enters ListState lock + }); + }); +} +``` -entity2.update(cx, |state2, cx| { - // Update entity2 - state2.value = 100; - cx.notify(); -}); +**Fix:** Use the direct `&mut` reference the callback provides instead of going through the entity handle: + +```rust +// ✅ Correct: direct mutable access — no lock needed +fn confirm(&mut self, _: bool, window: &mut Window, cx: &mut Context>) { + cx.defer_in(window, |list_state, window, cx| { + // Step 1: call hooks directly through list_state (no entity lock) + list_state.delegate_mut().on_will_change(&mut op, &snapshot); + + // Step 2: update the parent entity — different lock, safe + let new_sel = parent.update(cx, |this, cx| { + this.state.apply_change(op); + cx.notify(); + this.state.selection.clone() // return data needed for step 3 + }); + + // Step 3: sync list state directly — no entity lock + if let Ok(sel) = new_sel { + list_state.delegate_mut().update_snapshot(sel.clone()); + list_state.delegate_mut().on_confirm(&sel); + } + }); +} +``` + +### Render Callbacks Must Not Access External Entities + +`render_item` and any other rendering hook runs inside the entity's render pass. Calling `entity.read(cx)` or `entity.update(cx, …)` on an external entity (which may itself be in a render/update pass) panics with the same re-entrancy error. + +**Fix:** Keep a plain `snapshot` field updated eagerly from outside render after every mutation: + +```rust +// ❌ Panic — called during ListState render; external entity access re-enters +fn render_item(&mut self, ix: IndexPath, …) -> … { + let checked = parent.read(cx).selection.contains(&ix); // PANIC +} + +// ✅ Read from snapshot field — no entity access at all +fn render_item(&mut self, ix: IndexPath, …) -> … { + let checked = self.selection_snapshot.iter().any(|(sel_ix, _)| sel_ix == &ix); +} +// After every mutation from outside render: +list.update(cx, |l, _| l.delegate_mut().update_snapshot(new_snapshot)); ``` ### Use Weak References in Closures From 4221d7d6bfae5600c2b54c38614df1a86c1b6580 Mon Sep 17 00:00:00 2001 From: Vincent Esche Date: Tue, 12 May 2026 20:07:21 +0200 Subject: [PATCH 2/2] Expand "gpui-test" skill with re-entrancy crash-free testing patterns --- .claude/skills/gpui-test/SKILL.md | 1 + .claude/skills/gpui-test/reference.md | 119 ++++++++++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/.claude/skills/gpui-test/SKILL.md b/.claude/skills/gpui-test/SKILL.md index 48ef3050ff..ffe3840af5 100644 --- a/.claude/skills/gpui-test/SKILL.md +++ b/.claude/skills/gpui-test/SKILL.md @@ -91,4 +91,5 @@ fn test_with_window(cx: &mut TestAppContext) { ## Additional Resources - For detailed testing patterns and examples, see [reference.md](reference.md) + - Includes **"Testing Crash-Free State Management (Re-entrancy)"** — patterns for catching entity re-entrancy panics (`cannot update … while it is already being updated`) before they reach users. - For best practices and running tests, see [examples.md](examples.md) diff --git a/.claude/skills/gpui-test/reference.md b/.claude/skills/gpui-test/reference.md index eb53daaddf..4aecf439ae 100644 --- a/.claude/skills/gpui-test/reference.md +++ b/.claude/skills/gpui-test/reference.md @@ -198,6 +198,125 @@ async fn test_external_io(cx: &mut TestAppContext) { } ``` +## Testing Crash-Free State Management (Re-entrancy) + +The most dangerous class of GPUI bugs is **entity re-entrancy**: code that tries to update or read an entity while it is already locked in a render/update pass. These bugs are invisible at compile time — they only panic at runtime, typically when the user clicks something in a list or dropdown. + +**Key properties of re-entrancy panics:** +- Triggered by user interaction (click, key press), not during static rendering. +- `#[should_panic]` only confirms the bug exists — tests must pass *without* panicking. +- `cx.run_until_parked()` is required after the triggering action to let deferred callbacks (`defer_in`) execute. + +### Pattern: Drive `confirm` / `cancel` through the real delegate + +```rust +#[gpui::test] +fn test_confirm_does_not_panic(cx: &mut TestAppContext) { + // Build the component state the same way the real app does. + let state = cx.new(|cx| { + SelectState::new(MyDelegate::default(), None, &mut cx.window_handle(), cx) + }); + + // Simulate the user clicking the first item — this exercises on_confirm + // and its deferred callback, which is where re-entrancy bugs hide. + state.update(cx, |this, cx| { + this.state.list.update(cx, |list, cx| { + list.delegate_mut().set_selected_index(Some(IndexPath::new(0)), window, cx); + list.delegate_mut().confirm(false, window, cx); + }); + }); + + // Let defer_in callbacks execute — required to trigger the crash. + cx.run_until_parked(); + + // If we reach here without panicking, the re-entrancy is fixed. + let selected = state.read_with(cx, |s, _| s.selected_value().cloned()); + assert!(selected.is_some()); +} +``` + +### Pattern: Verify `on_will_change` and `on_confirm` hooks are called correctly + +Use a recording delegate to assert that hooks fire with the right arguments and in the right order: + +```rust +#[derive(Default)] +struct RecordingDelegate { + items: Vec, + will_change_calls: Vec>, + confirm_calls: Vec>, +} + +impl SearchableListDelegate for RecordingDelegate { + // … required impls … + + fn on_will_change( + &mut self, + change: &mut SearchableListChange, + _current: &[(IndexPath, Self::Item)], + ) { + self.will_change_calls.push( + change.select_queue.iter().map(|(ix, _)| *ix).collect() + ); + } + + fn on_confirm(&mut self, final_selection: &[(IndexPath, Self::Item)]) { + self.confirm_calls.push( + final_selection.iter().map(|(ix, _)| *ix).collect() + ); + } +} + +#[gpui::test] +fn test_hooks_fire_in_correct_order(cx: &mut TestAppContext) { + let state = cx.new(|cx| SelectState::new(RecordingDelegate::with_items(3), None, window, cx)); + + // Simulate confirm on item 0 + state.update(cx, |this, cx| { + // … trigger confirm … + }); + cx.run_until_parked(); + + state.read_with(cx, |s, cx| { + let delegate = s.state.list.read(cx).delegate().delegate; + assert_eq!(delegate.will_change_calls.len(), 1); + assert_eq!(delegate.confirm_calls.len(), 1); + assert_eq!(delegate.confirm_calls[0], vec![IndexPath::new(0)]); + }); +} +``` + +### Pattern: Rapid multiple confirms (snapshot consistency) + +```rust +#[gpui::test] +fn test_rapid_confirms_keep_consistent_snapshot(cx: &mut TestAppContext) { + let state = cx.new(|cx| SelectState::new(MyDelegate::with_items(5), None, window, cx)); + + for i in 0..5 { + state.update(cx, |this, cx| { + // trigger confirm on item i + }); + cx.run_until_parked(); + + state.read_with(cx, |s, cx| { + let snapshot = s.state.list.read(cx).delegate().selection_snapshot.clone(); + let selection = s.state.selection.clone(); + assert_eq!(snapshot, selection, "snapshot out of sync after confirm {i}"); + }); + } +} +``` + +### Checklist: What to test for each component that uses `defer_in` + +- [ ] `confirm` path: no panic, correct final selection, snapshot matches selection +- [ ] `cancel` path: no panic, selection unchanged, popover closed +- [ ] `on_will_change` veto: selection not changed, `on_confirm` not called +- [ ] `on_will_change` mutating the change: final selection reflects delegate's modification +- [ ] Rapid consecutive confirms: each one leaves snapshot consistent with selection +- [ ] `render_item` never panics even when called immediately after a mutation + ## Property Testing Use random data to test edge cases: