Features: diagnostics and LS extensions#1433
Conversation
67dfa7b to
e901434
Compare
ed57f0d to
37d8cc9
Compare
37d8cc9 to
6672a52
Compare
4e942e2 to
77ad04c
Compare
Removed some cases (haxe, msl) during cherry pick due to conflicts. We don't need to cover these languages in this test
…vior Also minor fix in go test expectations of symbol name
4344057 to
7d70d11
Compare
| return self._limit_length(result, max_answer_chars) | ||
|
|
||
|
|
||
| class FindDefiningSymbolTool(Tool, ToolMarkerSymbolicRead): |
There was a problem hiding this comment.
The implementation of this tool is rather complicated. I think it should be based on the same helper as JetBrainsFindDeclarationTool, which already does all of the heavy lifting.
And we should probably rename it to FindDeclarationTool or FindSymbolDeclarationTool.
wdyt?
There was a problem hiding this comment.
Yes, the implementation predates the work you did for the Jetbrains variant on main. I agre to rename to FindDeclaration and to use the same helper as in the JB tool
| def _diagnostic_severity_name(severity: int | None) -> str: | ||
| if severity is None: | ||
| return "Unknown" | ||
| try: | ||
| return DiagnosticSeverity(severity).name | ||
| except ValueError: | ||
| return f"Severity_{severity}" | ||
|
|
||
|
|
||
| def _diagnostic_output_dict(diagnostic: ls_types.Diagnostic) -> dict[str, Any]: | ||
| result: dict[str, Any] = { | ||
| "message": diagnostic["message"], | ||
| "range": diagnostic["range"], | ||
| } | ||
| if "code" in diagnostic: | ||
| result["code"] = diagnostic["code"] | ||
| if "source" in diagnostic: | ||
| result["source"] = diagnostic["source"] | ||
| return result | ||
|
|
||
|
|
||
| def _add_grouped_diagnostic( | ||
| grouped_result: dict[str, dict[str, dict[str, list[dict[str, Any]]]]], | ||
| relative_path: str, | ||
| severity_name: str, | ||
| name_path: str, | ||
| diagnostic: ls_types.Diagnostic, | ||
| ) -> None: | ||
| grouped_result.setdefault(relative_path, {}).setdefault(severity_name, {}).setdefault(name_path, []).append( | ||
| _diagnostic_output_dict(diagnostic) | ||
| ) | ||
|
|
||
|
|
||
| def _offset_to_line_and_column(text: str, offset: int) -> tuple[int, int]: | ||
| if offset < 0 or offset > len(text): | ||
| raise ValueError(f"Offset out of range: {offset}") | ||
| prefix = text[:offset] | ||
| line = prefix.count("\n") | ||
| previous_newline_offset = prefix.rfind("\n") | ||
| column = offset if previous_newline_offset == -1 else offset - previous_newline_offset - 1 | ||
| return line, column | ||
|
|
||
|
|
||
| def _line_and_column_to_offset(text: str, line: int, column: int) -> int: | ||
| if line < 0 or column < 0: | ||
| raise ValueError(f"Line and column must be non-negative, got {line=}, {column=}") | ||
|
|
||
| line_start_offsets = [0] | ||
| for match in re.finditer("\n", text): | ||
| line_start_offsets.append(match.end()) | ||
|
|
||
| if line >= len(line_start_offsets): | ||
| raise ValueError(f"Line out of range: {line}") | ||
|
|
||
| line_start_offset = line_start_offsets[line] | ||
| line_end_offset = line_start_offsets[line + 1] - 1 if line + 1 < len(line_start_offsets) else len(text) | ||
| if column > line_end_offset - line_start_offset: | ||
| raise ValueError(f"Column out of range for line {line}: {column}") | ||
| return line_start_offset + column |
There was a problem hiding this comment.
We should probably move these helpers to the new module ls_diagnostics and encapsulate them in an abstraction. Some of the functionality is probably already duplicated - and this is partly true for the implementation of the diagnostics tools as well.
We need to consolidate this.
There was a problem hiding this comment.
Yes, moving there sounds good. You can probably task an agent with deduplication and refactoring, the new tests and the demo script should allow it to perform this without breaking functionality
There was a problem hiding this comment.
Duplication of diagnostics-based functionality is now fixed in cdf2481 by introducing new abstraction GroupedDiagnostics. All of the functions were duplicates.
7d70d11 to
cdf2481
Compare
| for edited_file_path in edited_files: | ||
| try: | ||
| language_server = symbol_retriever.get_language_server(edited_file_path.after_relative_path) | ||
| except: |
| edited_file_path.after_relative_path, | ||
| min_severity=2, | ||
| ) | ||
| except: |
ed6cd16 to
65cc90b
Compare
e36ebef to
f6396f8
Compare
| agent.execute_task(lambda: None) | ||
|
|
||
| relative_path = SERVICES_FILE.as_posix() | ||
| services_abs_path = PYTHON_TEST_REPO / SERVICES_FILE |
37fab1f to
c035ac0
Compare
c035ac0 to
843277a
Compare
* Add context manager to simplify usage, improving output handling * Add util module ls_diagnostics, reifying DiagnosticsDiff
Management was unnecessarily introduced by AI in 133e1be
* Apply common functionality already used in JetBrainsFindDeclarationTool * Remove location-based tool (counteracts Serena's principle to eschew line numbers) * Apply renaming to use "find declaration" instead of "find defining symbol"
…implementations tools
843277a to
4f0c3f8
Compare
…and_ls_extensions Conflicts: CHANGELOG.md
Adds support for diagnostics (JB and LSP); find_implementation and find_definition (LSP)