mirror of
https://github.com/zed-industries/zed.git
synced 2026-08-14 19:44:27 +00:00
Send correct line endings to language servers (#59941)
# Objective
Zed normalizes all buffer text to `LF` internally, but was sending that
`LF`-normalized text to language servers even for `CRLF` files. This
caused servers such as ESLint (with a `linebreak-style` rule) to report
a false error on every line.
Fixes #38453
## Solution
Send the buffer's actual line endings to the language server instead:
- `didOpen` and full-document `didChange` now send
`text_with_line_endings()`, and incremental changes apply the buffer's
line ending to each edit.
- Normalize the line endings returning from the LSP before computing
changed regions
- This effectively incorporates the fix from #59151, which happens to be
the reason this change was [originally
reverted](1b6cde7032).
As such, that PR should likely be integrated first.
- Detect when a buffer's line ending differs from what a server was last
sent and force a full-document resync, without this the server would
keep stale line endings, as the incremental change tracking does not
consider line ending differences.
- Route the `UpdateLineEnding` operation to `on_buffer_edited` so
toggling line endings via the status bar notifies the server immediately
rather than waiting for the file to be edited or reopened.
## Testing
- Did you test these changes? If so, how?
Yes, in addition to new unit test coverage, I used a test project with
ESLint configured with the `linebreak-style` rule set to enforce CRLF
line endings to verify that the LSP integration worked as expected.
- Are there any parts that need more testing?
The original reversion seems to have been due to a regression in which
LSP formatting would cause the editor to scroll to the bottom. I'm not
seeing this in my reproduction, and I believe this was due to a failure
to normalize line endings coming back from the LSP, but I don't know the
exact circumstances that led to the original reversion, so there might
be some additional things to test there.
- How can other people (reviewers) test your changes? Is there anything
specific they need to know?
Not really! As mentioned above, configuring ESLint with the
`linebreak-style` rule is probably the easiest way to test.
- If relevant, what platforms did you test these changes on, and are
there any important ones you can't test?
I tested on Linux, but I don't believe it's relevant.
## Self-Review Checklist:
- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] The content adheres to Zed's UI standards
([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
and
[icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md)
guidelines)
- [x] Tests cover the new/changed behavior
- [x] Performance impact has been considered and is acceptable
## Showcase
https://github.com/user-attachments/assets/881c5758-a5de-433c-8fd6-3cad7478aa90
---
Release Notes:
- Fixed an issue where language servers received incorrect line endings
for `CRLF` files, causing linters and formatters to report false errors.
---------
Co-authored-by: Kirill Bulatov <kirill@zed.dev>
This commit is contained in:
parent
770a977c68
commit
bc463bc205
4 changed files with 305 additions and 12 deletions
|
|
@ -37583,6 +37583,116 @@ async fn test_paste_image_in_markdown_single_file_worktree_falls_through(cx: &mu
|
|||
);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_format_echoing_received_line_endings_keeps_cursor(cx: &mut TestAppContext) {
|
||||
init_test(cx, |settings| {
|
||||
settings.defaults.ensure_final_newline_on_save = Some(false);
|
||||
});
|
||||
|
||||
let lf_content = "// one\nstruct Foo {\n bar: Bar,\n}\n\n// two\nstruct Bar {\n foobar:u32,\n}\n\nfn main() {\n let foo = 1;\n}\n";
|
||||
let crlf_content = lf_content.replace('\n', "\r\n");
|
||||
|
||||
let fs = FakeFs::new(cx.executor());
|
||||
fs.insert_file(path!("/file.rs"), crlf_content.clone().into())
|
||||
.await;
|
||||
|
||||
let project = Project::test(fs, [path!("/file.rs").as_ref()], cx).await;
|
||||
let language_registry = project.read_with(cx, |project, _| project.languages().clone());
|
||||
language_registry.add(rust_lang());
|
||||
|
||||
let server_side_texts = Arc::new(Mutex::new(Vec::<String>::new()));
|
||||
let mut fake_servers = language_registry.register_fake_lsp(
|
||||
"Rust",
|
||||
FakeLspAdapter {
|
||||
capabilities: lsp::ServerCapabilities {
|
||||
document_formatting_provider: Some(lsp::OneOf::Left(true)),
|
||||
..Default::default()
|
||||
},
|
||||
initializer: Some({
|
||||
let server_side_texts = server_side_texts.clone();
|
||||
Box::new(move |fake_server| {
|
||||
fake_server.handle_notification::<lsp::notification::DidOpenTextDocument, _>({
|
||||
let server_side_texts = server_side_texts.clone();
|
||||
move |params, _| {
|
||||
server_side_texts.lock().push(params.text_document.text);
|
||||
}
|
||||
});
|
||||
})
|
||||
}),
|
||||
..FakeLspAdapter::default()
|
||||
},
|
||||
);
|
||||
|
||||
let buffer = project
|
||||
.update(cx, |project, cx| {
|
||||
project.open_local_buffer(path!("/file.rs"), cx)
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let multi_buffer = cx.new(|cx| MultiBuffer::singleton(buffer.clone(), cx));
|
||||
let (editor, cx) = cx.add_window_view(|window, cx| {
|
||||
build_editor_with_project(project.clone(), multi_buffer, window, cx)
|
||||
});
|
||||
|
||||
let fake_server = fake_servers.next().await.unwrap();
|
||||
cx.executor().run_until_parked();
|
||||
|
||||
editor.update_in(cx, |editor, window, cx| {
|
||||
editor.change_selections(SelectionEffects::default(), window, cx, |s| {
|
||||
s.select_ranges([Point::new(2, 4)..Point::new(2, 4)]);
|
||||
});
|
||||
});
|
||||
|
||||
let server_side_text = server_side_texts.lock().last().cloned().unwrap();
|
||||
assert_eq!(
|
||||
server_side_text, crlf_content,
|
||||
"server should have received the CRLF text on DidOpenTextDocument",
|
||||
);
|
||||
|
||||
let formatted_text = server_side_text.replace("foobar:u32", "foobar: u32");
|
||||
fake_server.set_request_handler::<lsp::request::Formatting, _, _>(move |_, _| {
|
||||
let formatted_text = formatted_text.clone();
|
||||
async move {
|
||||
Ok(Some(vec![lsp::TextEdit {
|
||||
range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(13, 0)),
|
||||
new_text: formatted_text,
|
||||
}]))
|
||||
}
|
||||
});
|
||||
|
||||
let save = editor
|
||||
.update_in(cx, |editor, window, cx| {
|
||||
editor.save(
|
||||
SaveOptions {
|
||||
format: true,
|
||||
force_format: false,
|
||||
autosave: false,
|
||||
},
|
||||
project.clone(),
|
||||
window,
|
||||
cx,
|
||||
)
|
||||
})
|
||||
.unwrap();
|
||||
save.await;
|
||||
|
||||
assert_eq!(
|
||||
editor.update(cx, |editor, cx| editor.text(cx)),
|
||||
lf_content.replace("foobar:u32", "foobar: u32"),
|
||||
"only the minimal formatting change should be applied",
|
||||
);
|
||||
editor.update(cx, |editor, cx| {
|
||||
let snapshot = editor.display_snapshot(cx);
|
||||
let cursor = editor.selections.newest::<Point>(&snapshot).head();
|
||||
assert_eq!(
|
||||
cursor,
|
||||
Point::new(2, 4),
|
||||
"whole-document echo of the received line endings must not move the cursor",
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_race_in_multibuffer_save(cx: &mut TestAppContext) {
|
||||
init_test(cx, |_| {});
|
||||
|
|
|
|||
|
|
@ -3051,7 +3051,7 @@ impl LocalLspStore {
|
|||
uri.clone(),
|
||||
adapter.language_id(&language.name()),
|
||||
0,
|
||||
initial_snapshot.text(),
|
||||
initial_snapshot.text_with_line_endings(),
|
||||
);
|
||||
|
||||
vec![snapshot]
|
||||
|
|
@ -4844,6 +4844,13 @@ impl LspStore {
|
|||
self.on_buffer_edited(buffer, cx);
|
||||
}
|
||||
|
||||
language::BufferEvent::Operation {
|
||||
operation: language::Operation::UpdateLineEnding { .. },
|
||||
..
|
||||
} => {
|
||||
self.on_buffer_edited(buffer, cx);
|
||||
}
|
||||
|
||||
language::BufferEvent::Saved => {
|
||||
self.on_buffer_saved(buffer, cx);
|
||||
}
|
||||
|
|
@ -8582,6 +8589,8 @@ impl LspStore {
|
|||
.with_context(|| format!("Failed to convert path to URI: {}", abs_path.display()))
|
||||
.log_err()?;
|
||||
let next_snapshot = buffer.text_snapshot();
|
||||
let line_ending = next_snapshot.line_ending();
|
||||
|
||||
for language_server in language_servers {
|
||||
let language_server = language_server.clone();
|
||||
|
||||
|
|
@ -8592,6 +8601,19 @@ impl LspStore {
|
|||
.and_then(|m| m.get_mut(&language_server.server_id()))?;
|
||||
let previous_snapshot = buffer_snapshots.last()?;
|
||||
|
||||
// If the line ending differs from what this server was last sent, the LF-normalized
|
||||
// rope is byte-identical so `edits_since` yields no diffs. We must resync the whole
|
||||
// document, otherwise the server keeps the stale line endings indefinitely.
|
||||
let line_ending_changed = line_ending != previous_snapshot.snapshot.line_ending();
|
||||
|
||||
let build_full_change = || {
|
||||
vec![lsp::TextDocumentContentChangeEvent {
|
||||
range: None,
|
||||
range_length: None,
|
||||
text: next_snapshot.text_with_line_endings(),
|
||||
}]
|
||||
};
|
||||
|
||||
let build_incremental_change = || {
|
||||
buffer
|
||||
.edits_since::<Dimensions<PointUtf16, usize>>(
|
||||
|
|
@ -8609,7 +8631,7 @@ impl LspStore {
|
|||
point_to_lsp(edit_end),
|
||||
)),
|
||||
range_length: None,
|
||||
text: new_text,
|
||||
text: line_ending.apply(new_text),
|
||||
}
|
||||
})
|
||||
.collect()
|
||||
|
|
@ -8624,19 +8646,21 @@ impl LspStore {
|
|||
lsp::TextDocumentSyncCapability::Options(options) => options.change,
|
||||
});
|
||||
|
||||
let content_changes: Vec<_> = match document_sync_kind {
|
||||
Some(lsp::TextDocumentSyncKind::FULL) => {
|
||||
vec![lsp::TextDocumentContentChangeEvent {
|
||||
range: None,
|
||||
range_length: None,
|
||||
text: next_snapshot.text(),
|
||||
}]
|
||||
let build_change = || {
|
||||
if line_ending_changed {
|
||||
build_full_change()
|
||||
} else {
|
||||
build_incremental_change()
|
||||
}
|
||||
Some(lsp::TextDocumentSyncKind::INCREMENTAL) => build_incremental_change(),
|
||||
};
|
||||
|
||||
let content_changes: Vec<_> = match document_sync_kind {
|
||||
Some(lsp::TextDocumentSyncKind::FULL) => build_full_change(),
|
||||
Some(lsp::TextDocumentSyncKind::INCREMENTAL) => build_change(),
|
||||
_ => {
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
{
|
||||
build_incremental_change()
|
||||
build_change()
|
||||
}
|
||||
|
||||
#[cfg(not(any(test, feature = "test-support")))]
|
||||
|
|
@ -12451,7 +12475,7 @@ impl LspStore {
|
|||
uri,
|
||||
adapter.language_id(&language.name()),
|
||||
version,
|
||||
initial_snapshot.text(),
|
||||
initial_snapshot.text_with_line_endings(),
|
||||
);
|
||||
buffer_paths_registered.push((buffer_id, abs_path));
|
||||
local
|
||||
|
|
|
|||
|
|
@ -5194,6 +5194,142 @@ async fn test_edits_from_lsp_with_crlf_line_endings(cx: &mut gpui::TestAppContex
|
|||
});
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_lsp_server_document_matches_buffer_line_endings(cx: &mut gpui::TestAppContext) {
|
||||
init_test(cx);
|
||||
|
||||
fn offset_of(document: &str, position: lsp::Position) -> usize {
|
||||
let mut offset = 0;
|
||||
for _ in 0..position.line {
|
||||
offset += document[offset..]
|
||||
.find('\n')
|
||||
.map(|newline_offset| newline_offset + 1)
|
||||
.unwrap_or_else(|| document.len() - offset);
|
||||
}
|
||||
offset + position.character as usize
|
||||
}
|
||||
|
||||
let crlf_content = "fn main() {\r\n let a = 5;\r\n let b = 6;\r\n}\r\n";
|
||||
|
||||
let fs = FakeFs::new(cx.executor());
|
||||
fs.insert_tree(path!("/dir"), json!({ "a.rs": crlf_content }))
|
||||
.await;
|
||||
|
||||
let project = Project::test(fs, [path!("/dir").as_ref()], cx).await;
|
||||
let language_registry = project.read_with(cx, |project, _| project.languages().clone());
|
||||
language_registry.add(rust_lang());
|
||||
|
||||
let server_document = Arc::new(Mutex::new(String::new()));
|
||||
let mut fake_servers = language_registry.register_fake_lsp(
|
||||
"Rust",
|
||||
FakeLspAdapter {
|
||||
capabilities: lsp::ServerCapabilities {
|
||||
text_document_sync: Some(lsp::TextDocumentSyncCapability::Kind(
|
||||
lsp::TextDocumentSyncKind::INCREMENTAL,
|
||||
)),
|
||||
..Default::default()
|
||||
},
|
||||
initializer: Some({
|
||||
let server_document = server_document.clone();
|
||||
Box::new(move |fake_server| {
|
||||
fake_server.handle_notification::<lsp::notification::DidOpenTextDocument, _>({
|
||||
let server_document = server_document.clone();
|
||||
move |params, _| {
|
||||
*server_document.lock() = params.text_document.text;
|
||||
}
|
||||
});
|
||||
fake_server.handle_notification::<lsp::notification::DidChangeTextDocument, _>(
|
||||
{
|
||||
let server_document = server_document.clone();
|
||||
move |params, _| {
|
||||
let mut document = server_document.lock();
|
||||
for change in params.content_changes {
|
||||
match change.range {
|
||||
Some(range) => {
|
||||
let start = offset_of(&document, range.start);
|
||||
let end = offset_of(&document, range.end);
|
||||
document.replace_range(start..end, &change.text);
|
||||
}
|
||||
None => *document = change.text,
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
);
|
||||
})
|
||||
}),
|
||||
..FakeLspAdapter::default()
|
||||
},
|
||||
);
|
||||
|
||||
let (buffer, _lsp_handle) = project
|
||||
.update(cx, |project, cx| {
|
||||
project.open_local_buffer_with_lsp(path!("/dir/a.rs"), cx)
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
let _fake_server = fake_servers.next().await.unwrap();
|
||||
cx.executor().run_until_parked();
|
||||
|
||||
assert_eq!(
|
||||
server_document.lock().as_str(),
|
||||
crlf_content,
|
||||
"DidOpenTextDocument should carry the buffer's CRLF line endings",
|
||||
);
|
||||
|
||||
buffer.update(cx, |buffer, cx| {
|
||||
buffer.edit(
|
||||
[(Point::new(1, 14)..Point::new(1, 14), "\n let a2 = 55;")],
|
||||
None,
|
||||
cx,
|
||||
);
|
||||
});
|
||||
cx.executor().run_until_parked();
|
||||
|
||||
assert_eq!(
|
||||
server_document.lock().as_str(),
|
||||
"fn main() {\r\n let a = 5;\r\n let a2 = 55;\r\n let b = 6;\r\n}\r\n",
|
||||
"incremental DidChangeTextDocument should carry the buffer's CRLF line endings",
|
||||
);
|
||||
|
||||
buffer.update(cx, |buffer, cx| {
|
||||
buffer.set_line_ending(LineEnding::Unix, cx);
|
||||
});
|
||||
cx.executor().run_until_parked();
|
||||
|
||||
assert_eq!(
|
||||
server_document.lock().as_str(),
|
||||
"fn main() {\n let a = 5;\n let a2 = 55;\n let b = 6;\n}\n",
|
||||
"a line ending change should resync the whole document with the new line endings",
|
||||
);
|
||||
|
||||
buffer.update(cx, |buffer, cx| {
|
||||
buffer.edit(
|
||||
[(Point::new(3, 14)..Point::new(3, 14), "\n let c = 7;")],
|
||||
None,
|
||||
cx,
|
||||
);
|
||||
});
|
||||
cx.executor().run_until_parked();
|
||||
|
||||
assert_eq!(
|
||||
server_document.lock().as_str(),
|
||||
"fn main() {\n let a = 5;\n let a2 = 55;\n let b = 6;\n let c = 7;\n}\n",
|
||||
"incremental DidChangeTextDocument should carry the buffer's LF line endings after the change",
|
||||
);
|
||||
|
||||
buffer.update(cx, |buffer, cx| {
|
||||
buffer.set_line_ending(LineEnding::Windows, cx);
|
||||
});
|
||||
cx.executor().run_until_parked();
|
||||
|
||||
assert_eq!(
|
||||
server_document.lock().as_str(),
|
||||
"fn main() {\r\n let a = 5;\r\n let a2 = 55;\r\n let b = 6;\r\n let c = 7;\r\n}\r\n",
|
||||
"switching back to CRLF should resync the whole document with CRLF line endings",
|
||||
);
|
||||
}
|
||||
|
||||
#[gpui::test(iterations = 10)]
|
||||
async fn test_definition(cx: &mut gpui::TestAppContext) {
|
||||
init_test(cx);
|
||||
|
|
|
|||
|
|
@ -2194,6 +2194,10 @@ impl BufferSnapshot {
|
|||
self.visible_text.to_string()
|
||||
}
|
||||
|
||||
pub fn text_with_line_endings(&self) -> String {
|
||||
chunks_with_line_ending(&self.visible_text, self.line_ending).collect()
|
||||
}
|
||||
|
||||
pub fn line_ending(&self) -> LineEnding {
|
||||
self.line_ending
|
||||
}
|
||||
|
|
@ -3643,6 +3647,25 @@ impl LineEnding {
|
|||
text
|
||||
}
|
||||
}
|
||||
|
||||
/// Converts `text` to use this line ending.
|
||||
///
|
||||
/// Detects the existing line ending of `text` first; if it already matches
|
||||
/// `self`, the string is returned unchanged. Mixed line endings are not
|
||||
/// supported: detection is based on the first newline found.
|
||||
pub fn apply(&self, text: String) -> String {
|
||||
match (LineEnding::detect(&text), self) {
|
||||
(LineEnding::Unix, LineEnding::Unix) | (LineEnding::Windows, LineEnding::Windows) => {
|
||||
text
|
||||
}
|
||||
(LineEnding::Unix, LineEnding::Windows) => text.replace('\n', "\r\n"),
|
||||
(LineEnding::Windows, LineEnding::Unix) => {
|
||||
let mut result = text;
|
||||
LineEnding::normalize(&mut result);
|
||||
result
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub fn chunks_with_line_ending(rope: &Rope, line_ending: LineEnding) -> impl Iterator<Item = &str> {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue