Cargo.lock π
@@ -4920,6 +4920,7 @@ dependencies = [
"theme",
"time",
"tree-sitter-bash",
+ "tree-sitter-c",
"tree-sitter-html",
"tree-sitter-python",
"tree-sitter-rust",
Fix panic with completion ranges and autoclose regions interop (#35408)
Kirill Bulatov and Max Brunsfeld created
As reported [in
Discord](https://discord.com/channels/869392257814519848/1106226198494859355/1398470747227426948)
C projects with `"` as "brackets" that autoclose, may invoke panics when
edited at the end of the file.
With a single selection-caret (`Λ`), at the end of the file,
```c
ifndef BAR_H
#define BAR_H
#include <stdbool.h>
int fn_branch(bool do_branch1, bool do_branch2);
#endif // BAR_H
#include"Λ"
```
gets an LSP response from clangd
```jsonc
{
"filterText": "AGL/",
"insertText": "AGL/",
"insertTextFormat": 1,
"kind": 17,
"label": " AGL/",
"labelDetails": {},
"score": 0.78725427389144897,
"sortText": "40b67681AGL/",
"textEdit": {
"newText": "AGL/",
"range": { "end": { "character": 11, "line": 8 }, "start": { "character": 10, "line": 8 } }
}
}
```
which replaces `"` after the caret (character/column 11, 0-indexed).
This is reasonable, as regular follow-up (proposed in further
completions), is a suffix + a closing `"`:
<img width="842" height="259" alt="image"
src="https://github.com/user-attachments/assets/ea56f621-7008-4ce2-99ba-87344ddf33d2"
/>
Yet when Zed handles user input of `"`, it panics due to multiple
reasons:
* after applying any snippet text edit, Zed did a selection change:
https://github.com/zed-industries/zed/blob/55379876301bd4dcfe054a146b66288d2e60a523/crates/editor/src/editor.rs#L9539-L9545
which caused eventual autoclose region invalidation:
https://github.com/zed-industries/zed/blob/55379876301bd4dcfe054a146b66288d2e60a523/crates/editor/src/editor.rs#L2970
This covers all cases that insert the `include""` text.
* after applying any user input and "plain" text edit, Zed did not
invalidate any autoclose regions at all, relying on the "bracket" (which
includes `"`) autoclose logic to rule edge cases out
* bracket autoclose logic detects previous `"` and considers the new
user input as a valid closure, hence no autoclose region needed.
But there is an autoclose bracket data after the plaintext completion
insertion (`AGL/`) really, and it's not invalidated after `"` handling
* in addition to that, `Anchor::is_valid` method in `text` panicked, and
required `fn try_fragment_id_for_anchor` to handle "pointing at odd,
after the end of the file, offset" cases as `false`
A test reproducing the feedback and 2 fixes added: proper, autoclose
region invalidation call which required the invalidation logic tweaked a
bit, and "superficial", "do not apply bad selections that cause panics"
fix in the editor to be more robust
Release Notes:
- Fixed panic with completion ranges and autoclose regions interop
---------
Co-authored-by: Max Brunsfeld <maxbrunsfeld@gmail.com>Change summary
Cargo.lock | 1
crates/editor/Cargo.toml | 3
crates/editor/src/editor.rs | 79 ++++++++++----
crates/editor/src/editor_tests.rs | 172 +++++++++++++++++++++++++++++++++
crates/multi_buffer/src/anchor.rs | 8
crates/project/src/lsp_command.rs | 6
crates/text/src/anchor.rs | 4
crates/text/src/text.rs | 33 +++---
8 files changed, 258 insertions(+), 48 deletions(-)
Detailed changes
crates/editor/Cargo.toml π
@@ -22,6 +22,7 @@ test-support = [
"theme/test-support",
"util/test-support",
"workspace/test-support",
+ "tree-sitter-c",
"tree-sitter-rust",
"tree-sitter-typescript",
"tree-sitter-html",
@@ -76,6 +77,7 @@ telemetry.workspace = true
text.workspace = true
time.workspace = true
theme.workspace = true
+tree-sitter-c = { workspace = true, optional = true }
tree-sitter-html = { workspace = true, optional = true }
tree-sitter-rust = { workspace = true, optional = true }
tree-sitter-typescript = { workspace = true, optional = true }
@@ -106,6 +108,7 @@ settings = { workspace = true, features = ["test-support"] }
tempfile.workspace = true
text = { workspace = true, features = ["test-support"] }
theme = { workspace = true, features = ["test-support"] }
+tree-sitter-c.workspace = true
tree-sitter-html.workspace = true
tree-sitter-rust.workspace = true
tree-sitter-typescript.workspace = true
crates/editor/src/editor.rs π
@@ -1305,6 +1305,7 @@ impl Default for SelectionHistoryMode {
///
/// Similarly, you might want to disable scrolling if you don't want the viewport to
/// move.
+#[derive(Clone)]
pub struct SelectionEffects {
nav_history: Option<bool>,
completions: bool,
@@ -2944,10 +2945,12 @@ impl Editor {
}
}
+ let selection_anchors = self.selections.disjoint_anchors();
+
if self.focus_handle.is_focused(window) && self.leader_id.is_none() {
self.buffer.update(cx, |buffer, cx| {
buffer.set_active_selections(
- &self.selections.disjoint_anchors(),
+ &selection_anchors,
self.selections.line_mode,
self.cursor_shape,
cx,
@@ -2964,9 +2967,8 @@ impl Editor {
self.select_next_state = None;
self.select_prev_state = None;
self.select_syntax_node_history.try_clear();
- self.invalidate_autoclose_regions(&self.selections.disjoint_anchors(), buffer);
- self.snippet_stack
- .invalidate(&self.selections.disjoint_anchors(), buffer);
+ self.invalidate_autoclose_regions(&selection_anchors, buffer);
+ self.snippet_stack.invalidate(&selection_anchors, buffer);
self.take_rename(false, window, cx);
let newest_selection = self.selections.newest_anchor();
@@ -4047,7 +4049,8 @@ impl Editor {
// then don't insert that closing bracket again; just move the selection
// past the closing bracket.
let should_skip = selection.end == region.range.end.to_point(&snapshot)
- && text.as_ref() == region.pair.end.as_str();
+ && text.as_ref() == region.pair.end.as_str()
+ && snapshot.contains_str_at(region.range.end, text.as_ref());
if should_skip {
let anchor = snapshot.anchor_after(selection.end);
new_selections
@@ -4973,13 +4976,17 @@ impl Editor {
})
}
- /// Remove any autoclose regions that no longer contain their selection.
+ /// Remove any autoclose regions that no longer contain their selection or have invalid anchors in ranges.
fn invalidate_autoclose_regions(
&mut self,
mut selections: &[Selection<Anchor>],
buffer: &MultiBufferSnapshot,
) {
self.autoclose_regions.retain(|state| {
+ if !state.range.start.is_valid(buffer) || !state.range.end.is_valid(buffer) {
+ return false;
+ }
+
let mut i = 0;
while let Some(selection) = selections.get(i) {
if selection.end.cmp(&state.range.start, buffer).is_lt() {
@@ -5891,18 +5898,20 @@ impl Editor {
text: new_text[common_prefix_len..].into(),
});
- self.transact(window, cx, |this, window, cx| {
+ self.transact(window, cx, |editor, window, cx| {
if let Some(mut snippet) = snippet {
snippet.text = new_text.to_string();
- this.insert_snippet(&ranges, snippet, window, cx).log_err();
+ editor
+ .insert_snippet(&ranges, snippet, window, cx)
+ .log_err();
} else {
- this.buffer.update(cx, |buffer, cx| {
+ editor.buffer.update(cx, |multi_buffer, cx| {
let auto_indent = match completion.insert_text_mode {
Some(InsertTextMode::AS_IS) => None,
- _ => this.autoindent_mode.clone(),
+ _ => editor.autoindent_mode.clone(),
};
let edits = ranges.into_iter().map(|range| (range, new_text.as_str()));
- buffer.edit(edits, auto_indent, cx);
+ multi_buffer.edit(edits, auto_indent, cx);
});
}
for (buffer, edits) in linked_edits {
@@ -5921,8 +5930,9 @@ impl Editor {
})
}
- this.refresh_inline_completion(true, false, window, cx);
+ editor.refresh_inline_completion(true, false, window, cx);
});
+ self.invalidate_autoclose_regions(&self.selections.disjoint_anchors(), &snapshot);
let show_new_completions_on_confirm = completion
.confirm
@@ -9562,27 +9572,46 @@ impl Editor {
// Check whether the just-entered snippet ends with an auto-closable bracket.
if self.autoclose_regions.is_empty() {
let snapshot = self.buffer.read(cx).snapshot(cx);
- for selection in &mut self.selections.all::<Point>(cx) {
+ let mut all_selections = self.selections.all::<Point>(cx);
+ for selection in &mut all_selections {
let selection_head = selection.head();
let Some(scope) = snapshot.language_scope_at(selection_head) else {
continue;
};
let mut bracket_pair = None;
- let next_chars = snapshot.chars_at(selection_head).collect::<String>();
- let prev_chars = snapshot
- .reversed_chars_at(selection_head)
- .collect::<String>();
- for (pair, enabled) in scope.brackets() {
- if enabled
- && pair.close
- && prev_chars.starts_with(pair.start.as_str())
- && next_chars.starts_with(pair.end.as_str())
- {
- bracket_pair = Some(pair.clone());
- break;
+ let max_lookup_length = scope
+ .brackets()
+ .map(|(pair, _)| {
+ pair.start
+ .as_str()
+ .chars()
+ .count()
+ .max(pair.end.as_str().chars().count())
+ })
+ .max();
+ if let Some(max_lookup_length) = max_lookup_length {
+ let next_text = snapshot
+ .chars_at(selection_head)
+ .take(max_lookup_length)
+ .collect::<String>();
+ let prev_text = snapshot
+ .reversed_chars_at(selection_head)
+ .take(max_lookup_length)
+ .collect::<String>();
+
+ for (pair, enabled) in scope.brackets() {
+ if enabled
+ && pair.close
+ && prev_text.starts_with(pair.start.as_str())
+ && next_text.starts_with(pair.end.as_str())
+ {
+ bracket_pair = Some(pair.clone());
+ break;
+ }
}
}
+
if let Some(pair) = bracket_pair {
let snapshot_settings = snapshot.language_settings_at(selection_head, cx);
let autoclose_enabled =
crates/editor/src/editor_tests.rs π
@@ -13400,6 +13400,178 @@ async fn test_as_is_completions(cx: &mut TestAppContext) {
cx.assert_editor_state("fn a() {}\n unsafeΛ");
}
+#[gpui::test]
+async fn test_panic_during_c_completions(cx: &mut TestAppContext) {
+ init_test(cx, |_| {});
+ let language =
+ Arc::try_unwrap(languages::language("c", tree_sitter_c::LANGUAGE.into())).unwrap();
+ let mut cx = EditorLspTestContext::new(
+ language,
+ lsp::ServerCapabilities {
+ completion_provider: Some(lsp::CompletionOptions {
+ ..lsp::CompletionOptions::default()
+ }),
+ ..lsp::ServerCapabilities::default()
+ },
+ cx,
+ )
+ .await;
+
+ cx.set_state(
+ "#ifndef BAR_H
+#define BAR_H
+
+#include <stdbool.h>
+
+int fn_branch(bool do_branch1, bool do_branch2);
+
+#endif // BAR_H
+Λ",
+ );
+ cx.executor().run_until_parked();
+ cx.update_editor(|editor, window, cx| {
+ editor.handle_input("#", window, cx);
+ });
+ cx.executor().run_until_parked();
+ cx.update_editor(|editor, window, cx| {
+ editor.handle_input("i", window, cx);
+ });
+ cx.executor().run_until_parked();
+ cx.update_editor(|editor, window, cx| {
+ editor.handle_input("n", window, cx);
+ });
+ cx.executor().run_until_parked();
+ cx.assert_editor_state(
+ "#ifndef BAR_H
+#define BAR_H
+
+#include <stdbool.h>
+
+int fn_branch(bool do_branch1, bool do_branch2);
+
+#endif // BAR_H
+#inΛ",
+ );
+
+ cx.lsp
+ .set_request_handler::<lsp::request::Completion, _, _>(move |_, _| async move {
+ Ok(Some(lsp::CompletionResponse::List(lsp::CompletionList {
+ is_incomplete: false,
+ item_defaults: None,
+ items: vec![lsp::CompletionItem {
+ kind: Some(lsp::CompletionItemKind::SNIPPET),
+ label_details: Some(lsp::CompletionItemLabelDetails {
+ detail: Some("header".to_string()),
+ description: None,
+ }),
+ label: " include".to_string(),
+ text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
+ range: lsp::Range {
+ start: lsp::Position {
+ line: 8,
+ character: 1,
+ },
+ end: lsp::Position {
+ line: 8,
+ character: 1,
+ },
+ },
+ new_text: "include \"$0\"".to_string(),
+ })),
+ sort_text: Some("40b67681include".to_string()),
+ insert_text_format: Some(lsp::InsertTextFormat::SNIPPET),
+ filter_text: Some("include".to_string()),
+ insert_text: Some("include \"$0\"".to_string()),
+ ..lsp::CompletionItem::default()
+ }],
+ })))
+ });
+ cx.update_editor(|editor, window, cx| {
+ editor.show_completions(&ShowCompletions { trigger: None }, window, cx);
+ });
+ cx.executor().run_until_parked();
+ cx.update_editor(|editor, window, cx| {
+ editor.confirm_completion(&ConfirmCompletion::default(), window, cx)
+ });
+ cx.executor().run_until_parked();
+ cx.assert_editor_state(
+ "#ifndef BAR_H
+#define BAR_H
+
+#include <stdbool.h>
+
+int fn_branch(bool do_branch1, bool do_branch2);
+
+#endif // BAR_H
+#include \"Λ\"",
+ );
+
+ cx.lsp
+ .set_request_handler::<lsp::request::Completion, _, _>(move |_, _| async move {
+ Ok(Some(lsp::CompletionResponse::List(lsp::CompletionList {
+ is_incomplete: true,
+ item_defaults: None,
+ items: vec![lsp::CompletionItem {
+ kind: Some(lsp::CompletionItemKind::FILE),
+ label: "AGL/".to_string(),
+ text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
+ range: lsp::Range {
+ start: lsp::Position {
+ line: 8,
+ character: 10,
+ },
+ end: lsp::Position {
+ line: 8,
+ character: 11,
+ },
+ },
+ new_text: "AGL/".to_string(),
+ })),
+ sort_text: Some("40b67681AGL/".to_string()),
+ insert_text_format: Some(lsp::InsertTextFormat::PLAIN_TEXT),
+ filter_text: Some("AGL/".to_string()),
+ insert_text: Some("AGL/".to_string()),
+ ..lsp::CompletionItem::default()
+ }],
+ })))
+ });
+ cx.update_editor(|editor, window, cx| {
+ editor.show_completions(&ShowCompletions { trigger: None }, window, cx);
+ });
+ cx.executor().run_until_parked();
+ cx.update_editor(|editor, window, cx| {
+ editor.confirm_completion(&ConfirmCompletion::default(), window, cx)
+ });
+ cx.executor().run_until_parked();
+ cx.assert_editor_state(
+ r##"#ifndef BAR_H
+#define BAR_H
+
+#include <stdbool.h>
+
+int fn_branch(bool do_branch1, bool do_branch2);
+
+#endif // BAR_H
+#include "AGL/Λ"##,
+ );
+
+ cx.update_editor(|editor, window, cx| {
+ editor.handle_input("\"", window, cx);
+ });
+ cx.executor().run_until_parked();
+ cx.assert_editor_state(
+ r##"#ifndef BAR_H
+#define BAR_H
+
+#include <stdbool.h>
+
+int fn_branch(bool do_branch1, bool do_branch2);
+
+#endif // BAR_H
+#include "AGL/"Λ"##,
+ );
+}
+
#[gpui::test]
async fn test_no_duplicated_completion_requests(cx: &mut TestAppContext) {
init_test(cx, |_| {});
crates/multi_buffer/src/anchor.rs π
@@ -167,10 +167,10 @@ impl Anchor {
if *self == Anchor::min() || *self == Anchor::max() {
true
} else if let Some(excerpt) = snapshot.excerpt(self.excerpt_id) {
- excerpt.contains(self)
- && (self.text_anchor == excerpt.range.context.start
- || self.text_anchor == excerpt.range.context.end
- || self.text_anchor.is_valid(&excerpt.buffer))
+ (self.text_anchor == excerpt.range.context.start
+ || self.text_anchor == excerpt.range.context.end
+ || self.text_anchor.is_valid(&excerpt.buffer))
+ && excerpt.contains(self)
} else {
false
}
crates/project/src/lsp_command.rs π
@@ -2269,7 +2269,7 @@ impl LspCommand for GetCompletions {
// the range based on the syntax tree.
None => {
if self.position != clipped_position {
- log::info!("completion out of expected range");
+ log::info!("completion out of expected range ");
return false;
}
@@ -2483,7 +2483,9 @@ pub(crate) fn parse_completion_text_edit(
let start = snapshot.clip_point_utf16(range.start, Bias::Left);
let end = snapshot.clip_point_utf16(range.end, Bias::Left);
if start != range.start.0 || end != range.end.0 {
- log::info!("completion out of expected range");
+ log::info!(
+ "completion out of expected range, start: {start:?}, end: {end:?}, range: {range:?}"
+ );
return None;
}
snapshot.anchor_before(start)..snapshot.anchor_after(end)
crates/text/src/anchor.rs π
@@ -99,7 +99,9 @@ impl Anchor {
} else if self.buffer_id != Some(buffer.remote_id) {
false
} else {
- let fragment_id = buffer.fragment_id_for_anchor(self);
+ let Some(fragment_id) = buffer.try_fragment_id_for_anchor(self) else {
+ return false;
+ };
let mut fragment_cursor = buffer.fragments.cursor::<(Option<&Locator>, usize)>(&None);
fragment_cursor.seek(&Some(fragment_id), Bias::Left);
fragment_cursor
crates/text/src/text.rs π
@@ -2330,10 +2330,19 @@ impl BufferSnapshot {
}
fn fragment_id_for_anchor(&self, anchor: &Anchor) -> &Locator {
+ self.try_fragment_id_for_anchor(anchor).unwrap_or_else(|| {
+ panic!(
+ "invalid anchor {:?}. buffer id: {}, version: {:?}",
+ anchor, self.remote_id, self.version,
+ )
+ })
+ }
+
+ fn try_fragment_id_for_anchor(&self, anchor: &Anchor) -> Option<&Locator> {
if *anchor == Anchor::MIN {
- Locator::min_ref()
+ Some(Locator::min_ref())
} else if *anchor == Anchor::MAX {
- Locator::max_ref()
+ Some(Locator::max_ref())
} else {
let anchor_key = InsertionFragmentKey {
timestamp: anchor.timestamp,
@@ -2354,20 +2363,12 @@ impl BufferSnapshot {
insertion_cursor.prev();
}
- let Some(insertion) = insertion_cursor.item().filter(|insertion| {
- if cfg!(debug_assertions) {
- insertion.timestamp == anchor.timestamp
- } else {
- true
- }
- }) else {
- panic!(
- "invalid anchor {:?}. buffer id: {}, version: {:?}",
- anchor, self.remote_id, self.version
- );
- };
-
- &insertion.fragment_id
+ insertion_cursor
+ .item()
+ .filter(|insertion| {
+ !cfg!(debug_assertions) || insertion.timestamp == anchor.timestamp
+ })
+ .map(|insertion| &insertion.fragment_id)
}
}