Repository navigation
Per-edit diagnostics as a project setting, and the answer names the language server they come from - #2025
Per-edit diagnostics as a project setting, and the answer names the language server they come from#2025MarioRial22 wants to merge 11 commits into
Conversation
…anguage server they come from `EditingToolWithDiagnostics.ENABLE_DIAGNOSTICS` is a hard-coded False with no way for a project to opt in, while the machinery behind it is complete and tested. A new `edit_diagnostics` key in project.yml (default off, so nothing changes for anyone) turns it on for the projects whose workflow wants the verdict after every edit; the global flag keeps its meaning. The answer also says, per edited file, which language server produced the diagnostics (`diagnostics_from`): a project that grades with a second type checker at its gate should not read one server's "no errors" as the gate's.
AmirF194
left a comment
AmirF194
left a comment
There was a problem hiding this comment.
I ran ProjectConfig.load() against this PR's head (87f874b9) in a clean container to check whether edit_diagnostics: true in project.yml actually reaches the loaded config.
ProjectConfig._from_dict (src/serena/config/serena_config.py:638-661) builds the returned instance with an explicit keyword for every other field (read_only=data["read_only"], etc.) but never reads data["edit_diagnostics"]. Since the dataclass field defaults to False, any value set in project.yml is silently dropped and the loaded ProjectConfig always comes back with edit_diagnostics=False, regardless of what the file says.
$ python3 /repro.py
edit_diagnostics on loaded ProjectConfig: False
Traceback (most recent call last):
...
AssertionError: BUG: project.yml's edit_diagnostics: true was not honored by ProjectConfig.load
(repro: write a project.yml with edit_diagnostics: true under a temp project root, call ProjectConfig.load(project_root, serena_config), assert .edit_diagnostics is True.)
The new test in this PR doesn't catch it because it sets project_config.edit_diagnostics = True directly on the Python object rather than loading it from project.yml, so it never exercises _from_dict.
Fix: add edit_diagnostics=data["edit_diagnostics"], to the cls(...) call in _from_dict, next to read_only=data["read_only"],.
…field explicitly and left this one at its default
|
Confirmed: c76bdf7 adds edit_diagnostics to the _from_dict call and covers it with a test that loads project.yml directly. Thanks for the fix. |
|
|
…stics-setting # Conflicts: # CHANGELOG.md
|
Right — |
…xtracted DiagnosticsContext Upstream did half of this branch's work in the meantime: DiagnosticsContext left EditingToolWithDiagnostics for serena/lsp/lsp_diagnostics.py and gained an `enable` parameter, with the tool base passing `enable=ENABLE_DIAGNOSTICS` through a `diagnostics_context()` factory. So the contribution is smaller than it was and sits in the new seams: - `diagnostics_context()` passes `ENABLE_DIAGNOSTICS or _is_edit_diagnostics_configured()`, so a project's `edit_diagnostics: true` turns the feature on without touching the global flag, whose meaning and default are unchanged. - `DiagnosticsContext.DIAGNOSTICS_SOURCE_KEY` names, per edited file, the language server the reported diagnostics come from, and `DiagnosticsDiff` records the id of whichever server answered. The loader fix (`_from_dict` reading `edit_diagnostics`) and the project template's documentation of the key merged unchanged. The test follows the constants to their new home.
The diagnostics source map read `language_server.ls_id.value`, which is an attribute of the LanguageServerId enum and not part of the LanguageServerIdLike protocol: an externally registered language server (ExternalLanguageServerId) has no `value`, so the label crashed for exactly the adapters the protocol exists to support. `get_key()` is the protocol's accessor and returns `self.value` for enum members, so the label is unchanged for the bundled servers. `ty check src/serena src/solidlsp` reported this as unresolved-attribute; it now passes.
opcode81
left a comment
opcode81
left a comment
There was a problem hiding this comment.
Please see my comments.
@MischaPanch, do you think it is a good idea to expose the feature in this way (opt in)?
| ls_workspace_folders: list[str] = field(default_factory=lambda: ["."]) | ||
| ls_additional_workspace_folders: list[str] = field(default_factory=list) | ||
| read_only: bool = False | ||
| edit_diagnostics: bool = False |
There was a problem hiding this comment.
This would need to be called ls_edit_diagnostics to make clear that applies to language server applications (the LSP backend) only.
| # whether editing tools report the diagnostics (warnings and errors) an edit newly introduced in the edited | ||
| # files, as the language server sees them. The answer names the language server the diagnostics come from | ||
| # (`diagnostics_from`), since one server's verdict is not every checker's. | ||
| # Off by default: intermediate edits often introduce diagnostics on purpose (a signature changed here, its | ||
| # callers in the next edit), and reporting them at every step is noise for such workflows. | ||
| edit_diagnostics: false |
There was a problem hiding this comment.
The docstring would have to make clear that this feature currently affects the traditional tools agent interface only; the REPL interface does not provide diagnostics.
…ate its scope * Name the setting `ls_edit_diagnostics`, like the other settings that apply to the LSP backend only (`ls_workspace_folders`, `ls_additional_workspace_folders`) * State in the template that it currently affects the `tools` agent interface only; the REPL interface does not report per-edit diagnostics * Document the setting in the template alone, as every other project setting is
* `DiagnosticsContext` gated on the language backend object, which is always truthy, instead of on `is_lsp()`, the check `is_using_language_server()` made before the backends were reified * With `ls_edit_diagnostics` set under the JetBrains backend, every editing tool failed before its edit: the context asked for a language server manager the project does not have
opcode81
added
the
internal discussion
This branch has not been deployed
4 participants
EditingToolWithDiagnostics.ENABLE_DIAGNOSTICSis a hard-codedFalsewith no way for a project to opt in, while the machinery behind it is complete and tested. This addsedit_diagnosticstoproject.yml(defaultfalse, so nothing changes for anyone): a project whose workflow wants the verdict after every edit turns it on; the global flag keeps its meaning (on regardless of the project). The answer also names, per edited file, the language server the diagnostics come from (diagnostics_from): a project that grades with a second type checker at its gate should not read one server's "no errors" as the gate's.Test (
test/serena/test_serena_agent.py): with the flag off and the setting off, an edit reports nothing; with the setting on, the same edit reports the new diagnostic anddiagnostics_fromnames the server. The project template documents the key.Checklist
CONTRIBUTING.mdregarding the scope of PRs.CHANGELOG.md, which concisely describes the change.