Repository: MarketSquare/robotframework-robocop

Issue:
Symbolic links cannot be excluded and will cause a crash
Robocop 6.0.0 doesn't seem to be able to handle a symbolic link when linting a project. It causes the following exception:

`ValueError: '/<symbolic link resolved>' is not in the subpath of 
'<project root>'
`

Last stack trace from Robocop code:

```
 /home/user/.pyenv/versions/3.12.7/envs/robot-venv-3.12/lib/python3.12/site-packages/robocop/linter/reports/print_issues.py:214 in                     │
│ print_diagnostics_extended                                                                                                                            │
│                                                                                                                                                       │
│   211 │   def print_diagnostics_extended(self, diagnostics: Diagnostics) -> None:                                                                     │
│   212 │   │   cwd = Path.cwd()                                                                                                                        │
│   213 │   │   for source, diag_by_source in diagnostics.diag_by_source.items():                                                                       │
│ ❱ 214 │   │   │   source_rel = Path(source).relative_to(cwd)                                                                                          │
│   215 │   │   │   source_lines = None                                                                                                                 │
│   216 │   │   │   for diagnostic in diag_by_source:                                                                                                   │
│   217 │   │   │   │   if not source_lines:  # TODO: model should be coming from source, not di 
```

Trying to exclude the symlink directory seems to have no effect. I tried this:

```
[tool.robocop]

exclude = ["subdir/symlinkdir/"]
```

But the same error occurs anyway.

Discussion:
@bhirsz: I see two issues there:

1) Not being able to exclude symlinks
2) Failing to use symbolic link when outputing linter results

Thanks for the report, I will investigate it.@bhirsz: Third issue: If the symlink is resolved, we are then searching for configuration file for this file:

```
hard/path/test.robot
...
my_project/
   pyproject.toml
   symlink_to_hard/path
```

``symlink_to_hard/path`` will find and use configuration from ``hard/path/test.robot`` instead ``my_project/pyproject.toml``.
@bhirsz: I have modified it so it doesn't resolve paths until we output the diagnostic. If we fail to create relative path, we use at is. It seems to work but I will add some extra tests@bhirsz: Fixed, will be releases as 6.0.1

Solution patch:
diff --git a/src/robocop/config.py b/src/robocop/config.py
index 53a8838..4299d8d 100644
--- a/src/robocop/config.py
+++ b/src/robocop/config.py
@@ -749,7 +749,7 @@ class ConfigManager:
         """Get default config either from --config option or from the cli."""
         if config_path:
             configuration = files.read_toml_config(config_path)
-            config = Config.from_toml(configuration, config_path)
+            config = Config.from_toml(configuration, config_path.resolve())
         else:
             config = Config()
         config.overwrite_from_config(self.overwrite_config)
@@ -776,7 +776,7 @@ class ConfigManager:
                 if (config_path := (check_dir / config_filename)).is_file():
                     configuration = files.read_toml_config(config_path)
                     if configuration is not None:
-                        config = Config.from_toml(configuration, config_path)
+                        config = Config.from_toml(configuration, config_path.resolve())
                         config.overwrite_from_config(self.overwrite_config)  # TODO those two lines together
                         self.cached_configs.update({sub_dir: config for sub_dir in seen})
                         if config.verbose:
@@ -820,7 +820,7 @@ class ConfigManager:
 
         """
         for source in sources:
-            source = Path(source).resolve()
+            source = Path(source)
             if source in self._paths:
                 continue
             if not source.exists():  # TODO only for passed sources
diff --git a/src/robocop/linter/reports/print_issues.py b/src/robocop/linter/reports/print_issues.py
index 74f7736..bac96c4 100644
--- a/src/robocop/linter/reports/print_issues.py
+++ b/src/robocop/linter/reports/print_issues.py
@@ -28,6 +28,13 @@ class OutputFormat(Enum):
         raise ValueError(f"{value} is not a valid {cls.__name__}, please choose from {choices}") from None
 
 
+def get_relative_path(path: str, cwd: Path) -> Path | str:
+    try:
+        return Path(path).relative_to(cwd)
+    except ValueError:  # symlink etc
+        return path
+
+
 class PrintIssuesReport(robocop.linter.reports.Report):
     """
     **Report name**: ``print_issues``
@@ -92,12 +99,12 @@ class PrintIssuesReport(robocop.linter.reports.Report):
     def print_diagnostics_simple(self, diagnostics: Diagnostics) -> None:
         cwd = Path.cwd()
         for source, diag_by_source in diagnostics.diag_by_source.items():
-            source_rel = Path(source).relative_to(cwd)
+            source_rel = get_relative_path(source, cwd)
             for diagnostic in diag_by_source:
                 print(
                     self.config.linter.issue_format.format(
                         source=source_rel,
-                        source_abs=diagnostic.source,
+                        source_abs=str(Path(source).resolve()),
                         line=diagnostic.range.start.line,
                         col=diagnostic.range.start.character,
                         end_line=diagnostic.range.end.line,
@@ -122,7 +129,7 @@ class PrintIssuesReport(robocop.linter.reports.Report):
         cwd = Path.cwd()
         grouped_format = "  {line}:{col} {rule_id} {desc} ({name})"
         for source, diag_by_source in diagnostics.diag_by_source.items():
-            source_rel = Path(source).relative_to(cwd)
+            source_rel = get_relative_path(source, cwd)
             print(f"{source_rel}:")
             for diagnostic in diag_by_source:
                 print(
@@ -211,7 +218,7 @@ class PrintIssuesReport(robocop.linter.reports.Report):
     def print_diagnostics_extended(self, diagnostics: Diagnostics) -> None:
         cwd = Path.cwd()
         for source, diag_by_source in diagnostics.diag_by_source.items():
-            source_rel = Path(source).relative_to(cwd)
+            source_rel = get_relative_path(source, cwd)
             source_lines = None
             for diagnostic in diag_by_source:
                 if not source_lines:  # TODO: model should be coming from source, not diagnostics
diff --git a/src/robocop/linter/reports/text_file.py b/src/robocop/linter/reports/text_file.py
index 5798c9d..248afcc 100644
--- a/src/robocop/linter/reports/text_file.py
+++ b/src/robocop/linter/reports/text_file.py
@@ -32,11 +32,14 @@ class TextFile(robocop.linter.reports.Report):
         cwd = Path.cwd()
         messages = []
         for source, diag_by_source in diagnostics.diag_by_source.items():
-            source_rel = Path(source).relative_to(cwd)
+            try:
+                source_rel = Path(source).relative_to(cwd)
+            except ValueError:  # symlink etc
+                source_rel = source
             messages.extend(
                 self.config.linter.issue_format.format(
                     source=source_rel,
-                    source_abs=diagnostic.source,
+                    source_abs=Path(source).resolve(),
                     line=diagnostic.range.start.line,
                     col=diagnostic.range.start.character,
                     end_line=diagnostic.range.end.line,


Test patch:
diff --git a/tests/config/test_config_manager.py b/tests/config/test_config_manager.py
index e16dee0..0c97e18 100644
--- a/tests/config/test_config_manager.py
+++ b/tests/config/test_config_manager.py
@@ -1,4 +1,5 @@
 import shutil
+import sys
 from pathlib import Path
 
 import pytest
@@ -73,9 +74,9 @@ class TestConfigFinder:
         config.linter.configure = ["line-too-long.line_length=110"]
 
         expected_results = {
-            config_dir / "file1.robot": config,
-            config_dir / "subdir" / "file2.robot": config,
-            config_dir / "subdir" / "file3.resource": config,
+            Path("file1.robot"): config,
+            Path("subdir") / "file2.robot": config,
+            Path("subdir") / "file3.resource": config,
         }
 
         # Act
@@ -129,7 +130,7 @@ class TestConfigFinder:
         config.formatter.skip_config.keyword_call_pattern = {"DeprecatedKeyword$"}
         config.formatter.reruns = 5
 
-        expected_results = {config_dir / "test.robot": config}
+        expected_results = {Path("test.robot"): config}
 
         # Act
         actual_results = get_sources_and_configs(config_dir)
@@ -151,8 +152,8 @@ class TestConfigFinder:
         subdir_config.linter.configure = ["line-too-long.line_length=110"]
         subdir_config.file_filters.default_exclude = {"file3.resource"}
         expected_results = {
-            config_dir / "file1.robot": default_config,
-            config_dir / "subdir" / "file2.robot": subdir_config,
+            Path("file1.robot"): default_config,
+            Path("subdir") / "file2.robot": subdir_config,
             # file3.resource is excluded in the subdir config
         }
 
@@ -169,10 +170,10 @@ class TestConfigFinder:
         # Arrange
         config_dir = test_data / "one_config_subdir"
         expected_results = {
-            config_dir / "file1.robot": cli_config,
-            config_dir / "subdir" / "file2.robot": cli_config,
+            Path("file1.robot"): cli_config,
+            Path("subdir") / "file2.robot": cli_config,
             # file3.resource would be excluded by config, but cli config overwrites it
-            config_dir / "subdir" / "file3.resource": cli_config,
+            Path("subdir") / "file3.resource": cli_config,
         }
 
         # Act
@@ -198,11 +199,11 @@ class TestConfigFinder:
         subdir_config.linter.configure = ["line-too-long.line_length=110"]
         subdir_config.file_filters.default_exclude = overwrite_option
         expected_results = {
-            config_dir / "file1.robot": default_config,
+            Path("file1.robot"): default_config,
             # file2.robot should be included, but cli option overwrites it
-            # config_dir / "subdir" / "file2.robot": subdir_config,
+            # "subdir" / "file2.robot": subdir_config,
             # file3.resource would be excluded by config, but cli config overwrites it
-            config_dir / "subdir" / "file3.resource": subdir_config,
+            Path("subdir") / "file3.resource": subdir_config,
         }
 
         # Act
@@ -224,9 +225,9 @@ class TestConfigFinder:
             # excluded by cli option
             # config_dir / "file1.robot": cli_config,
             # file2.robot is excluded by cli config, but cli option overwrites it
-            config_dir / "subdir" / "file2.robot": cli_config,
+            Path("subdir") / "file2.robot": cli_config,
             # file3.resource would be excluded by config, but cli option overwrites it
-            config_dir / "subdir" / "file3.resource": cli_config,
+            Path("subdir") / "file3.resource": cli_config,
         }
 
         # Act
@@ -255,9 +256,9 @@ class TestConfigFinder:
         second_config.file_filters.default_exclude = {"file1.robot"}
         second_config.linter.configure = ["line-too-long.line_length=140"]
         expected_results = {
-            config_dir / "file1.robot": first_config,
-            config_dir / "subdir" / "file2.robot": second_config,
-            config_dir / "subdir" / "file3.resource": second_config,
+            Path("file1.robot"): first_config,
+            Path("subdir") / "file2.robot": second_config,
+            Path("subdir") / "file3.resource": second_config,
         }
 
         # Act
@@ -315,7 +316,7 @@ class TestConfigFinder:
         actual_results = get_sources_and_configs(tmp_test_files)
 
         # Assert
-        assert tmp_test_files / "file.robot" in actual_results
+        assert Path("file.robot") in actual_results
         assert len(actual_results) == 1
 
     def test_filter_paths_from_gitignore(self, test_data):
@@ -326,7 +327,7 @@ class TestConfigFinder:
         actual_results = get_sources_and_configs(test_dir)
 
         # Assert
-        assert test_dir / "file.robot" in actual_results
+        assert Path("file.robot") in actual_results
         assert len(actual_results) == 1
 
     def test_invalid_option_value(self, test_data):
@@ -384,3 +385,37 @@ class TestConfigFinder:
 
         # Assert
         assert list(actual_results.keys()) == expected_paths
+
+    @pytest.mark.skipif(sys.platform != "linux", reason="Test only runs on Linux")
+    def test_symlink_path(self, test_data, tmp_path):
+        # Arrange
+        absolute_path = tmp_path / "test.robot"
+        absolute_path2 = test_data / "simple" / "test.robot"
+        absolute_path.write_text("*** Settings ***")
+        (tmp_path / "test2.robot").symlink_to(absolute_path2)
+        (tmp_path / "test3.robot").symlink_to(absolute_path2)
+        expected_paths = [Path("test.robot"), Path("test2.robot"), Path("test3.robot")]
+
+        # Act
+        actual_results = get_sources_and_configs(tmp_path)
+
+        # Assert
+        assert sorted(actual_results.keys()) == expected_paths
+
+    @pytest.mark.skipif(sys.platform != "linux", reason="Test only runs on Linux")
+    def test_exclude_symlink_path(self, test_data, tmp_path, overwrite_config):
+        # Arrange
+        absolute_path = tmp_path / "test.robot"
+        absolute_path2 = test_data / "simple" / "test.robot"
+        absolute_path3 = test_data / "simple" / "test2.robot"
+        absolute_path.write_text("*** Settings ***")
+        (tmp_path / "test2.robot").symlink_to(absolute_path2)
+        (tmp_path / "test3.robot").symlink_to(absolute_path3)
+        expected_paths = [Path("test.robot"), Path("test2.robot")]
+        overwrite_config.file_filters.exclude = {"test3.robot"}
+
+        # Act
+        actual_results = get_sources_and_configs(tmp_path, overwrite_config=overwrite_config)
+
+        # Assert
+        assert sorted(actual_results.keys()) == expected_paths
diff --git a/tests/linter/rules/errors/invalid_if/test_rule.py b/tests/linter/rules/errors/invalid_if/test_rule.py
index f976be0..ba3ba0c 100644
--- a/tests/linter/rules/errors/invalid_if/test_rule.py
+++ b/tests/linter/rules/errors/invalid_if/test_rule.py
@@ -1,3 +1,9 @@
+import sys
+from pathlib import Path
+
+import pytest
+
+from tests import working_directory
 from tests.linter.utils import RuleAcceptance
 
 
@@ -21,3 +27,15 @@ class TestRuleAcceptance(RuleAcceptance):
             output_format="extended",
             test_on_version=">=6.1",
         )
+
+    @pytest.mark.skipif(sys.platform != "linux", reason="Test only runs on Linux")
+    def test_with_symlink(self, tmp_path):
+        absolute_path = Path(__file__).parent / "test.robot"
+        (tmp_path / "test.robot").symlink_to(absolute_path)
+        with working_directory(tmp_path):
+            self.check_rule(
+                src_files=["test.robot"],
+                expected_file="expected_output_rf6.1.txt",
+                test_on_version=">=6.1",
+                test_dir=tmp_path,
+            )
diff --git a/tests/linter/utils/__init__.py b/tests/linter/utils/__init__.py
index 666e01c..3fcfa8e 100644
--- a/tests/linter/utils/__init__.py
+++ b/tests/linter/utils/__init__.py
@@ -98,11 +98,12 @@ class RuleAcceptance:
         deprecated: bool = False,
         exit_code: int | None = None,
         compare_output: bool = True,
+        test_dir: Path | None = None,
         **kwargs,
     ) -> str | None:
         if not self.enabled_in_version(test_on_version):
             pytest.skip(f"Test enabled only for RF {test_on_version}")
-        test_data = self.test_class_dir
+        test_data = test_dir or self.test_class_dir
         sort_lines = output_format == "simple"
         issue_format = self.get_issue_format(issue_format)
         if select is None:
@@ -135,7 +136,7 @@ class RuleAcceptance:
         if not compare_output:
             return result
         parsed_results = result.splitlines()
-        expected = load_expected_file(test_data, expected_file, sort_lines=sort_lines)
+        expected = load_expected_file(self.test_class_dir, expected_file, sort_lines=sort_lines)
         actual = normalize_result(parsed_results, test_data, sort_lines=sort_lines)
         if deprecated:
             assert actual