crates/config/src/rule/range.rs (1)

4-9: Enhance documentation and add validation for position fields.

While the structure is well-designed, consider these improvements:

1. Expand documentation to explain:
   • Zero vs. one-based indexing for line/column
   • Expected range of values
2. Add validation to prevent invalid positions

Consider adding a constructor with validation:

impl SerializablePosition {
    pub fn new(line: usize, column: usize) -> Result<Self, &'static str> {
        // Assuming 0-based indexing
        if line == 0 && column == 0 {
            return Err("Position cannot be 0,0");
        }
        Ok(Self { line, column })
    }
}

crates/napi/manual.d.ts (2)

36-41: LGTM! Consider enhancing type documentation.

The Position interface is well-defined and aligns with standard LSP implementations. The JSDoc comments clearly indicate 0-based indexing.

Consider adding a brief interface-level JSDoc comment to provide context:

+/**
+ * Represents a position in the source code using 0-based indexing.
+ * This aligns with LSP's position representation.
+ */
 export interface Position {
   /** 0-indexed row number. */
   row: number
   /** 0-indexed column number. */
   column: number
 }

---

43-46: Add JSDoc documentation for the Range interface.

The Range interface structure is correct, but would benefit from clear documentation about range boundaries (inclusive/exclusive) and usage context.

Consider adding detailed JSDoc documentation:

+/**
+ * Represents a range in the source code.
+ * @property start The position where the range begins (inclusive)
+ * @property end The position where the range ends (exclusive)
+ * 
+ * This interface is used by the RangeMatcher to select nodes within specific boundaries,
+ * particularly useful for LSP integration scenarios.
+ */
 export interface Range {
   start: Position
   end: Position
 }

crates/core/src/matcher/range.rs (2)

80-128: Expand test coverage

The current test suite could be enhanced with additional test cases:

1. Error cases (invalid ranges)
2. Edge cases (zero-length ranges, end of file)
3. Multi-line ranges
4. Overlapping ranges
5. Position validation in try_new()

Add these test cases:

#[test]
fn test_invalid_range() {
    let result = RangeMatcher::<Tsx>::try_new(0, 10, 0, 5);
    assert!(matches!(result, Err(RangeMatcherError::InvalidRange)));
}

#[test]
fn test_multi_line_range() {
    let cand = pattern_node("class A {\n  a = 123\n}");
    let cand = cand.root();
    let pattern = RangeMatcher::new(0, 10, 1, 8);
    assert!(pattern.find_node(cand.clone()).is_some());
}

---

1-128: Consider architectural improvements for better integration

While the basic implementation is sound, consider these architectural improvements:

1. Add a builder pattern for constructing ranges with validation
2. Implement Debug and Display traits for better error messages
3. Consider adding a RangeMatcherConfig for customizable matching behavior (exact vs. overlap)
4. Add serialization support for integration with the rule system

These improvements would make the RangeMatcher more flexible and easier to integrate with the larger system, especially for LSP integration mentioned in the PR objectives.

crates/core/src/matcher.rs (1)

11-11: Update module documentation to include RangeMatcher

The file's doc comment lists "notable implementations" but should be updated to include the new RangeMatcher implementation, as it's a significant addition for LSP integration.

Apply this diff to update the documentation:

 //! `Matcher` has three notable implementations in this module:
 //! * Pattern: matches against a tree-sitter node based on its tree structure.
 //! * KindMatcher: matches a node based on its `kind`
 //! * RegexMatcher: matches a node based on its textual content using regex.
+//! * RangeMatcher: matches a node based on its position in source code.

crates/core/src/source.rs (1)

Line range hint 143-195: Add tests for the new get_source_text method.

While the implementation is straightforward, it would be beneficial to add tests to verify the behavior and prevent regressions.

Consider adding the following test to the test module:

#[test]
fn test_get_source_text() {
    let src = "var a = 1234".to_string();
    assert_eq!(src.get_source_text(), "var a = 1234");
}

crates/core/src/node.rs (1)

186-222: LGTM! The implementation correctly handles UTF-8 and line endings.

The method successfully converts byte offsets to codepoint positions while properly handling UTF-8 characters and CRLF line endings.

Consider caching the results for frequently accessed nodes to improve performance, as the current implementation needs to iterate through all characters up to the end position.

+use std::cell::RefCell;
+use std::collections::HashMap;
+
 pub struct Node<'r, D: Doc> {
   pub(crate) inner: tree_sitter::Node<'r>,
   pub(crate) root: &'r Root<D>,
+  pos_cache: RefCell<HashMap<usize, std::ops::Range<(usize, usize)>>>,
 }

crates/config/src/rule/mod.rs (1)

61-62: Improve documentation comments for 'range' field

Capitalize the first letter of the sentence and ensure consistent punctuation.

Apply this diff to improve the comments:

 /// `range` accepts a range object.
-/// the target node must exactly appear in the range.
+/// The target node must exactly appear in the range.

Configuration used: CodeRabbit UI
Review profile: CHILL

-  /** The exact range of the node in the source code. */
+  /**
+   * The exact range of the node in the source code.
+   * When specified, matches nodes that exactly span this range.
+   * Can be combined with other matchers like `pattern` or `kind` for more precise matching.
+   * Particularly useful for LSP integration scenarios where positions are provided in row/column format.
+   */
   range?: Range

-  /** The exact range of the node in the source code. */
+  /**
+   * The exact range of the node in the source code.
+   * Matches nodes that precisely span the specified start and end positions.
+   * Can be used standalone or combined with other matchers (pattern, kind).
+   * Positions are zero-based and specified in {line, column} format.
+   */
   range?: Range

#!/bin/bash
# Description: Check for test cases that combine range matcher with other matchers

# Look for test cases combining range with other matchers
rg -l "range.*pattern|pattern.*range" "test" || echo "No combined matcher tests found"

# Check if there are examples in documentation
fd -e md | xargs rg -l "range.*matcher" || echo "No range matcher documentation found"

#!/bin/bash
# Search for test files in the entire repository
fd -e test.ts -e test.js -e spec.ts -e spec.js

# Search for any files containing 'range' in combination with matching-related terms
rg -l "range.*match|match.*range"

# Look for actual implementation of range matcher to understand integration
ast-grep --pattern 'function $_($$_) {
  $$$
  range
  $$$
}'

#!/bin/bash
# Check the test file for range matcher usage
rg -A 5 -B 5 "range" "crates/napi/__test__/index.spec.ts"

# Check the range matcher implementation
cat "crates/core/src/matcher/range.rs"

# Check node matcher for range integration
rg -A 10 "range" "crates/core/src/matcher/node_match.rs"

if c == '\r' && source.chars().nth(i + 1) == Some('\n') {
  current_byte += 1; // Skip the '\n' in CRLF
}

#!/bin/bash
# Search for edge case tests in the codebase
echo "Searching for empty node tests..."
rg -l "empty.*node" --type rust

echo "Searching for EOF handling tests..."
rg -l "end.*file|eof" --type rust

echo "Searching for line ending tests..."
rg -l "CRLF|\\r\\n|line.*ending" --type rust

#!/bin/bash
# Search for test cases related to node positions and ranges
echo "Searching for node position/range tests..."
rg -l "test.*pos|test.*range" --type rust

# Look for actual test implementations
echo "Checking test implementations..."
rg -A 10 "fn.*test.*pos|fn.*test.*range" --type rust

# Search for specific handling of empty nodes
echo "Checking empty node handling..."
rg -A 5 "start_byte.*==.*end_byte" --type rust

# Check for line ending related code
echo "Checking line ending handling..."
rg -U -A 5 "\\r\\n|\\n|line.*end" --type rust crates/core/src/

#!/bin/bash
# Search for tests in node.rs that might cover empty nodes and line endings
echo "Checking node.rs tests..."
rg -A 10 "fn test.*" crates/core/src/node.rs

# Look for specific test cases with empty nodes
echo "Checking empty node test cases..."
ast-grep --pattern 'fn test_$_() {
  $$$
  assert_eq!($_, $_, "empty node");
  $$$
}'

# Look for line ending related tests
echo "Checking line ending tests..."
rg -U -A 10 "\\r\\n|CRLF|line.*end" crates/core/src/node.rs

crates/core/src/matcher/range.rs (1)

16-20: Consider implementing Display trait for Position

While previous comments covered documentation and comparison methods, the Position struct could benefit from implementing the Display trait for better error messages and debugging, especially when used with LSP integration.

 pub struct Position<L: Language> {
     row: usize,
     column: usize,
     lang: PhantomData<L>,
 }
+
+impl<L: Language> std::fmt::Display for Position<L> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}:{}", self.row, self.column)
+    }
+}

crates/napi/src/doc.rs (1)

111-114: LGTM! Consider a small optimization.

The implementation is correct and aligns well with the Content trait requirements. However, we could avoid the temporary variable s and directly return the result of encode_bytes.

  fn get_source_text(&self) -> Cow<'_, str> {
-    let s = Self::encode_bytes(&self.inner);
-    s
+    Self::encode_bytes(&self.inner)
  }

crates/core/src/node.rs (1)

186-222: Consider optimizing the implementation for better performance

The current implementation iterates through all characters up to the end position. This could be optimized by:

1. Using chars().take_while() to stop at end_byte
2. Pre-calculating the string slice up to end_byte
3. Using the tree-sitter's built-in position information where possible

 pub fn codepoint_pos_range(&self) -> std::ops::Range<(usize, usize)> {
   let source = self.root.doc.get_source().get_source_text();
   let start_byte = self.range().start;
   let end_byte = self.range().end;
-
   let mut start_pos = (0, 0);
   let mut end_pos = (0, 0);
   let mut current_pos = (0, 0);
   let mut current_byte = 0;
-
-  for (i, c) in source.chars().enumerate() {
+  // Optimize by only processing up to end_byte
+  let relevant_text = &source[..end_byte];
+  for (i, c) in relevant_text.chars().enumerate() {
     if current_byte == start_byte {
       start_pos = current_pos;
     }
     if current_byte == end_byte {
       end_pos = current_pos;
       break;
     }
     if c == '\n' || c == '\r' {
       if c == '\r' && source.chars().nth(i + 1) == Some('\n') {
         current_byte += 1; // Skip the '\n' in CRLF
       }
       current_pos.0 += 1;
       current_pos.1 = 0;
     } else {
       current_pos.1 += 1;
     }
     current_byte += c.len_utf8();
   }
-
-  if current_byte == end_byte {
-    end_pos = current_pos;
-  }
+  // Set end_pos if we haven't found it yet
+  if end_pos == (0, 0) {
+    end_pos = current_pos;
+  }
   start_pos..end_pos
 }

Configuration used: CodeRabbit UI
Review profile: CHILL

 #[test]
 fn test_codepoint_range() {
     let multi_line_code = r#"let a = 123;
 let b = "Übergänge";
 "#;
     let root = Tsx.ast_grep(multi_line_code);
     let node = root.root().find("\"Übergänge\"").expect("should exist");
     let range = node.codepoint_pos_range();
     assert_eq!(range.start, (1, 8));
     assert_eq!(range.end, (1, 19));
+
+    // Test CRLF line endings
+    let crlf_code = "let a = 123;\r\nlet b = 456;";
+    let root = Tsx.ast_grep(crlf_code);
+    let node = root.root().find("456").expect("should exist");
+    let range = node.codepoint_pos_range();
+    assert_eq!(range.start, (1, 8));
+    assert_eq!(range.end, (1, 11));
+
+    // Test empty nodes
+    let empty_code = "let a = '';";
+    let root = Tsx.ast_grep(empty_code);
+    let node = root.root().find("''").expect("should exist");
+    let range = node.codepoint_pos_range();
+    assert_eq!(range.start, (0, 8));
+    assert_eq!(range.end, (0, 10));
+
+    // Test multi-line nodes
+    let multi_line = "let a = `\nline1\nline2\n`;";
+    let root = Tsx.ast_grep(multi_line);
+    let node = root.root().find("`$_`").expect("should exist");
+    let range = node.codepoint_pos_range();
+    assert_eq!(range.start, (0, 8));
+    assert_eq!(range.end, (3, 1));
 }

crates/core/src/node.rs (1)

186-222: Consider optimizing the position calculation implementation.

The implementation is correct but could be optimized in a few ways:

1. Pre-calculate the end position if we know the byte length
2. Use chars().take_while() to avoid iterating past the end position
3. Consider caching the results for frequently accessed nodes

 pub fn codepoint_pos_range(&self) -> std::ops::Range<(usize, usize)> {
   let source = self.root.doc.get_source().get_source_text();
   let start_byte = self.range().start;
   let end_byte = self.range().end;

   let mut start_pos = (0, 0);
   let mut end_pos = (0, 0);
   let mut current_pos = (0, 0);
   let mut current_byte = 0;

-  for (i, c) in source.chars().enumerate() {
+  for (i, c) in source.chars()
+    .take_while(|_| current_byte <= end_byte)
+    .enumerate() {
     if current_byte == start_byte {
       start_pos = current_pos;
     }
     if current_byte == end_byte {
       end_pos = current_pos;
       break;
     }
     if c == '\n' || c == '\r' {
       if c == '\r' && source.chars().nth(i + 1) == Some('\n') {
         current_byte += 1; // Skip the '\n' in CRLF
       }
       current_pos.0 += 1;
       current_pos.1 = 0;
     } else {
       current_pos.1 += 1;
     }
     current_byte += c.len_utf8();
   }

   if current_byte == end_byte {
     end_pos = current_pos;
   }

   start_pos..end_pos
 }

crates/config/src/rule/mod.rs (3)

61-64: Consider enhancing the range field documentation

While the documentation is good, it could be more descriptive by including:

• Example usage with line/column numbers
• Common use cases (e.g., LSP integration)
• Relationship with other matchers

---

500-507: Consider improving range deserialization robustness

The current implementation could be enhanced for better maintainability and validation:

1. Consider using a builder pattern or constructor method for RangeMatcher to encapsulate the creation logic
2. Add validation for row/column values before creating the matcher

Example approach:

-  if let Some(range) = atomic.range {
-    rules.push(R::Range(RangeMatcher::try_new(
-      range.start.row,
-      range.start.column,
-      range.end.row,
-      range.end.column,
-    )?));
-  }
+  if let Some(range) = atomic.range {
+    rules.push(R::Range(RangeMatcher::builder()
+      .with_start_position(range.start.row, range.start.column)
+      .with_end_position(range.end.row, range.end.column)
+      .validate()?
+      .build()?));
+  }

---

Line range hint 511-685: Add test coverage for range matcher

The test module lacks coverage for the new range matcher functionality. Consider adding tests for:

• Range matcher serialization/deserialization
• Invalid range scenarios
• Integration with other matchers

Example test case:

#[test]
fn test_range_matcher() {
    let src = r#"
    range:
      start:
        row: 1
        column: 0
      end:
        row: 2
        column: 10
    "#;
    let rule: SerializableRule = from_str(src).expect("cannot parse rule");
    let env = DeserializeEnv::new(TypeScript::Tsx);
    let rule = deserialize_rule(rule, &env).expect("should deserialize");
    // Add assertions here
}

Configuration used: CodeRabbit UI
Review profile: CHILL

 /// Represents a position in a document
+/// 
+/// Position in a document expressed as zero-based line and character offset.
+/// A position is between two characters like an 'insert' cursor in a editor.
 #[derive(Serialize, Deserialize, Clone, JsonSchema)]
 pub struct SerializablePosition {
-    pub row: usize,
-    pub column: usize,
+    pub line: u32,
+    pub character: u32,
 }

 /// Represents a range rule with a start and end position
+/// 
+/// A range represents a span between two positions.
+/// The start position is inclusive and the end position is exclusive.
- #[derive(Serialize, Deserialize, Clone, JsonSchema)]
+ #[derive(Debug, Serialize, Deserialize, Clone, JsonSchema)]
 pub struct SerializableRange {
     pub start: SerializablePosition,
     pub end: SerializablePosition,
 }
+
+impl SerializableRange {
+    /// Creates a new range after validating that start precedes end
+    pub fn new(start: SerializablePosition, end: SerializablePosition) -> Result<Self, &'static str> {
+        if start.line > end.line || (start.line == end.line && start.character > end.character) {
+            return Err("start position must precede end position");
+        }
+        Ok(Self { start, end })
+    }
+
+    /// Returns true if the given position is within this range
+    pub fn contains(&self, pos: &SerializablePosition) -> bool {
+        (self.start.line < pos.line || 
+         (self.start.line == pos.line && self.start.character <= pos.character))
+        &&
+        (pos.line < self.end.line ||
+         (pos.line == self.end.line && pos.character < self.end.character))
+    }
+}

crates/config/src/rule/range.rs (3)

12-19: Enhance range documentation for clarity.

The documentation should specify:

1. Whether the end position is inclusive or exclusive
2. That start position must precede end position

 /// Represents a position in source code using 0-based row and column numbers
+/// A range represents a span of text in source code. The end position is exclusive.
+/// The start position must precede the end position in the document.
 #[derive(Serialize, Deserialize, Clone, JsonSchema)]
 pub struct SerializableRange {
   /// 0-based row number in the source code
   pub start: SerializablePosition,
-  /// 0-based column number in the source code
+  /// 0-based column number in the source code (exclusive)
   pub end: SerializablePosition,
 }

---

61-74: Extract range validation logic for reusability.

Consider moving the range validation logic to SerializableRange for reuse:

 impl SerializableRange {
+    pub fn validate(&self) -> Result<(), RangeMatcherError> {
+        if self.start.row > self.end.row
+            || (self.start.row == self.end.row && self.start.column > self.end.column)
+        {
+            return Err(RangeMatcherError::InvalidRange);
+        }
+        Ok(())
+    }
 }

 impl<L: Language> RangeMatcher<L> {
     pub fn try_new(
         start_pos: SerializablePosition,
         end_pos: SerializablePosition,
     ) -> Result<RangeMatcher<L>, RangeMatcherError> {
-        if start_pos.row > end_pos.row
-            || (start_pos.row == end_pos.row && start_pos.column > end_pos.column)
-        {
-            return Err(RangeMatcherError::InvalidRange);
-        }
+        let range = SerializableRange { start: start_pos, end: end_pos };
+        range.validate()?;
         let range = Self::new(start_pos, end_pos);
         Ok(range)
     }
 }

---

99-161: Add test cases for edge cases.

Consider adding tests for:

1. Zero-length ranges (where start equals end)
2. Ranges spanning multiple lines with Unicode characters
3. Ranges at file boundaries (start of file and end of file)

Configuration used: CodeRabbit UI
Review profile: CHILL

 #[derive(Serialize, Deserialize, Clone, JsonSchema)]
 pub struct SerializablePosition {
-  pub row: usize,
-  pub column: usize,
+  pub line: u32,
+  pub character: u32,
 }

crates/config/src/rule/range.rs (3)

46-74: Add documentation and consider extracting validation logic.

The RangeMatcher implementation could be improved:

1. Add documentation for the struct explaining its purpose
2. Consider moving validation logic to SerializableRange for reuse

+/// Matches nodes that exactly appear within the specified range.
+/// The range is inclusive of both start and end positions.
 pub struct RangeMatcher<L: Language> {
   start: SerializablePosition,
   end: SerializablePosition,
   lang: PhantomData<L>,
 }

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 69-72: crates/config/src/rule/range.rs#L69-L72
Added lines #L69 - L72 were not covered by tests

---

76-97: Consider caching node positions for better performance.

The current implementation calls start_pos() and end_pos() for each node check. Consider caching these values when processing multiple nodes:

 fn match_node_with_env<'tree, D: Doc<Lang = L>>(
   &self,
   node: Node<'tree, D>,
   _env: &mut Cow<MetaVarEnv<'tree, D>>,
 ) -> Option<Node<'tree, D>> {
+  // Cache positions to avoid multiple calls
+  let (node_start_pos, node_end_pos) = (node.start_pos(), node.end_pos());
-  let node_start_pos = node.start_pos();
-  let node_end_pos = node.end_pos();

   if self.start.equals_node_pos(&node_start_pos, &node)
     && self.end.equals_node_pos(&node_end_pos, &node)
   {
     Some(node)
   } else {
     None
   }
 }

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 94-96: crates/config/src/rule/range.rs#L94-L96
Added lines #L94 - L96 were not covered by tests

---

99-161: Enhance test coverage with edge cases.

Consider adding tests for:

1. Positions at file boundaries (0,0)
2. Empty ranges (start = end)
3. Unicode character handling
4. Very large position values

Would you like me to help generate these additional test cases?

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 123-125: crates/config/src/rule/range.rs#L123-L125
Added lines #L123 - L125 were not covered by tests

---

[warning] 139-141: crates/config/src/rule/range.rs#L139-L141
Added lines #L139 - L141 were not covered by tests

---

[warning] 156-158: crates/config/src/rule/range.rs#L156-L158
Added lines #L156 - L158 were not covered by tests

Configuration used: CodeRabbit UI
Review profile: CHILL