Improve command logging and `log_err` module paths (#39674)

Lukas Wirth created

Prior we only logged the crate in `log_err`, which is not too helpful.
We now assemble the module path from the file system path.

Release Notes:

- N/A *or* Added/Fixed/Improved ...

Change summary

crates/context_server/src/transport/stdio_transport.rs |  9 
crates/dap/src/transport.rs                            | 20 +-
crates/extensions_ui/src/extensions_ui.rs              | 11 -
crates/gpui/src/platform/windows/clipboard.rs          | 86 ++++++-----
crates/lsp/src/lsp.rs                                  | 13 -
crates/onboarding/src/welcome.rs                       | 11 -
crates/repl/src/repl_sessions_ui.rs                    | 10 -
crates/util/src/util.rs                                | 10 
crates/workspace/src/workspace.rs                      |  2 
crates/zed/src/zed.rs                                  | 77 +++++-----
10 files changed, 112 insertions(+), 137 deletions(-)

Detailed changes

crates/context_server/src/transport/stdio_transport.rs 🔗

@@ -41,12 +41,9 @@ impl StdioTransport {
             command.current_dir(working_directory);
         }
 
-        let mut server = command.spawn().with_context(|| {
-            format!(
-                "failed to spawn command. (path={:?}, args={:?})",
-                binary.executable, &binary.args
-            )
-        })?;
+        let mut server = command
+            .spawn()
+            .with_context(|| format!("failed to spawn command {command:?})",))?;
 
         let stdin = server.stdin.take().unwrap();
         let stdout = server.stdout.take().unwrap();

crates/dap/src/transport.rs 🔗

@@ -674,13 +674,7 @@ impl StdioTransport {
         command.args(&binary.arguments);
         command.envs(&binary.envs);
 
-        let mut process = Child::spawn(command, Stdio::piped()).with_context(|| {
-            format!(
-                "failed to spawn command `{} {}`.",
-                binary_command,
-                binary.arguments.join(" ")
-            )
-        })?;
+        let mut process = Child::spawn(command, Stdio::piped())?;
 
         let err_task = process.stderr.take().map(|stderr| {
             cx.background_spawn(TransportDelegate::handle_adapter_log(
@@ -1058,11 +1052,13 @@ impl Child {
     #[cfg(not(windows))]
     fn spawn(mut command: std::process::Command, stdin: Stdio) -> Result<Self> {
         util::set_pre_exec_to_start_new_session(&mut command);
-        let process = smol::process::Command::from(command)
+        let mut command = smol::process::Command::from(command);
+        let process = command
             .stdin(stdin)
             .stdout(Stdio::piped())
             .stderr(Stdio::piped())
-            .spawn()?;
+            .spawn()
+            .with_context(|| format!("failed to spawn command `{command:?}`",))?;
         Ok(Self { process })
     }
 
@@ -1070,11 +1066,13 @@ impl Child {
     fn spawn(command: std::process::Command, stdin: Stdio) -> Result<Self> {
         // TODO(windows): create a job object and add the child process handle to it,
         // see https://learn.microsoft.com/en-us/windows/win32/procthread/job-objects
-        let process = smol::process::Command::from(command)
+        let mut command = smol::process::Command::from(command);
+        let process = command
             .stdin(stdin)
             .stdout(Stdio::piped())
             .stderr(Stdio::piped())
-            .spawn()?;
+            .spawn()
+            .with_context(|| format!("failed to spawn command `{command:?}`",))?;
         Ok(Self { process })
     }
 

crates/extensions_ui/src/extensions_ui.rs 🔗

@@ -29,7 +29,7 @@ use ui::{
 };
 use vim_mode_setting::VimModeSetting;
 use workspace::{
-    Workspace, WorkspaceId,
+    Workspace,
     item::{Item, ItemEvent},
 };
 use zed_actions::ExtensionCategoryFilter;
@@ -1551,15 +1551,6 @@ impl Item for ExtensionsPage {
         false
     }
 
-    fn clone_on_split(
-        &self,
-        _workspace_id: Option<WorkspaceId>,
-        _window: &mut Window,
-        _: &mut Context<Self>,
-    ) -> Option<Entity<Self>> {
-        None
-    }
-
     fn to_item_events(event: &Self::Event, mut f: impl FnMut(workspace::item::ItemEvent)) {
         f(*event)
     }

crates/gpui/src/platform/windows/clipboard.rs 🔗

@@ -3,7 +3,6 @@ use std::sync::LazyLock;
 use anyhow::Result;
 use collections::{FxHashMap, FxHashSet};
 use itertools::Itertools;
-use util::ResultExt;
 use windows::Win32::{
     Foundation::{HANDLE, HGLOBAL},
     System::{
@@ -76,14 +75,18 @@ enum ClipboardFormatType {
 }
 
 pub(crate) fn write_to_clipboard(item: ClipboardItem) {
-    write_to_clipboard_inner(item).log_err();
-    unsafe { CloseClipboard().log_err() };
+    with_clipboard(|| write_to_clipboard_inner(item));
 }
 
 pub(crate) fn read_from_clipboard() -> Option<ClipboardItem> {
-    let result = read_from_clipboard_inner();
-    unsafe { CloseClipboard().log_err() };
-    result
+    with_clipboard(|| {
+        with_best_match_format(|item_format| match format_to_type(item_format) {
+            ClipboardFormatType::Text => read_string_from_clipboard(),
+            ClipboardFormatType::Image => read_image_from_clipboard(item_format),
+            ClipboardFormatType::Files => read_files_from_clipboard(),
+        })
+    })
+    .flatten()
 }
 
 pub(crate) fn with_file_names<F>(hdrop: HDROP, mut f: F)
@@ -96,11 +99,33 @@ where
         let mut buffer = vec![0u16; filename_length + 1];
         let ret = unsafe { DragQueryFileW(hdrop, file_index, Some(buffer.as_mut_slice())) };
         if ret == 0 {
-            log::error!("unable to read file name");
+            log::error!("unable to read file name of dragged file");
             continue;
         }
-        if let Some(file_name) = String::from_utf16(&buffer[0..filename_length]).log_err() {
-            f(file_name);
+        match String::from_utf16(&buffer[0..filename_length]) {
+            Ok(file_name) => f(file_name),
+            Err(e) => {
+                log::error!("dragged file name is not UTF-16: {}", e)
+            }
+        }
+    }
+}
+
+fn with_clipboard<F, T>(f: F) -> Option<T>
+where
+    F: FnOnce() -> T,
+{
+    match unsafe { OpenClipboard(None) } {
+        Ok(()) => {
+            let result = f();
+            if let Err(e) = unsafe { CloseClipboard() } {
+                log::error!("Failed to close clipboard: {e}",);
+            }
+            Some(result)
+        }
+        Err(e) => {
+            log::error!("Failed to open clipboard: {e}",);
+            None
         }
     }
 }
@@ -124,7 +149,6 @@ fn format_to_type(item_format: u32) -> &'static ClipboardFormatType {
 // Currently, we only write the first item.
 fn write_to_clipboard_inner(item: ClipboardItem) -> Result<()> {
     unsafe {
-        OpenClipboard(None)?;
         EmptyClipboard()?;
     }
     match item.entries().first() {
@@ -215,15 +239,6 @@ fn convert_image_to_png_format(bytes: &[u8], image_format: ImageFormat) -> Resul
     Ok(output_buf)
 }
 
-fn read_from_clipboard_inner() -> Option<ClipboardItem> {
-    unsafe { OpenClipboard(None) }.log_err()?;
-    with_best_match_format(|item_format| match format_to_type(item_format) {
-        ClipboardFormatType::Text => read_string_from_clipboard(),
-        ClipboardFormatType::Image => read_image_from_clipboard(item_format),
-        ClipboardFormatType::Files => read_files_from_clipboard(),
-    })
-}
-
 // Here, we enumerate all formats on the clipboard and find the first one that we can process.
 // The reason we don't use `GetPriorityClipboardFormat` is that it sometimes returns the
 // wrong format.
@@ -266,7 +281,7 @@ where
 }
 
 fn read_string_from_clipboard() -> Option<ClipboardEntry> {
-    let text = with_clipboard_data(CF_UNICODETEXT.0 as u32, |data_ptr| {
+    let text = with_clipboard_data(CF_UNICODETEXT.0 as u32, |data_ptr, _| {
         let pcwstr = PCWSTR(data_ptr as *const u16);
         String::from_utf16_lossy(unsafe { pcwstr.as_wide() })
     })?;
@@ -290,20 +305,22 @@ fn read_hash_from_clipboard() -> Option<u64> {
     if unsafe { IsClipboardFormatAvailable(*CLIPBOARD_HASH_FORMAT).is_err() } {
         return None;
     }
-    with_clipboard_data(*CLIPBOARD_HASH_FORMAT, |data_ptr| {
+    with_clipboard_data(*CLIPBOARD_HASH_FORMAT, |data_ptr, size| {
+        if size < 8 {
+            return None;
+        }
         let hash_bytes: [u8; 8] = unsafe {
             std::slice::from_raw_parts(data_ptr.cast::<u8>(), 8)
-                .to_vec()
                 .try_into()
-                .log_err()
+                .ok()
         }?;
         Some(u64::from_ne_bytes(hash_bytes))
     })?
 }
 
 fn read_metadata_from_clipboard() -> Option<String> {
-    unsafe { IsClipboardFormatAvailable(*CLIPBOARD_METADATA_FORMAT).log_err()? };
-    with_clipboard_data(*CLIPBOARD_METADATA_FORMAT, |data_ptr| {
+    unsafe { IsClipboardFormatAvailable(*CLIPBOARD_METADATA_FORMAT).ok()? };
+    with_clipboard_data(*CLIPBOARD_METADATA_FORMAT, |data_ptr, _size| {
         let pcwstr = PCWSTR(data_ptr as *const u16);
         String::from_utf16_lossy(unsafe { pcwstr.as_wide() })
     })
@@ -320,7 +337,7 @@ fn format_number_to_image_format(format_number: u32) -> Option<&'static ImageFor
 }
 
 fn read_image_for_type(format_number: u32, format: ImageFormat) -> Option<ClipboardEntry> {
-    let (bytes, id) = with_clipboard_data_and_size(format_number, |data_ptr, size| {
+    let (bytes, id) = with_clipboard_data(format_number, |data_ptr, size| {
         let bytes = unsafe { std::slice::from_raw_parts(data_ptr as *mut u8 as _, size).to_vec() };
         let id = hash(&bytes);
         (bytes, id)
@@ -329,7 +346,7 @@ fn read_image_for_type(format_number: u32, format: ImageFormat) -> Option<Clipbo
 }
 
 fn read_files_from_clipboard() -> Option<ClipboardEntry> {
-    let text = with_clipboard_data(CF_HDROP.0 as u32, |data_ptr| {
+    let text = with_clipboard_data(CF_HDROP.0 as u32, |data_ptr, _size| {
         let hdrop = HDROP(data_ptr);
         let mut filenames = String::new();
         with_file_names(hdrop, |file_name| {
@@ -344,25 +361,14 @@ fn read_files_from_clipboard() -> Option<ClipboardEntry> {
 }
 
 fn with_clipboard_data<F, R>(format: u32, f: F) -> Option<R>
-where
-    F: FnOnce(*mut std::ffi::c_void) -> R,
-{
-    let global = HGLOBAL(unsafe { GetClipboardData(format).log_err() }?.0);
-    let data_ptr = unsafe { GlobalLock(global) };
-    let result = f(data_ptr);
-    unsafe { GlobalUnlock(global).log_err() };
-    Some(result)
-}
-
-fn with_clipboard_data_and_size<F, R>(format: u32, f: F) -> Option<R>
 where
     F: FnOnce(*mut std::ffi::c_void, usize) -> R,
 {
-    let global = HGLOBAL(unsafe { GetClipboardData(format).log_err() }?.0);
+    let global = HGLOBAL(unsafe { GetClipboardData(format).ok() }?.0);
     let size = unsafe { GlobalSize(global) };
     let data_ptr = unsafe { GlobalLock(global) };
     let result = f(data_ptr, size);
-    unsafe { GlobalUnlock(global).log_err() };
+    unsafe { GlobalUnlock(global).ok() };
     Some(result)
 }
 

crates/lsp/src/lsp.rs 🔗

@@ -336,21 +336,18 @@ impl LanguageServer {
             &binary.arguments
         );
 
-        let mut server = util::command::new_smol_command(&binary.path)
+        let mut command = util::command::new_smol_command(&binary.path);
+        command
             .current_dir(working_dir)
             .args(&binary.arguments)
             .envs(binary.env.clone().unwrap_or_default())
             .stdin(Stdio::piped())
             .stdout(Stdio::piped())
             .stderr(Stdio::piped())
-            .kill_on_drop(true)
+            .kill_on_drop(true);
+        let mut server = command
             .spawn()
-            .with_context(|| {
-                format!(
-                    "failed to spawn command. path: {:?}, working directory: {:?}, args: {:?}",
-                    binary.path, working_dir, &binary.arguments
-                )
-            })?;
+            .with_context(|| format!("failed to spawn command {command:?}",))?;
 
         let stdin = server.stdin.take().unwrap();
         let stdout = server.stdout.take().unwrap();

crates/onboarding/src/welcome.rs 🔗

@@ -5,7 +5,7 @@ use gpui::{
 use menu::{SelectNext, SelectPrevious};
 use ui::{ButtonLike, Divider, DividerColor, KeyBinding, Vector, VectorName, prelude::*};
 use workspace::{
-    NewFile, Open, WorkspaceId,
+    NewFile, Open,
     item::{Item, ItemEvent},
     with_active_or_new_workspace,
 };
@@ -339,15 +339,6 @@ impl Item for WelcomePage {
         false
     }
 
-    fn clone_on_split(
-        &self,
-        _workspace_id: Option<WorkspaceId>,
-        _: &mut Window,
-        _: &mut Context<Self>,
-    ) -> Option<Entity<Self>> {
-        None
-    }
-
     fn to_item_events(event: &Self::Event, mut f: impl FnMut(workspace::item::ItemEvent)) {
         f(*event)
     }

crates/repl/src/repl_sessions_ui.rs 🔗

@@ -6,7 +6,6 @@ use gpui::{
 use project::ProjectItem as _;
 use ui::{ButtonLike, ElevationIndex, KeyBinding, prelude::*};
 use util::ResultExt as _;
-use workspace::WorkspaceId;
 use workspace::item::ItemEvent;
 use workspace::{Workspace, item::Item};
 
@@ -192,15 +191,6 @@ impl Item for ReplSessionsPage {
         false
     }
 
-    fn clone_on_split(
-        &self,
-        _workspace_id: Option<WorkspaceId>,
-        _window: &mut Window,
-        _: &mut Context<Self>,
-    ) -> Option<Entity<Self>> {
-        None
-    }
-
     fn to_item_events(event: &Self::Event, mut f: impl FnMut(workspace::item::ItemEvent)) {
         f(*event)
     }

crates/util/src/util.rs 🔗

@@ -605,13 +605,15 @@ where
     // so discard the prefix up to that segment to find the crate name
     let target = file
         .split_once("crates/")
-        .and_then(|(_, s)| s.split_once('/'))
-        .map(|(p, _)| p);
+        .and_then(|(_, s)| s.split_once("/src/"));
 
+    let module_path = target.map(|(krate, module)| {
+        krate.to_owned() + "::" + &module.trim_end_matches(".rs").replace('/', "::")
+    });
     log::logger().log(
         &log::Record::builder()
-            .target(target.unwrap_or(""))
-            .module_path(target)
+            .target(target.map_or("", |(krate, _)| krate))
+            .module_path(module_path.as_deref())
             .args(format_args!("{:?}", error))
             .file(Some(caller.file()))
             .line(Some(caller.line()))

crates/workspace/src/workspace.rs 🔗

@@ -4106,11 +4106,11 @@ impl Workspace {
                     pane.add_item(clone, true, true, None, window, cx)
                 });
                 self.center.split(&pane, &new_pane, direction).unwrap();
+                cx.notify();
                 Some(new_pane)
             } else {
                 None
             };
-        cx.notify();
         maybe_pane_handle
     }
 

crates/zed/src/zed.rs 🔗

@@ -1231,54 +1231,57 @@ pub fn handle_settings_file_changes(
     MigrationNotification::set_global(cx.new(|_| MigrationNotification), cx);
 
     // Helper function to process settings content
-    let process_settings = move |content: String,
-                                 is_user: bool,
-                                 store: &mut SettingsStore,
-                                 cx: &mut App|
-          -> bool {
-        let id = NotificationId::Named("failed-to-migrate-settings".into());
-        // Apply migrations to both user and global settings
-        let (processed_content, content_migrated) = match migrate_settings(&content) {
-            Ok(result) => {
-                dismiss_app_notification(&id, cx);
-                if let Some(migrated_content) = result {
-                    (migrated_content, true)
-                } else {
-                    (content, false)
+    let process_settings =
+        move |content: String, is_user: bool, store: &mut SettingsStore, cx: &mut App| -> bool {
+            let id = NotificationId::Named("failed-to-migrate-settings".into());
+            // Apply migrations to both user and global settings
+            let (processed_content, content_migrated) = match migrate_settings(&content) {
+                Ok(result) => {
+                    dismiss_app_notification(&id, cx);
+                    if let Some(migrated_content) = result {
+                        (migrated_content, true)
+                    } else {
+                        (content, false)
+                    }
                 }
-            }
-            Err(err) => {
-                show_app_notification(id, cx, move |cx| {
-                    cx.new(|cx| {
-                        MessageNotification::new(format!("Failed to migrate settings\n{err}"), cx)
+                Err(err) => {
+                    show_app_notification(id, cx, move |cx| {
+                        cx.new(|cx| {
+                            MessageNotification::new(
+                                format!(
+                                    "Failed to migrate settings\n\
+                                    {err}"
+                                ),
+                                cx,
+                            )
                             .primary_message("Open Settings File")
                             .primary_icon(IconName::Settings)
                             .primary_on_click(|window, cx| {
                                 window.dispatch_action(zed_actions::OpenSettings.boxed_clone(), cx);
                                 cx.emit(DismissEvent);
                             })
-                    })
-                });
-                // notify user here
-                (content, false)
-            }
-        };
+                        })
+                    });
+                    // notify user here
+                    (content, false)
+                }
+            };
 
-        let result = if is_user {
-            store.set_user_settings(&processed_content, cx)
-        } else {
-            store.set_global_settings(&processed_content, cx)
-        };
+            let result = if is_user {
+                store.set_user_settings(&processed_content, cx)
+            } else {
+                store.set_global_settings(&processed_content, cx)
+            };
 
-        if let Err(err) = &result {
-            let settings_type = if is_user { "user" } else { "global" };
-            log::error!("Failed to load {} settings: {err}", settings_type);
-        }
+            if let Err(err) = &result {
+                let settings_type = if is_user { "user" } else { "global" };
+                log::error!("Failed to load {} settings: {err}", settings_type);
+            }
 
-        settings_changed(result.err(), cx);
+            settings_changed(result.err(), cx);
 
-        content_migrated
-    };
+            content_migrated
+        };
 
     // Initial load of both settings files
     let global_content = cx