Don’t insert \n if the file is empty

[bestgopher]

I found that when I save the unsaved file, zed insert a '\n' at the end of file automatically(it is fine to me). But it is meaningless to do this for an empty file.

For example, there is an empty file: Then I press ctrl+s to save the file. before: now:

Release Notes:

• N/A

[JunkuiZhang]

This actually matches VSCode’s behavior.

[bestgopher]

This actually matches VSCode’s behavior.

sorry i did not reproduce it on vscode.

[JunkuiZhang]

This is how it behaves on my end.

https://github.com/user-attachments/assets/6a94031e-2efe-44db-9fd6-477ae5d915a6

[bestgopher]

This is how it behaves on my end.

Screen.Recording.2024-08-18.115030.mp4

It looks like the rust-formatter's behavior. I reproduced it only with rust file(.rs file). You could turn off editor.formatOnSave then try it again.

[bestgopher]

@JunkuiZhang

https://github.com/user-attachments/assets/c6be104d-6e0f-4918-b504-d73ab53e8fbf

[JunkuiZhang]

You are right, sorry for the misinformation!

[bestgopher]

Great, can we test this?

https://github.com/zed-industries/zed/blob/10a996cbc40498f58a91fe1a2433bd81ea3c0218/crates/editor/src/editor_tests.rs#L6119

has some formatting tests, presumably we can add another one nearby?

Done, please take a look.

[bestgopher]

test_document_format_during_save

Hi, I have a question: Whether editor.update(cx, |editor, cx| editor.set_text("", cx)); can clean the file?

[SomeoneToIgnore]

It does clean the text, in the file, so not sure what do you mean.

[bestgopher]

It does clean the text, in the file, so not sure what do you mean.

 editor.update(cx, |editor, cx| editor.set_text("", cx));
    assert!(cx.read(|cx| editor.is_dirty(cx)));
    let save = editor
        .update(cx, |editor, cx| editor.save(true, project.clone(), cx))
        .unwrap();
    fake_server.handle_request::<lsp::request::Formatting, _, _>(move |params, _| async move {
        assert_eq!(
            params.text_document.uri,
            lsp::Url::from_file_path("/file.rs").unwrap()
        );
        assert_eq!(params.options.insert_final_newline, Some(true));
        Ok(Some(vec![lsp::TextEdit::new(
            lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(u32::MAX, 0)),
            "".to_string(),
        )]))
    });

    cx.executor().start_waiting();
    save.await;

    assert_eq!(editor.update(cx, |editor, cx| editor.text(cx)), "");

This case will fail because of last assert statement.

assertion `left == right` failed
  left: "one\ntwo\nthree\n"
 right: ""

I don't known why the file's contents are one\ntwo\nthree\n if the file is cleared.

[SomeoneToIgnore]

Sorry, it does not look wrong for me and does not fail on main@e68b2d5ecc8076a80d7d8829b8bddf6658367ef3 either:

diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs
index 091eca5e8e..c6849b7046 100644
--- a/crates/editor/src/editor_tests.rs
+++ b/crates/editor/src/editor_tests.rs
@@ -6241,6 +6241,28 @@ async fn test_document_format_during_save(cx: &mut gpui::TestAppContext) {
         .await;
     cx.executor().start_waiting();
     save.await;
+
+    editor.update(cx, |editor, cx| editor.set_text("", cx));
+    assert!(cx.read(|cx| editor.is_dirty(cx)));
+    let save = editor
+        .update(cx, |editor, cx| editor.save(true, project.clone(), cx))
+        .unwrap();
+    fake_server.handle_request::<lsp::request::Formatting, _, _>(move |params, _| async move {
+        assert_eq!(
+            params.text_document.uri,
+            lsp::Url::from_file_path("/file.rs").unwrap()
+        );
+        assert_eq!(params.options.insert_final_newline, Some(true));
+        Ok(Some(vec![lsp::TextEdit::new(
+            lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(u32::MAX, 0)),
+            "".to_string(),
+        )]))
+    });
+
+    cx.executor().start_waiting();
+    save.await;
+
+    assert_eq!(editor.update(cx, |editor, cx| editor.text(cx)), "");
 }
 
 #[gpui::test]

I would suggest creating a totally new test with `` and assert that, so ignore any potential side-effects.

[bestgopher]

Ok(Some(vec![lsp::TextEdit::new(

•        lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(u32::MAX, 0)),
  

•        "".to_string(),
  

•    )]))
  

If the end of Range were changed to lsp::Position::new(0, 0), it will be failed.

[bestgopher]

My original idea is: First, clear the file. Then, mock the lsp return an empty string ``(Ok(Some(vec![lsp::TextEdit::new(lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)),"".to_string())]))). Because the file is empty, ensure_final_newline is called and returns nothing(not insert a '\n'), so the file is still empty. Last, assert the contents of file. However, the last step panic because the file is not empty(one\ntwo\nthree\n)

[SomeoneToIgnore]

I think I start to understand https://github.com/zed-industries/zed/pull/16423#issuecomment-2296534769 after https://github.com/zed-industries/zed/pull/16423#issuecomment-2296554264

The semantics of the lsp::TextEdit is to replace a given range (or insert into a point, if range's start == end) with the text given.

So, those two cases work very very different: the one with 0-MAX range and "" text "formats" the entire file as "replace all file contents with an empty string"; while the other one with 0-0 range and "" text "formats" the file as "append to very beginning of the file an empty string".

---

I believe, we have to take a step back and think a bit on a general scenario, if server formatting becomes so important. Since we write our test in Rust, should we repeat the very same way it breaks?

We have a debug: open language server logs command in the command palette, which allows to select a Rust one in the tab, and, most importantly, LSP RPC messages.

It shows me, that for "ensure_final_newline_on_save": false, in Zed's settings.json, I get no formatting at all:

// Send:
{"jsonrpc":"2.0","id":389,"method":"textDocument/formatting","params":{"textDocument":{"uri":"file:///Users/someonetoignore/work/zed/zed/crates/cli/src/main.rs"},"options":{"tabSize":4,"insertSpaces":true,"trimTrailingWhitespace":true,"insertFinalNewline":true,"trimFinalNewlines":true}}}
//...... snip
// Receive:
{"jsonrpc":"2.0","id":389,"result":null}

and the culprit seems to be deeper, particularly in

• the fact that we do send insertFinalNewline: true into the LSP server instead of false for the settings (it's clearly an error, but it's not the main one as we get zero formatting for whatever ensure_final_newline_on_save setting value now)
• the fact that Zed itself seems to edit the file incorrectly, why does an empty file with the ensure_final_newline_on_save: false in the settings calls ensure_final_newline at all?

So, seems that:

• we have to write a correct test
• change things deeper — formatting request does not matter at all as it seems, that's why I think we can even disable it, if needed. But I would still extract this case into a separate test, as we need to test the next bullet (below) too.
• ensure, that for ensure_final_newline_on_save: true (default settings) we do insert a newline

[bestgopher]

@SomeoneToIgnore Thanks for your comment and advice. I have wrote another testcase, and it looks good to me.

[bestgopher]

There's more than that, isn't it? My comment from #16423 (comment) pretty much states we should close this PR and use a proper approach.

Also, I did check out this branch and run the branch — it still adds a newline to me, so the fix does not work for me at all.

This pr fixes that prevent the empty text file be insert a '\n'. Show as the follow video, I opened an empty file, and created an empty text file, made the file unsaved then saved it:

https://github.com/user-attachments/assets/22c9eba6-5188-48f9-bc08-c590d863527d

These files can't be formatted by language server.

[SomeoneToIgnore]

On your video, I see on seconds 9 and 10, how an empty file is being saved and got an extra \n inside.

So

This pr fixes that prevent the empty text file be insert a '\n'

I do not see that in the video?

Here's my attempt: same project, *.rs files in both cases, the left is main built, the right is your version.

https://github.com/user-attachments/assets/c3c19b05-1d06-4d2a-993d-cbe3a3797ee8

I see not difference, both add \n when saving an empty file, same as your video.

[bestgopher]

As you can see, Here are two ways to insert '\n': rust-lsp and ensure_final_newline_on_save. Your video shows that you tested it with *.rs file and the '\n' inserted by lsp. You maybe test it with an non-rust file.

On your video, I see on seconds 9 and 10, how an empty file is being saved and got an extra \n inside.

First, create a file named 'aaaa' （non-rust file). Second, insert a line. Third, delete the line and make the file empty but stay 'unsaved'. Fourth, press ctrl+s to save the file.

https://github.com/user-attachments/assets/553d6e72-095d-4997-ba62-d472492fbfa8

[SomeoneToIgnore]

Your video shows that you tested it with *.rs file and the '\n' inserted by lsp

The one that's messed up due to https://github.com/zed-industries/zed/pull/16423#issuecomment-2296640464 as I noted.

To restate: the fix, if it claims to fix the newline issues, HAS TO NOT USE THE ensure_final_newline AT ALL!

First, create a file named 'aaaa' （non-rust file). Second, insert a line. Third, delete the line and make the file empty but stay 'unsaved'. Fourth, press ctrl+s to save the file.

So now, I have a ensure_final_newline_on_save: true and I have no newlines inserted, which is a bug — it has to insert a newline UNLESS I have ensure_final_newline_on_save: false. Which is now broken.

---

I am tired to repeat the same things over and over, and there are clearly some communication issues. I will close this PR and again repeat that this this is not a good fix.

[bestgopher]

Thank you for taking the time to review this PR. It seems there may have been some miscommunication earlier, and you might not have fully understood my intention. This is my final explanation:

We can use Zed to edit any file, not just Rust code files. When we save Rust code files, Zed calls the rust-analyzer RPC interface to format the file, and the LSP adds a newline at the end of the file. However, for regular files (non-code files), when we save them, since they are not code files, there's no corresponding LSP to format them, but Zed still adds a newline at the end of the file. This is a useful feature; for example, when we modify the .gitignore file and create a PR, GitHub might prompt "no newline".

But for an empty file, like a .gitignore file that has been cleared of its contents, when we save this file, Zed also adds a newline to the .gitignore. I believe that adding a newline to an empty file is unnecessary. VSCode also has a feature to automatically add a newline at the end of files, but it only activates when the file is not empty. This is the issue I intended to fix with this PR.

You can also try clearing a non-code file, such as the .gitignore file in a Zed project, then saving it, and you'll notice that .gitignore will have a newline.

[SomeoneToIgnore]

Next time, it would be really beneficial to write a description in the PR, so we do not have to collect the context during reviews.

We can use Zed to edit any file, not just Rust code files. .......................................snip

I do understand that people can edit different files with Zed, LSP-backed or not.

Zed calls the rust-analyzer RPC interface to format the file, and the LSP adds a newline at the end of the file

My point was that this is working wrong with ensure_final_newline_on_save: false, at least it passes wrong parameters.

However, for regular files (non-code files), when we save them, since they are not code files, there's no corresponding LSP to format them, but Zed still adds a newline at the end of the file.

So you seem to be fixing that case, and only this one? But why on Earth do you use *.rs files and deal with formatting requests in your test then??

The whole approach is wrong, as it was the wrong place to change. This is the place: https://github.com/zed-industries/zed/blob/1f0dc8b7541ab18c9c0a7bb8a1299c11d49f8eb1/crates/project/src/project.rs#L4994

and somewhere there, we should make a decision like "if this buffer is empty, I do not deal with any whitespaces at all", so whitespace_transaction_id should be None.

Instead, we do some odd things like "ok, we still try to apply all whitespace diffs for the file, will try to insert a newline but return early". This works, but it's not a right place.

[bestgopher]

@SomeoneToIgnore Thank you again for your patient explanation. If this counts as an optimization, I hope the official team can fix it soon.