Merge pull request #1912 from zed-industries/matching-brackets-must-contain-range

Max Brunsfeld created 3 years ago

Fix enclosing-bracket bug that appeared in JS for loops

Change summary

crates/language/src/buffer.rs       | 31 ++++++++-------
crates/language/src/buffer_tests.rs | 62 ++++++++++++++++++++++++++++++-
2 files changed, 77 insertions(+), 16 deletions(-)

Detailed changes

crates/language/src/buffer.rs

@@ -2225,11 +2225,12 @@ impl BufferSnapshot {
         range: Range<T>,
     ) -> Option<(Range<usize>, Range<usize>)> {
         // Find bracket pairs that *inclusively* contain the given range.
-        let range = range.start.to_offset(self).saturating_sub(1)
-            ..self.len().min(range.end.to_offset(self) + 1);
-        let mut matches = self.syntax.matches(range, &self.text, |grammar| {
-            grammar.brackets_config.as_ref().map(|c| &c.query)
-        });
+        let range = range.start.to_offset(self)..range.end.to_offset(self);
+        let mut matches = self.syntax.matches(
+            range.start.saturating_sub(1)..self.len().min(range.end + 1),
+            &self.text,
+            |grammar| grammar.brackets_config.as_ref().map(|c| &c.query),
+        );
         let configs = matches
             .grammars()
             .iter()
@@ -2252,18 +2253,20 @@ impl BufferSnapshot {
 
             matches.advance();
 
-            if let Some((open, close)) = open.zip(close) {
-                let len = close.end - open.start;
+            let Some((open, close)) = open.zip(close) else { continue };
+            if open.start > range.start || close.end < range.end {
+                continue;
+            }
+            let len = close.end - open.start;
 
-                if let Some((existing_open, existing_close)) = &result {
-                    let existing_len = existing_close.end - existing_open.start;
-                    if len > existing_len {
-                        continue;
-                    }
+            if let Some((existing_open, existing_close)) = &result {
+                let existing_len = existing_close.end - existing_open.start;
+                if len > existing_len {
+                    continue;
                 }
-
-                result = Some((open, close));
             }
+
+            result = Some((open, close));
         }
 
         result

crates/language/src/buffer_tests.rs

@@ -573,14 +573,72 @@ fn test_enclosing_bracket_ranges(cx: &mut MutableAppContext) {
         ))
     );
 
-    // Regression test: avoid crash when querying at the end of the buffer.
     assert_eq!(
-        buffer.enclosing_bracket_point_ranges(buffer.len() - 1..buffer.len()),
+        buffer.enclosing_bracket_point_ranges(Point::new(4, 1)..Point::new(4, 1)),
         Some((
             Point::new(0, 6)..Point::new(0, 7),
             Point::new(4, 0)..Point::new(4, 1)
         ))
     );
+
+    // Regression test: avoid crash when querying at the end of the buffer.
+    assert_eq!(
+        buffer.enclosing_bracket_point_ranges(Point::new(4, 1)..Point::new(5, 0)),
+        None
+    );
+}
+
+#[gpui::test]
+fn test_enclosing_bracket_ranges_where_brackets_are_not_outermost_children(
+    cx: &mut MutableAppContext,
+) {
+    let javascript_language = Arc::new(
+        Language::new(
+            LanguageConfig {
+                name: "JavaScript".into(),
+                ..Default::default()
+            },
+            Some(tree_sitter_javascript::language()),
+        )
+        .with_brackets_query(
+            r#"
+            ("{" @open "}" @close)
+            ("(" @open ")" @close)
+            "#,
+        )
+        .unwrap(),
+    );
+
+    cx.set_global(Settings::test(cx));
+    let buffer = cx.add_model(|cx| {
+        let text = "
+            for (const a in b) {
+                // a comment that's longer than the for-loop header
+            }
+        "
+        .unindent();
+        Buffer::new(0, text, cx).with_language(javascript_language, cx)
+    });
+
+    let buffer = buffer.read(cx);
+    assert_eq!(
+        buffer.enclosing_bracket_point_ranges(Point::new(0, 18)..Point::new(0, 18)),
+        Some((
+            Point::new(0, 4)..Point::new(0, 5),
+            Point::new(0, 17)..Point::new(0, 18)
+        ))
+    );
+
+    // Regression test: even though the parent node of the parentheses (the for loop) does
+    // intersect the given range, the parentheses themselves do not contain the range, so
+    // they should not be returned. Only the curly braces contain the range.
+    assert_eq!(
+        buffer.enclosing_bracket_point_ranges(Point::new(0, 20)..Point::new(0, 20)),
+        Some((
+            Point::new(0, 19)..Point::new(0, 20),
+            Point::new(2, 0)..Point::new(2, 1)
+        ))
+    );
 }
 
 #[gpui::test]