Merge pull request #2227 from zed-industries/strip-trailing-whitespace

Add settings to normalize whitespace on save

assets/settings/default.json

@@ -51,6 +51,12 @@
     // 3. Position the dock full screen over the entire workspace"
     //     "default_dock_anchor": "expanded"
     "default_dock_anchor": "right",
+    // Whether or not to remove any trailing whitespace from lines of a buffer
+    // before saving it.
+    "remove_trailing_whitespace_on_save": true,
+    // Whether or not to ensure there's a single newline at the end of a buffer
+    // when saving it.
+    "ensure_final_newline_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/collab/src/tests/integration_tests.rs

@@ -3892,9 +3892,11 @@ async fn test_formatting_buffer(
         })
         .await
         .unwrap();
+
+    // The edits from the LSP are applied, and a final newline is added.
     assert_eq!(
         buffer_b.read_with(cx_b, |buffer, _| buffer.text()),
-        "let honey = \"two\""
+        "let honey = \"two\"\n"
     );
 
     // Ensure buffer can be formatted using an external command. Notice how the

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,121 @@ 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 and no trailing newline.
+    cx.set_state(
+        &[
+            "one ",   //
+            "twoˇ",  //
+            "three ", //
+            "four",   //
+        ]
+        .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,
+            // and the trailing newline has already been added.
+            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()
+                    ),
+                    (
+                        lsp::Range::new(lsp::Position::new(3, 4), lsp::Position::new(3, 4)),
+                        "\n".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,
+    // a newline is appended, and the edits provided by the language server
+    // have been applied.
+    format.await.unwrap();
+    cx.assert_editor_state(
+        &[
+            "one",   //
+            "",      //
+            "twoˇ", //
+            "",      //
+            "three", //
+            "four",  //
+            "",      //
+        ]
+        .join("\n"),
+    );
+
+    // Undoing the formatting undoes the trailing whitespace removal, the
+    // trailing newline, and the LSP edits.
+    cx.update_buffer(|buffer, cx| buffer.undo(cx));
+    cx.assert_editor_state(
+        &[
+            "one ",   //
+            "twoˇ",  //
+            "three ", //
+            "four",   //
+        ]
+        .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

@@ -305,7 +305,7 @@ pub struct Chunk<'a> {
 }
 
 pub struct Diff {
-    base_version: clock::Global,
+    pub(crate) base_version: clock::Global,
     line_ending: LineEnding,
     edits: Vec<(Range<usize>, Arc<str>)>,
 }
@@ -569,18 +569,21 @@ impl Buffer {
                     .read_with(&cx, |this, cx| this.diff(new_text, cx))
                     .await;
                 this.update(&mut cx, |this, cx| {
-                    if let Some(transaction) = this.apply_diff(diff, cx).cloned() {
+                    if this.version() == diff.base_version {
-                        this.did_reload(
+                        this.finalize_last_transaction();
-                            this.version(),
+                        this.apply_diff(diff, cx);
-                            this.as_rope().fingerprint(),
+                        if let Some(transaction) = this.finalize_last_transaction().cloned() {
-                            this.line_ending(),
+                            this.did_reload(
-                            new_mtime,
+                                this.version(),
-                            cx,
+                                this.as_rope().fingerprint(),
-                        );
+                                this.line_ending(),
-                        Ok(Some(transaction))
+                                new_mtime,
-                    } else {
+                                cx,
-                        Ok(None)
+                            );
+                            return Ok(Some(transaction));
+                        }
                     }
+                    Ok(None)
                 })
             } else {
                 Ok(None)
@@ -1154,20 +1157,84 @@ impl Buffer {
         })
     }
 
-    pub fn apply_diff(&mut self, diff: Diff, cx: &mut ModelContext<Self>) -> Option<&Transaction> {
+    /// Spawn a background task that searches the buffer for any whitespace
-        if self.version == diff.base_version {
+    /// at the ends of a lines, and returns a `Diff` that removes that whitespace.
-            self.finalize_last_transaction();
+    pub fn remove_trailing_whitespace(&self, cx: &AppContext) -> Task<Diff> {
-            self.start_transaction();
+        let old_text = self.as_rope().clone();
-            self.text.set_line_ending(diff.line_ending);
+        let line_ending = self.line_ending();
-            self.edit(diff.edits, None, cx);
+        let base_version = self.version();
-            if self.end_transaction(cx).is_some() {
+        cx.background().spawn(async move {
-                self.finalize_last_transaction()
+            let ranges = trailing_whitespace_ranges(&old_text);
-            } else {
+            let empty = Arc::<str>::from("");
-                None
+            Diff {
+                base_version,
+                line_ending,
+                edits: ranges
+                    .into_iter()
+                    .map(|range| (range, empty.clone()))
+                    .collect(),
+            }
+        })
+    }
+
+    /// Ensure that the buffer ends with a single newline character, and
+    /// no other whitespace.
+    pub fn ensure_final_newline(&mut self, cx: &mut ModelContext<Self>) {
+        let len = self.len();
+        let mut offset = len;
+        for chunk in self.as_rope().reversed_chunks_in_range(0..len) {
+            let non_whitespace_len = chunk
+                .trim_end_matches(|c: char| c.is_ascii_whitespace())
+                .len();
+            offset -= chunk.len();
+            offset += non_whitespace_len;
+            if non_whitespace_len != 0 {
+                if offset == len - 1 && chunk.get(non_whitespace_len..) == Some("\n") {
+                    return;
+                }
+                break;
             }
-        } else {
-            None
         }
+        self.edit([(offset..len, "\n")], None, cx);
+    }
+
+    /// Apply a diff to the buffer. If the buffer has changed since the given diff was
+    /// calculated, then adjust the diff to account for those changes, and discard any
+    /// parts of the diff that conflict with those changes.
+    pub fn apply_diff(&mut self, diff: Diff, cx: &mut ModelContext<Self>) -> Option<TransactionId> {
+        // Check for any edits to the buffer that have occurred since this diff
+        // was computed.
+        let snapshot = self.snapshot();
+        let mut edits_since = snapshot.edits_since::<usize>(&diff.base_version).peekable();
+        let mut delta = 0;
+        let adjusted_edits = diff.edits.into_iter().filter_map(|(range, new_text)| {
+            while let Some(edit_since) = edits_since.peek() {
+                // If the edit occurs after a diff hunk, then it does not
+                // affect that hunk.
+                if edit_since.old.start > range.end {
+                    break;
+                }
+                // If the edit precedes the diff hunk, then adjust the hunk
+                // to reflect the edit.
+                else if edit_since.old.end < range.start {
+                    delta += edit_since.new_len() as i64 - edit_since.old_len() as i64;
+                    edits_since.next();
+                }
+                // If the edit intersects a diff hunk, then discard that hunk.
+                else {
+                    return None;
+                }
+            }
+
+            let start = (range.start as i64 + delta) as usize;
+            let end = (range.end as i64 + delta) as usize;
+            Some((start..end, new_text))
+        });
+
+        self.start_transaction();
+        self.text.set_line_ending(diff.line_ending);
+        self.edit(adjusted_edits, None, cx);
+        self.end_transaction(cx)
     }
 
     pub fn is_dirty(&self) -> bool {
@@ -2840,3 +2907,42 @@ pub fn char_kind(c: char) -> CharKind {
         CharKind::Punctuation
     }
 }
+
+/// Find all of the ranges of whitespace that occur at the ends of lines
+/// in the given rope.
+///
+/// This could also be done with a regex search, but this implementation
+/// avoids copying text.
+pub fn trailing_whitespace_ranges(rope: &Rope) -> Vec<Range<usize>> {
+    let mut ranges = Vec::new();
+
+    let mut offset = 0;
+    let mut prev_chunk_trailing_whitespace_range = 0..0;
+    for chunk in rope.chunks() {
+        let mut prev_line_trailing_whitespace_range = 0..0;
+        for (i, line) in chunk.split('\n').enumerate() {
+            let line_end_offset = offset + line.len();
+            let trimmed_line_len = line.trim_end_matches(|c| matches!(c, ' ' | '\t')).len();
+            let mut trailing_whitespace_range = (offset + trimmed_line_len)..line_end_offset;
+
+            if i == 0 && trimmed_line_len == 0 {
+                trailing_whitespace_range.start = prev_chunk_trailing_whitespace_range.start;
+            }
+            if !prev_line_trailing_whitespace_range.is_empty() {
+                ranges.push(prev_line_trailing_whitespace_range);
+            }
+
+            offset = line_end_offset + 1;
+            prev_line_trailing_whitespace_range = trailing_whitespace_range;
+        }
+
+        offset -= 1;
+        prev_chunk_trailing_whitespace_range = prev_line_trailing_whitespace_range;
+    }
+
+    if !prev_chunk_trailing_whitespace_range.is_empty() {
+        ranges.push(prev_chunk_trailing_whitespace_range);
+    }
+
+    ranges
+}

crates/language/src/buffer_tests.rs

@@ -6,6 +6,7 @@ use gpui::{ModelHandle, MutableAppContext};
 use indoc::indoc;
 use proto::deserialize_operation;
 use rand::prelude::*;
+use regex::RegexBuilder;
 use settings::Settings;
 use std::{
     cell::RefCell,
@@ -18,6 +19,13 @@ use text::network::Network;
 use unindent::Unindent as _;
 use util::{assert_set_eq, post_inc, test::marked_text_ranges, RandomCharIter};
 
+lazy_static! {
+    static ref TRAILING_WHITESPACE_REGEX: Regex = RegexBuilder::new("[ \t]+$")
+        .multi_line(true)
+        .build()
+        .unwrap();
+}
+
 #[cfg(test)]
 #[ctor::ctor]
 fn init_logger() {
@@ -211,6 +219,79 @@ async fn test_apply_diff(cx: &mut gpui::TestAppContext) {
     });
 }
 
+#[gpui::test(iterations = 10)]
+async fn test_normalize_whitespace(cx: &mut gpui::TestAppContext) {
+    let text = [
+        "zero",     //
+        "one  ",    // 2 trailing spaces
+        "two",      //
+        "three   ", // 3 trailing spaces
+        "four",     //
+        "five    ", // 4 trailing spaces
+    ]
+    .join("\n");
+
+    let buffer = cx.add_model(|cx| Buffer::new(0, text, cx));
+
+    // 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.remove_trailing_whitespace(cx));
+    smol::future::yield_now().await;
+
+    // Edit the buffer while the normalization task is running.
+    let version_before_edit = buffer.read_with(cx, |buffer, _| buffer.version());
+    buffer.update(cx, |buffer, cx| {
+        buffer.edit(
+            [
+                (Point::new(0, 1)..Point::new(0, 1), "EE"),
+                (Point::new(3, 5)..Point::new(3, 5), "EEE"),
+            ],
+            None,
+            cx,
+        );
+    });
+
+    let format_diff = format.await;
+    buffer.update(cx, |buffer, cx| {
+        let version_before_format = format_diff.base_version.clone();
+        buffer.apply_diff(format_diff, cx);
+
+        // The outcome depends on the order of concurrent taks.
+        //
+        // If the edit occurred while searching for trailing whitespace ranges,
+        // then the trailing whitespace region touched by the edit is left intact.
+        if version_before_format == version_before_edit {
+            assert_eq!(
+                buffer.text(),
+                [
+                    "zEEero",      //
+                    "one",         //
+                    "two",         //
+                    "threeEEE   ", //
+                    "four",        //
+                    "five",        //
+                ]
+                .join("\n")
+            );
+        }
+        // Otherwise, all trailing whitespace is removed.
+        else {
+            assert_eq!(
+                buffer.text(),
+                [
+                    "zEEero",   //
+                    "one",      //
+                    "two",      //
+                    "threeEEE", //
+                    "four",     //
+                    "five",     //
+                ]
+                .join("\n")
+            );
+        }
+    });
+}
+
 #[gpui::test]
 async fn test_reparse(cx: &mut gpui::TestAppContext) {
     let text = "fn a() {}";
@@ -1943,6 +2024,45 @@ fn test_contiguous_ranges() {
     );
 }
 
+#[gpui::test(iterations = 500)]
+fn test_trailing_whitespace_ranges(mut rng: StdRng) {
+    // Generate a random multi-line string containing
+    // some lines with trailing whitespace.
+    let mut text = String::new();
+    for _ in 0..rng.gen_range(0..16) {
+        for _ in 0..rng.gen_range(0..36) {
+            text.push(match rng.gen_range(0..10) {
+                0..=1 => ' ',
+                3 => '\t',
+                _ => rng.gen_range('a'..'z'),
+            });
+        }
+        text.push('\n');
+    }
+
+    match rng.gen_range(0..10) {
+        // sometimes remove the last newline
+        0..=1 => drop(text.pop()), //
+
+        // sometimes add extra newlines
+        2..=3 => text.push_str(&"\n".repeat(rng.gen_range(1..5))),
+        _ => {}
+    }
+
+    let rope = Rope::from(text.as_str());
+    let actual_ranges = trailing_whitespace_ranges(&rope);
+    let expected_ranges = TRAILING_WHITESPACE_REGEX
+        .find_iter(&text)
+        .map(|m| m.range())
+        .collect::<Vec<_>>();
+    assert_eq!(
+        actual_ranges,
+        expected_ranges,
+        "wrong ranges for text lines:\n{:?}",
+        text.split("\n").collect::<Vec<_>>()
+    );
+}
+
 fn ruby_lang() -> Language {
     Language::new(
         LanguageConfig {

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,
@@ -2858,9 +2858,11 @@ impl Project {
                 .filter_map(|buffer_handle| {
                     let buffer = buffer_handle.read(cx);
                     let file = File::from_dyn(buffer.file())?;
-                    let buffer_abs_path = file.as_local()?.abs_path(cx);
+                    let buffer_abs_path = file.as_local().map(|f| f.abs_path(cx));
-                    let (_, server) = self.language_server_for_buffer(buffer, cx)?;
+                    let server = self
-                    Some((buffer_handle, buffer_abs_path, server.clone()))
+                        .language_server_for_buffer(buffer, cx)
+                        .map(|s| s.1.clone());
+                    Some((buffer_handle, buffer_abs_path, server))
                 })
                 .collect::<Vec<_>>();
 
@@ -2875,10 +2877,10 @@ impl Project {
                 let _cleanup = defer({
                     let this = this.clone();
                     let mut cx = cx.clone();
-                    let local_buffers = &buffers_with_paths_and_servers;
+                    let buffers = &buffers_with_paths_and_servers;
                     move || {
                         this.update(&mut cx, |this, _| {
-                            for (buffer, _, _) in local_buffers {
+                            for (buffer, _, _) in buffers {
                                 this.buffers_being_formatted.remove(&buffer.id());
                             }
                         });
@@ -2887,60 +2889,138 @@ 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 (
-                        buffer.read_with(&cx, |buffer, cx| {
+                        format_on_save,
-                            let settings = cx.global::<Settings>();
+                        remove_trailing_whitespace,
-                            let language_name = buffer.language().map(|language| language.name());
+                        ensure_final_newline,
-                            (
+                        formatter,
-                                settings.format_on_save(language_name.as_deref()),
+                        tab_size,
-                                settings.formatter(language_name.as_deref()),
+                    ) = buffer.read_with(&cx, |buffer, cx| {
-                                settings.tab_size(language_name.as_deref()),
+                        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.ensure_final_newline_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) {
+                    // First, format buffer's whitespace according to the settings.
-                        (_, FormatOnSave::Off) if trigger == FormatTrigger::Save => continue,
+                    let trailing_whitespace_diff = if remove_trailing_whitespace {
+                        Some(
+                            buffer
+                                .read_with(&cx, |b, cx| b.remove_trailing_whitespace(cx))
+                                .await,
+                        )
+                    } else {
+                        None
+                    };
+                    let whitespace_transaction_id = buffer.update(&mut cx, |buffer, cx| {
+                        buffer.finalize_last_transaction();
+                        buffer.start_transaction();
+                        if let Some(diff) = trailing_whitespace_diff {
+                            buffer.apply_diff(diff, cx);
+                        }
+                        if ensure_final_newline {
+                            buffer.ensure_final_newline(cx);
+                        }
+                        buffer.end_transaction(cx)
+                    });
+
+                    // Currently, formatting operations are represented differently depending on
+                    // whether they come from a language server or an external command.
+                    enum FormatOperation {
+                        Lsp(Vec<(Range<Anchor>, String)>),
+                        External(Diff),
+                    }
+
+                    // Apply language-specific formatting using either a language server
+                    // or external command.
+                    let mut format_operation = 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,
+                            if let Some((language_server, buffer_abs_path)) =
-                            &buffer,
+                                language_server.as_ref().zip(buffer_abs_path.as_ref())
-                            &buffer_abs_path,
+                            {
-                            &language_server,
+                                format_operation = Some(FormatOperation::Lsp(
-                            tab_size,
+                                    Self::format_via_lsp(
-                            &mut cx,
+                                        &this,
-                        )
+                                        &buffer,
-                        .await
+                                        buffer_abs_path,
-                        .context("failed to format via language server")?,
+                                        &language_server,
+                                        tab_size,
+                                        &mut cx,
+                                    )
+                                    .await
+                                    .context("failed to format via language server")?,
+                                ));
+                            }
+                        }
 
                         (
                             Formatter::External { command, arguments },
                             FormatOnSave::On | FormatOnSave::Off,
                         )
                         | (_, FormatOnSave::External { command, arguments }) => {
-                            Self::format_via_external_command(
+                            if let Some(buffer_abs_path) = buffer_abs_path {
-                                &buffer,
+                                format_operation = Self::format_via_external_command(
-                                &buffer_abs_path,
+                                    &buffer,
-                                &command,
+                                    &buffer_abs_path,
-                                &arguments,
+                                    &command,
-                                &mut cx,
+                                    &arguments,
-                            )
+                                    &mut cx,
-                            .await
+                                )
-                            .context(format!(
+                                .await
-                                "failed to format via external command {:?}",
+                                .context(format!(
-                                command
+                                    "failed to format via external command {:?}",
-                            ))?
+                                    command
+                                ))?
+                                .map(FormatOperation::External);
+                            }
                         }
                     };
 
-                    if let Some(transaction) = transaction {
+                    buffer.update(&mut cx, |b, cx| {
-                        if !push_to_history {
+                        // If the buffer had its whitespace formatted and was edited while the language-specific
-                            buffer.update(&mut cx, |buffer, _| {
+                        // formatting was being computed, avoid applying the language-specific formatting, because
-                                buffer.forget_transaction(transaction.id)
+                        // it can't be grouped with the whitespace formatting in the undo history.
-                            });
+                        if let Some(transaction_id) = whitespace_transaction_id {
+                            if b.peek_undo_stack()
+                                .map_or(true, |e| e.transaction_id() != transaction_id)
+                            {
+                                format_operation.take();
+                            }
                         }
-                        project_transaction.0.insert(buffer.clone(), transaction);
+
-                    }
+                        // Apply any language-specific formatting, and group the two formatting operations
+                        // in the buffer's undo history.
+                        if let Some(operation) = format_operation {
+                            match operation {
+                                FormatOperation::Lsp(edits) => {
+                                    b.edit(edits, None, cx);
+                                }
+                                FormatOperation::External(diff) => {
+                                    b.apply_diff(diff, cx);
+                                }
+                            }
+
+                            if let Some(transaction_id) = whitespace_transaction_id {
+                                b.group_until_transaction(transaction_id);
+                            }
+                        }
+
+                        if let Some(transaction) = b.finalize_last_transaction().cloned() {
+                            if !push_to_history {
+                                b.forget_transaction(transaction.id);
+                            }
+                            project_transaction.0.insert(buffer.clone(), transaction);
+                        }
+                    });
                 }
 
                 Ok(project_transaction)
@@ -2981,7 +3061,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 +3108,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 +3123,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 +3166,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,8 @@ 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 ensure_final_newline_on_save: Option<bool>,
     pub formatter: Option<Formatter>,
     pub enable_language_server: Option<bool>,
 }
@@ -361,6 +363,12 @@ 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,
+                ),
+                ensure_final_newline_on_save: required(
+                    defaults.editor.ensure_final_newline_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 +468,18 @@ 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 ensure_final_newline_on_save(&self, language: Option<&str>) -> bool {
+        self.language_setting(language, |settings| {
+            settings.ensure_final_newline_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 +578,8 @@ impl Settings {
                 hard_tabs: Some(false),
                 soft_wrap: Some(SoftWrap::None),
                 preferred_line_length: Some(80),
+                remove_trailing_whitespace_on_save: Some(true),
+                ensure_final_newline_on_save: Some(true),
                 format_on_save: Some(FormatOnSave::On),
                 formatter: Some(Formatter::LanguageServer),
                 enable_language_server: Some(true),