From d49cd0019f4d2789c87eab651bc7bd2cbd8004e3 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 30 Oct 2024 14:49:47 +0200 Subject: [PATCH] Log prettier errors on failures (#19951) Closes https://github.com/zed-industries/zed/issues/11987 Release Notes: - Fixed prettier not reporting failures in the status panel on formatting and installation errors --- Cargo.lock | 1 + crates/activity_indicator/Cargo.toml | 1 + .../src/activity_indicator.rs | 22 ++- crates/prettier/src/prettier.rs | 6 +- crates/project/src/lsp_store.rs | 55 +++++--- crates/proto/src/error.rs | 14 +- crates/remote/src/ssh_session.rs | 132 ++++++++++-------- 7 files changed, 146 insertions(+), 85 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 56d538a883..266c1922bb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -16,6 +16,7 @@ dependencies = [ "project", "smallvec", "ui", + "util", "workspace", ] diff --git a/crates/activity_indicator/Cargo.toml b/crates/activity_indicator/Cargo.toml index 9761a08238..b4fb2ec5b0 100644 --- a/crates/activity_indicator/Cargo.toml +++ b/crates/activity_indicator/Cargo.toml @@ -23,6 +23,7 @@ language.workspace = true project.workspace = true smallvec.workspace = true ui.workspace = true +util.workspace = true workspace.workspace = true [dev-dependencies] diff --git a/crates/activity_indicator/src/activity_indicator.rs b/crates/activity_indicator/src/activity_indicator.rs index 8020e0665a..90410d534c 100644 --- a/crates/activity_indicator/src/activity_indicator.rs +++ b/crates/activity_indicator/src/activity_indicator.rs @@ -13,7 +13,8 @@ use language::{ use project::{EnvironmentErrorMessage, LanguageServerProgress, Project, WorktreeId}; use smallvec::SmallVec; use std::{cmp::Reverse, fmt::Write, sync::Arc, time::Duration}; -use ui::{prelude::*, ButtonLike, ContextMenu, PopoverMenu, PopoverMenuHandle}; +use ui::{prelude::*, ButtonLike, ContextMenu, PopoverMenu, PopoverMenuHandle, Tooltip}; +use util::truncate_and_trailoff; use workspace::{item::ItemHandle, StatusItemView, Workspace}; actions!(activity_indicator, [ShowErrorMessage]); @@ -446,6 +447,8 @@ impl ActivityIndicator { impl EventEmitter for ActivityIndicator {} +const MAX_MESSAGE_LEN: usize = 50; + impl Render for ActivityIndicator { fn render(&mut self, cx: &mut ViewContext) -> impl IntoElement { let result = h_flex() @@ -456,6 +459,7 @@ impl Render for ActivityIndicator { return result; }; let this = cx.view().downgrade(); + let truncate_content = content.message.len() > MAX_MESSAGE_LEN; result.gap_2().child( PopoverMenu::new("activity-indicator-popover") .trigger( @@ -464,7 +468,21 @@ impl Render for ActivityIndicator { .id("activity-indicator-status") .gap_2() .children(content.icon) - .child(Label::new(content.message).size(LabelSize::Small)) + .map(|button| { + if truncate_content { + button + .child( + Label::new(truncate_and_trailoff( + &content.message, + MAX_MESSAGE_LEN, + )) + .size(LabelSize::Small), + ) + .tooltip(move |cx| Tooltip::text(&content.message, cx)) + } else { + button.child(Label::new(content.message).size(LabelSize::Small)) + } + }) .when_some(content.on_click, |this, handler| { this.on_click(cx.listener(move |this, _, cx| { handler(this, cx); diff --git a/crates/prettier/src/prettier.rs b/crates/prettier/src/prettier.rs index 4dc5bca40f..d7b13c9992 100644 --- a/crates/prettier/src/prettier.rs +++ b/crates/prettier/src/prettier.rs @@ -329,11 +329,7 @@ impl Prettier { })? .context("prettier params calculation")?; - let response = local - .server - .request::(params) - .await - .context("prettier format request")?; + let response = local.server.request::(params).await?; let diff_task = buffer.update(cx, |buffer, cx| buffer.diff(response.text, cx))?; Ok(diff_task.await) } diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index e04577e551..fe4127d536 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -29,6 +29,7 @@ use gpui::{ Task, WeakModel, }; use http_client::HttpClient; +use itertools::Itertools as _; use language::{ language_settings::{ language_settings, FormatOnSave, Formatter, LanguageSettings, SelectedFormatter, @@ -144,7 +145,6 @@ pub struct LocalLspStore { HashMap)>, prettier_store: Model, current_lsp_settings: HashMap, - last_formatting_failure: Option, _subscription: gpui::Subscription, } @@ -563,9 +563,7 @@ impl LocalLspStore { })?; prettier_store::format_with_prettier(&prettier, &buffer.handle, cx) .await - .transpose() - .ok() - .flatten() + .transpose()? } Formatter::External { command, arguments } => { Self::format_via_external_command(buffer, command, arguments.as_deref(), cx) @@ -705,6 +703,7 @@ impl LspStoreMode { pub struct LspStore { mode: LspStoreMode, + last_formatting_failure: Option, downstream_client: Option<(AnyProtoClient, u64)>, nonce: u128, buffer_store: Model, @@ -907,7 +906,6 @@ impl LspStore { language_server_watcher_registrations: Default::default(), current_lsp_settings: ProjectSettings::get_global(cx).lsp.clone(), buffers_being_formatted: Default::default(), - last_formatting_failure: None, prettier_store, environment, http_client, @@ -917,6 +915,7 @@ impl LspStore { this.as_local_mut().unwrap().shutdown_language_servers(cx) }), }), + last_formatting_failure: None, downstream_client: None, buffer_store, worktree_store, @@ -977,6 +976,7 @@ impl LspStore { upstream_project_id: project_id, }), downstream_client: None, + last_formatting_failure: None, buffer_store, worktree_store, languages: languages.clone(), @@ -5265,9 +5265,9 @@ impl LspStore { .map(language::proto::serialize_transaction), }) } + pub fn last_formatting_failure(&self) -> Option<&str> { - self.as_local() - .and_then(|local| local.last_formatting_failure.as_deref()) + self.last_formatting_failure.as_deref() } pub fn environment_for_buffer( @@ -5338,23 +5338,16 @@ impl LspStore { cx.clone(), ) .await; - lsp_store.update(&mut cx, |lsp_store, _| { - let local = lsp_store.as_local_mut().unwrap(); - match &result { - Ok(_) => local.last_formatting_failure = None, - Err(error) => { - local.last_formatting_failure.replace(error.to_string()); - } - } + lsp_store.update_last_formatting_failure(&result); })?; result }) } else if let Some((client, project_id)) = self.upstream_client() { let buffer_store = self.buffer_store(); - cx.spawn(move |_, mut cx| async move { - let response = client + cx.spawn(move |lsp_store, mut cx| async move { + let result = client .request(proto::FormatBuffers { project_id, trigger: trigger as i32, @@ -5365,13 +5358,21 @@ impl LspStore { }) .collect::>()?, }) - .await? - .transaction - .ok_or_else(|| anyhow!("missing transaction"))?; + .await + .and_then(|result| result.transaction.context("missing transaction")); + lsp_store.update(&mut cx, |lsp_store, _| { + lsp_store.update_last_formatting_failure(&result); + })?; + + let transaction_response = result?; buffer_store .update(&mut cx, |buffer_store, cx| { - buffer_store.deserialize_project_transaction(response, push_to_history, cx) + buffer_store.deserialize_project_transaction( + transaction_response, + push_to_history, + cx, + ) })? .await }) @@ -7366,6 +7367,18 @@ impl LspStore { lsp_action, }) } + + fn update_last_formatting_failure(&mut self, formatting_result: &anyhow::Result) { + match &formatting_result { + Ok(_) => self.last_formatting_failure = None, + Err(error) => { + let error_string = format!("{error:#}"); + log::error!("Formatting failed: {error_string}"); + self.last_formatting_failure + .replace(error_string.lines().join(" ")); + } + } + } } impl EventEmitter for LspStore {} diff --git a/crates/proto/src/error.rs b/crates/proto/src/error.rs index 8a87d6fdc9..680056fc1c 100644 --- a/crates/proto/src/error.rs +++ b/crates/proto/src/error.rs @@ -104,7 +104,19 @@ impl ErrorExt for anyhow::Error { if let Some(rpc_error) = self.downcast_ref::() { rpc_error.to_proto() } else { - ErrorCode::Internal.message(format!("{}", self)).to_proto() + ErrorCode::Internal + .message( + format!("{self:#}") + .lines() + .fold(String::new(), |mut message, line| { + if !message.is_empty() { + message.push(' '); + } + message.push_str(line); + message + }), + ) + .to_proto() } } diff --git a/crates/remote/src/ssh_session.rs b/crates/remote/src/ssh_session.rs index 0f6c90de43..a69f0330ff 100644 --- a/crates/remote/src/ssh_session.rs +++ b/crates/remote/src/ssh_session.rs @@ -2017,77 +2017,97 @@ impl ChannelClient { mut incoming_rx: mpsc::UnboundedReceiver, cx: &AsyncAppContext, ) -> Task> { - cx.spawn(|cx| { - async move { - let peer_id = PeerId { owner_id: 0, id: 0 }; - while let Some(incoming) = incoming_rx.next().await { - let Some(this) = this.upgrade() else { - return anyhow::Ok(()); - }; - if let Some(ack_id) = incoming.ack_id { - let mut buffer = this.buffer.lock(); - while buffer.front().is_some_and(|msg| msg.id <= ack_id) { - buffer.pop_front(); + cx.spawn(|cx| async move { + let peer_id = PeerId { owner_id: 0, id: 0 }; + while let Some(incoming) = incoming_rx.next().await { + let Some(this) = this.upgrade() else { + return anyhow::Ok(()); + }; + if let Some(ack_id) = incoming.ack_id { + let mut buffer = this.buffer.lock(); + while buffer.front().is_some_and(|msg| msg.id <= ack_id) { + buffer.pop_front(); + } + } + if let Some(proto::envelope::Payload::FlushBufferedMessages(_)) = &incoming.payload + { + log::debug!( + "{}:ssh message received. name:FlushBufferedMessages", + this.name + ); + { + let buffer = this.buffer.lock(); + for envelope in buffer.iter() { + this.outgoing_tx + .lock() + .unbounded_send(envelope.clone()) + .ok(); } } - if let Some(proto::envelope::Payload::FlushBufferedMessages(_)) = - &incoming.payload - { - log::debug!("{}:ssh message received. name:FlushBufferedMessages", this.name); - { - let buffer = this.buffer.lock(); - for envelope in buffer.iter() { - this.outgoing_tx.lock().unbounded_send(envelope.clone()).ok(); - } + let mut envelope = proto::Ack {}.into_envelope(0, Some(incoming.id), None); + envelope.id = this.next_message_id.fetch_add(1, SeqCst); + this.outgoing_tx.lock().unbounded_send(envelope).ok(); + continue; + } + + this.max_received.store(incoming.id, SeqCst); + + if let Some(request_id) = incoming.responding_to { + let request_id = MessageId(request_id); + let sender = this.response_channels.lock().remove(&request_id); + if let Some(sender) = sender { + let (tx, rx) = oneshot::channel(); + if incoming.payload.is_some() { + sender.send((incoming, tx)).ok(); } - let mut envelope = proto::Ack{}.into_envelope(0, Some(incoming.id), None); - envelope.id = this.next_message_id.fetch_add(1, SeqCst); - this.outgoing_tx.lock().unbounded_send(envelope).ok(); - continue; + rx.await.ok(); } - - this.max_received.store(incoming.id, SeqCst); - - if let Some(request_id) = incoming.responding_to { - let request_id = MessageId(request_id); - let sender = this.response_channels.lock().remove(&request_id); - if let Some(sender) = sender { - let (tx, rx) = oneshot::channel(); - if incoming.payload.is_some() { - sender.send((incoming, tx)).ok(); - } - rx.await.ok(); - } - } else if let Some(envelope) = - build_typed_envelope(peer_id, Instant::now(), incoming) - { - let type_name = envelope.payload_type_name(); - if let Some(future) = ProtoMessageHandlerSet::handle_message( - &this.message_handlers, - envelope, - this.clone().into(), - cx.clone(), - ) { - log::debug!("{}:ssh message received. name:{type_name}", this.name); - cx.foreground_executor().spawn(async move { + } else if let Some(envelope) = + build_typed_envelope(peer_id, Instant::now(), incoming) + { + let type_name = envelope.payload_type_name(); + if let Some(future) = ProtoMessageHandlerSet::handle_message( + &this.message_handlers, + envelope, + this.clone().into(), + cx.clone(), + ) { + log::debug!("{}:ssh message received. name:{type_name}", this.name); + cx.foreground_executor() + .spawn(async move { match future.await { Ok(_) => { - log::debug!("{}:ssh message handled. name:{type_name}", this.name); + log::debug!( + "{}:ssh message handled. name:{type_name}", + this.name + ); } Err(error) => { log::error!( - "{}:error handling message. type:{type_name}, error:{error}", this.name, + "{}:error handling message. type:{}, error:{}", + this.name, + type_name, + format!("{error:#}").lines().fold( + String::new(), + |mut message, line| { + if !message.is_empty() { + message.push(' '); + } + message.push_str(line); + message + } + ) ); } } - }).detach() - } else { - log::error!("{}:unhandled ssh message name:{type_name}", this.name); - } + }) + .detach() + } else { + log::error!("{}:unhandled ssh message name:{type_name}", this.name); } } - anyhow::Ok(()) } + anyhow::Ok(()) }) }