From f63ae4388dae45d75a1fe0bdde95355f6e00351b Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Fri, 13 Jun 2025 00:05:57 -0600 Subject: [PATCH] debugger: Show errors loading stack (#32658) - **TEMP** - **Show errors loading stack frames** - **Stop cloning every DAP response unnecessarily** Closes #ISSUE Release Notes: - debugger: Show errors loading stack frames. Screenshot 2025-06-12 at 23 53 42 --- .../src/session/running/stack_frame_list.rs | 57 +++++-- crates/project/src/debugger/session.rs | 152 ++++++++++-------- 2 files changed, 123 insertions(+), 86 deletions(-) diff --git a/crates/debugger_ui/src/session/running/stack_frame_list.rs b/crates/debugger_ui/src/session/running/stack_frame_list.rs index a821b984d3..246f582e37 100644 --- a/crates/debugger_ui/src/session/running/stack_frame_list.rs +++ b/crates/debugger_ui/src/session/running/stack_frame_list.rs @@ -36,6 +36,7 @@ pub struct StackFrameList { opened_stack_frame_id: Option, scrollbar_state: ScrollbarState, list_state: ListState, + error: Option, _refresh_task: Task<()>, } @@ -82,6 +83,7 @@ impl StackFrameList { state, _subscription, entries: Default::default(), + error: None, selected_ix: None, opened_stack_frame_id: None, list_state, @@ -113,21 +115,19 @@ impl StackFrameList { .collect::>() } - fn stack_frames(&self, cx: &mut App) -> Vec { - self.state - .read_with(cx, |state, _| state.thread_id) - .ok() - .flatten() - .map(|thread_id| { - self.session - .update(cx, |this, cx| this.stack_frames(thread_id, cx)) - }) - .unwrap_or_default() + fn stack_frames(&self, cx: &mut App) -> Result> { + if let Ok(Some(thread_id)) = self.state.read_with(cx, |state, _| state.thread_id) { + self.session + .update(cx, |this, cx| this.stack_frames(thread_id, cx)) + } else { + Ok(Vec::default()) + } } #[cfg(test)] pub(crate) fn dap_stack_frames(&self, cx: &mut App) -> Vec { self.stack_frames(cx) + .unwrap_or_default() .into_iter() .map(|stack_frame| stack_frame.dap.clone()) .collect() @@ -149,7 +149,7 @@ impl StackFrameList { let debounce = this .update(cx, |this, cx| { let new_stack_frames = this.stack_frames(cx); - new_stack_frames.is_empty() && !this.entries.is_empty() + new_stack_frames.unwrap_or_default().is_empty() && !this.entries.is_empty() }) .ok() .unwrap_or_default(); @@ -185,7 +185,18 @@ impl StackFrameList { let mut first_stack_frame = None; let mut first_not_subtle_frame = None; - let stack_frames = self.stack_frames(cx); + let stack_frames = match self.stack_frames(cx) { + Ok(stack_frames) => stack_frames, + Err(e) => { + self.error = Some(format!("{}", e).into()); + self.entries.clear(); + self.selected_ix = None; + self.list_state.reset(0); + cx.emit(StackFrameListEvent::BuiltEntries); + cx.notify(); + return; + } + }; for stack_frame in &stack_frames { match stack_frame.dap.presentation_hint { Some(dap::StackFramePresentationHint::Deemphasize) => { @@ -212,8 +223,7 @@ impl StackFrameList { if !collapsed_entries.is_empty() { entries.push(StackFrameEntry::Collapsed(collapsed_entries.clone())); } - - std::mem::swap(&mut self.entries, &mut entries); + self.entries = entries; if let Some(ix) = first_not_subtle_frame .or(first_stack_frame) @@ -657,7 +667,7 @@ impl StackFrameList { } fn render_list(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { - list(self.list_state.clone()).size_full() + div().p_1().child(list(self.list_state.clone()).size_full()) } } @@ -666,12 +676,27 @@ impl Render for StackFrameList { div() .track_focus(&self.focus_handle) .size_full() - .p_1() .on_action(cx.listener(Self::select_next)) .on_action(cx.listener(Self::select_previous)) .on_action(cx.listener(Self::select_first)) .on_action(cx.listener(Self::select_last)) .on_action(cx.listener(Self::confirm)) + .when_some(self.error.clone(), |el, error| { + el.child( + h_flex() + .bg(cx.theme().status().warning_background) + .border_b_1() + .border_color(cx.theme().status().warning_border) + .pl_1() + .child(Icon::new(IconName::Warning).color(Color::Warning)) + .gap_2() + .child( + Label::new(error) + .size(LabelSize::Small) + .color(Color::Warning), + ), + ) + }) .child(self.render_list(window, cx)) .child(self.render_vertical_scrollbar(cx)) } diff --git a/crates/project/src/debugger/session.rs b/crates/project/src/debugger/session.rs index cbcc05443f..917aae513b 100644 --- a/crates/project/src/debugger/session.rs +++ b/crates/project/src/debugger/session.rs @@ -36,6 +36,7 @@ use gpui::{ Task, WeakEntity, }; +use rpc::ErrorExt; use serde_json::Value; use smol::stream::StreamExt; use std::any::TypeId; @@ -108,6 +109,7 @@ impl ThreadStatus { pub struct Thread { dap: dap::Thread, stack_frames: Vec, + stack_frames_error: Option, _has_stopped: bool, } @@ -116,6 +118,7 @@ impl From for Thread { Self { dap, stack_frames: Default::default(), + stack_frames_error: None, _has_stopped: false, } } @@ -1388,12 +1391,7 @@ impl Session { fn fetch( &mut self, request: T, - process_result: impl FnOnce( - &mut Self, - Result, - &mut Context, - ) -> Option - + 'static, + process_result: impl FnOnce(&mut Self, Result, &mut Context) + 'static, cx: &mut Context, ) { const { @@ -1422,7 +1420,10 @@ impl Session { &self.capabilities, &self.mode, command, - process_result, + |this, result, cx| { + process_result(this, result, cx); + None + }, cx, ); let task = cx @@ -1438,17 +1439,6 @@ impl Session { } } - pub async fn request2( - &self, - request: T, - ) -> Result { - if !T::is_supported(&self.capabilities) { - anyhow::bail!("DAP request {:?} is not supported", request); - } - - self.mode.request_dap(request).await - } - fn request_inner( capabilities: &Capabilities, mode: &Mode, @@ -1535,18 +1525,18 @@ impl Session { self.fetch( dap_command::ThreadsCommand, |this, result, cx| { - let result = result.log_err()?; + let Some(result) = result.log_err() else { + return; + }; this.threads = result - .iter() + .into_iter() .map(|thread| (ThreadId(thread.id), Thread::from(thread.clone()))) .collect(); this.invalidate_command_type::(); cx.emit(SessionEvent::Threads); cx.notify(); - - Some(result) }, cx, ); @@ -1566,13 +1556,13 @@ impl Session { self.fetch( dap_command::ModulesCommand, |this, result, cx| { - let result = result.log_err()?; + let Some(result) = result.log_err() else { + return; + }; - this.modules = result.iter().cloned().collect(); + this.modules = result; cx.emit(SessionEvent::Modules); cx.notify(); - - Some(result) }, cx, ); @@ -1651,11 +1641,12 @@ impl Session { self.fetch( dap_command::LoadedSourcesCommand, |this, result, cx| { - let result = result.log_err()?; - this.loaded_sources = result.iter().cloned().collect(); + let Some(result) = result.log_err() else { + return; + }; + this.loaded_sources = result; cx.emit(SessionEvent::LoadedSources); cx.notify(); - Some(result) }, cx, ); @@ -1961,7 +1952,11 @@ impl Session { .detach(); } - pub fn stack_frames(&mut self, thread_id: ThreadId, cx: &mut Context) -> Vec { + pub fn stack_frames( + &mut self, + thread_id: ThreadId, + cx: &mut Context, + ) -> Result> { if self.thread_states.thread_status(thread_id) == ThreadStatus::Stopped && self.requests.contains_key(&ThreadsCommand.type_id()) && self.threads.contains_key(&thread_id) @@ -1977,48 +1972,63 @@ impl Session { levels: None, }, move |this, stack_frames, cx| { - let stack_frames = stack_frames.log_err()?; - - let entry = this.threads.entry(thread_id).and_modify(|thread| { - thread.stack_frames = - stack_frames.iter().cloned().map(StackFrame::from).collect(); - }); + let entry = + this.threads + .entry(thread_id) + .and_modify(|thread| match &stack_frames { + Ok(stack_frames) => { + thread.stack_frames = stack_frames + .iter() + .cloned() + .map(StackFrame::from) + .collect(); + thread.stack_frames_error = None; + } + Err(error) => { + thread.stack_frames.clear(); + thread.stack_frames_error = Some(error.cloned()); + } + }); debug_assert!( matches!(entry, indexmap::map::Entry::Occupied(_)), "Sent request for thread_id that doesn't exist" ); - - this.stack_frames.extend( - stack_frames - .iter() - .filter(|frame| { - // Workaround for JavaScript debug adapter sending out "fake" stack frames for delineating await points. This is fine, - // except that they always use an id of 0 for it, which collides with other (valid) stack frames. - !(frame.id == 0 - && frame.line == 0 - && frame.column == 0 - && frame.presentation_hint - == Some(StackFramePresentationHint::Label)) - }) - .cloned() - .map(|frame| (frame.id, StackFrame::from(frame))), - ); + if let Ok(stack_frames) = stack_frames { + this.stack_frames.extend( + stack_frames + .into_iter() + .filter(|frame| { + // Workaround for JavaScript debug adapter sending out "fake" stack frames for delineating await points. This is fine, + // except that they always use an id of 0 for it, which collides with other (valid) stack frames. + !(frame.id == 0 + && frame.line == 0 + && frame.column == 0 + && frame.presentation_hint + == Some(StackFramePresentationHint::Label)) + }) + .map(|frame| (frame.id, StackFrame::from(frame))), + ); + } this.invalidate_command_type::(); this.invalidate_command_type::(); cx.emit(SessionEvent::StackTrace); - cx.notify(); - Some(stack_frames) }, cx, ); } - self.threads - .get(&thread_id) - .map(|thread| thread.stack_frames.clone()) - .unwrap_or_default() + match self.threads.get(&thread_id) { + Some(thread) => { + if let Some(error) = &thread.stack_frames_error { + Err(error.cloned()) + } else { + Ok(thread.stack_frames.clone()) + } + } + None => Ok(Vec::new()), + } } pub fn scopes(&mut self, stack_frame_id: u64, cx: &mut Context) -> &[dap::Scope] { @@ -2030,9 +2040,11 @@ impl Session { self.fetch( ScopesCommand { stack_frame_id }, move |this, scopes, cx| { - let scopes = scopes.log_err()?; + let Some(scopes) = scopes.log_err() else { + return + }; - for scope in scopes .iter(){ + for scope in scopes.iter() { this.variables(scope.variables_reference, cx); } @@ -2040,7 +2052,7 @@ impl Session { .stack_frames .entry(stack_frame_id) .and_modify(|stack_frame| { - stack_frame.scopes = scopes.clone(); + stack_frame.scopes = scopes; }); cx.emit(SessionEvent::Variables); @@ -2049,8 +2061,6 @@ impl Session { matches!(entry, indexmap::map::Entry::Occupied(_)), "Sent scopes request for stack_frame_id that doesn't exist or hasn't been fetched" ); - - Some(scopes) }, cx, ); @@ -2092,13 +2102,14 @@ impl Session { self.fetch( command, move |this, variables, cx| { - let variables = variables.log_err()?; - this.variables - .insert(variables_reference, variables.clone()); + let Some(variables) = variables.log_err() else { + return; + }; + + this.variables.insert(variables_reference, variables); cx.emit(SessionEvent::Variables); cx.emit(SessionEvent::InvalidateInlineValue); - Some(variables) }, cx, ); @@ -2209,9 +2220,10 @@ impl Session { self.fetch( LocationsCommand { reference }, move |this, response, _| { - let response = response.log_err()?; - this.locations.insert(reference, response.clone()); - Some(response) + let Some(response) = response.log_err() else { + return; + }; + this.locations.insert(reference, response); }, cx, );