Add setting for removing trailing whitespace on save

assets/settings/default.json

@@ -51,6 +51,8 @@
     // 3. Position the dock full screen over the entire workspace"
     //     "default_dock_anchor": "expanded"
     "default_dock_anchor": "right",
+    // Whether or not to remove trailing whitespace from lines before saving.
+    "remove_trailing_whitespace_on_save": true,
     // Whether or not to perform a buffer format before saving
     "format_on_save": "on",
     // How to perform a buffer format. This setting can take two values:

crates/editor/src/editor_tests.rs

@@ -1,23 +1,23 @@
-use drag_and_drop::DragAndDrop;
-use futures::StreamExt;
-use indoc::indoc;
-use std::{cell::RefCell, rc::Rc, time::Instant};
-use unindent::Unindent;
-
 use super::*;
 use crate::test::{
     assert_text_with_selections, build_editor, editor_lsp_test_context::EditorLspTestContext,
     editor_test_context::EditorTestContext, select_ranges,
 };
+use drag_and_drop::DragAndDrop;
+use futures::StreamExt;
 use gpui::{
     executor::Deterministic,
     geometry::{rect::RectF, vector::vec2f},
     platform::{WindowBounds, WindowOptions},
     serde_json,
 };
+use indoc::indoc;
 use language::{BracketPairConfig, FakeLspAdapter, LanguageConfig, LanguageRegistry, Point};
+use parking_lot::Mutex;
 use project::FakeFs;
 use settings::EditorSettings;
+use std::{cell::RefCell, rc::Rc, time::Instant};
+use unindent::Unindent;
 use util::{
     assert_set_eq,
     test::{marked_text_ranges, marked_text_ranges_by, sample_text, TextRangeMarker},
@@ -4301,6 +4301,111 @@ async fn test_concurrent_format_requests(cx: &mut gpui::TestAppContext) {
     "});
 }
 
+#[gpui::test]
+async fn test_strip_whitespace_and_format_via_lsp(cx: &mut gpui::TestAppContext) {
+    cx.foreground().forbid_parking();
+
+    let mut cx = EditorLspTestContext::new_rust(
+        lsp::ServerCapabilities {
+            document_formatting_provider: Some(lsp::OneOf::Left(true)),
+            ..Default::default()
+        },
+        cx,
+    )
+    .await;
+
+    // Set up a buffer white some trailing whitespace.
+    cx.set_state(
+        &[
+            "one ",   //
+            "twoˇ",  //
+            "three ", //
+        ]
+        .join("\n"),
+    );
+
+    // Submit a format request.
+    let format = cx
+        .update_editor(|editor, cx| editor.format(&Format, cx))
+        .unwrap();
+
+    // Record which buffer changes have been sent to the language server
+    let buffer_changes = Arc::new(Mutex::new(Vec::new()));
+    cx.lsp
+        .handle_notification::<lsp::notification::DidChangeTextDocument, _>({
+            let buffer_changes = buffer_changes.clone();
+            move |params, _| {
+                buffer_changes.lock().extend(
+                    params
+                        .content_changes
+                        .into_iter()
+                        .map(|e| (e.range.unwrap(), e.text)),
+                );
+            }
+        });
+
+    // Handle formatting requests to the language server.
+    cx.lsp.handle_request::<lsp::request::Formatting, _, _>({
+        let buffer_changes = buffer_changes.clone();
+        move |_, _| {
+            // When formatting is requested, trailing whitespace has already been stripped.
+            assert_eq!(
+                &buffer_changes.lock()[1..],
+                &[
+                    (
+                        lsp::Range::new(lsp::Position::new(0, 3), lsp::Position::new(0, 4)),
+                        "".into()
+                    ),
+                    (
+                        lsp::Range::new(lsp::Position::new(2, 5), lsp::Position::new(2, 6)),
+                        "".into()
+                    ),
+                ]
+            );
+
+            // Insert blank lines between each line of the buffer.
+            async move {
+                Ok(Some(vec![
+                    lsp::TextEdit {
+                        range: lsp::Range::new(lsp::Position::new(1, 0), lsp::Position::new(1, 0)),
+                        new_text: "\n".into(),
+                    },
+                    lsp::TextEdit {
+                        range: lsp::Range::new(lsp::Position::new(2, 0), lsp::Position::new(2, 0)),
+                        new_text: "\n".into(),
+                    },
+                ]))
+            }
+        }
+    });
+
+    // After formatting the buffer, the trailing whitespace is stripped and the
+    // edits provided by the language server have been applied.
+    format.await.unwrap();
+    cx.assert_editor_state(
+        &[
+            "one",   //
+            "",      //
+            "twoˇ", //
+            "",      //
+            "three", //
+        ]
+        .join("\n"),
+    );
+
+    // Undoing the formatting undoes both the trailing whitespace removal and the
+    // LSP edits.
+    cx.update_buffer(|buffer, cx| buffer.undo(cx));
+    cx.assert_editor_state(
+        &[
+            "one ",   //
+            "twoˇ",  //
+            "three ", //
+        ]
+        .join("\n"),
+    );
+}
+
 #[gpui::test]
 async fn test_completion(cx: &mut gpui::TestAppContext) {
     let mut cx = EditorLspTestContext::new_rust(

crates/editor/src/test/editor_test_context.rs

@@ -185,6 +185,7 @@ impl<'a> EditorTestContext<'a> {
     /// of its selections using a string containing embedded range markers.
     ///
     /// See the `util::test::marked_text_ranges` function for more information.
+    #[track_caller]
     pub fn assert_editor_state(&mut self, marked_text: &str) {
         let (unmarked_text, expected_selections) = marked_text_ranges(marked_text, true);
         let buffer_text = self.buffer_text();

crates/language/src/buffer.rs

@@ -1154,7 +1154,7 @@ impl Buffer {
         })
     }
 
-    pub fn normalize_whitespace(&self, cx: &AppContext) -> Task<Diff> {
+    pub fn remove_trailing_whitespace(&self, cx: &AppContext) -> Task<Diff> {
         let old_text = self.as_rope().clone();
         let line_ending = self.line_ending();
         let base_version = self.version();
@@ -1189,7 +1189,7 @@ impl Buffer {
         &mut self,
         diff: Diff,
         cx: &mut ModelContext<Self>,
-    ) -> Option<&Transaction> {
+    ) -> Option<TransactionId> {
         // Check for any edits to the buffer that have occurred since this diff
         // was computed.
         let snapshot = self.snapshot();
@@ -1219,12 +1219,10 @@ impl Buffer {
             Some((start..end, new_text))
         });
 
-        self.finalize_last_transaction();
         self.start_transaction();
         self.text.set_line_ending(diff.line_ending);
         self.edit(adjusted_edits, None, cx);
-        self.end_transaction(cx)?;
+        self.end_transaction(cx)
-        self.finalize_last_transaction()
     }
 
     pub fn is_dirty(&self) -> bool {

crates/language/src/buffer_tests.rs

@@ -235,7 +235,7 @@ async fn test_normalize_whitespace(cx: &mut gpui::TestAppContext) {
 
     // Spawn a task to format the buffer's whitespace.
     // Pause so that the foratting task starts running.
-    let format = buffer.read_with(cx, |buffer, cx| buffer.normalize_whitespace(cx));
+    let format = buffer.read_with(cx, |buffer, cx| buffer.remove_trailing_whitespace(cx));
     smol::future::yield_now().await;
 
     // Edit the buffer while the normalization task is running.

crates/lsp/src/lsp.rs

@@ -422,6 +422,10 @@ impl LanguageServer {
         self.notification_handlers.lock().remove(T::METHOD);
     }
 
+    pub fn remove_notification_handler<T: notification::Notification>(&self) {
+        self.notification_handlers.lock().remove(T::METHOD);
+    }
+
     #[must_use]
     pub fn on_custom_notification<Params, F>(&self, method: &'static str, mut f: F) -> Subscription
     where
@@ -780,6 +784,26 @@ impl FakeLanguageServer {
         responded_rx
     }
 
+    pub fn handle_notification<T, F>(
+        &self,
+        mut handler: F,
+    ) -> futures::channel::mpsc::UnboundedReceiver<()>
+    where
+        T: 'static + notification::Notification,
+        T::Params: 'static + Send,
+        F: 'static + Send + FnMut(T::Params, gpui::AsyncAppContext),
+    {
+        let (handled_tx, handled_rx) = futures::channel::mpsc::unbounded();
+        self.server.remove_notification_handler::<T>();
+        self.server
+            .on_notification::<T, _>(move |params, cx| {
+                handler(params, cx.clone());
+                handled_tx.unbounded_send(()).ok();
+            })
+            .detach();
+        handled_rx
+    }
+
     pub fn remove_request_handler<T>(&mut self)
     where
         T: 'static + request::Request,

crates/project/src/project.rs

@@ -26,7 +26,7 @@ use language::{
         serialize_anchor, serialize_version,
     },
     range_from_lsp, range_to_lsp, Anchor, Bias, Buffer, CachedLspAdapter, CharKind, CodeAction,
-    CodeLabel, Completion, Diagnostic, DiagnosticEntry, DiagnosticSet, Event as BufferEvent,
+    CodeLabel, Completion, Diagnostic, DiagnosticEntry, DiagnosticSet, Diff, Event as BufferEvent,
     File as _, Language, LanguageRegistry, LanguageServerName, LocalFile, OffsetRangeExt,
     Operation, Patch, PointUtf16, RopeFingerprint, TextBufferSnapshot, ToOffset, ToPointUtf16,
     Transaction, Unclipped,
@@ -2887,38 +2887,68 @@ impl Project {
 
                 let mut project_transaction = ProjectTransaction::default();
                 for (buffer, buffer_abs_path, language_server) in &buffers_with_paths_and_servers {
-                    let (format_on_save, formatter, tab_size) =
+                    let (format_on_save, remove_trailing_whitespace, formatter, tab_size) = buffer
-                        buffer.read_with(&cx, |buffer, cx| {
+                        .read_with(&cx, |buffer, cx| {
                             let settings = cx.global::<Settings>();
                             let language_name = buffer.language().map(|language| language.name());
                             (
                                 settings.format_on_save(language_name.as_deref()),
+                                settings
+                                    .remove_trailing_whitespace_on_save(language_name.as_deref()),
                                 settings.formatter(language_name.as_deref()),
                                 settings.tab_size(language_name.as_deref()),
                             )
                         });
 
-                    let transaction = match (formatter, format_on_save) {
+                    let whitespace_transaction_id = if remove_trailing_whitespace {
-                        (_, FormatOnSave::Off) if trigger == FormatTrigger::Save => continue,
+                        let diff = buffer
+                            .read_with(&cx, |buffer, cx| buffer.remove_trailing_whitespace(cx))
+                            .await;
+                        buffer.update(&mut cx, move |buffer, cx| {
+                            buffer.finalize_last_transaction();
+                            buffer.apply_diff_force(diff, cx)
+                        })
+                    } else {
+                        None
+                    };
+
+                    match (formatter, format_on_save) {
+                        (_, FormatOnSave::Off) if trigger == FormatTrigger::Save => {}
 
                         (Formatter::LanguageServer, FormatOnSave::On | FormatOnSave::Off)
-                        | (_, FormatOnSave::LanguageServer) => Self::format_via_lsp(
+                        | (_, FormatOnSave::LanguageServer) => {
-                            &this,
+                            let edits = Self::format_via_lsp(
-                            &buffer,
+                                &this,
-                            &buffer_abs_path,
+                                &buffer,
-                            &language_server,
+                                &buffer_abs_path,
-                            tab_size,
+                                &language_server,
-                            &mut cx,
+                                tab_size,
-                        )
+                                &mut cx,
-                        .await
+                            )
-                        .context("failed to format via language server")?,
+                            .await
+                            .context("failed to format via language server")?;
+
+                            buffer.update(&mut cx, |buffer, cx| {
+                                if let Some(tx_id) = whitespace_transaction_id {
+                                    if buffer
+                                        .peek_undo_stack()
+                                        .map_or(false, |e| e.transaction_id() == tx_id)
+                                    {
+                                        buffer.edit(edits, None, cx);
+                                    }
+                                    buffer.group_until_transaction(tx_id);
+                                } else {
+                                    buffer.edit(edits, None, cx);
+                                }
+                            });
+                        }
 
                         (
                             Formatter::External { command, arguments },
                             FormatOnSave::On | FormatOnSave::Off,
                         )
                         | (_, FormatOnSave::External { command, arguments }) => {
-                            Self::format_via_external_command(
+                            let diff = Self::format_via_external_command(
                                 &buffer,
                                 &buffer_abs_path,
                                 &command,
@@ -2929,10 +2959,30 @@ impl Project {
                             .context(format!(
                                 "failed to format via external command {:?}",
                                 command
-                            ))?
+                            ))?;
+
+                            if let Some(diff) = diff {
+                                buffer.update(&mut cx, |buffer, cx| {
+                                    if let Some(tx_id) = whitespace_transaction_id {
+                                        if buffer
+                                            .peek_undo_stack()
+                                            .map_or(false, |e| e.transaction_id() == tx_id)
+                                        {
+                                            buffer.apply_diff(diff, cx);
+                                        }
+                                        buffer.group_until_transaction(tx_id);
+                                    } else {
+                                        buffer.apply_diff(diff, cx);
+                                    }
+                                });
+                            }
                         }
                     };
 
+                    let transaction = buffer.update(&mut cx, |buffer, _| {
+                        buffer.finalize_last_transaction().cloned()
+                    });
+
                     if let Some(transaction) = transaction {
                         if !push_to_history {
                             buffer.update(&mut cx, |buffer, _| {
@@ -2981,7 +3031,7 @@ impl Project {
         language_server: &Arc<LanguageServer>,
         tab_size: NonZeroU32,
         cx: &mut AsyncAppContext,
-    ) -> Result<Option<Transaction>> {
+    ) -> Result<Vec<(Range<Anchor>, String)>> {
         let text_document =
             lsp::TextDocumentIdentifier::new(lsp::Url::from_file_path(abs_path).unwrap());
         let capabilities = &language_server.capabilities();
@@ -3028,26 +3078,12 @@ impl Project {
         };
 
         if let Some(lsp_edits) = lsp_edits {
-            let edits = this
+            this.update(cx, |this, cx| {
-                .update(cx, |this, cx| {
+                this.edits_from_lsp(buffer, lsp_edits, None, cx)
-                    this.edits_from_lsp(buffer, lsp_edits, None, cx)
-                })
-                .await?;
-            buffer.update(cx, |buffer, cx| {
-                buffer.finalize_last_transaction();
-                buffer.start_transaction();
-                for (range, text) in edits {
-                    buffer.edit([(range, text)], None, cx);
-                }
-                if buffer.end_transaction(cx).is_some() {
-                    let transaction = buffer.finalize_last_transaction().unwrap().clone();
-                    Ok(Some(transaction))
-                } else {
-                    Ok(None)
-                }
             })
+            .await
         } else {
-            Ok(None)
+            Ok(Default::default())
         }
     }
 
@@ -3057,7 +3093,7 @@ impl Project {
         command: &str,
         arguments: &[String],
         cx: &mut AsyncAppContext,
-    ) -> Result<Option<Transaction>> {
+    ) -> Result<Option<Diff>> {
         let working_dir_path = buffer.read_with(cx, |buffer, cx| {
             let file = File::from_dyn(buffer.file())?;
             let worktree = file.worktree.read(cx).as_local()?;
@@ -3100,10 +3136,11 @@ impl Project {
             }
 
             let stdout = String::from_utf8(output.stdout)?;
-            let diff = buffer
+            Ok(Some(
-                .read_with(cx, |buffer, cx| buffer.diff(stdout, cx))
+                buffer
-                .await;
+                    .read_with(cx, |buffer, cx| buffer.diff(stdout, cx))
-            Ok(buffer.update(cx, |buffer, cx| buffer.apply_diff(diff, cx).cloned()))
+                    .await,
+            ))
         } else {
             Ok(None)
         }

crates/settings/src/settings.rs

@@ -94,6 +94,7 @@ pub struct EditorSettings {
     pub soft_wrap: Option<SoftWrap>,
     pub preferred_line_length: Option<u32>,
     pub format_on_save: Option<FormatOnSave>,
+    pub remove_trailing_whitespace_on_save: Option<bool>,
     pub formatter: Option<Formatter>,
     pub enable_language_server: Option<bool>,
 }
@@ -361,6 +362,9 @@ impl Settings {
                 hard_tabs: required(defaults.editor.hard_tabs),
                 soft_wrap: required(defaults.editor.soft_wrap),
                 preferred_line_length: required(defaults.editor.preferred_line_length),
+                remove_trailing_whitespace_on_save: required(
+                    defaults.editor.remove_trailing_whitespace_on_save,
+                ),
                 format_on_save: required(defaults.editor.format_on_save),
                 formatter: required(defaults.editor.formatter),
                 enable_language_server: required(defaults.editor.enable_language_server),
@@ -460,6 +464,12 @@ impl Settings {
         self.language_setting(language, |settings| settings.preferred_line_length)
     }
 
+    pub fn remove_trailing_whitespace_on_save(&self, language: Option<&str>) -> bool {
+        self.language_setting(language, |settings| {
+            settings.remove_trailing_whitespace_on_save.clone()
+        })
+    }
+
     pub fn format_on_save(&self, language: Option<&str>) -> FormatOnSave {
         self.language_setting(language, |settings| settings.format_on_save.clone())
     }
@@ -558,6 +568,7 @@ impl Settings {
                 hard_tabs: Some(false),
                 soft_wrap: Some(SoftWrap::None),
                 preferred_line_length: Some(80),
+                remove_trailing_whitespace_on_save: Some(true),
                 format_on_save: Some(FormatOnSave::On),
                 formatter: Some(Formatter::LanguageServer),
                 enable_language_server: Some(true),