fix(langserver): prevent inconsistent Robocop formatting

Formatting a file with Robocop repeatedly no longer oscillates between states (for example a blank line being added and removed before a section header on every other format). The formatter mutated the shared, cached document model in place; it now works on a fresh, uncached model so repeated formatting stays idempotent.

Fixes #612

Author: d-biehl

packages/language_server/src/robotcode/language_server/robotframework/parts/formatting.py

@@ -89,7 +89,7 @@ def format_robocop(
         runner = RobocopFormatter(config_manager)
         runner.config = config
 
-        model = self.parent.documents_cache.get_model(document)
+        model = self.parent.documents_cache.get_uncached_model(document)
         if self.parent.robocop_helper.robocop_version >= (8, 0):
             from robocop.source_file import SourceFile
 

packages/robot/src/robotcode/robot/diagnostics/document_cache_helper.py

@@ -303,6 +303,23 @@ def get_model(self, document: TextDocument) -> ast.AST:
 
         return self.get_general_model(document)
 
+    def get_uncached_model(self, document: TextDocument) -> ast.AST:
+        """Build a fresh model that is never cached on the document.
+
+        The cached model returned by `get_model` is shared between all features.
+        Consumers that mutate the model in place (e.g. the Robocop formatter) must
+        use this instead, otherwise the mutations corrupt the cached model and break
+        subsequent operations.
+        """
+        document_type = self.get_document_type(document)
+
+        if document_type == DocumentType.INIT:
+            return self.__get_init_model(document)
+        if document_type == DocumentType.RESOURCE:
+            return self.__get_resource_model(document)
+
+        return self.__get_general_model(document)
+
     def __get_model(
         self,
         document: TextDocument,

tests/robotcode/language_server/robotframework/parts/test_formatting.py

@@ -0,0 +1,138 @@
+"""Regression tests for Robocop based formatting.
+
+Reproduces https://github.com/robotcodedev/robotcode/issues/612: formatting an
+already formatted document repeatedly must be a no-op. The Robocop formatter
+mutates the model in place, so `format_robocop` has to work on an *uncached*
+model - otherwise the mutation corrupts the shared cached model and repeated
+formatting starts to oscillate (a blank line is added, removed, added, ...).
+
+The test drives the real `RobotFormattingProtocolPart.format_robocop` and only
+fakes the surrounding protocol wiring, so a regression in the model handling of
+the formatter is caught here.
+"""
+
+from pathlib import Path
+from types import SimpleNamespace
+from typing import Any, List, Optional
+from unittest.mock import MagicMock
+
+import pytest
+
+from robotcode.core.lsp.types import FormattingOptions, TextEdit
+from robotcode.core.text_document import TextDocument
+from robotcode.core.uri import Uri
+from robotcode.core.utils.version import create_version_from_str
+from robotcode.core.workspace import WorkspaceFolder
+from robotcode.language_server.robotframework.parts.formatting import RobotFormattingProtocolPart
+from robotcode.robot.diagnostics.document_cache_helper import DocumentsCacheHelper
+from robotcode.robot.diagnostics.workspace_config import RobotConfig
+
+# Importing robotcode above does not pull in robocop (the formatter imports it
+# lazily), so the skip guard can live after the regular imports.
+robocop = pytest.importorskip("robocop")
+
+ROBOCOP_VERSION = create_version_from_str(robocop.__version__)
+
+pytestmark = pytest.mark.skipif(
+    ROBOCOP_VERSION < (6, 0),
+    reason="Robocop >= 6.0 is required for formatting",
+)
+
+UNFORMATTED = (
+    '*** Test Cases ***\nTest\n    Embedded "args"\n\n\n*** Keywords ***\nEmbedded "${args}"\n    No Operation\n'
+)
+
+# Both `section_lines` and `test_case_lines` greater than zero is what makes a
+# single NormalizeNewLines pass non-idempotent on this input (see #612/#361).
+CONFIGURE_VARIANTS = [
+    pytest.param(["NormalizeNewLines.enabled=True"], id="defaults"),
+    pytest.param(
+        [
+            "NormalizeNewLines.enabled=True",
+            "NormalizeNewLines.section_lines=2",
+            "NormalizeNewLines.test_case_lines=1",
+        ],
+        id="explicit-section-and-test-case-lines",
+    ),
+]
+
+
+def _config_manager(root: Path) -> Any:
+    if ROBOCOP_VERSION >= (8, 0):
+        from robocop.config.manager import ConfigManager
+    else:
+        from robocop.config import ConfigManager
+
+    return ConfigManager([], root=root, config=root / "robot.toml")
+
+
+def _make_formatting_part(root: Path, document: TextDocument) -> RobotFormattingProtocolPart:
+    folder = WorkspaceFolder(name="test", uri=Uri.from_path(root))
+
+    workspace = MagicMock()
+    workspace.get_workspace_folder.return_value = folder
+    workspace.get_configuration.return_value = RobotConfig()
+
+    documents_cache = DocumentsCacheHelper(
+        workspace=workspace,
+        documents_manager=MagicMock(),
+        file_watcher_manager=MagicMock(),
+        robot_profile=None,
+        analysis_config=None,
+    )
+
+    config_manager = _config_manager(root)
+    robocop_helper = SimpleNamespace(
+        robocop_installed=True,
+        robocop_version=ROBOCOP_VERSION,
+        get_config_manager=lambda _folder: config_manager,
+    )
+
+    parent = SimpleNamespace(
+        workspace=workspace,
+        robocop_helper=robocop_helper,
+        documents_cache=documents_cache,
+    )
+
+    part = object.__new__(RobotFormattingProtocolPart)
+    part._parent = parent  # type: ignore[assignment]
+    return part
+
+
+@pytest.fixture
+def options() -> FormattingOptions:
+    return FormattingOptions(tab_size=4, insert_spaces=True)
+
+
+@pytest.mark.parametrize("configure", CONFIGURE_VARIANTS)
+def test_repeated_formatting_is_idempotent(tmp_path: Path, options: FormattingOptions, configure: List[str]) -> None:
+    """After reaching a fixed point, formatting again must not change anything."""
+    configure_lines = "\n".join(f'    "{c}",' for c in configure)
+    (tmp_path / "robot.toml").write_text(f"[tool.robocop.format]\nconfigure = [\n{configure_lines}\n]\n")
+
+    source = tmp_path / "test.robot"
+    source.write_text(UNFORMATTED)
+
+    document = TextDocument(
+        document_uri=str(Uri.from_path(source).normalized()),
+        language_id="robotframework",
+        version=1,
+        text=UNFORMATTED,
+    )
+
+    part = _make_formatting_part(tmp_path, document)
+
+    def format_once() -> Optional[List[TextEdit]]:
+        return part.format_robocop(document, options)
+
+    # Reach the formatter's fixed point first (apply the initial reformat, if any).
+    edits = format_once()
+    if edits:
+        document.apply_full_change((document.version or 0) + 1, edits[0].new_text)
+
+    # From the fixed point, formatting must stay a no-op - no oscillation (#612).
+    for _ in range(5):
+        edits = format_once()
+        if edits:
+            document.apply_full_change((document.version or 0) + 1, edits[0].new_text)
+        assert edits is None, f"formatting is not idempotent, produced: {edits!r}"

tests/robotcode/robot/diagnostics/test_document_cache_helper.py

@@ -0,0 +1,76 @@
+"""Tests for `DocumentsCacheHelper` model caching.
+
+`get_model` returns a model that is cached on (and shared across) the document,
+while `get_uncached_model` must always return a fresh, independent model. The
+Robocop formatter mutates the model in place, so it relies on the uncached
+variant - otherwise the mutations corrupt the shared cached model and repeated
+formatting becomes non-idempotent (see
+https://github.com/robotcodedev/robotcode/issues/612).
+"""
+
+from typing import cast
+from unittest.mock import MagicMock
+
+import pytest
+from robot.parsing.model.blocks import File
+
+from robotcode.core.text_document import TextDocument
+from robotcode.core.uri import Uri
+from robotcode.robot.diagnostics.document_cache_helper import DocumentsCacheHelper
+
+TEXT = '*** Test Cases ***\nTest\n    Embedded "args"\n\n\n*** Keywords ***\nEmbedded "${args}"\n    No Operation\n'
+
+
+@pytest.fixture
+def cache_helper() -> DocumentsCacheHelper:
+    workspace = MagicMock()
+    workspace.get_workspace_folder.return_value = None
+    return DocumentsCacheHelper(
+        workspace=workspace,
+        documents_manager=MagicMock(),
+        file_watcher_manager=MagicMock(),
+        robot_profile=None,
+        analysis_config=None,
+    )
+
+
+def _document(name: str = "test.robot") -> TextDocument:
+    return TextDocument(
+        document_uri=str(Uri.from_path(f"/tmp/{name}").normalized()),
+        language_id="robotframework",
+        version=1,
+        text=TEXT,
+    )
+
+
+def test_get_model_is_cached_on_the_document(cache_helper: DocumentsCacheHelper) -> None:
+    document = _document()
+
+    assert cache_helper.get_model(document) is cache_helper.get_model(document)
+
+
+@pytest.mark.parametrize("name", ["test.robot", "test.resource", "__init__.robot"])
+def test_get_uncached_model_returns_a_fresh_model_every_time(cache_helper: DocumentsCacheHelper, name: str) -> None:
+    document = _document(name)
+
+    first = cache_helper.get_uncached_model(document)
+    second = cache_helper.get_uncached_model(document)
+
+    assert isinstance(first, File)
+    assert first is not second
+    assert first is not cache_helper.get_model(document)
+
+
+def test_uncached_model_mutation_does_not_corrupt_the_cached_model(
+    cache_helper: DocumentsCacheHelper,
+) -> None:
+    document = _document()
+
+    cached = cast(File, cache_helper.get_model(document))
+    sections_before = len(cached.sections)
+
+    uncached = cast(File, cache_helper.get_uncached_model(document))
+    uncached.sections.clear()
+
+    assert len(cached.sections) == sections_before
+    assert cache_helper.get_model(document) is cached