From a7f1af7174d689eeff26bf474ec69970f701f90c Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Tue, 20 May 2025 15:40:45 +0200 Subject: [PATCH] Allow finding symbols by qualname. Also allow finding parents in Symbol Fairly extensive testing included --- src/serena/symbol.py | 215 +++++++++++++++++++++++++++---- test/__init__.py | 1 + test/serena/__init__.py | 1 + test/serena/test_serena_agent.py | 74 +++++++++++ test/serena/test_symbol.py | 133 +++++++++++++++++++ 5 files changed, 397 insertions(+), 27 deletions(-) create mode 100644 test/serena/__init__.py create mode 100644 test/serena/test_symbol.py diff --git a/src/serena/symbol.py b/src/serena/symbol.py index 0fcc553f..25c50e19 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -50,18 +50,90 @@ class SymbolLocation: class Symbol(ToStringMixin): - def __init__(self, s: UnifiedSymbolInformation) -> None: - self.s = s + _QAULNAME_SEPARATOR = "/" + + @staticmethod + def match_against_qualname( + name_pattern: str, + qual_name_parts: list[str], + substring_matching: bool, + ) -> bool: + """ + Checks if a given name/pattern matches a symbol's qualified name parts. + + :param name_pattern: The name or pattern to match. Can be a simple name + (e.g., "my_func") or a qualified name pattern + (e.g., "MyClass/my_method", "MyClass/"). + :param qual_name_parts: A list of strings representing the parts of the + a qualified name. + :param substring_matching: If True, allows substring matching for the relevant part(s). + - For simple names, the whole `name_pattern` is checked as a substring + of the symbol's simple name (`qual_name_parts[-1]`). + - For qualified name patterns, only the *last* part of the + `name_to_match` pattern is checked as a substring. + Other parts must match exactly. + :return: True if the name matches, False otherwise. + """ + assert name_pattern, "name_to_match must not be empty" + assert qual_name_parts, "symbol_qual_name_parts must not be empty" + qname_separator = Symbol._QAULNAME_SEPARATOR + is_qualified_pattern = qname_separator in name_pattern + + if not is_qualified_pattern: + # Simple name matching + symbol_simple_name = qual_name_parts[-1] + if substring_matching: + return name_pattern in symbol_simple_name + else: + return name_pattern == symbol_simple_name + # Qualified name pattern matching + # A pattern like "Lib/" or "Lib/Class/" is treated as a prefix. + # We strip the trailing separator to get the core parts to match. + # For example, "Lib/" becomes "Lib", "Lib/Class/" becomes "Lib/Class". + # A pattern of just "/" becomes an empty string. + cleaned_name_pattern = name_pattern.rstrip(qname_separator) + + # Splitting the cleaned pattern gives the segments to match. + # - "Lib" -> ["Lib"] + # - "Lib/Class" -> ["Lib", "Class"] + # - "" (from "/") -> [""] (split of empty string results in list with one empty string) + pattern_segments = cleaned_name_pattern.split(qname_separator) + + # The number of segments in the pattern must match the number of parts in the symbol's qualified name. + # Example: pattern "Lib/Class" (2 segments) should match a symbol like `MyClass` in `MyLib` (2 qual_name_parts: ["MyLib", "MyClass"]). + # It should not match `MyMethod` in `MyClass` in `MyLib` (3 qual_name_parts). + # Also, pattern "Lib/" (effectively "Lib", 1 segment) should match `MyLib` (1 qual_name_part: ["MyLib"]). + if len(pattern_segments) != len(qual_name_parts): + return False + + # Match all segments of the pattern except the last one. These must be exact matches. + # If pattern_segments has only one segment (e.g., "Lib", or from "Lib/", or from "/"), this loop is skipped. + for i in range(len(pattern_segments) - 1): + if pattern_segments[i] != qual_name_parts[i]: + return False + + # Match the last segment of the pattern against the last part of the symbol's qualified name. + last_pattern_segment = pattern_segments[-1] + last_qual_name_segment = qual_name_parts[-1] + + # The `substring_matching` flag applies to this last segment comparison. + if substring_matching: + return last_pattern_segment in last_qual_name_segment + else: + return last_pattern_segment == last_qual_name_segment + + def __init__(self, symbol_root_from_ls: UnifiedSymbolInformation) -> None: + self.symbol_root = symbol_root_from_ls def _tostring_includes(self) -> list[str]: return [] def _tostring_additional_entries(self) -> dict[str, Any]: - return dict(name=self.name, kind=self.kind, num_children=len(self.s["children"])) + return dict(name=self.name, kind=self.kind, num_children=len(self.symbol_root["children"])) @property def name(self) -> str: - return self.s["name"] + return self.symbol_root["name"] @property def kind(self) -> str: @@ -69,11 +141,14 @@ class Symbol(ToStringMixin): @property def symbol_kind(self) -> SymbolKind: - return self.s["kind"] + return self.symbol_root["kind"] @property def relative_path(self) -> str | None: - return self.s["location"]["relativePath"] + location = self.symbol_root.get("location") + if location: + return location.get("relativePath") + return None @property def location(self) -> SymbolLocation: @@ -83,36 +158,84 @@ class Symbol(ToStringMixin): return SymbolLocation(relative_path=self.relative_path, line=self.line, column=self.column) @property - def body_start_position(self) -> Position: - return self.s["location"]["range"]["start"] + def body_start_position(self) -> Position | None: + location = self.symbol_root.get("location") + if location: + range_info = location.get("range") + if range_info: + start_pos = range_info.get("start") + if start_pos: + return start_pos + return None @property - def body_end_position(self) -> Position: - return self.s["location"]["range"]["end"] + def body_end_position(self) -> Position | None: + location = self.symbol_root.get("location") + if location: + range_info = location.get("range") + if range_info: + end_pos = range_info.get("end") + if end_pos: + return end_pos + return None @property def line(self) -> int | None: - if "selectionRange" in self.s: - return self.s["selectionRange"]["start"]["line"] + if "selectionRange" in self.symbol_root: + return self.symbol_root["selectionRange"]["start"]["line"] else: # line is expected to be undefined for some types of symbols (e.g. SymbolKind.File) return None @property def column(self) -> int | None: - if "selectionRange" in self.s: - return self.s["selectionRange"]["start"]["character"] + if "selectionRange" in self.symbol_root: + return self.symbol_root["selectionRange"]["start"]["character"] else: # precise location is expected to be undefined for some types of symbols (e.g. SymbolKind.File) return None @property def body(self) -> str | None: - return self.s.get("body") + return self.symbol_root.get("body") - def iter_children(self) -> Iterator["Symbol"]: - for c in self.s["children"]: - yield Symbol(c) + def get_qualified_name(self) -> str: + """ + Get the qualified name of the symbol (e.g. "class/method/inner_function"). + """ + return self._QAULNAME_SEPARATOR.join(self.get_qualified_name_parts()) + + def get_qualified_name_parts(self) -> list[str]: + """ + Get the parts of the qualified name of the symbol (e.g. ["class", "method", "inner_function"]). + """ + ancestors_within_file = list(self.iter_ancestors(up_to_symbol_kind=SymbolKind.File)) + ancestors_within_file.reverse() + return [a.name for a in ancestors_within_file] + [self.name] + + def iter_children(self) -> Iterator[Self]: + for c in self.symbol_root["children"]: + yield self.__class__(c) + + def iter_ancestors(self, up_to_symbol_kind: SymbolKind | None = None) -> Iterator[Self]: + """ + Iterate over all ancestors of the symbol, starting with the parent and going up to the root or + the given symbol kind. + + :param up_to_symbol_kind: if provided, iteration will stop *before* the first ancestor of the given kind. + A typical use case is to pass `SymbolKind.File` or `SymbolKind.Package`. + """ + parent = self.get_parent() + if parent is not None: + if up_to_symbol_kind is None or parent.symbol_kind != up_to_symbol_kind: + yield parent + yield from parent.iter_ancestors(up_to_symbol_kind=up_to_symbol_kind) + + def get_parent(self) -> Self | None: + parent_root = self.symbol_root.get("parent") + if parent_root is None: + return None + return self.__class__(parent_root) def find( self, @@ -120,17 +243,45 @@ class Symbol(ToStringMixin): substring_matching: bool = False, include_kinds: Sequence[SymbolKind] | None = None, exclude_kinds: Sequence[SymbolKind] | None = None, - ) -> list["Symbol"]: + ) -> list[Self]: + """ + Find all symbols within the symbol's subtree that match the given name. + The name matching behavior depends on whether a qualified name or a simple name is provided. + It is assumed that the provided name is a qualified name if it contains the `/` character. + If substring matching is allowed, only the last element of the qualified name will be checked against + the symbol name using substring matching. + + + Examples: + - Providing "foo" will find all symbols named "foo" regardless where they are contained in the symbol tree. + - Providing "bar/foo" will only find symbols named "foo" that are direct children of a symbol called "bar". + - Providing "foo/" will only find symbols named "foo" that are top-level symbols (have no parent). + - Allowing substring matching with "bar" will find symbols with names containing "foo" anywhere in the symbol tree. + - Allowing substring matching with "foo/" will find only top-level symbols with names containing "foo". + - Allowing substring matching with "bar/foo" will find only symbols with names containing "foo" that are direct children of a symbol named "bar". + + :param name: the name of the symbol to find. Can use a qualified name (e.g. "class/method/inner_function") + to restrict the search. + :param substring_matching: whether to use substring matching for the symbol name. + If a qualified name is provided, the last element of the qualified name will be checked against + the symbol name using substring matching. + :param include_kinds: an optional sequence of ints representing the LSP symbol kind. + If provided, only symbols of the given kinds will be included in the result. + :param exclude_kinds: If provided, symbols of the given kinds will be excluded from the result. + + """ result = [] def should_include(s: "Symbol") -> bool: - if not ((substring_matching and name in s.name) or name == s.name): - return False if include_kinds is not None and s.symbol_kind not in include_kinds: return False if exclude_kinds is not None and s.symbol_kind in exclude_kinds: return False - return True + return Symbol.match_against_qualname( + name_pattern=name, + qual_name_parts=s.get_qualified_name_parts(), + substring_matching=substring_matching, + ) def traverse(s: "Symbol") -> None: if should_include(s): @@ -322,10 +473,13 @@ class SymbolManager: body += "\n" with self._edited_symbol_location(location) as symbol: assert location.relative_path is not None - self.lang_server.delete_text_between_positions(location.relative_path, symbol.body_start_position, symbol.body_end_position) - self.lang_server.insert_text_at_position( - location.relative_path, symbol.body_start_position["line"], symbol.body_start_position["character"], body - ) + start_pos = symbol.body_start_position + end_pos = symbol.body_end_position + if start_pos is None or end_pos is None: + raise ValueError(f"Symbol at {location} does not have a defined body range.") + # At this point, start_pos and end_pos are guaranteed to be Position objects + self.lang_server.delete_text_between_positions(location.relative_path, start_pos, end_pos) + self.lang_server.insert_text_at_position(location.relative_path, start_pos["line"], start_pos["character"], body) def insert_after(self, location: SymbolLocation, body: str) -> None: """ @@ -339,6 +493,9 @@ class SymbolManager: body += "\n" with self._edited_symbol_location(location) as symbol: pos = symbol.body_end_position + if pos is None: + raise ValueError(f"Symbol at {location} does not have a defined end position.") + # At this point, pos is guaranteed to be a Position object assert location.relative_path is not None self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) @@ -353,7 +510,11 @@ class SymbolManager: if not body.endswith("\n"): body += "\n" with self._edited_symbol_location(location) as symbol: - pos = copy(symbol.body_start_position) + original_start_pos = symbol.body_start_position + if original_start_pos is None: + raise ValueError(f"Symbol at {location} does not have a defined start position.") + # At this point, original_start_pos is guaranteed to be a Position object, so copying is safe. + pos = copy(original_start_pos) assert location.relative_path is not None self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) diff --git a/test/__init__.py b/test/__init__.py index e69de29b..8b137891 100644 --- a/test/__init__.py +++ b/test/__init__.py @@ -0,0 +1 @@ + diff --git a/test/serena/__init__.py b/test/serena/__init__.py new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/test/serena/__init__.py @@ -0,0 +1 @@ + diff --git a/test/serena/test_serena_agent.py b/test/serena/test_serena_agent.py index 143bdfe1..9fb3783e 100644 --- a/test/serena/test_serena_agent.py +++ b/test/serena/test_serena_agent.py @@ -87,3 +87,77 @@ class TestSerenaAgent: assert any( ref["location"]["relative_path"] == ref_file for ref in refs ), f"Expected to find reference to {symbol_name} in {ref_file} for {agent.project_config.language.name}. refs={refs}" + + @pytest.mark.parametrize( + "serena_agent,qualified_name,substring_matching,expected_symbol_name,expected_kind,expected_file", + [ + pytest.param( + Language.PYTHON, + "OuterClass/NestedClass", + False, + "NestedClass", + "Class", + os.path.join("test_repo", "nested.py"), + id="exact_qualname_class", + marks=pytest.mark.python, + ), + pytest.param( + Language.PYTHON, + "OuterClass/NestedClass/find_me", + False, + "find_me", + "Method", + os.path.join("test_repo", "nested.py"), + id="exact_qualname_method", + marks=pytest.mark.python, + ), + pytest.param( + Language.PYTHON, + "OuterClass/NestedCl", # Substring for NestedClass + True, + "NestedClass", + "Class", + os.path.join("test_repo", "nested.py"), + id="substring_qualname_class", + marks=pytest.mark.python, + ), + pytest.param( + Language.PYTHON, + "OuterClass/NestedClass/find_m", # Substring for find_me + True, + "find_me", + "Method", + os.path.join("test_repo", "nested.py"), + id="substring_qualname_method", + marks=pytest.mark.python, + ), + ], + indirect=["serena_agent"], + ) + def test_find_symbol_qualified_name( + self, + serena_agent: SerenaAgent, + qualified_name: str, + substring_matching: bool, + expected_symbol_name: str, + expected_kind: str, + expected_file: str, + ): + agent = serena_agent + find_symbol_tool = agent.get_tool(FindSymbolTool) + result = find_symbol_tool.apply( + name=qualified_name, + depth=0, + within_relative_path=None, + include_body=False, + include_kinds=None, + exclude_kinds=None, + substring_matching=substring_matching, + ) + symbols = json.loads(result) + assert any( + expected_symbol_name == s["name"] + and expected_kind.lower() in s["kind"].lower() + and expected_file in s["location"]["relative_path"] + for s in symbols + ), f"Expected to find {qualified_name} ({expected_kind}) in {expected_file} for {agent.project_config.language.name}. Symbols: {symbols}" diff --git a/test/serena/test_symbol.py b/test/serena/test_symbol.py new file mode 100644 index 00000000..fdd48f7e --- /dev/null +++ b/test/serena/test_symbol.py @@ -0,0 +1,133 @@ +import pytest + +from src.serena.symbol import Symbol + + +class TestSymbolNameMatching: + def _create_assertion_error_message( + self, + name_pattern: str, + qual_name_parts: list[str], + is_substring_match: bool, + expected_result: bool, + actual_result: bool, + ) -> str: + """Helper to create a detailed error message for assertions.""" + qnp_repr = "/".join(qual_name_parts) + + return ( + f"Pattern '{name_pattern}' (substring: {is_substring_match}) vs " + f"Qualname parts {qual_name_parts} (as '{qnp_repr}'). " + f"Expected: {expected_result}, Got: {actual_result}" + ) + + @pytest.mark.parametrize( + "name_pattern, qual_name_parts, is_substring_match, expected", + [ + # Exact matches (is_substring_match=False) + pytest.param("foo", ["foo"], False, True, id="'foo' matches 'foo' exactly"), + pytest.param("foo", ["bar", "foo"], False, True, id="'foo' matches 'bar,foo' exactly"), + pytest.param("foo", ["foobar"], False, False, id="'foo' does not match 'foobar' exactly"), + pytest.param("foo", ["bar", "foobar"], False, False, id="'foo' does not match 'bar,foobar' exactly"), + pytest.param("foo", ["path", "to", "foo"], False, True, id="'foo' matches 'path,to,foo' exactly"), + # Substring matches (is_substring_match=True) + pytest.param("foo", ["foobar"], True, True, id="'foo' matches 'foobar' as substring"), + pytest.param("foo", ["bar", "foobar"], True, True, id="'foo' matches 'bar,foobar' as substring"), + pytest.param("foo", ["barfoo"], True, True, id="'foo' matches 'barfoo' as substring"), + pytest.param("foo", ["baz"], True, False, id="'foo' does not match 'baz' as substring"), + pytest.param("foo", ["bar", "baz"], True, False, id="'foo' does not match 'bar,baz' as substring"), + pytest.param("foo", ["my_foobar_func"], True, True, id="'foo' matches 'my_foobar_func' as substring"), + pytest.param("foo", ["ClassA", "my_foobar_method"], True, True, id="'foo' matches 'ClassA,my_foobar_method' as substring"), + pytest.param("foo", ["my_bar_func"], True, False, id="'foo' does not match 'my_bar_func' as substring"), + ], + ) + def test_match_simple_name(self, name_pattern, qual_name_parts, is_substring_match, expected): + """Tests matching for simple names (no '/' in pattern).""" + result = Symbol.match_against_qualname(name_pattern, qual_name_parts, is_substring_match) + error_msg = self._create_assertion_error_message(name_pattern, qual_name_parts, is_substring_match, expected, result) + assert result == expected, error_msg + + @pytest.mark.parametrize( + "name_pattern, qual_name_parts, is_substring_match, expected", + [ + # Exact matches (is_substring_match=False) + pytest.param("bar/foo", ["bar", "foo"], False, True, id="'bar/foo' matches 'bar,foo' exactly"), + pytest.param( + "bar/foo", ["bar", "foo", "baz"], False, False, id="'bar/foo' does not match 'bar,foo,baz' exactly (len mismatch)" + ), + pytest.param("bar/foo", ["bar"], False, False, id="'bar/foo' does not match 'bar' exactly (len mismatch)"), + pytest.param("bar/foo", ["baz", "foo"], False, False, id="'bar/foo' does not match 'baz,foo' exactly (first mismatch)"), + pytest.param("bar/foo", ["bar", "baz"], False, False, id="'bar/foo' does not match 'bar,baz' exactly (last mismatch)"), + # from docstring examples + pytest.param("bar/foo", ["foo"], False, False, id="'bar/foo' does not match 'foo' exactly"), + pytest.param("bar/foo", ["other", "foo"], False, False, id="'bar/foo' does not match 'other,foo' exactly"), + pytest.param("bar/foo", ["bar", "otherfoo"], False, False, id="'bar/foo' does not match 'bar,otherfoo' exactly"), + # Substring matches (is_substring_match=True) + pytest.param("bar/foo", ["bar", "foobar"], True, True, id="'bar/foo' matches 'bar,foobar' as substring"), + pytest.param("bar/foo", ["bar", "bazfoo"], True, True, id="'bar/foo' matches 'bar,bazfoo' as substring"), + pytest.param("bar/fo", ["bar", "foo"], True, True, id="'bar/fo' matches 'bar,foo' as substring"), + pytest.param("bar/foo", ["bar", "baz"], True, False, id="'bar/foo' does not match 'bar,baz' as substring (last no substr)"), + pytest.param( + "bar/foo", ["baz", "foobar"], True, False, id="'bar/foo' does not match 'baz,foobar' as substring (first mismatch)" + ), + # from docstring examples + pytest.param("bar/foo", ["bar", "my_foobar_method"], True, True, id="'bar/foo' matches 'bar,my_foobar_method' as substring"), + pytest.param( + "bar/foo", ["bar", "another_method"], True, False, id="'bar/foo' does not match 'bar,another_method' as substring" + ), + pytest.param( + "bar/foo", ["other", "my_foobar_method"], True, False, id="'bar/foo' does not match 'other,my_foobar_method' as substring" + ), + pytest.param("bar/f", ["bar", "foo"], True, True, id="'bar/f' matches 'bar,foo' as substring"), + ], + ) + def test_match_name_pattern_ends_without_slash(self, name_pattern, qual_name_parts, is_substring_match, expected): + """Tests matching for qualified names (e.g. 'module/class/func').""" + result = Symbol.match_against_qualname(name_pattern, qual_name_parts, is_substring_match) + error_msg = self._create_assertion_error_message(name_pattern, qual_name_parts, is_substring_match, expected, result) + assert result == expected, error_msg + + @pytest.mark.parametrize( + "name_pattern, qual_name_parts, is_substring_match, expected", + [ + # Exact matches (is_substring_match=False) + pytest.param("foo/", ["foo"], False, True, id="'foo/' matches 'foo' exactly"), + pytest.param("bar/foo/", ["bar", "foo"], False, True, id="'bar/foo/' matches 'bar,foo' exactly"), + pytest.param("foo/", ["foobar"], False, False, id="'foo/' does not match 'foobar' exactly"), + pytest.param("foo/", ["bar", "foo"], False, False, id="'foo/' does not match 'bar,foo' exactly (not toplevel)"), + # Substring matches (is_substring_match=True) + pytest.param("foo/", ["foobar"], True, True, id="'foo/' matches 'foobar' as substring"), + pytest.param( + "bar/foo/", ["bar", "the_foobar_method"], True, True, id="'bar/foo/' matches 'bar,the_foobar_method' as substring" + ), + pytest.param( + "bar/foo/", + ["bar", "another_method"], + True, + False, + id="'bar/foo/' does not match 'bar,another_method' as substring (last no substr)", + ), + pytest.param( + "bar/foo/", + ["baz", "the_foobar_method"], + True, + False, + id="'bar/foo/' does not match 'baz,the_foobar_method' as substring (first mismatch)", + ), + # from docstring examples + pytest.param("foo/", ["my_foobar_func"], True, True, id="'foo/' matches 'my_foobar_func' as substring"), + pytest.param( + "foo/", + ["ClassA", "my_foobar_method"], + True, + False, + id="'foo/' does not match 'ClassA,my_foobar_method' as substring (not toplevel)", + ), + pytest.param("foo/", ["foo"], True, True, id="'foo/' matches 'foo' as substring (exact is substr)"), + ], + ) + def test_match_name_pattern_ending_in_slash(self, name_pattern, qual_name_parts, is_substring_match, expected): + """Tests matching for patterns ending with '/' (prefix/namespace style).""" + result = Symbol.match_against_qualname(name_pattern, qual_name_parts, is_substring_match) + error_msg = self._create_assertion_error_message(name_pattern, qual_name_parts, is_substring_match, expected, result) + assert result == expected, error_msg